[obj2yaml] Add support for dumping the .debug_str section.

[Higuoxing]

Summary

This patch adds support for dumping the .debug_str section to obj2yaml.

[Higuoxing]

Update.

[Higuoxing]

Fix test case (c)

[grimar]

llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml

29

I'd add a short comment for each of these sub-cases to make it clear what they want to test.

34

I think you have SHDR TYPE FLAGS ADDRALIGN ENTSIZE prefixes in each of the sub cases.
Perhaps merge them into single prefix, e.g COMMON?

56

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.

llvm/tools/obj2yaml/elf2yaml.cpp

213

Perhaps it would be easier to read it when there is no CommonProperties variable,
but an early return is used instead.

if (RawSec->Type != ELF::SHT_PROGBITS || !RawSec->Link.empty() ||
    RawSec->Info || (uint64_t)RawSec->AddressAlign != 1 ||
    RawSec->Address)
  return true;

if (SecName == "debug_str")
  return !(RawSec->EntSize && (uint64_t)*RawSec->EntSize == 1) ||
         RawSec->Flags.getValueOr(ELFYAML::ELF_SHF(0)) !=
             ELFYAML::ELF_SHF(ELF::SHF_MERGE | ELF::SHF_STRINGS);

return RawSec->EntSize || RawSec->Flags;

[grimar]

llvm/tools/obj2yaml/elf2yaml.cpp

417

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.

[jhenderson]

llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml

27

56

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.

llvm/tools/obj2yaml/elf2yaml.cpp

417

.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.

[grimar]

llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml

56

Yes, seems the first YAML and this one can be merged.

[Higuoxing]

Address comments.

It looks like FileCheck isn't happy when I put the BASIC: checking labels before the COMMON: labels, so I adjust the order a little.

Thanks for reviewing!

llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml

56

I merged them into the first YAML.

llvm/tools/obj2yaml/elf2yaml.cpp

417

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.

[Higuoxing]

Add test case (c): Test dumping an empty .debug_str section.

[jhenderson]

llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml

14

Here and in each other case, rather than having to put leading 0s in every value, you could switch to FileCheck numeric variables. I think the invocation is something like:

# RUN: FIleCheck ... -D#%x,ADDRALIGN=1
...
#  COMMON-NEXT:   AddressAlign: 0x[[#%.16x, ADDRALIGN]]

llvm/tools/obj2yaml/elf2yaml.cpp

209

I'm struggling with this code. Under what circumstances can RawSec->EntSize by None here? Similar question for the Flags below.

[Higuoxing]

Update.

llvm/tools/obj2yaml/elf2yaml.cpp

209

I check it because EntSize and Flags are Optional values. It seems that you are right, I cannot create a test case that make them None. Could you please tell me why they cannot be None here?

[grimar]

llvm/tools/obj2yaml/elf2yaml.cpp

209

Well. I haven't debugged this, but I *think* they can be None.

Looking on the code, we have:

Error ELFDumper<ELFT>::dumpCommonSection(const Elf_Shdr *Shdr,
                                         ELFYAML::Section &S) {
...
  if (Shdr->sh_entsize != getDefaultShEntSize<ELFT>(S.Type))
    S.EntSize = static_cast<llvm::yaml::Hex64>(Shdr->sh_entsize);

So i'd expect to have None value when sh_entsize == 0. Isn't it true?

[jhenderson]

llvm/tools/obj2yaml/elf2yaml.cpp

209

@grimar is more familiar with this code than I am, so perhaps he can explain. I unfortunately have higher priority work that needs doing today, or I'd go digging myself!

[grimar]

llvm/tools/obj2yaml/elf2yaml.cpp

209

So yes, I've debugged this and it is possible to get None for EntSize when sh_entsize == 0.
(I've used the YAML below).

--- !ELF
FileHeader:
  Class: ELFCLASS64
  Data:  ELFDATA2LSB
  Type:  ET_EXEC
Sections:
  - Name:   .debug_str
    Type:    SHT_PROGBITS
    EntSize: 0
    Size:    10
    ShFlags: 0

When there are no flags, we have 'None` for Flags.

[Higuoxing]

Check the optional value.

llvm/tools/obj2yaml/elf2yaml.cpp

209

Thank you @grimar . I reproduce it using the YAML provided by you with a slight modification.

--- !ELF
FileHeader:
  Class: ELFCLASS64
  Data:  ELFDATA2LSB
  Type:  ET_EXEC
Sections:
  - Name:         .debug_str
    Type:         SHT_PROGBITS
    AddressAlign: 1
    Flags:        []

Because the default value of AddressAlign is 0. If we don't specify it, this function will return in the first if branch and it will not reach the second if branch where the optional value is dereferenced.

if (RawSec->Type != ELF::SHT_PROGBITS || !RawSec->Link.empty() ||
    RawSec->Info || RawSec->AddressAlign != 1 || RawSec->Address)
                    ^~~~~~~~~~~~~~~~~~~~~~~~~
  return true;

[jhenderson]

llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml

37–39

I think you need an ENTSIZE= 0 test case too, since that would flag up if we dereferenced the Entsize value without checking.

llvm/tools/obj2yaml/elf2yaml.cpp

209

Can you do this? I think it's nice to be symmetrical with the flags line below.

[Higuoxing]

I modified the getDefaultShEntSize() function in this revision. If the section name is '.debug_str', the default value of sh_entsize is 1 rather than 0.

llvm/tools/obj2yaml/elf2yaml.cpp

209

I mark this as done since this comment is not applied here ...

[jhenderson]

llvm/tools/obj2yaml/elf2yaml.cpp

213

I'm not sure casting this here is the clearest of things. I'd actually make it more explicit and check it doesn't equal 0 (if the (bool) is even required).

[Higuoxing]

Explicitly check the sh_flag.

[jhenderson]

LGTM, with suggestion.

llvm/tools/obj2yaml/elf2yaml.cpp

213

It may be best to introduce a helper variable so that we don't repeat RawSec->Flags.getValueOr(ELFYAML::ELF_SHF(0)) twice.

[Higuoxing]

Avoid using Flags.getValueOr() twice.

[jhenderson]

LGTM.

llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml

+## Test how we dump the .debug_str section.
+
+## a) Test dumping a .debug_str section with a default section header.
+
+# RUN: yaml2obj --docnum=1 %s | obj2yaml | \
+# RUN:   FileCheck %s --check-prefix=BASIC --implicit-check-not='Name: .debug_str'
+
+## b) Test dumping a .debug_str section whose section header properties are overridden.
+
+## Override the sh_type field.
+# RUN: yaml2obj --docnum=1 -DTYPE=STRTAB %s | obj2yaml | \
+# RUN:   FileCheck %s --check-prefixes=BASIC,COMMON \
+# RUN:     -DTYPE=STRTAB -DFLAGS="[ SHF_MERGE, SHF_STRINGS ]" -DADDRALIGN=0001 -DENTSIZE=0001
+
+## Override the sh_flags field.
+# RUN: yaml2obj --docnum=1 -DFLAGS=[SHF_ALLOC] %s | obj2yaml | \
+# RUN:   FileCheck %s --check-prefixes=BASIC,COMMON \
+# RUN:     -DTYPE=PROGBITS -DFLAGS="[ SHF_ALLOC ]" -DADDRALIGN=0001 -DENTSIZE=0001
+
+## Override the sh_link field.
+# RUN: yaml2obj --docnum=1 -DLINK=.sec %s | obj2yaml | \
+# RUN:   FileCheck %s --check-prefixes=BASIC,COMMON,LINK \
+# RUN:     -DTYPE=PROGBITS -DFLAGS="[ SHF_MERGE, SHF_STRINGS ]" -DLINK=.sec -DADDRALIGN=0001 -DENTSIZE=0001
+
+## Override the sh_addr field.
+# RUN: yaml2obj --docnum=1 -DADDRESS=0x2020 %s | obj2yaml | \
+# RUN:   FileCheck %s --check-prefixes=BASIC,COMMON,ADDRESS \
+# RUN:     -DTYPE=PROGBITS -DFLAGS="[ SHF_MERGE, SHF_STRINGS ]" -DADDRALIGN=0001 -DENTSIZE=0001 -DADDRESS=2020
+
+## Override the sh_addralign field
+# RUN: yaml2obj --docnum=1 -DADDRALIGN=3 %s | obj2yaml | \
+# RUN:   FileCheck %s --check-prefixes=BASIC,COMMON \
+# RUN:     -DTYPE=PROGBITS -DFLAGS="[ SHF_MERGE, SHF_STRINGS ]" -DADDRALIGN=0003 -DENTSIZE=0001
+
+## Override the sh_entsize field.
+# RUN: yaml2obj --docnum=1 -DENTSIZE=3 %s | obj2yaml | \
+# RUN:   FileCheck %s --check-prefixes=BASIC,COMMON \
+# RUN:     -DTYPE=PROGBITS -DFLAGS="[ SHF_MERGE, SHF_STRINGS ]" -DADDRALIGN=0001 -DENTSIZE=0003
+
+## Override the sh_info field.
+# RUN: yaml2obj --docnum=1 -DINFO=3 %s | obj2yaml | \
+# RUN:   FileCheck %s --check-prefixes=BASIC,COMMON,INFO \
+# RUN:     -DTYPE=PROGBITS -DFLAGS="[ SHF_MERGE, SHF_STRINGS ]" -DINFO=0003 -DADDRALIGN=0001 -DENTSIZE=0001
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_EXEC
+Sections:
+  - Name:         .debug_str
+    Type:         SHT_[[TYPE=PROGBITS]]
+    Flags:        [[FLAGS=<none>]]
+    Link:         [[LINK='']]
+    EntSize:      [[ENTSIZE=1]]
+    Info:         [[INFO=<none>]]
+    AddressAlign: [[ADDRALIGN=1]]
+    Address:      [[ADDRESS=<none>]]
+  - Name:         .sec
+    Type:         SHT_PROGBITS
+DWARF:
+  debug_str:
+    - a
+    - b
+    - abc
+
+#       COMMON: - Name:         .debug_str
+#  COMMON-NEXT:   Type:         SHT_[[TYPE]]
+#  COMMON-NEXT:   Flags:        [[FLAGS]]
+#    LINK-NEXT:   Link:         .sec
+# ADDRESS-NEXT:   Address:      0x000000000000[[ADDRESS]]
+#  COMMON-NEXT:   AddressAlign: 0x000000000000[[ADDRALIGN]]
+#  COMMON-NEXT:   EntSize:      0x000000000000[[ENTSIZE]]
+#    INFO-NEXT:   Info:         0x000000000000[[INFO]]
+#        BASIC: DWARF:
+#   BASIC-NEXT:   debug_str:
+#   BASIC-NEXT:     - a
+#   BASIC-NEXT:     - b
+#   BASIC-NEXT:     - abc
+#   BASIC-NEXT: ...

llvm/tools/obj2yaml/elf2yaml.cpp

if (S.Type == ELF::SHT_NULL && (&SHdr == &Sections[0])) {
     return std::find_if(Begin, End, [](uint8_t V) { return V != 0; }) != End;
   }
 
   // Normally we use "DWARF:" to describe contents of DWARF sections. Sometimes
   // the content of DWARF sections can be successfully parsed into the "DWARF:"
   // entry but their section headers may have special flags, entry size, address
   // alignment, etc. We will preserve the header for them under such
   // circumstances.
-  if (DWARF && DWARF->getNonEmptySectionNames().count(S.Name.substr(1))) {
+  StringRef SecName = S.Name.substr(1);
+  if (DWARF && DWARF->getNonEmptySectionNames().count(SecName)) {
     if (const ELFYAML::RawContentSection *RawSec =
-            dyn_cast<const ELFYAML::RawContentSection>(&S))
-      return RawSec->Type != ELF::SHT_PROGBITS || RawSec->Flags ||
-             !RawSec->Link.empty() || RawSec->Info ||
-             RawSec->AddressAlign != 1 || RawSec->EntSize;
+            dyn_cast<const ELFYAML::RawContentSection>(&S)) {
+      if (RawSec->Type != ELF::SHT_PROGBITS || !RawSec->Link.empty() ||
+          RawSec->Info || RawSec->AddressAlign != 1 || RawSec->Address)
+        return true;
+
+      if (SecName == "debug_str")
+        return !(RawSec->EntSize && (uint64_t)*RawSec->EntSize == 1) ||
+               RawSec->Flags.getValueOr(ELFYAML::ELF_SHF(0)) !=
+                   ELFYAML::ELF_SHF(ELF::SHF_MERGE | ELF::SHF_STRINGS);
+
+      return RawSec->EntSize || RawSec->Flags;
+    }
   }
 
   // Normally we use "Symbols:" and "DynamicSymbols:" to describe contents of
   // symbol tables. We also build and emit corresponding string tables
   // implicitly. But sometimes it is important to preserve positions and virtual
   // addresses of allocatable sections, e.g. for creating program headers.
   // Generally we are trying to reduce noise in the YAML output. Because
   // of that we do not print non-allocatable versions of such sections and
for (std::unique_ptr<ELFYAML::Chunk> &C : Sections) {
 
     if (ELFYAML::RawContentSection *RawSec =
             dyn_cast<ELFYAML::RawContentSection>(C.get())) {
       Error Err = Error::success();
       cantFail(std::move(Err));
 
       if (RawSec->Name == ".debug_aranges")
         Err = dumpDebugARanges(*DWARFCtx.get(), DWARF);
+      else if (RawSec->Name == ".debug_str")
+        dumpDebugStrings(*DWARFCtx.get(), DWARF);
 
       // If the DWARF section cannot be successfully parsed, emit raw content
       // instead of an entry in the DWARF section of the YAML.
       if (Err)
         consumeError(std::move(Err));
       else
         RawSec->Content.reset();
     }