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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:320
+  else
+    Header.e_shentsize = 0;
+
----------------
jhenderson wrote:
> 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).
OK. I've reverted this piece.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:339
+  else if (!Doc.SectionHeader)
+    Header.e_shnum = Doc.getSections().size();
+  else if (Doc.SectionHeader->Sections.empty())
----------------
jhenderson wrote:
> I take it `Doc.getSections()` includes a null section header already?
Yes. It is either defined explicitly in a YAML or we implicilty add it much earlier:

```
template <class ELFT>
ELFState<ELFT>::ELFState(ELFYAML::Object &D, yaml::ErrorHandler EH)
    : Doc(D), ErrHandler(EH) {
  std::vector<ELFYAML::Section *> Sections = Doc.getSections();
  // Insert SHT_NULL section implicitly when it is not defined in YAML.
  if (Sections.empty() || Sections.front()->Type != ELF::SHT_NULL)
    Doc.Chunks.insert(
        Doc.Chunks.begin(),
        std::make_unique<ELFYAML::Section>(
            ELFYAML::Chunk::ChunkKind::RawContent, /*IsImplicit=*/true));
...
```


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:343
+  else
+    Header.e_shnum = Doc.SectionHeader->Sections.size() + /*Null section*/ 1;
+
----------------
jhenderson wrote:
> 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.
> This allows us to omit that section header entirely, or to have just a section header table containing only the null shdr.

Is it useful? (I am not sure I can imagine a use case except creating of a specific broken object,
though it should be possible to create such using `SHOff` even right now. I.e. `SHOff` could point to a hand crafted table. Or just to the first header directly, to skip the null one. I agree that is not that straightforward though).

The current implementation is that `SectionHeaderTable` must list all sections (null section is an exception). I think it is a nice propery,
because it seems can be easy to forget to mention a section and omit it by mistake. E.g. in this patch I've to add implicit sections like ".shstrtab" and ".strtab" manually.

Perhaps, we might introduce a new explicit syntax to describe omitted sections if you want to have such feature. E.g:

```
SectionHeaderTable:
  Sections:
  ...
  Excluded:
    - foo
    - bar
```

> I'm wondering whether we should make the null section explicit

My concern is that we'll have an unnecessary noise in most of the test cases that use `SectionHeaderTable` then. Usually
test cases do not need to do anything with the `SHT_NULL` section. Perhaps, the new explicit syntax might help here too.
Like:

```
SectionHeaderTable:
  CreateImplicitNullSection: false
```




================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1457
+  for (const ELFYAML::SecHdr &Hdr : Doc.SectionHeader->Sections) {
+    if (!Ret.insert({Hdr.Name, ++SecNdx}).second)
+      reportError("repeated section name: '" + Hdr.Name +
----------------
MaskRay wrote:
> try_emplace can remove `{}`
Nice!


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1463
+
+  for (const ELFYAML::Section *S : Doc.getSections()) {
+    // Ignore special first SHT_NULL section.
----------------
MaskRay wrote:
> Use `ArrayRef::slice` to skip SHT_NULL.
`getSections()` returns `std::vector`..


================
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.
----------------
jhenderson wrote:
> 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.
I usually also do in this way I think. Not sure why did differently that time. Fixed.


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

https://reviews.llvm.org/D80002