[PATCH] D54867: [yaml2obj] Introduce symbol version section to yaml2obj

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm still not particularly familiar with yaml2obj, so my comments might not always make sense, so feel free to correct me.

On the two flag fields, it might be nice if the user can explicitly specify the flags they want setting (e.g. VER_FLG_BASE), as well as an arbitrary value.

I've not yet reviewed the writeSectionContent functions. I'll do those later.



================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:184
 
+struct VersionDefParent {
+  llvm::yaml::Hex64 Name;
----------------
Is this supposed to be the Elfxx_Verdaux struct? If so, it should probably be renamed to VersionDefAux, or similar.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:185
+struct VersionDefParent {
+  llvm::yaml::Hex64 Name;
+};
----------------
This should probably be a string, rather than a hex number, since it is just a name. yaml2obj should then automatically add it to a string table, referenced by the sh_link in the VerDef header.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:189
+struct VersionDef {
+  int32_t Revision;
+  llvm::yaml::Hex32 Flags;
----------------
Revision is an Elfxx_Half, so should be a uint16_t, right?


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:190-191
+  int32_t Revision;
+  llvm::yaml::Hex32 Flags;
+  llvm::yaml::Hex32 Index;
+  llvm::yaml::Hex64 Hash;
----------------
Ditto for these two fields. They should be Hex16.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:192
+  llvm::yaml::Hex32 Index;
+  llvm::yaml::Hex64 Hash;
+  llvm::yaml::Hex64 Name;
----------------
The is an Elfxx_Word, which is always 32-bits, so should be Hex32.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:193
+  llvm::yaml::Hex64 Hash;
+  llvm::yaml::Hex64 Name;
+  std::vector<VersionDefParent> Parents;
----------------
What's the purpose of this field? It doesn't seem to map to any of the struct's fields.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:197
+
+// Represents .gnu.version_d
+struct VersionDefSection : Section {
----------------
Maybe also worth mentioning the section type here, since technically SHT_GNU_verdef sections could be called other things.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:209-211
+  llvm::yaml::Hex64 Hash;
+  llvm::yaml::Hex32 Flags;
+  llvm::yaml::Hex32 Other;
----------------
These are all twice the size I'd expect them to be, like in the VersionDef struct above. Why?


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:212
+  llvm::yaml::Hex32 Other;
+  llvm::yaml::Hex64 Name;
+};
----------------
Similar to the VersionDefParent, this name should be a string.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:216-217
+struct VersionNeed {
+  int32_t Version;
+  llvm::yaml::Hex64 File;
+  std::vector<VersionNeedAux> Auxiliaries;
----------------
Similar to above, these are the wrong types.


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:221
+
+// Represents .gnu.version_r section
+struct VersionNeedSection : Section {
----------------
Similar to above, mention the section type here.


================
Comment at: test/tools/yaml2obj/symbol-version-section.yaml:74
+        Hash:        0x1234
+        Auxiliaries: 
+          - Name:    0x3e
----------------
Higuoxing wrote:
> According to  https://www.akkadia.org/drepper/symbol-versioning
> 
> >  vda_next: byte offset to the next Elfxx_Verdaux entry.  The first entry (pointed to by the Elfxx_Verdef entry, contains the actual defined name.  The second and all later entries name predecessor versions.
> 
> What if change `Auxiliaries` to
> 
> ```
> Name <Required>
> Predecessor/Parent <Optional> (Parent is used by `gnu-readelf` and `gnu-objdump`)
> ```
Sorry, I'm not sure I understand this comment.


================
Comment at: test/tools/yaml2obj/symbol-version-section.yaml:57
+  Data:              ELFDATA2LSB
+  Type:              ET_REL
+  Machine:           EM_X86_64
----------------
Nit: This should be ET_DYN or ET_EXEC, since it's got dynamic information.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:235-243
+static bool checkSectionIndex(NameToIdxMap &SX2I, StringRef SecName,
+                              StringRef SecField, unsigned &Index) {
+  if (SX2I.lookup(SecField, Index) && !to_integer(SecField, Index)) {
+    WithColor::error() << "Unknown section referenced: '" << SecField
+                       << "' at YAML section '" << SecName << "'.\n";
+    return false;
+  }
----------------
Thanks for the refactor. Given the growing size of this change, would you mind doing it in a separate change and first, please, to reduce the complexity of this change?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54867