[Lldb-commits] [PATCH] D62894: DWARF: Share line tables of type units

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:309
+      decl.SetFile(
+          die.GetDWARF()->GetSupportFile(*die.GetCU(), form_value.Unsigned()));
       break;
----------------
clayborg wrote:
> Maybe make a "FileSpec DWARFDie::GetFile(uint32_t idx);" function? 
I'm trying to avoid diverging llvm and lldb DWARFDies more than they already are. As llvm DWARFDie will not be returning a FileSpec, I think going through the symbol file makes sense (that's the level at which the sharing happens anyway.

But even without that, the api doesn't seem right, as this is not a query that can be answered by the DWARFDie class alone.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:392-395
+  if (FileSpec spec =
+          unit->GetCompilationDirectory().CopyByAppendingPathComponent(
+              unit->GetUnitDIEOnly().GetName()))
+    return spec.GetPath();
----------------
clayborg wrote:
> Maybe create a DWARFUnit function?:
> ```
> const lldb_private::FileSpec &DWARFUnit::GetFileSpec();
> ```
> 
Sounds good. This also diverges from llvm DWARFUnit, but OTOH, it fits in nicely with the GetCompilationDirectory function we have there already, so these two can be resolved later at the same time.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:393
+  if (FileSpec spec =
+          unit->GetCompilationDirectory().CopyByAppendingPathComponent(
+              unit->GetUnitDIEOnly().GetName()))
----------------
clayborg wrote:
> Does this do the right thing if DW_AT_name is absolute?
Woops. No, it doesn't.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:286-306
   dw_addr_t addr_base = cu_die.GetAttributeValueAsUnsigned(
       this, DW_AT_addr_base, LLDB_INVALID_ADDRESS);
   if (addr_base != LLDB_INVALID_ADDRESS)
     SetAddrBase(addr_base);
 
   dw_addr_t ranges_base = cu_die.GetAttributeValueAsUnsigned(
       this, DW_AT_rnglists_base, LLDB_INVALID_ADDRESS);
----------------
clayborg wrote:
> Many attributes being individually fetched here. This is slow. We should probably iterate over all attributes?
Sounds good.


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:812-813
+  if (offset == DW_INVALID_OFFSET ||
+      offset == llvm::DenseMapInfo<dw_offset_t>::getEmptyKey() ||
+      offset == llvm::DenseMapInfo<dw_offset_t>::getTombstoneKey())
+    return empty_list;
----------------
clayborg wrote:
> why are these checks needed? Remove? Maybe left over from previous approach?
The offset is going to be used as a key in a DenseMap, and DenseMap reserves two values of each type for the "empty" and "tombstone" keys. These are something like 0xff..ff and 0xff.ffe, so valid DWARF will not contain those values, but malformed can (and if it did, this code would crash when trying to insert those keys).


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:814
+      offset == llvm::DenseMapInfo<dw_offset_t>::getTombstoneKey())
+    return empty_list;
+
----------------
aprantl wrote:
> `return {};` ?
That wouldn't work, because `{}` is a temporary, and this function is returning a reference. Returning by reference saves a copy in the common case where DW_AT_stmt_list is valid, but I also need to handle the invalid case somehow. I could have this function return a pointer and return a nullptr in the invalid case (or something equivalent), but this saves the caller from checking that.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62894/new/

https://reviews.llvm.org/D62894