[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.


================
Comment at: packages/Python/lldbsuite/test/test_categories.py:27
     'gmodules': 'Tests that can be run with -gmodules debug information',
+    'dwarf_type_units' : 'Tests using the DWARF type units (-fdebug-types-section)',
     'expression': 'Tests related to the expression parser',
----------------
clayborg wrote:
> aprantl wrote:
> > This is conflating two things (-fdebug-types-section and type units) DWARF5 doesn't have a debug_types section but it still offers type units. Clang doesn't implement this yet, but GCC might, I haven't checked.
> So what is the suggestion here?
I would mention type units in the comment rather than -fdebug-types-section.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:97
 void DWARFDebugInfo::ParseCompileUnitHeadersIfNeeded() {
   if (m_compile_units.empty()) {
     if (m_dwarf2Data != NULL) {
----------------
We probably should convert this to an early exit.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:106
         offset = cu_sp->GetNextCompileUnitOffset();
       }
+
----------------
Perhaps add a comment explaining what is being done here?


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:121
+          break;
+        }
+      }
----------------
I find hard to follow. What do you think about:
```
   while (debug_types_data.ValidOffset(offset)) {
        cu_sp = DWARFCompileUnit::Extract(m_dwarf2Data, debug_types_data,
                                               &offset, true)));
        if (!cu_sp)
           return;
        m_type_sig_to_cu_index[cu_sp->GetTypeSignature()] =
            m_compile_units.size();
        m_compile_units.push_back(cu_sp);
        offset = cu_sp->GetNextCompileUnitOffset();
      }
```
?

Also, shouldn't the error be propagated?


https://reviews.llvm.org/D32167