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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In https://reviews.llvm.org/D32167#1019399, @davide wrote:

> This commit has no tests. It should have many.


I has full testing. Please read the commit and notice there is a new debug info flavor that is added. So of course it is tested.

> It's very big, so it could be split in several pieces.

This is one thing that goes together. It isn't that big. I adds support for .debug_types into the ObjectFile and parses the information. How would you propose breaking this up?

> I'd really appreciate if you can take the time to do so. For now, some comments.

I believe I did take the time to test this properly. Let me know if you still think otherwise.



================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:718
                             self.getBuildDirBasename())
-    
-     
+
+
----------------
davide wrote:
> spurious whitespaces, please revert.
It is removing spurious whitespaces and there are other changes in this file. Most editors these days will remove spurious spaces and we should strive to remove them where possibly no?


================
Comment at: packages/Python/lldbsuite/test/lldbtest.py:1782
                         x, target_platform, configuration.compiler)]
-                
+
                 if "dsym" in supported_categories:
----------------
davide wrote:
> ditto.
Again, removing spurious whitespaces?


================
Comment at: packages/Python/lldbsuite/test/make/Makefile.rules:197
+ifeq "$(DWARF_TYPE_UNITS)" "YES"
+	DEBUG_INFO_FLAG ?= -gdwarf-4
+else
----------------
aprantl wrote:
> This shouldn't be necessary on any platform LLDB cares about. DWARF 4 should be the default everywhere.
Ok, I can remove this.


================
Comment at: packages/Python/lldbsuite/test/plugins/builder_base.py:204
+        commands.append([getMake(), "clean", getCmdLine(dictionary)])
+    commands.append([getMake(), "MAKE_DSYM=NO", "DWARF_TYPE_UNITS=YES",
+                    getArchSpec( architecture), getCCSpec(compiler),
----------------
aprantl wrote:
> Why does the type unit configuration care about whether there are dsyms or not? Shouldn't this be orthogonal?
All other "def buildXXX" functions do this, so I am following the convention.


================
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',
----------------
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?


================
Comment at: source/Plugins/SymbolFile/DWARF/DIERef.cpp:50
     if (dwarf_cu) {
-      if (dwarf_cu->GetBaseObjOffset() != DW_INVALID_OFFSET)
-        cu_offset = dwarf_cu->GetBaseObjOffset();
-      else
-        cu_offset = dwarf_cu->GetOffset();
+      // Replace the compile unit with the type signature compile unit for
+      // type signature attributes.
----------------
davide wrote:
> Why? This needs to be explained in a comment.
Will do


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDataExtractor.h:51
+  /// section as compile units and type units are in the .debug_info.
+  //------------------------------------------------------------------
+  void OffsetData(lldb::offset_t offset)
----------------
aprantl wrote:
> I think the //--- line might confuse Doxygen, but I'm not sure.
It doesn't. I checked the docs that show up in the doxygen in http://lldb.llvm.org/cpp_reference/html/index.html and these trailing comments don't hose it up.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:121
+        if (!cu_sp)
+          break;
+        
----------------
aprantl wrote:
> Is that check common LLDB style?
I am copying the first loop.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h:64
   typedef std::vector<DWARFCompileUnitSP> CompileUnitColl;
-
+  typedef std::unordered_map<uint64_t, uint32_t> TypeSignatureMap;
   //----------------------------------------------------------------------
----------------
davide wrote:
> any reason why you can't use LLVM adt?
No reason. IMHO STL is fine unless there is a real reason llvm adt is better?


https://reviews.llvm.org/D32167