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

[James Henderson via Phabricator via llvm-commits]

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

Looks good, once remaining typos/grammar errors fixed.



================
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:
> > 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...!
> I'll try to implement this, but at first I believe the following cleanup should be done (I've briefly mentioned it earlier):
> there are few sections, e.g. `StackSizesSection`, `HashSection`, `AddrsigSection`, `NoteSection`, `GnuHashSection`,
> that don't support being just empty:
> 
> ```
>      if (!NS->Content && !NS->Size && !NS->Notes)
>        return "one of \"Content\", \"Size\" or \"Notes\" must be "
>               "specified";
> ```
> 
> To generalize error reporting properly, we should allow them to be first.
Yes, of course.


================
Comment at: llvm/test/tools/yaml2obj/ELF/nobits.yaml:29
+
+## Check we create an empty section when none of "Size" or "Content" are specified.
+
----------------
Actually, here it should be "neither ... nor ..." - for two cases that sounds fine to me. I wouldn't use neither/nor for the case where there are 3 or more possibilities.


================
Comment at: llvm/test/tools/yaml2obj/ELF/sht-symtab-shndx.yaml:148
+## llvm-readobj validates that SHT_SYMTAB_SHNDX section has the same number
+## of entries, that the symbol table associated has.
+    Size: [[SYMTAB=0x24]]
----------------



================
Comment at: llvm/test/tools/yaml2obj/ELF/versym-section.yaml:154
+
+## Check we can't use the the "Entries" key together with the "Content" or "Size" keys.
+
----------------



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

https://reviews.llvm.org/D89039