This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by grimar on Feb 12 2019, 7:22 AM.

Details

Summary

The section is described here:
https://refspecs.linuxfoundation.org/LSB_1.3.0/gLSB/gLSB/symverrqmts.html

Patch just teaches obj2yaml/yaml2obj to dump and parse such sections.

We did the finalization of string tables very late,
and I had to move the logic to make it a bit earlier.
That was needed in this patch since .gnu.version_r adds strings to .dynstr.
This also probably might be useful for implementing other special sections.

Everything else changed in this patch seems to be straightforward.

Diff Detail

Event Timeline

grimar created this revision.Feb 12 2019, 7:22 AM
grimar updated this revision to Diff 187194.Feb 18 2019, 12:31 AM
  • Renamed the test cases, rebased.
grimar updated this revision to Diff 187195.Feb 18 2019, 1:33 AM
  • Corrected the types in VernauxEntry/VerneedEntry structs.

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.

include/llvm/ObjectYAML/ELFYAML.h
186

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).

417

Need blank line after this one.

test/tools/obj2yaml/verneed-section.yaml
60

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.

test/tools/yaml2obj/verneed-section.yaml
64

Same comments as the other test.

tools/obj2yaml/elf2yaml.cpp
481

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.

493–494

Ditto.

grimar edited the summary of this revision. (Show Details)Feb 19 2019, 12:44 AM
grimar marked an inline comment as done.

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

I hope you're not adding any strings to .dynsym ;-)

That would be interesting behavior though.. Thanks! :)

include/llvm/ObjectYAML/ELFYAML.h
186

Its mentioned in a general spec about sh_info/sh_link fields for sections,
see "Table 7-15 ELF sh_link and sh_info Interpretation":
https://docs.oracle.com/cd/E19683-01/816-1386/chapter6-94076/index.html
sh_info contains "The number of version dependencies within the section. ".

Both GNU and LLD linkers set this field.

jhenderson added inline comments.Feb 19 2019, 1:39 AM
include/llvm/ObjectYAML/ELFYAML.h
186

Ah, okay. It's not listed in the description of the specific section, unlike the sh_link field, which would have made much more sense...

grimar updated this revision to Diff 187330.Feb 19 2019, 2:05 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.
test/tools/obj2yaml/verneed-section.yaml
60

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.

tools/obj2yaml/elf2yaml.cpp
481

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.

jhenderson accepted this revision.Feb 19 2019, 2:13 AM

LGTM.

test/tools/obj2yaml/verneed-section.yaml
60

Ah, I see your point. In that case, I think the user should use the Content field (assuming that's allowed) instead of the explicit fields, because it is unclear what fields should exist. The version field explicitly exists to mark different formats for the section, so explicit fields may not make that much sense.

This revision is now accepted and ready to land.Feb 19 2019, 2:13 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 6:02 AM