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

[George Rimar via Phabricator via llvm-commits]

grimar 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();
 }
----------------
jhenderson wrote:
> 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)`)
Done.


================
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 {};
----------------
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.


================
Comment at: llvm/test/tools/yaml2obj/ELF/nobits.yaml:28
+
+# CONTENT-ERR: error: SHT_NOBITS section cannot have "Content"
+
----------------
jhenderson wrote:
> Would be nice to have the section name in this message, if available.
Not possible to create craft a message on the fly atm, because `validate` returns `StringRef`:

```
StringRef MappingTraits<std::unique_ptr<ELFYAML::Chunk>>::validate
```

Though we report an exact place and reveal the section name currently already:

```
# command output:
YAML:17:5: error: SHT_NOBITS section cannot have "Content"
  - Name:    .nobits
    ^
yaml2obj: error: failed to parse YAML input: invalid argument
```


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

I don't think so. The following will not work:

```
Symbols:
  [[SYM1=- Name: foo]]
  [[SYM2=- Name: bar]]
```

because for the last case we need to create an empty (size == 0, or at least size < size of a one symbol) `.symtab`
It is needed to pass llvm-readelf validation:

```
  uint64_t Syms = SymTable.sh_size / sizeof(Elf_Sym);
  if (V.size() != Syms)
    return createError("SHT_SYMTAB_SHNDX has " + Twine(V.size()) +
                       " entries, but the symbol table associated has " +
                       Twine(Syms));
```



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

https://reviews.llvm.org/D89039