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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/BinaryFormat/Dwarf.def:874
 HANDLE_DWARF_SECTION(DebugRnglists, ".debug_rnglists", "debug-rnglists")
+HANDLE_DWARF_SECTION(DebugAddr, ".debug_addr", "debug-addr")
 HANDLE_DWARF_SECTION(DebugStr, ".debug_str", "debug-str")
----------------
These look to be in alphabetical order for the most part too, so you should probably move this.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugAddr.h:67
+      return Addrs[Index];
+    return createStringError(errc::argument_out_of_domain,
+                             "Index %" PRIu32 " is out of range");
----------------
I'm wondering whether errc::invalid_argument might be more applicable. It looks like argument_out_of_domain is more to do with Mathematical functions, which this isn't.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:28
+  if (!Data.isValidOffsetForDataOfSize(*OffsetPtr, sizeof(uint32_t)))
+    return createStringError(errc::argument_out_of_domain,
+                       "section is not large enough to contain a "
----------------
Again, I'd say this should be invalid_argument (also applies to other uses of argument_out_of_domain below).


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:34
+  if (Version == 0) {
+    errs() << "WARNING: CU version is not defined, assuming version 5\n";
+    SuggestedVersion = 5;
----------------
Please don't warn directly in library functions like this! And even if you do, please use WithColor, to ensure it is consistent.

Different executables will have different styles for how they handle warnings (some might want them printing, others might want to ignore them, and yet more might want them to be treated as a failure). Since this warning doesn't prevent continuation, I recommend you use some sort of callback. You can see similar things in the DWARFDebugLine::LineTable class, for what I mean, and would recommend.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:44
+    if (HeaderData.Length == 0xffffffffu)
+      return createStringError(errc::invalid_argument,
+          "DWARF64 is not supported in .debug_addr at offset 0x%" PRIx32 "\n",
----------------
There is errc::not_supported, which I'd recommend for this.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:72
+
+  // Perform basic validation of the remaining header fields.
+  if (HeaderData.Version != SuggestedVersion)
----------------
It may be worth saying that Version != 5 is not supported (except for the special case)?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:80
+  if (HeaderData.AddrSize != 4 && HeaderData.AddrSize != 8)
+    return createStringError(errc::invalid_argument,
+                       ".debug_addr table at offset 0x%" PRIx32
----------------
This should probably be not_supported too.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:93
+  if (HeaderData.SegSize != 0)
+    return createStringError(errc::invalid_argument,
+                       ".debug_addr table at offset 0x%" PRIx32
----------------
not_supported again.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:125
+    for (const auto &Addr : Addrs) {
+      OS << format("\n0x%8.8" PRIx32, Addr); // TODO: 64bit addrs
+      if (DumpOpts.Verbose)
----------------
Are you going to do 64-bit addresses immediately after this?


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_absent.s:5
+# CHECK-NOT: Addr
+# CHECK-NOT: error:
----------------
You'll want to redirect stderr to stdout too, if you are going to check for warnings etc. I'd also go as far as to say that you should do `CHECK-NOT: {{.}}` at the end to show that we printed nothing else.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_dwarf4.s:21
+  .long 0x00000001
+.Lline_table_start0:
----------------
I think you can delete this label.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_empty.s:1
+# RUN: llvm-mc %s -filetype obj -triple i386-pc-linux -o - | \
+# RUN: llvm-dwarfdump -debug-addr - | FileCheck %s
----------------
jhenderson wrote:
> There's another test case probably needed, similar to this one: no .debug_addr section at all.
And one more I thought of: an addr section with a header but no members. You might want to add that to the basic test case, rather than as a new file, as that will show what happens when there are multiple tables in the same file.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_invalid.s:6
+# CHECK: unsupported segment selector size 1
+# CHECK: is not a multiple of addr size 4
+
----------------
You should probably add tests for each of the different error messages you can emit, at least reasonably. The ones that I can see are where the section is too small for a length field, the section is shorter than the (otherwise valid) length field (both as too short for the header and too short for the supposed payload), if the version is not specified (i.e. Version == 0), mismatching or invalid versions, and mismatching addresses (essentially that's all the ones that are not "not_supported".

You might also want tests for the currently unsupported cases, such as DWARF64 too.


================
Comment at: test/tools/llvm-dwarfdump/X86/debug_addr_invalid.s:24
+  .byte 4 # address_size
+  .byte 1 # segment_selector_size
+  .long 0x00000000
----------------
You should probably make this a valid section, given a non-zero segment_selector size.


https://reviews.llvm.org/D49676