[Lldb-commits] [PATCH] D61908: DWARF: Add ability to reference debug info coming from multiple sections

[Pavel Labath via Phabricator via lldb-commits]

labath marked 10 inline comments as done.
labath added inline comments.


================
Comment at: source/Plugins/SymbolFile/DWARF/DIERef.cpp:18
   if (form_value.IsValid()) {
     const DWARFUnit *dwarf_cu = form_value.GetCompileUnit();
     if (dwarf_cu) {
----------------
clayborg wrote:
> We should maybe rename DWARFFormValue::GetCompileUnit() to DWARFFormValue::GetUnit()? Also we might use "unit" instead of "dwarf_cu" here?
Done in r360754.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:141
+  uint32_t idx = FindUnitIndex(section, cu_offset);
   DWARFUnit *result = GetUnitAtIndex(idx);
   if (result && result->GetOffset() != cu_offset) {
----------------
clayborg wrote:
> Does the "GetUnitAtIndex" call need to take "section" to get the right unit?
No, the idea is that the unit index will still uniquely determine the DWARFUnit, regardless of the section it is in (so the sections will still be kind of concatenated, but only at the index level). This makes it easy to convert back and forth between DWARFUnits and lldb_private::CompileUnits.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:162
+  uint32_t idx = FindUnitIndex(section, die_offset);
   DWARFUnit *result = GetUnitAtIndex(idx);
   if (result && !result->ContainsDIEOffset(die_offset))
----------------
clayborg wrote:
> Does the "GetUnitAtIndex" call need to take "section" to get the right unit?
> 
> 
Same comment as above.


================
Comment at: source/Plugins/SymbolFile/DWARF/HashedNameToDIE.h:51-52
   struct DIEInfo {
     dw_offset_t cu_offset;
     dw_offset_t offset; // The DIE offset
     dw_tag_t tag;
----------------
clayborg wrote:
> Should we use a DIERef here instead of these two members?
Sounds like a good idea. I'll update the patch accordingly.


================
Comment at: source/Plugins/SymbolFile/DWARF/HashedNameToDIE.h:61
+    explicit operator DIERef() const {
+      return {DIERef::InfoSection, cu_offset, offset};
+    }
----------------
clayborg wrote:
> just return the DIERef member variable if we switch according to above inline comment
Done. I'm not sure the operator makes that much sense now as one can just access the contained DIERef directly.  I've kept it for now to avoid needing to update all callsites, but I can also delete it if you prefer.


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

https://reviews.llvm.org/D61908