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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:309
+      decl.SetFile(
+          die.GetDWARF()->GetSupportFile(*die.GetCU(), form_value.Unsigned()));
       break;
----------------
Maybe make a "FileSpec DWARFDie::GetFile(uint32_t idx);" function? 


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:388
+static std::string GetUnitName(const DWARFDIE &die) {
+  std::string result = "the source file";
+  DWARFUnit *unit = die.GetCU();
----------------
Maybe:
```
std::string result = "<missing DWARF unit path>";
```


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:392-395
+  if (FileSpec spec =
+          unit->GetCompilationDirectory().CopyByAppendingPathComponent(
+              unit->GetUnitDIEOnly().GetName()))
+    return spec.GetPath();
----------------
Maybe create a DWARFUnit function?:
```
const lldb_private::FileSpec &DWARFUnit::GetFileSpec();
```



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


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2266-2267
             case DW_AT_decl_file:
-              decl.SetFile(sc.comp_unit->GetSupportFiles().GetFileSpecAtIndex(
-                  form_value.Unsigned()));
+              decl.SetFile(die.GetDWARF()->GetSupportFile(
+                  *die.GetCU(), form_value.Unsigned()));
               break;
----------------
Use new DWARFDie::GetFile()


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2424-2425
         decl_up.reset(new Declaration(
-            comp_unit.GetSupportFiles().GetFileSpecAtIndex(decl_file),
-            decl_line, decl_column));
+            die.GetDWARF()->GetSupportFile(*die.GetCU(), decl_file), decl_line,
+            decl_column));
 
----------------
Use new DWARFDie::GetFile()


================
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);
----------------
Many attributes being individually fetched here. This is slow. We should probably iterate over all attributes?


================
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;
----------------
why are these checks needed? Remove? Maybe left over from previous approach?


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:303
 
+  lldb_private::FileSpec GetSupportFile(DWARFUnit &unit, size_t file_idx);
+
----------------
Can probably just be named GetFile


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

https://reviews.llvm.org/D62894