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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with two nits.



================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:343
+  else
+    Header.e_shnum = Doc.SectionHeader->Sections.size() + /*Null section*/ 1;
+
----------------
MaskRay wrote:
> grimar wrote:
> > 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
> > ```
> > 
> > 
> Making the SHT_NULL section explicit in `SectionHeaderTable` seems fine, but how do we encode it?
> 
> The SHT_NULL section in `Sections` remains on-demand implicit.
Let's leave it for a future extension for when the need arises, with the suggested explicit syntax.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1483
+
+  for (auto& It : Seen)
+    reportError("section header contains undefined section '" + It.getKey() + "'");
----------------
Formatting here looks off to me? Also, why not `const &`?

(I think @MaskRay was suggesting `const auto &It` with a space in the right place)


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


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

https://reviews.llvm.org/D80002