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

[Xing GUO via Phabricator via llvm-commits]

Higuoxing added inline comments.


================
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:
> jhenderson wrote:
> > 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.
> Yes, seems the first YAML and this one can be merged.
I merged them into the first YAML.


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:417
+      else if (RawSec->Name == ".debug_str")
+        dumpDebugStrings(*DWARFCtx.get(), DWARF);
 
----------------
jhenderson wrote:
> 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.
Yes, I think I want to do it in a follow-up patch. I've fixed the behavior for llvm-dwarfdump recently and it seems that we can refactor it to reuse the parser in lib/DebugInfo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86867