Page MenuHomePhabricator

Implement --dependent-libraries for GNU output
ClosedPublic

Authored by jhenderson on Thu, May 14, 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.Thu, May 14, 5:58 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar added inline comments.Thu, May 14, 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.

5349

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

5366

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

5383

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

5397

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.

5399

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.Thu, May 14, 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
5366

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.Thu, May 14, 7:30 AM
grimar added inline comments.Thu, May 14, 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.Thu, May 14, 8:35 AM

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

This revision is now accepted and ready to land.Thu, May 14, 8:35 AM
jhenderson marked an inline comment as done.Fri, May 15, 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.Fri, May 15, 2:19 AM

LGTM

This revision was automatically updated to reflect the committed changes.