[PATCH] D58119: [obj2yaml][yaml2obj] - Add support of parsing/dumping of the .gnu.version_r section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

> That was needed in this patch since .gnu.version_r adds strings to .dynsym.

I hope you're not adding any strings to .dynsym ;-) Also, your link in the description points to verdef sections, rather than verneed.



================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:186
+  std::vector<VerneedEntry> VerneedV;
+  llvm::yaml::Hex64 Info;
+
----------------
`Info`? This section doesn't have anything to do with sh_info according to the specification (it uses sh_link to point at its string table).


================
Comment at: include/llvm/ObjectYAML/ELFYAML.h:417
+  static void mapping(IO &IO, ELFYAML::VernauxEntry &E);
+};
 template <> struct MappingTraits<ELFYAML::Relocation> {
----------------
Need blank line after this one.


================
Comment at: test/tools/obj2yaml/verneed-section.yaml:60
+            Other:           4
+      - Version:         2
+        File:            dso.so.1
----------------
I'm not sure we should be testing version 2, because the structure will change between versions (otherwise it would still be version 1). Indeed, this could cause the whole yaml2obj design to fall apart! I think a second Version 1 dependency would be better.


================
Comment at: test/tools/yaml2obj/verneed-section.yaml:64
+            Other:           4
+      - Version:         2
+        File:            dso.so.1
----------------
Same comments as the other test.


================
Comment at: tools/obj2yaml/elf2yaml.cpp:481
+  while (Buf) {
+    S->VerneedV.resize(S->VerneedV.size() + 1);
+    ELFYAML::VerneedEntry &Entry = S->VerneedV.back();
----------------
Rather than resizing to default construct an entry, then fetching and updating it, could you not simply make a new local variable, set its fields and use `push_back`/`emplace_back`? That would be more natural, I think.


================
Comment at: tools/obj2yaml/elf2yaml.cpp:493-494
+          reinterpret_cast<const Elf_Vernaux *>(BufAux);
+      Entry.AuxV.resize(Entry.AuxV.size() + 1);
+      ELFYAML::VernauxEntry &Aux = Entry.AuxV.back();
+
----------------
Ditto.


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

https://reviews.llvm.org/D58119