[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:
> 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.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1423-1427
+  if (const auto *F = dyn_cast<ELFYAML::Fill>(C.get())) {
+    if (F->Pattern && F->Pattern->binary_size() != 0 && !F->Size)
+      return "\"Size\" can't be 0 when \"Pattern\" is not empty";
+    return {};
+  }
----------------
jhenderson wrote:
> I'm struggling to follow why this bit has been added. Could you clarify please?
It was not added, but moved. `Fill` is special, because it is not a `Section`, but a `Chunk`.
This is the only non-Section type that can be in the `Sections` list in a YAML, so I am handling it first
to be able to do `const ELFYAML::Section &Sec = *cast<ELFYAML::Section>(C.get());` below.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1441-1442
   if (const auto *SS = dyn_cast<ELFYAML::StackSizesSection>(C.get())) {
     if (!SS->Entries && !SS->Content && !SS->Size)
       return ".stack_sizes: one of Content, Entries and Size must be specified";
 
----------------
jhenderson wrote:
> These sort of checks can probably be deleted in a separate patch. I think defaulting to an empty section when nothing is specified makes sense.
Right. That was my plan.

Also, I think, there is no need to handle `Size` and `Content` separatelly and mention the `.stack_sizes` in messages (we report an exact error place anyways currently).


================
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:
> 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.


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

https://reviews.llvm.org/D89039