[PATCH] D49676: [DWARF] support for .debug_addr (consumer)

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Not specific to your change, but I noticed that a lot of the DWARFContext code uses 32-bit offsets. Would it not make sense where we are writing new stuff, to use 64-bit offsets, since DWARF does support this?

I haven't looked at the details of the new address table class. I'll look at that later.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:62
+  /// Return the address based on a given index.
+  Optional<uint64_t> getAddrEntry(uint32_t Index) const {
+    if (Index < Addrs.size())
----------------
I'm wondering if this should return an Error instance instead of Optional for the out-of-range case. It seems like a non-existent index request would indicate a malformed section, right?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:78
+    }
+    llvm_unreachable("Invalid DWARF format (expected DWARF32 or DWARF64");
+  }
----------------
Missing trailing ')'


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:253
+// Dump the .debug_addr section (DWARF v5).
+static void dumpAddrSection(raw_ostream &OS, DWARFDataExtractor &AddrData,
+                            DIDumpOptions DumpOpts) {
----------------
I see that this is practically identical to dumpRnglistsSection below (and it may well be similar to other stuff too). Perhaps we should generalise the mechanism for extracting and dumping a given DWARF section, made up of distinct sub-sections/tables?


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:261-265
+        uint64_t Length = AddrTable.length();
+        // Keep going after an error, if we can, assuming that the length field
+        // could be read. If it couldn't, stop reading the section.
+        if (Length == 0)
+          break;
----------------
I'm not particularly comfortable with the DWARFContext knowing that a Length 0 is the indicator of whether the parsing can continue. I had similar issues with the .debug_line behaviour until I added a parser to wrap the extraction process. My personal inclination (but others may disagree) is that the AddrTable should have a "validLength()" method or similar, that can be queried.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:504-509
+  if (shouldDump(Explicit, ".debug_addr", DIDT_ID_DebugAddr,
+                 DObj->getAddrSection().Data)) {
+    DWARFDataExtractor AddrData(*DObj, DObj->getAddrSection(),
+                                isLittleEndian(), 0);
+    dumpAddrSection(OS, AddrData, DumpOpts);
+  }
----------------
This should probably not happen in between .debug_ranges and .debug_rnglists, just because those two sections are effectively equivalent.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:25
+template <typename... Ts>
+static Error createError(char const *Fmt, const Ts &... Vals) {
+  std::string Buffer;
----------------
I feel like this function is something we use repeatedly throughout the DWARF library, and quite probably elsewhere too. That's probably an indication that it needs moving into a support library.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_X86.s:10
+
+  .text
+  .file "foo.c"
----------------
There's a lot of unnecessary noise in this test. Could you remove everything apart from what is actually needed to test the section dumping, please. You can even use explicit addresses in the .debug_addr section, so that there doesn't need to be any labels.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_empty.s:7
+
+.section .debug_rnglists,"", at progbits
----------------
Why .debug_rnglists here?


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_invalid.s:9
+
+  .text
+  .file "foo.c"
----------------
Again, most of this is probably unnecessary.


https://reviews.llvm.org/D49676