This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] - Teach llvm-readobj to print dependencies of SHT_GNU_verdef and refactor dumping method.
ClosedPublic

Authored by grimar on Jun 21 2016, 4:35 AM.

Details

Summary

This patch changes single method of llvm-readobj.
It teaches SHT_GNU_verdef dumper to print version dependencies,
also it removes few fields from output that can be dumped with other keys
and slightly refactors code.
Testcase was also modified to match the changes.
Change is required for testcases of upcoming lld patches.

Diff Detail

Event Timeline

grimar updated this revision to Diff 61354.Jun 21 2016, 4:35 AM
grimar retitled this revision from to [ELF] - Teach llvm-readobj to print dependencies of SHT_GNU_verdef and refactor dumping method..
grimar updated this object.
grimar added reviewers: rafael, davide.
grimar added subscribers: llvm-commits, grimar.
davide added inline comments.Jun 21 2016, 11:17 AM
tools/llvm-readobj/ELFDumper.cpp
571

How many dependencies can you have?
IIRC you can have only one immediate parent/dependency so it's probably easier/more reasonable to only print that, no?
If you can have multiple dependencies, instead, can you please add a test exercising that behaviour?

rafael added inline comments.Jun 21 2016, 12:23 PM
tools/llvm-readobj/ELFDumper.cpp
570

The "spec" used the name "predecessor versions". Maybe this should be "Predecessors". I assume that if you have

foo {
} bar;

bar {
} zed;

it will chain them?

If that is the case, maybe it would be better to print just the first predecessor?

In any case, I agree with Davide, we need a test :-)

ruiu added a subscriber: ruiu.Jun 21 2016, 4:04 PM
ruiu added inline comments.
test/tools/llvm-readobj/elf-versioninfo.test
58

Is it a common practice for llvm-dump* tools to print out an empty set {} if an element is missing? An alternative way is to not print out Dependencies {} if a version definition has no dependencies.

grimar added inline comments.Jun 22 2016, 12:25 AM
test/tools/llvm-readobj/elf-versioninfo.test
58

I think it is common, for example we often have relocations empty in testcases:

Relocations [
]

I am going to change this in according to my answers to Rafael and Davide.

tools/llvm-readobj/ELFDumper.cpp
570

No, it will not chain. Foo will have [foo, bar], where bar is dependency. Bar will have [bar, zed].
About terminology, I used sun docs: https://docs.oracle.com/cd/E19120-01/open.solaris/819-0690/chapter6-61174/index.html, it says "version dependencies".

And like I wrote in my answer to Davide, I was unable to get more than one "dependency", so looks "predecessor" word is more correct here.
So I plan to use it and print predecessor without a loop.

571

I did not found how to get more that one dependency here. I also think that it can be only one now. But spec says: "At least one version dependency must exist. Additional version dependencies can be present, the number being indicated by the vn_cnt value.".
So initially I implemented it in a more generic way, basing on what it says.
I am ok to print only one since it looks to be the only case and it is more "predecessor" than "dependency".

grimar added inline comments.Jun 22 2016, 1:10 AM
tools/llvm-readobj/ELFDumper.cpp
570

Ah, I was mean: "yes, it will chain" zed<-bar<-foo, but will not add zed as a "dependency" for bar.

grimar added inline comments.Jun 22 2016, 1:14 AM
tools/llvm-readobj/ELFDumper.cpp
570

*for foo
sorry :)

grimar updated this revision to Diff 61513.Jun 22 2016, 1:33 AM
  • Addressed review comments.
rafael accepted this revision.Jun 22 2016, 6:27 AM
rafael edited edge metadata.

LGTM with nits.

test/tools/llvm-readobj/elf-versioninfo.test
102

Count is redundant, no? It is always 1 or 2 and being 2 is visible by Predecessor being present.

tools/llvm-readobj/ELFDumper.cpp
572

Produce an error if is > 2.

This revision is now accepted and ready to land.Jun 22 2016, 6:27 AM
grimar added inline comments.Jun 22 2016, 6:28 AM
tools/llvm-readobj/ELFDumper.cpp
572

I do that above :)

grimar retitled this revision from [ELF] - Teach llvm-readobj to print dependencies of SHT_GNU_verdef and refactor dumping method. to [llvm-readobj] - Teach llvm-readobj to print dependencies of SHT_GNU_verdef and refactor dumping method..Jun 22 2016, 6:48 AM
grimar edited edge metadata.
This revision was automatically updated to reflect the committed changes.
grimar marked 2 inline comments as done.
davide edited edge metadata.Jun 22 2016, 10:52 AM

lg, thanks.

Higuoxing added inline comments.
llvm/trunk/tools/llvm-readobj/ELFDumper.cpp
569 ↗(On Diff #61543)

Hi, I am curious about why the Parents/Predecessor more than one. According to https://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.

Feel free to ignore me :) just out of curiosity

Higuoxing added inline comments.Nov 30 2018, 6:18 AM
llvm/trunk/tools/llvm-readobj/ELFDumper.cpp
569 ↗(On Diff #61543)

Besides, gnu-readelf could print multiple parents

Higuoxing added inline comments.Nov 30 2018, 6:26 AM
llvm/trunk/tools/llvm-readobj/ELFDumper.cpp
569 ↗(On Diff #61543)

Sorry, I am curious about why the Parents/Predecessor *cannot be* more than one.

You could use my patch to make a simple test, if needed

https://reviews.llvm.org/D54867

feel free to ignore me :)

grimar marked an inline comment as done.Nov 30 2018, 6:32 AM
grimar added inline comments.
llvm/trunk/tools/llvm-readobj/ELFDumper.cpp
569 ↗(On Diff #61543)

Hi! You can find some answers in the comments for this patch, starting from this one:
https://reviews.llvm.org/D21552?id=61354#inline-182645

In short - I was unable to produce the output that has more than one predecessor,
so having the code for that was unuseful and we stopped on this version of code that time.
It still seems reasonable nowadays, I think.

Higuoxing added inline comments.Nov 30 2018, 6:35 AM
llvm/trunk/tools/llvm-readobj/ELFDumper.cpp
569 ↗(On Diff #61543)

Oh, sorry for bothering, thank you very much!