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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:69
+    return createStringError(errc::invalid_argument,
+                             "Index %" PRIu32 " is out of range");
+  }
----------------
It might be worth making this message a little more verbose, to mention the fact that it's out of range of the address table at offset x, if possible.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:91
+
+  /// Invalidate Length field to stop further processing
+  void invalidateLength() { HeaderData.Length = 0; }
----------------
Nit: missing full stop.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:253
 
+// Dump the .debug_addr section (DWARF v5).
+static void dumpAddrSection(raw_ostream &OS, DWARFDataExtractor &AddrData,
----------------
Since .debug_addr can pre-date DWARF v5, you should probably just remove this comment (or at least the bit to do with the version).


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:258
+  // implementation and make all DWARF classes use them.
+  static auto WarnCB = [](Error Warn) {
+    handleAllErrors(std::move(Warn), [](ErrorInfoBase &Info) {
----------------
"CB" is not an obvious abbreviation to me. Please just call it "WarnCallback" or similar (ditto in the actual extract function).


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:72
+
+  // Perform basic validation of the remaining header fields.
+  if (HeaderData.Version != SuggestedVersion)
----------------
jhenderson wrote:
> It may be worth saying that Version != 5 is not supported (except for the special case)?
It might have lost track. Which error message shows this exactly?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:52
+    if (HeaderData.Length + sizeof(uint32_t) < sizeof(Header)) {
+      // Invalidate Length field to stop further processing
+      uint32_t TmpLength = getLength();
----------------
Missing full stop.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:54
+      uint32_t TmpLength = getLength();
+      HeaderData.Length = 0;
+      return createStringError(errc::invalid_argument,
----------------
You might as well use the existing function for this.

But actually, here and elsewhere below, I suggest you just assume that the Length is valid. I'd only invalidate it when the version is unsupported, DWARF64 is detected or the section is too short for a length field. That way the caller can just skip the rest of the table and attempt to read another one a bit later, which might work.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:87
+  // implementation of .debug_addr table which doesn't contain header
+  // and consists only of series of addresses.
+  if (HeaderData.Version != SuggestedVersion)
----------------
We do support DWARF version 5 for now as well as... -> We support DWARF version 5 and the...

table which -> table, which

header -> a header

series of -> a series of




================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:92
+                       " has version %" PRIu16
+                       " which is different from suggested version %" PRIu16,
+                       HeaderOffset, HeaderData.Version, SuggestedVersion);
----------------
Probably worth putting here where the version is suggested from.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:97
+                       ".debug_addr table at offset 0x%" PRIx32
+                       " has unsupported address size %hhu",
+                       HeaderOffset, HeaderData.AddrSize);
----------------
Here and elsewhere, use the PRIu8 and similar instead of explicit "hhu" when the type is uint8_t etc.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr.s:22
+
+# ERR:            DWARF version is not defined, assuming version 5
+# ERR:            DWARF version is not defined, assuming version 5
----------------
It would be nice to have a test case that doesn't produce this warning.


https://reviews.llvm.org/D49676