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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml:28
+
+# RUN: yaml2obj --docnum=2 -DTYPE=STRTAB %s | obj2yaml | \
+# RUN:   FileCheck %s --check-prefixes=SHDR,TYPE,FLAGS,ADDRALIGN,ENTSIZE \
----------------
I'd add a short comment for each of these sub-cases to make it clear what they want to test.


================
Comment at: llvm/test/tools/obj2yaml/ELF/DWARF/debug-str.yaml:33
+# RUN: yaml2obj --docnum=2 -DFLAGS=[SHF_ALLOC] %s | obj2yaml | \
+# RUN:   FileCheck %s --check-prefixes=SHDR,TYPE,FLAGS,ADDRALIGN,ENTSIZE \
+# RUN:     -DTYPE=PROGBITS -DFLAGS="[ SHF_ALLOC ]" -DADDRALIGN=0001 -DENTSIZE=0001
----------------
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`?


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


================
Comment at: llvm/tools/obj2yaml/elf2yaml.cpp:213
+                   ELFYAML::ELF_SHF(ELF::SHF_MERGE | ELF::SHF_STRINGS);
+      return !CommonProperties || RawSec->EntSize || RawSec->Flags;
+    }
----------------
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;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86867