[PATCH] D28040: Don't store the NULL DIEs in the DIE array in the DWARFUnit.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h:37-38
+  uint32_t Depth : 31;
+  /// If non-zero this DIE has a NULL DIE that should have followed this DIE.
+  /// NULL DIEs are now removed when storing DWARFDebugInfoEntry
+  uint32_t HasNullSibling : 1;
----------------
Comment seems temporal - it describes the change ("are /now/ removed") rather than the current state, which might be confusing for future readers.

It also might be easier if the member was called "isLastChild".

(similar comment on other comments - and a comment below might remove the need for this HasNullSibling/isLastChild member)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h:39
+  /// NULL DIEs are now removed when storing DWARFDebugInfoEntry
+  uint32_t HasNullSibling : 1;
 
----------------
This should be a bool.


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:276-277
 DWARFDie::getAddressRanges() const {
-  if (isNULL())
+  if (!isValid())
     return DWARFAddressRangesVector();
   // Single range specified by low/high PC.
----------------
Is this necessary? (similarly for collectChildrenAddressRanges, dump, etc) - or should we just require clients not to call these operations on invalid DIEs?


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:395-402
+        // We don't have NULL DIEs in our DWARFUnit anymore so we have to
+        // conjure up the NULL DIE when printing.
+        if (getDebugInfoEntry()->getHasNULLSibling()) {
+          WithColor(OS, syntax::Address).get() << format("\n0x%8.8x: ", Offset);
+          OS.indent(Indent) << "NULL\n";
+          // Add 1 to the offset for the ULEB128 abbreviation code byte.
+          Offset += 1;
----------------
I'm still not sure it should be the responsibility of this node to print the NULL entry here. Perhaps it should be handled when printing children instead? Between line 392 and 393 seems pretty ideal.

& it doesn't look like the 'getHasNULLSibling' property would be needed to correctly emit the NULL element at that location. 


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:397
+        // conjure up the NULL DIE when printing.
+        if (getDebugInfoEntry()->getHasNULLSibling()) {
+          WithColor(OS, syntax::Address).get() << format("\n0x%8.8x: ", Offset);
----------------
Seems unnecessary to call 'getDebugInfoEntry' from within DWARFDie - rather than accessing the 'Die' member directly.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1042-1043
   EXPECT_EQ(A.getParent(), CUDie);
   EXPECT_EQ(B.getParent(), CUDie);
-  EXPECT_EQ(Null.getParent(), CUDie);
+  EXPECT_EQ(C.getParent(), CUDie);
 
----------------
As mentioned in previous comments - I'd suggest just dropping more of these checks. Do they add substantial value over the existing checks? (eg: are there bugs that are likely to be found by examining B & C's parentage that wouldn't be found by testing A's parentage?)


https://reviews.llvm.org/D28040