Page MenuHomePhabricator

[llvm-readobj] - Implement --dependent-libraries flag.
ClosedPublic

Authored by grimar on Nov 25 2019, 5:14 AM.

Details

Summary

There is no way to dump SHT_LLVM_DEPENDENT_LIBRARIES sections
currently. This patch implements this.

The section is described here:
https://llvm.org/docs/Extensions.html#sht-llvm-dependent-libraries-section-dependent-libraries

Diff Detail

Event Timeline

grimar created this revision.Nov 25 2019, 5:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2019, 5:14 AM
Herald added a subscriber: seiya. · View Herald Transcript
grimar updated this revision to Diff 230878.Nov 25 2019, 5:17 AM
  • Include a few forgotten comments.
jhenderson added a comment.EditedNov 25 2019, 6:56 AM

The llvm-readobj doc needs updating.

llvm/test/tools/llvm-readobj/elf-dependent-libraries.test
12

I don't see the need for the Entries: list. Can't you just print them as:

DependentLibs [
  foo,
  bar,
  foo
]
46

when have not null-terminated -> for a non-null-terminated

58

aall -> all

62

when unable to get the content -> when the section offset is invalid

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

reportUniqueWarning?

grimar updated this revision to Diff 231019.Nov 26 2019, 1:24 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.
llvm/test/tools/llvm-readobj/elf-dependent-libraries.test
12

Done.
(Without a comma. It seems to be excessive to have it here + scoped printer's printString() does not print, I think we should not add it manually).

I've been thinking about this, and I'm not convinced that the switch name should have "elf" in it. I'd prefer simply "--dependent-libraries". Although the option is currently only implemented for ELF, I don't know of any inherent reason it couldn't apply to COFF for example at a future point, and we'd then just need to add an alias to the switch. See also other switches such as --stack-sizes and --addrsig. I'd keep "elf" prefixes to switches that are inherently tied to the ELF file format, such as --elf-section-groups, or maybe where the same term means completely different things for different formats (I'm not sure about this one).

I've subscribed @bd1976llvm who was heavily involved in getting the dependent libraries stuff implemented. He might have some thoughts too.

llvm/test/tools/llvm-readobj/elf-dependent-libraries.test
37

Is there a clean way of not having this empty line? I'm not sure there's any benefit to it. It's not a big deal though if it makes the code significantly more complex.

I've been thinking about this, and I'm not convinced that the switch name should have "elf" in it. I'd prefer simply "--dependent-libraries". Although the option is currently only implemented for ELF, I don't know of any inherent reason it couldn't apply to COFF for example at a future point, and we'd then just need to add an alias to the switch. See also other switches such as --stack-sizes and --addrsig. I'd keep "elf" prefixes to switches that are inherently tied to the ELF file format, such as --elf-section-groups, or maybe where the same term means completely different things for different formats (I'm not sure about this one).

I've subscribed @bd1976llvm who was heavily involved in getting the dependent libraries stuff implemented. He might have some thoughts too.

This llvm ELF feature had a discussion here http://lists.llvm.org/pipermail/llvm-dev/2019-March/131004.html . The feature originated from COFF https://docs.microsoft.com/en-us/cpp/preprocessor/comment-c-cpp?view=vs-2019 So I think this option can apply to COFF as well, though I know too little about COFF to say how that will going to work. @bd1976llvm is the expert.

The feature certainly could be implemented for COFF, so I think changing the name of the command line option is reasonable. The strings in the .deplibs sections map to libraries in an implementation defined manner, so I refer to them as "specifiers". In COFF dependent libraries are specified via "directives" in the object files. It might be worth naming the option something like: --dependent-lib-directives (as directives works for the entries in an ELF .deplibs sections equally as well as specifiers) ?

grimar updated this revision to Diff 231896.Dec 3 2019, 6:09 AM
grimar marked 2 inline comments as done.
grimar retitled this revision from [llvm-readobj] - Implement --elf-dependent-libs flag. to [llvm-readobj] - Implement --dependent-lib-directives flag..
  • Renamed the option to --dependent-lib-directives
llvm/test/tools/llvm-readobj/elf-dependent-libraries.test
37

So, the reason is more global and described here:
https://reviews.llvm.org/D70826#inline-639055

I am going to investigate how can we fix this soon.

jhenderson added inline comments.Dec 3 2019, 7:48 AM
llvm/docs/CommandGuide/llvm-readobj.rst
157

section -> directives (in case it starts getting used by COFF)

llvm/test/tools/llvm-readobj/elf-dependent-libraries.test
75

This test will fail right?

The code change looks good.

llvm/docs/CommandGuide/llvm-readobj.rst
157

@bd1976llvm @jhenderson Do we have to call it -directives? A #pragma is referred to as a directive, but the section is not.

We support other options that dump extension sections (e.g. --stack-sizes). None has the suffix -directives. I think --dependent-libs (I presume you will prefer the plural form) should be fine.

grimar marked 3 inline comments as done.Dec 4 2019, 12:14 AM
grimar added inline comments.
llvm/docs/CommandGuide/llvm-readobj.rst
157

section -> directives (in case it starts getting used by COFF)

The section type is called "SHT_LLVM_DEPENDENT_LIBRARIES". That is why I haven't changed the description.
Given this the description seems to be fine?

llvm/test/tools/llvm-readobj/elf-dependent-libraries.test
75

Oops. I guess this my last minute change wasn't included into the diff. I definitely fixed this.

grimar marked 2 inline comments as done.Dec 4 2019, 12:23 AM
grimar added inline comments.
llvm/docs/CommandGuide/llvm-readobj.rst
157

Or.. Does "SHT_LLVM_DEPENDENT_LIBRARIES" used by COFF somehow or this section type is needed only for ELF?
(I know nothing about COFF. I've supposed it still creates a section of this type under hood, but may be I getting it wrong?).

jhenderson added inline comments.Dec 4 2019, 2:04 AM
llvm/docs/CommandGuide/llvm-readobj.rst
157

Do we have to call it -directives? A #pragma is referred to as a directive, but the section is not.

Or.. Does "SHT_LLVM_DEPENDENT_LIBRARIES" used by COFF somehow or this section type is needed only for ELF?

I actually prefer it without, but I chatted offline with @bd1976llvm who explained that for the equivalent feature in COFF, they are not listed as libraries, but rather command-line like directives e.g. /DEFAULTLIB:library.lib. Microsoft's dumpbin uses /directives as the option to dump them, for the record, but that dumps more than just library directives.

Happy to hear what other people think though. --dependent-libs is fine for me (or perhaps we should use --dependent-libraries) and I think would be sufficient for COFF too.

MaskRay accepted this revision.Dec 4 2019, 9:36 AM
MaskRay added inline comments.
llvm/docs/CommandGuide/llvm-readobj.rst
157

--dependent-libraries is fine for me (the section type is called SHT_LLVM_DEPENDENT_LIBRARIES).

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

Unintended change?

This revision is now accepted and ready to land.Dec 4 2019, 9:36 AM
grimar updated this revision to Diff 232299.Dec 5 2019, 3:33 AM
grimar marked 5 inline comments as done.
grimar retitled this revision from [llvm-readobj] - Implement --dependent-lib-directives flag. to [llvm-readobj] - Implement --dependent-libraries flag..
  • Renamed the option to --dependent-libraries.
This revision was automatically updated to reflect the committed changes.