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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

See inline comments. Nice patch overall though, real close.



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


================
Comment at: source/Plugins/SymbolFile/DWARF/DIERef.h:19
 struct DIERef {
+  enum DebugSection : uint8_t { InfoSection, TypesSection };
+
----------------
Use enum class?
```
enum class Section : uint8_t { DebugInfo, DebugTypes };
```


================
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) {
----------------
Does the "GetUnitAtIndex" call need to take "section" to get the right unit?


================
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))
----------------
Does the "GetUnitAtIndex" call need to take "section" to get the right unit?




================
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;
----------------
Should we use a DIERef here instead of these two members?


================
Comment at: source/Plugins/SymbolFile/DWARF/HashedNameToDIE.h:61
+    explicit operator DIERef() const {
+      return {DIERef::InfoSection, cu_offset, offset};
+    }
----------------
just return the DIERef member variable if we switch according to above inline comment


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

https://reviews.llvm.org/D61908