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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/ELFEmitter.cpp:1308
+
+  if (Section.Entries) {
+    for (uint16_t Version : *Section.Entries)
----------------
Maybe am early return instead to be more consistent with other sections?


================
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();
 }
----------------
Perhaps that? I feel like this line and the earlier sh_entsize should match.


================
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 {};
+  }
----------------
I'm struggling to follow why this bit has been added. Could you clarify please?


================
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";
 
----------------
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.


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


================
Comment at: llvm/test/tools/yaml2obj/ELF/call-graph-profile-section.yaml:71
 ## Check we can set arbitrary sh_link and sh_entsize values.
-## Check we can specify neither "Content" nor "Entries" tags.
+## Check we can specify neither "Content" nor "Entries" not "Size" tags.
 # RUN: yaml2obj --docnum=2 %s -o %t.link
----------------



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

https://reviews.llvm.org/D89039