[PATCH] D89039: [yaml2obj/obj2yaml] - Add support of 'Size' and 'Content' keys for all sections.

Tue Oct 13 01:23:08 PDT 2020

jhenderson added inline comments.

================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1633
   }
-  if (Section.Content)
-    CBA.writeAsBinary(*Section.Content);
+  SHeader.sh_size = 2 * sizeof(uintX_t) * Section.Entries->size();
 }
----------------
grimar wrote:
> jhenderson wrote:
> > Perhaps that? I feel like this line and the earlier sh_entsize should match.
> Honestly I'd slightly prefer to keep `uintX_t`, because we write 2 `uintX_t` when do `CBA.write` right above.
Could you change the line setting sh_entsize then to match? (i.e. `sh_entsize = 2 * sizeof(uintX_t)`)

================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1516-1517
   if (const auto *Sec = dyn_cast<ELFYAML::LinkerOptionsSection>(C.get())) {
-    if (Sec->Options && Sec->Content)
-      return "\"Options\" and \"Content\" can't be used together";
+    if ((Sec->Content || Sec->Size) && Sec->Options)
+      return "\"Options\" cannot be used with \"Content\" or \"Size\"";
     return {};
----------------
grimar wrote:
> jhenderson wrote:
> > I've been thinking, and I think there might be a way to avoid duplicating this sort of logic for each section with a specific set of entries or similar fields.
> > 
> > # Add a class that acts as a parent class for all Entry kinds (e.g. Options in this case). It would have a Name field (or virtual function to retrieve it).
> > # Entry subclasses would set the Name/implement getName to say e.g. "Options", and would define the relevant data structure (presumably a vector) to contain the actual entries.
> > # In the Section class, have a getEntries method, which is implemented in the Section subclasses, with a default return of an empty `vector<vector<Entry>*>`. Having a vector of vector pointers would allow for more than one Entry kind per subclass (e.g. for hash tables).
> > # In the Section subclasses that have Entries (or equivalents), define getEntries to point to their own vector(s) of Entries.
> > # Call getEntries around about here, and report if any of the vectors returned are non-empty when Content or Size is specified.
> > 
> > This pattern might even be expandable to do the writing etc of the entries in a generic way, but that's probably not necessary. What do you think?
> Honestly, I am not sure. This probably needs an experiment to say how it will look like.
> The problems/concerns I see:
> 
> 1) `validate` returns `StringRef` currently. To build a error message we will probably need to switch to `std::string`.
> Alternatively, perhaps it is might be OK to have a static StringSaver local variable.
> 
> 2) It adds some complexity, but the benefit is not that large. I probably would prefer a bit simpler approach (without adding a base class for all Entry kinds etc). E.g. we could probably add
> a `virtual std::vector<StringRef> getEntriesNames() = 0` to `Sections` class. This method could return an empty
> vector when there are no entries and names of existent ones otherwise.
> 
> Then we should be able to do something like this:
> 
> ```
> std::vector<StringRef> Entries = Sec.getEntriesNames();
> if ((Sec.Content || Sec.Size) && !Entries .empty())
>       return buildContentSizeVsEntriesErrorMessage(Entries);
> ```
> 
> Such approach needs only a one (and usually trivial) method per section type and probably nothing else. Given that this change
> is probably only usefull to simplify/reduce the code in `validate()`, I think it is might be enough.
Yes, it sounds like it might be. Worth an experiment to see if your suggestion looks good (I think it sounds okay to me). You could probably have `getEntriesNames()` return an empty vector by default, if there are any cases that don't have any entries ever. Not sure if there are though...!

================
Comment at: llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml:211
+
+## Check we can use "Content" key with "Size" key when the size is greater
+## than or equal to the content size.
----------------
Same applies in many places.

================
Comment at: llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml:268
+
+## Check we can't use "Entries" key together with "Content" or "Size" keys.
+
----------------
Same applies in many places.

================
Comment at: llvm/test/tools/yaml2obj/ELF/comdat.yaml:1-2
+## Check how "Content", "Size" and "Members" keys can be used to
+## describe a content for group sections.
+
----------------
Same elsewhere.

Also, maybe this should be called `group.yaml`, since COMDATs are just one style of groups.

================
Comment at: llvm/test/tools/yaml2obj/ELF/comdat.yaml:70
+
+## Check we create an empty section when neither "Size", "Content" nor "Members" are specified.
+
----------------
Same elsewhere

================
Comment at: llvm/test/tools/yaml2obj/ELF/nobits.yaml:10
+# CHECK:      [ 1] .nobits NOBITS 0000000000000000 000040 000003 00      0   0  0
+# CHECK-NEXT: [ 2] .strtab STRTAB 0000000000000000 000040 000001 00      0   0  1
+
----------------
I don't think you need to check the .strtab line. It has no relevance to the test.

================
Comment at: llvm/test/tools/yaml2obj/ELF/nobits.yaml:28
+
+# CONTENT-ERR: error: SHT_NOBITS section cannot have "Content"
+
----------------
Would be nice to have the section name in this message, if available.

================
Comment at: llvm/test/tools/yaml2obj/ELF/sht-symtab-shndx.yaml:147
+    Type: SHT_SYMTAB
+    Size: [[SYMTAB=0x24]]
+
----------------
Probably worth a comment saying why we need to have content in the symbol table. Also, can we just have a `Symbols:` tag?

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

https://reviews.llvm.org/D89039