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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
