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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/tools/obj2yaml/verneed-section.yaml:60
+            Other:           4
+      - Version:         2
+        File:            dso.so.1
----------------
jhenderson wrote:
> 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.
Did that, but FTR my vision of the problem is a bit different.
yaml2obj is a convenient tool for creating outputs and also test cases. For the latter sometimes
we want to create broken outputs, i.e. ones that are possible to produce only with a hex editing
the binaries normally.

I did not have plans to write a test for versions other than 1 (though as far I can see, we do
not check the version field when parsing inputs in LLD atm, and hence have no test),
but generally, we should probably keep in mind that might want to produce invalid outputs and yaml2obj should
allow doing that probably.

Anyways, I am ok to use version 1 here, as it was not the main aim of this patch to allow changing it.




================
Comment at: tools/obj2yaml/elf2yaml.cpp:481
+  while (Buf) {
+    S->VerneedV.resize(S->VerneedV.size() + 1);
+    ELFYAML::VerneedEntry &Entry = S->VerneedV.back();
----------------
jhenderson wrote:
> 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.
OK, that is not a style invented by me though. I think LLVM code use it sometimes.
It can be faster because allows avoiding expensive copying of members, like `Entry.AuxV` vector here.
Though in this particular place is not very important I think.


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

https://reviews.llvm.org/D58119