[PATCH] D81005: [yaml2obj] - Add a way to exclude specified sections from the section header.

Wed Jun 3 01:36:06 PDT 2020

jhenderson added a comment.

Perhaps worth an additional test case for excluded sections linking to other excluded sections.

================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:131

+  llvm::SetVector<StringRef> ExcludedSectionHeaders;
+
----------------
I might have missed something, but what's the benefit of making this a `SetVector` rather than some form of `unordered_map`?

================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:523
+uint64_t ELFState<ELFT>::getSectionNameOffset(StringRef Name) {
+  // If a section is excluded from section headers, we do not save it's name in
+  // the string table.
----------------
it's -> its

(it's == it is, its == belonging to it)

================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1618
       reportError("section '" + S->Name +
-                  "' should be present in the 'Sections' list");
+                  "' should be present in the 'Sections' or `Excluded` lists");
     Seen.erase(S->Name);
----------------
"Sections" and "Excluded" are using different markers for their formatting.

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:2
+## Check how we can use the "Excluded" key of the "SectionHeaderTable" tag to exclude
+## entries from the section headers.
+
----------------
Perhaps "from the section header table."

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:4
+
+## Check we can use the "Excluded" key to omit a section from the sections header table.
+## Check we do not include the name of the excluded section to the sections string table.
----------------
sections header table -> section header table

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:5
+## Check we can use the "Excluded" key to omit a section from the sections header table.
+## Check we do not include the name of the excluded section to the sections string table.
+# RUN: yaml2obj %s -DINCLUDED=.foo -DEXCLUDED=.bar --docnum=1 -o %t1
----------------
include ... to the -> include ... in the

I'd probably just delete the "sections" from "sections string table". I think it's fairly obvious what we're referring to here.

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:7
+# RUN: yaml2obj %s -DINCLUDED=.foo -DEXCLUDED=.bar --docnum=1 -o %t1
+# RUN: llvm-readelf --section-headers -x .shstrtab %t1 | \
+# RUN:   FileCheck %s -DSEC=.foo --check-prefixes=INCLUDE-SEC,INCLUDE-FOO
----------------
Use `-p .shstrtab` for a string dump of the string table here and below. I think that would be cleaner.

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:13-16
+# INCLUDE-SEC:      [Nr] Name      Type     Address          Off    Size   ES Flg Lk Inf Al
+# INCLUDE-SEC:      [ 1] [[SEC]]   PROGBITS 0000000000000000 000040 000000 00 0   0  0
+# INCLUDE-SEC-NEXT: [ 2] .strtab   STRTAB   0000000000000000 000040 000001 00 0   0  1
+# INCLUDE-SEC-NEXT: [ 3] .shstrtab STRTAB   0000000000000000 000041 000018 00 0   0  1
----------------
Can we get rid of the ES and onwards columns? I'm not sure they add much value (but could be persuaded otherwise).

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:42
+
+## Check we report an error when a section is in the both "Sections" and "Excluded" lists at the same time.
+# RUN: not yaml2obj %s -DINCLUDED=.bar -DEXCLUDED=.strtab --docnum=1 -o /dev/null 2>&1 | \
----------------
the both -> both the

Perhaps also worth mentioning: "Also check that we report an error if a section is missing from the lists." or something like that.

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:77
+## Check we report an error when the `Excluded` key is used, but an empty
+## section header is requested.
+# RUN: not yaml2obj %s --docnum=3 -o /dev/null 2>&1 | FileCheck %s --check-prefix=NO-SECTIONS
----------------
section header -> section header table (?)

Assuming I've understood this correctly, why should this be an error? Perhaps a user wants to omit ALL section headers except the implicit null one?

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:104
+
+## Check how we handle cases when a section is excluded, but it's section index is needed.
+## The general rule is: when a section is explicitly linked with another section, which is
----------------
it's -> its

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:109
+
+## Case A: check we report an error when a regular section has the Link field which
+##         points to an excluded section.
----------------
Here and below: "has the Link" -> "has a Link"

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:113-114
+# RUN:   FileCheck %s --check-prefix=LINK -DSEC=.foo -DTARGET=.bar
+# RUN: not yaml2obj %s --docnum=5 -DINCLUDED=.bar -DEXCLUDED=.foo -o /dev/null 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=LINK -DSEC=.bar -DTARGET=.foo
+
----------------
Do we really need both cases? It seems unnecessary to me.

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:139
+
+## Case B.1: check we report an error when a symbol table section has the Link field which
+##           points to an excluded section.
----------------
We probably want a SHT_DYNSYM version of this same case.

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:215
+
+## Case C: check we report an error when a debug section has the Link field which
+##         points to an excluded section.
----------------
What's the purpose of the debug section case?

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:237
+
+## Case D.1: check we do not link the SHT_REL[A] section with .symtab implicitly
+##           when the latter one is excluded.
----------------
We probably want a case for an explicit .symtab reference when .symtab has been excluded.

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:245
+
+## Case D.2: check we report an error when a relocatable section has the Info field which
+##           points to an excluded section.
----------------
has the -> has a

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:271
+
+## Case C: check we report an error when a symbol references an excluded section.
+# RUN: not yaml2obj %s --docnum=11 -o /dev/null 2>&1 | \
----------------
Case E?

Similar issues below.

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:275
+
+# SYMBOL-SECTION: error: excluded section referenced: '.foo' by YAML symbol 'foo'
+
----------------
We probably don't need the "YAML" part of this message.

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:321-322
+
+## Case D.2: check we do not link the SHT_GROUP section with .symtab
+##           implicitly when the latter one is excluded.
+# RUN: yaml2obj %s --docnum=13 -o %t9
----------------
Similar to above, we also probably want a test case for an explicit Link reference to an excluded section.

================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers-exclude.yaml:349-350
+
+## Case E: check we do not link SHT_LLVM_CALL_GRAPH_PROFILE/SHT_LLVM_ADDRSIG sections
+##         with .symtab implicitly when the latter one is excluded.
+# RUN: yaml2obj %s --docnum=14 -o %t10
----------------
Similar to above, we also probably want a test case for an explicit Link references to an excluded section.

Could all these implicit cases linking to .symtab be folded together into one?

Here and below: "the latter one" -> "the latter" (feels more natural to me).

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

https://reviews.llvm.org/D81005