[PATCH] D86867: [obj2yaml] Add support for dumping the .debug_str section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml:26
+
+## b) Test dumping an .debug_str section whose section header properties are overridden.
+
----------------



================
Comment at: llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml:55
+# RUN:     -DTYPE=PROGBITS -DFLAGS="[ SHF_MERGE, SHF_STRINGS ]" -DINFO=0003 -DADDRALIGN=0001 -DENTSIZE=0001
+
+#           SHDR: - Name:         .debug_str
----------------
grimar wrote:
> Probably you need a test showing that by default we don't print "Sections:" with the YAML below.
> I.e. a case when you don't override fields for this particular YAML.
Seems like adding this to the BASIC case above would be enough, right? You just need to show that when the object has default header properties, it doesn't add it to "Sections" too.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:417
+      else if (RawSec->Name == ".debug_str")
+        dumpDebugStrings(*DWARFCtx.get(), DWARF);
 
----------------
grimar wrote:
> I'd probably expect to see that `dumpDebugStrings` returns an error too, btw.
> I can imagine cases when it should error out probably:
> 1) A string table that doesn't start with the null-byte.
> 2) A string table that doesn't end with the null byte.
.debug_str is not an ELF `SHT_STRTAB` and doesn't require a leading null-byte by the standard. It does require a trailing null byte (since the contents is a series of null terminated strings).

As `dumpDebugStrings` already exists, this change should just work with its current behaviour, and a separate patch should address error handling, if needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86867/new/

https://reviews.llvm.org/D86867