Page MenuHomePhabricator

[llvm-readobj] Add a flag to dump just the section-to-segment mapping.
ClosedPublic

Authored by mattd on Jan 28 2019, 5:19 PM.

Details

Summary

The following patch introduces a new function printSectionMapping which is responsible for dumping just the section-to-segment mapping.
This patch also introduces a n option -section-mapping that outputs that mapping without the program headers.

Previously, this functionality was controlled by printProgramHeaders, and the output from -program-headers has not been changed. I am happy to change the option name, I copied the name that was displayed when outputting the mapping table.

Diff Detail

Repository
rL LLVM

Event Timeline

mattd created this revision.Jan 28 2019, 5:19 PM

FYI, I've added a couple more reviewers who have reviewed some of my recent changes in this area too.

How about a test case for the new switch?

llvm/tools/llvm-readobj/ELFDumper.cpp
341 ↗(On Diff #183995)

Could this have a default definition, like printHashSymbols above?

467 ↗(On Diff #183995)

Not that it needs to be part of this change, but it might be nice if this were to be implemented for LLVM output style at some point.

llvm/tools/llvm-readobj/llvm-readobj.cpp
466–467 ↗(On Diff #183995)

One thing I think we want to avoid is printing this mapping separately if --section-mapping and --program-headers are both specified.

Also, my gut feeling is that the output would be easier to read for --section-headers --section-mapping, if the section mapping appears after the section headers, but I don't feel strongly about it.

Perhaps any ordering inconsistency could be solved by moving this to immediately after printProgramHeaders?

rupprecht added inline comments.Jan 29 2019, 9:21 AM
llvm/tools/llvm-readobj/llvm-readobj.cpp
466–467 ↗(On Diff #183995)

+1, to both those points

Alternatively, maybe don't call printSectionMapping() from within printProgramHeaders(), but instead have opts::ProgramHeaders imply opts::SectionMapping, and make sure to call printSectionMapping() right after printProgramHeaders().

Also worth considering is if you want to handle "--program-headers --section-mapping=false" differently. Not sure it's necessary to handle that.

mattd marked an inline comment as done.Jan 29 2019, 9:32 AM
mattd added inline comments.
llvm/tools/llvm-readobj/llvm-readobj.cpp
466–467 ↗(On Diff #183995)

Alternatively, maybe don't call printSectionMapping() from within printProgramHeaders(), but instead have opts::ProgramHeaders imply opts::SectionMapping, and make sure to call printSectionMapping() right after printProgramHeaders().

I like that suggestion. Thanks for the feedback.

mattd marked an inline comment as not done.Jan 29 2019, 9:32 AM
mattd updated this revision to Diff 184158.Jan 29 2019, 12:52 PM
mattd marked an inline comment as done.
  • Moved the call to printSectionMapping after printSectionHeaders`
  • Specifying --program-headers will still print the section mapping; however, the calls to printSectionMapping has been removed from the body of printProgramHeaders

This still needs a few tests, such as:

  1. llvm-readelf -section-mapping prints the section mapping (and no program headers)
  2. llvm-readelf -section-mapping -program-headers produces the same output as llvm-readelf -program-headers
  3. llvm-readelf -section-mapping -program-headers (or equivalently llvm-readelf -program-headers) prints the section to segment mapping exactly once (RUN: llvm-readelf ... | grep "Section to Segment mapping:" | count 1)
llvm/tools/llvm-readobj/llvm-readobj.cpp
486 ↗(On Diff #184158)

This can still show up twice, i.e.

$ llvm-readelf --section-mapping --program-headers llvm-readelf
# ... section to segment mapping ...
# ... program headers ...
# ... section to segment mapping ...

I was thinking something like this would work:

if (opts::ProgramHeaders)
  Dumper->printProgramHeaders();
if (opts::ProgramHeaders || opts::SectionMapping)
  Dumper->printSectionMapping();

Or equivalently,

// In args parsing section above
if (opts::ProgramHeaders)
  opts::SectionMapping = true; // Maybe also check --section-mapping=false wasn't explicitly specified
...
// Here
if (opts::ProgramHeaders)
  Dumper->printProgramHeaders();
if (opts::SectionMapping)
  Dumper->printSectionMapping();
mattd updated this revision to Diff 184227.Jan 29 2019, 6:18 PM
mattd marked an inline comment as done.

Thanks for the feedback, and my apologies for not originally providing as thorough of
testing as I should have.

This updated patch introduces the -section-mapping flag which can safely
operate (e.g., no duplicate mappings displayed) with -program-headers.

I've modified the tests to ELF64 in llvm/test/tools/llvm-readobj/gnu-phdrs.test
such that they are specialized on both the program headers and the section mapping.
This allows us to reuse some of the existing test output. I realize that the order of the prefixes is not
respected when using FileCheck's -check-prefixes; however, the
initial ELF test does recognize order as it only has one FileCheck prefix.

Added tests:
1 Check that llvm-readobj -program-headers generates the same output as
llvm-readelf -program-headers. Similar, -section-mapping -program-headers also
produces the same output as -program-headers

2 Check that -section-mapping and -program-headers observes the correct
order (display the headers followed by the section mapping).

3 Check that specifying -section-mapping only produces the mapping.

4 Check that specifying -section-mapping=false -program-headers only
produces the headers.

5 Check that only a single copy of the mapping table is produced when
combining -section-mapping and -program-headers options.

This looks good to me.
(I had a few NFC comments about GNUStyle<ELFT>::printSectionMapping implementation but then realized you only moved that code.)

jhenderson requested changes to this revision.Jan 30 2019, 2:20 AM

It occurred to me that if we were to implement section mapping for other output styles, e.g. LLVM, or formats (I don't know if that concept applies to COFF/MachO/Wasm etc mind you!), then we probably don't want to print the section mapping when program-headers is specified, except in GNU output style. This implies that the decision on whether to print the section mapping should be down to the dumper layer, not the command-line handling layer. This is very similar to the situation with --symbols/--dyn-symbols for GNU output style, and I suspect the solution should be essentially the same as rL351960.

llvm/test/tools/llvm-readobj/gnu-phdrs.test
17–18 ↗(On Diff #184227)

I'm not sure you need this test case, as it is identical to the previous one essentially. llvm-readobj --elf-output-style=GNU and llvm-readelf are essentially identical (ignoring certain unrelated switch interpretations and aliases).

19–20 ↗(On Diff #184227)

What is this check giving us that's different to the one on line 13?

26 ↗(On Diff #184227)

Using grep in lit has been deprecated, I believe. Use FileCheck CHECK-NOT patterns.

You can do the checking that the program headers are not present at the same time as the section mapping test, by using careful placement of the NOT patterns. NOT patterns fail if the output appears between the two positive checks either side of the NOT (or the start/end of file if there are none before/after it). Example that would fail if "Program Headers:" appears before or after the section mapping:

CHECK-NOT: Program Headers:
CHECK: Mapping
CHECK-NOT: Program Headers:

You can even use check-prefixes to share check patterns in a mix-and-match kind of way to avoid duplicating common things. All the names get added to one big bucket. When FileCheck scans the file for patterns it uses any with a matching prefix, in sequence, regardless of which prefix (i.e. the order of prefixes in the option is irrelevant).

For example, for the following case, using FileCheck --check-prefixes=NO-PHDRS,CHECK would be the equivalent of the above test case, whilst FileCheck --check-prefixes=PHDRS,CHECK or FileCheck --check-prefixes=CHECK,PHDRS would match the program headers followed by the mapping. I think you could even use FileCheck --check-prefixes=NO-PHDRS,CHECK,PHDRS to show that the Program Headers appear followed by the mapping, but that no more program headers appear after the mapping (I'm not quite sure whether the first NO-PHDRS-NOT would have any effect in this case though).

NO-PHDRS-NOT: Program Headers:
PHDRS: Program Headers:
CHECK: Mapping
NO-PHDRS-NOT: Program Headers:
This revision now requires changes to proceed.Jan 30 2019, 2:20 AM
grimar added inline comments.Jan 30 2019, 2:26 AM
llvm/test/tools/llvm-readobj/gnu-phdrs.test
26 ↗(On Diff #184227)

Using grep in lit has been deprecated, I believe. Use FileCheck CHECK-NOT patterns.

Oh, I wanted to write the same comment at first but found that grep is also used in LLVM. I did not realize it is deprecated, supposed we just use both ways.

jhenderson added inline comments.Jan 30 2019, 2:35 AM
llvm/test/tools/llvm-readobj/gnu-phdrs.test
26 ↗(On Diff #184227)

I found the reference in the testing documentation:

The recommended way to examine output to figure out if the test passes is using the FileCheck tool. [The usage of grep in RUN lines is deprecated - please do not send or commit patches that use it.]

mattd added inline comments.Jan 30 2019, 9:27 AM
llvm/test/tools/llvm-readobj/gnu-phdrs.test
17–18 ↗(On Diff #184227)

I understand the differences between the two tools, I just wanted to test the output. It agree it does feel a bit redundant.

19–20 ↗(On Diff #184227)

Since ELF32 is a single prefix, the order of the output, the headers immediately followed by the mapping, is checked. I want to check the case where both options -section-mapping -program-headers is presented, to ensure that the headers are still displayed before the mapping.

26 ↗(On Diff #184227)

When FileCheck scans the file for patterns it uses any with a matching prefix, in sequence, regardless of which prefix (i.e. the order of prefixes in the option is irrelevant).

Yep, that's what I meant in my patch description when I mentioned that the order of the prefixes is not respected. I tested that by just swapping the order of the arguments to the --check-prefixes=PHDRS,MAPPINGS and --check-prefixes=MAPPINGS,PHDRS.

Using grep in lit has been deprecated, I believe. Use FileCheck CHECK-NOT patterns.

Thanks for pointing the grep deprecation out! Honestly, when I was updating this patch, I just grepped around the source code looking for instances of "RUN.*grep" to see if my run-line looked correct. Since I found similar implementations, I ran with it. I'll update to CHECK-NOT Grepping for "grep" is rather satisfying.

mattd updated this revision to Diff 184379.Jan 30 2019, 2:50 PM
  • Removed a test
  • Replaced the uses of grep in the tests with CHECK-NOT and CHECK-COUNT
mattd added a comment.Jan 30 2019, 2:52 PM

It occurred to me that if we were to implement section mapping for other output styles, e.g. LLVM, or formats (I don't know if that concept applies to COFF/MachO/Wasm etc mind you!), then we probably don't want to print the section mapping when program-headers is specified, except in GNU output style. This implies that the decision on whether to print the section mapping should be down to the dumper layer, not the command-line handling layer. This is very similar to the situation with --symbols/--dyn-symbols for GNU output style, and I suspect the solution should be essentially the same as rL351960.

I agree, I'll look at making this change in this patch. Thanks for the suggestion.

mattd updated this revision to Diff 184444.Jan 30 2019, 8:12 PM
  • I took a hint and decided to more the decision to print the section mapping with the program headers down into the dumper classes. This is similar to how the printSymbols and printDynamicSymbols are handled.
  • I decided to pass the cl::boolOrDefault value down to the dumpers. As the GNU dumper will use this to dump the section mapping in the default or 'true' case. Where as the LLVM dumper will only dump the section mapping if the flag is 'true.' However, the LLVM section mapping dump is not yet implemented. That functionality should be added in a separate patch.
jhenderson added inline comments.Jan 31 2019, 4:30 AM
llvm/test/tools/llvm-readobj/gnu-phdrs.test
21–24 ↗(On Diff #184444)

You can combine these two into a single test, by using --implicit-check-not which I'd forgotten about until now. Just do the following:

RUN: llvm-readelf -section-mapping %p/Inputs/phdrs-elf.exe-x86_64 \
RUN:   --implicit-check-not="Program Headers:" | FileCheck %s -check-prefix ELF64-MAPPING
27–30 ↗(On Diff #184444)

Same as above, but with --implicit-check-not="Section to Segment mapping".

106 ↗(On Diff #184444)

I didn't know about CHECK-COUNT before, but my reading of the documentation is that this doesn't prevent further matches after this. You'll need a CHECK-NOT pattern after it (in which case, you don't need the COUNT).

llvm/tools/llvm-readobj/ELFDumper.cpp
158 ↗(On Diff #184444)

Do you need this? I think it's dead code.

345 ↗(On Diff #184444)

Can you remove this?

380 ↗(On Diff #184444)

This could be private.

474 ↗(On Diff #184444)

This could be private, or even not exist.

3338–3339 ↗(On Diff #184444)

Nit: looks like you manually wrapped this comment rather than let clang-format do, and the first line ends unnecessarily early.

4540–4541 ↗(On Diff #184444)

Given we don't currently support section mapping in LLVM style, does it make sense to have this here?

mattd updated this revision to Diff 184590.Jan 31 2019, 1:24 PM
mattd marked 9 inline comments as done.
  • Update two tests to use FileChecks --implicit-check-not
  • Remove some cruft that, I think, was in an earlier version of this patch.
  • Update the CHECK-COUNT test to simply perform a CHECK followed by a CHECK-NOT.
llvm/test/tools/llvm-readobj/gnu-phdrs.test
21–24 ↗(On Diff #184444)

Ah,interesting, I was unfamiliar with this option! Thanks for the tip.

llvm/tools/llvm-readobj/ELFDumper.cpp
4540–4541 ↗(On Diff #184444)

We don't need it, I anticipate we will eventually provide a definition to LLVMStyle<ELFT>::printSectionMapping. To me, the function feels incomplete if we take a parameter and do nothing with it. Additionally, I fear the annoyance of compiler warning messages about unused formal parameters.... if anyone is building this with -Wunused-parameter.

jhenderson accepted this revision.Feb 1 2019, 1:47 AM

Aside from the unnecessary pure virtual base class method for printSectionMapping, I think this is good to go. I'd like @rupprecht or @grimar to take another look though.

llvm/tools/llvm-readobj/ELFDumper.cpp
345 ↗(On Diff #184444)

Ping? I don't think you need this in the base class interface - it doesn't need to exist as the section mapping printing is a detail of the program header printing, so it probably should be removed.

4540–4541 ↗(On Diff #184444)

You can always comment out the name, but I'm okay leaving it in.

This revision is now accepted and ready to land.Feb 1 2019, 1:47 AM
grimar added a comment.Feb 1 2019, 3:34 AM

I have only a 2 minor nits. Maybe @rupprecht has more to add.

llvm/test/tools/llvm-readobj/gnu-phdrs.test
16 ↗(On Diff #184590)

nit: I suggest to follow the style of the previous line. I.e. place all arguments on the first line and FileCheck on the second.
(it is just a bit easier to read things when they are consistent):

RUN: llvm-readobj -program-headers %p/Inputs/phdrs-elf.exe-x86_64 --elf-output-style=GNU \
RUN:   | FileCheck %s -check-prefixes ELF64-PHDRS,ELF64-MAPPING
llvm/tools/llvm-readobj/ELFDumper.cpp
489 ↗(On Diff #184590)

I would group override methods together.

grimar accepted this revision.Feb 1 2019, 3:35 AM
rupprecht accepted this revision.Feb 1 2019, 9:16 AM

No additional concerns from me

mattd marked 2 inline comments as done and an inline comment as not done.Feb 1 2019, 10:47 AM
mattd added inline comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
345 ↗(On Diff #184444)

My bad, that should not have been in the current patch. I had marked it done, I'll make sure it doesn't show up when I land this, thanks.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 10:51 AM