[PATCH] D80002: [yaml2obj] - Implement the "SectionHeaderTable" tag.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Mostly looks good, but a number of small comments from me.



================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:88
 
+struct SecHdr {
+  StringRef Name;
----------------
If there's nothing stopping it, I'd prefer to make this `SectionHeader` (in commong with `FileHeader` and `ProgramHeader`). Variable names could then be changed to `SecHdr` where necessary.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:520
   FileHeader Header;
+  Optional<SectionHeaderTable> SectionHeader;
   std::vector<ProgramHeader> ProgramHeaders;
----------------
`SectionHeaders`


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:320
+  else
+    Header.e_shentsize = 0;
+
----------------
I'm not convinced we should do this. This field doesn't change according to the ELF gABI when there are no section headers (unlike e_shnum and e_shoff).


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:331
+    Header.e_shoff = SHOff;
+  else if (Doc.SectionHeader->Sections.empty())
+    Header.e_shoff = 0;
----------------
Maybe change this to `else if (Doc.SectionHeader && Doc.SectionHeader->sections.empty())` and then get rid of the `(!Doc.SectionHeader)` clause, since it has the same effect as the final `else`.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:339
+  else if (!Doc.SectionHeader)
+    Header.e_shnum = Doc.getSections().size();
+  else if (Doc.SectionHeader->Sections.empty())
----------------
I take it `Doc.getSections()` includes a null section header already?


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:343
+  else
+    Header.e_shnum = Doc.SectionHeader->Sections.size() + /*Null section*/ 1;
+
----------------
I'm wondering whether we should make the null section explicit in the `SectionHeaderTable` block. This allows us to omit that section header entirely, or to have just a section header table containing only the null shdr.


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers.yaml:61
+##  a) contains a repeated section name.
+##  b) does not list any of existent section(s).
+##  c) contains a undefined section.
----------------
I'm not sure what this message is trying to say, but I'm assuming it's "omits any section that exists." which I think would be a clearer way of phrasing.


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers.yaml:62
+##  b) does not list any of existent section(s).
+##  c) contains a undefined section.
+# RUN: not yaml2obj %s -o /dev/null -DSEC1=".section.foo" -DSEC2="unknown" -DSEC3=".section.foo" 2>&1 \
----------------
Rather than calling the section "undefined", which implies something to do with SHN_UNDEF, let's go with "non-existent".


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers.yaml:63-64
+##  c) contains a undefined section.
+# RUN: not yaml2obj %s -o /dev/null -DSEC1=".section.foo" -DSEC2="unknown" -DSEC3=".section.foo" 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=ERR1
+#   d) contains a repeated implicit section name.
----------------
I don't feel strongly about this, so if you prefer this way, that's fine, but I personally prefer the first line to end with `| \` and the second line to be indented slightly, i.e.

```
# RUN: not yaml2obj ... | \
# RUN:   FileCheck ...
```

The reasoning is 1) the '|' shows the command is done, and 2), the indentation shows it's a continuation implicitly.


================
Comment at: llvm/test/tools/yaml2obj/ELF/section-headers.yaml:105
+## Test that we are still able to override e_shentsize, e_shoff,
+## e_shnum and e_shstrndx fields even when do not produce the section header.
+# RUN: yaml2obj %s --docnum=3 -o %t4
----------------
"section header" -> "section header table" (or possibly just "section headers")


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

https://reviews.llvm.org/D80002