This is an archive of the discontinued LLVM Phabricator instance.

Implement --dependent-libraries for GNU output
ClosedPublic

Authored by jhenderson on May 14 2020, 5:58 AM.

Details

Summary

Previously, the option was only implemented for LLVM output. This fixes https://bugs.llvm.org/show_bug.cgi?id=45695.

The output roughly follows the output style for similar options like --syms/--notes etc, combined with --string-dump output.

Diff Detail

Event Timeline

jhenderson created this revision.May 14 2020, 5:58 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar added inline comments.May 14 2020, 6:31 AM
llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test
7

Probably it worth to mention that --dependent-libraries is not implemented in GNU at the moment of 14 may 2020.

9

Add --implicit-check-not="DependentLibs" for consistency with below line?

41

These could be 2 lines, seems no need to wrap.

llvm/tools/llvm-readobj/ELFDumper.cpp
729

Can this be a protected method?

I'd also not mix virtual vs non-virtual methods.

5329

S is not used as for variable/condition. Seems that inlining Contents.begin() would look cleaner.

5346

Seems you can replace SecName and SecOffset with a new NameOffset variable.

5363

You do not need to wrap a single line into brackets.

5377

It would be easier to use & here too, as this lambda is not supposed to be used outside of the scope of this method.
In this case using just & reads IMO better in compare with explicit listing of variables.

5379

I'd probably move out these helper functions. It is a bit hard to read them inlined.
Naming them should help for readability.

jhenderson marked 11 inline comments as done.May 14 2020, 7:21 AM
jhenderson added inline comments.
llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test
7

I'm not sure I follow. This change is adding support for GNU.

9

I can add it, but I don't think it's that useful. In the GNU mode, the header is printed for each SHT_LLVM_DEPEDENT_LIBRARIES section, so checking it doesn't appear other than when we expect helps show that the header is not printed for other section types, whereas for LLVM it is only ever printed once (and the -NEXT lines help ensure that other sections aren't interepreted as dependent library sections).

41

Yup, fair. That comes from when I had other options etc in an early version.

llvm/tools/llvm-readobj/ELFDumper.cpp
5346

Fair enough. NameOffset was intended for library names and offsets within the section, but I think it's fine sharing the struct in that way.

jhenderson marked 2 inline comments as done.

Address review comments.

jhenderson retitled this revision from Implement --depedent-libraries for GNU output to Implement --dependent-libraries for GNU output.May 14 2020, 7:30 AM
grimar added inline comments.May 14 2020, 7:37 AM
llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test
7

I mean GNU readelf does not implement this, right? So it is the difference we have atm.
Perhaps worth to mention it in a comment.

MaskRay accepted this revision.May 14 2020, 8:35 AM

Looks great! Add [llvm-readobj] to the subject.

This revision is now accepted and ready to land.May 14 2020, 8:35 AM
jhenderson marked an inline comment as done.May 15 2020, 2:10 AM
jhenderson added inline comments.
llvm/test/tools/llvm-readobj/ELF/dependent-libraries.test
7

Right, thanks. I'll add it to the commit message, I think. I could see GNU implementing it to match our output, so consequently the comment in code would go stale.

grimar accepted this revision.May 15 2020, 2:19 AM

LGTM

This revision was automatically updated to reflect the committed changes.