[PATCH] D51081: [DWARF v5] Refactoring range list dumping to fold DWARF v4 functionality into v5 handling (almost NFC).

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a subscriber: probinson.
dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFListTable.h:52
+  bool empty() const {
+    return Entries.empty() || Entries.begin()->isEndOfList();
+  }
----------------
Why this new special case - when would the range list contain an empty range?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFListTable.h:71
+            DIDumpOptions DumpOpts = {}) const;
+  void addEntry(const ListEntryType &Entry) { Entries.push_back(Entry); }
+  /// Print a header for the list. Returns the indentation that is to be used
----------------
This appears to be unused in this patch? (I see no other mention of 'addEntry') - delete it as dead code?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFListTable.h:72-77
+  /// Print a header for the list. Returns the indentation that is to be used
+  /// when subsequently dumping each list entry.
+  unsigned printListHeader(raw_ostream &OS, uint8_t AddrSize,
+                           DIDumpOptions DIDumpOpts) const {
+    return 0;
+  }
----------------
This seems to be dead in this patch - maybe remove it from here and add it in whichever patch adds its use?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFListTable.h:87-90
+    /// The DWARF version number. We assume version 5 until we either extract
+    /// the version from the section or explicitly set it (e.g. to fake a
+    /// table for DWARF v4 and earlier).
+    uint16_t Version = 5;
----------------
That sounds like in all cases we know the version from the input, right? So when/why would this assumption be needed?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp:30-33
+  // In DWARF v4 and earlier, a range list entry corresponds to DWARF v5
+  // entry kind DW_RLE_start_end, except that it is possibly subject to
+  // base adjustment, which is not relevant during extraction.
+  uint8_t Encoding = dwarf::DW_RLE_start_end;
----------------
Would it be simpler to model DWARFv4 range list entries as RLE_offset_pair, rather than as start_end with an offset? It looks like then it'd fall out without a special case in getAbsoluteRanges (& that function may not need to be touched at all - it wouldn't need to change behavior based on version & maybe the assertions aren't worth the extra API change?)


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp:84-92
+      if (Version < 5)
+        return createStringError(
+            errc::invalid_argument,
+            "invalid range list entry at offset 0x%" PRIx32, *OffsetPtr);
       return createStringError(errc::invalid_argument,
                          "insufficient space remaining in table for "
                          "DW_RLE_start_end encoding "
----------------
Suspect it might be fine to rephrase the DWARFv5 message ("start/end encoding" (or "offset pair at offset ..." ) - so it applies to the 4 and 5 case equally) to be more general & avoid the 4/5 special casing here. (or in offset_pair if the code moves there based on the previous comment)


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp:117-127
+  // For DWARF v4 and earlier, adjust the EntryKind for end-of-list and
+  // base_address based on the contents.
+  if (Version < 5) {
+    if (Value0 == maxUIntN(Data.getAddressSize() * 8)) {
+      EntryKind = dwarf::DW_RLE_base_address;
+      Value0 = Value1;
+      Value1 = 0;
----------------
Perhaps this logic should move into the relevant (start_end or offset_pair) case above rather than here - since it doesn't/can't apply to other cases in the switch?


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:318-319
+  // the table header which is expected to start at offset 0.
+  if (isDWO)
+    Base = Table.getHeaderSize();
+  return Table;
----------------
Maybe rather than propagating DWO information here, the Base could be set to Table.getHeaderSize() if there is no Base specified? (using Base == 0 or Base == -1 or something as a flagging value to communicate "no Base has been specified, fill one in")?

(though side rant: I don't understand why these offsets ever point past the header - that seems like it breaks the purpose of the header to provide section versioning, etc - it means the header format can never change and the parser would have to walk backwards from the base offset to parse the version to know how to parse the rest of the section anyway... @probinson @aprantl - though perhaps I've ranted sufficiently about this before)


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:493
   DWO->setAddrOffsetSection(AddrOffsetSection, AddrOffsetSectionBase);
-  if (getVersion() >= 5) {
+  if (getVersion() > 4) {
     DWO->setRangesSection(&Context.getDWARFObj().getRnglistsDWOSection(), 0);
----------------
I think the ">= 5" phrasing might make more sense to readers "supported since 5" is sort of what I'm thinking when I read it.


================
Comment at: test/DebugInfo/dwarfdump-ranges.test:18-23
 CHECK-NEXT: 00000000 000000000000062c 0000000000000637
-CHECK-NEXT: 00000000 0000000000000637 000000000000063d
-CHECK-NEXT: 00000000 <End of list>
+CHECK-NEXT: 00000010 0000000000000637 000000000000063d
+CHECK-NEXT: 00000020 <End of list>
 CHECK-NEXT: 00000030 0000000000000640 000000000000064b
-CHECK-NEXT: 00000030 0000000000000637 000000000000063d
-CHECK-NEXT: 00000030 <End of list>
+CHECK-NEXT: 00000040 0000000000000637 000000000000063d
+CHECK-NEXT: 00000050 <End of list>
----------------
If you're updating all these test cases anyway - is it worth revisiting what the best format for dumping debug_ranges is? I'm not sure printing those offsets is especially meaningful (either in the old behavior or the new behavior) rather than just printing the starting offset, then printing the pairs.

& printing it in a way that's more similar between ranges and rnglistsn wouldn't hurt either - not sure if there's something in common that'd suit everyone though.


https://reviews.llvm.org/D51081