Page MenuHomePhabricator

[llvm-readelf] Add -e/--headers support to readobj/elf
ClosedPublic

Authored by sidneym on Dec 4 2018, 2:53 PM.

Details

Summary

Several new options have been recently added to align with GNU. This patch adds -e/--headers.

Diff Detail

Repository
rL LLVM

Event Timeline

sidneym created this revision.Dec 4 2018, 2:53 PM
rupprecht accepted this revision.Dec 4 2018, 3:28 PM

Just a nit on the naming; actual functionality/test LGTM.

tools/llvm-readobj/llvm-readobj.cpp
62 ↗(On Diff #176715)

nit: "JustHeaders" kind of sounds like this will only print headers and nothing else, but "llvm-readelf -e -n" should still print notes in addition to headers (for example). Maybe "AllHeaders" would be a better name, and hopefully not confusing with the "All" option above?

This revision is now accepted and ready to land.Dec 4 2018, 3:28 PM
sidneym updated this revision to Diff 176725.Dec 4 2018, 3:39 PM

Change JustHeaders to AllHeaders

sidneym marked an inline comment as done.Dec 4 2018, 3:39 PM
jhenderson requested changes to this revision.Dec 5 2018, 2:36 AM

Please remember to include the context when creating the diff with Phabricator (see the section starting "To get a full diff" here.

test/tools/llvm-readobj/justheader.test
1 ↗(On Diff #176725)

The test needs renaming in line with the option name. I'd just suggest calling it "headers.test" since that is the option name.

I also think it would be a good idea to test that --headers produces the same headers for both llvm-readelf and llvm-readobj. In other cases (I'm thinking --program-headers specifically), the two produce different output, and not just in terms of style.

Also, rather than running FileCheck on both outputs, I personally would prefer for the second test case to compare its output to the first case to show that the --headers output is identical to -e. You should still run FileCheck on the first one, but you will need to run it using --input-file instead of stdin instead.

Finally, I'm not sure either way as to whether you should be checking more of the output (e.g. the contents of the ELF header, section headers etc). I can be persuaded either way, but I'd like to hear your justification, please.

9–10 ↗(On Diff #176725)

You may not be aware of this, but CHECK-NOT only checks the area between the previous and next matches, so in this case, it will only check that no relocation section or symbol table are dumped at the end. They could be printed e.g. between the section headers and program headers, and you wouldn't know.

11 ↗(On Diff #176725)

Nit: Did you mean to have a blank line at the end of this file?

tools/llvm-readobj/llvm-readobj.cpp
62 ↗(On Diff #176715)

Why not simply "Headers", since that is what the option is called?

This revision now requires changes to proceed.Dec 5 2018, 2:36 AM
sidneym marked 3 inline comments as done.Dec 5 2018, 8:41 AM
sidneym added inline comments.
test/tools/llvm-readobj/justheader.test
1 ↗(On Diff #176725)

Yes, I like the name "headers.test" better.

The output from llvm-readobj vs. llvm-readelf is almost always different and that is either by design or accident. Are you suggesting syncing the output between readobj and readelf for this case, basically setting, --elf-output-style=GNU when --headers is specified. Is that what you want?

The option -e is an alias for --headers so the output will match unless the meaning of one of the options is changed.

I used "all.test" as my template reasoning that -e is "almost all".

9–10 ↗(On Diff #176725)

I'll add a new pass for negative testing.

tools/llvm-readobj/llvm-readobj.cpp
62 ↗(On Diff #176715)

Sounds good.

sidneym updated this revision to Diff 176839.Dec 5 2018, 8:55 AM

The output from llvm-readobj vs. llvm-readelf is almost always different and that is either by design or accident. Are you suggesting syncing the output between readobj and readelf for this case, basically setting, --elf-output-style=GNU when --headers is specified. Is that what you want?

We shouldn't sync the output between llvm-readobj and llvm-readelf, although both should produce the right set of headers when requested. For llvm-readelf, we should match what GNU readelf does. llvm-readobj does not need to match this.

The test should test the following:

  • For llvm-readelf:
    • That the correct set of headers are dumped when specifying --headers.
    • That the -e switch produces the exact same output.
  • For llvm-readobj (if desired, it could probably be argued that having both is unnecessary):
    • That the correct set of headers are dumped when specifying --headers.
    • That the -e switch produces the exact same output.

Hopefully that explains things.

P.S. Please remember to include the full context in the diff...

Full context? I use git format-patch -1 and upload the file it produces.

bcain added a subscriber: bcain.Dec 6 2018, 6:20 AM

Full context? I use git format-patch -1 and upload the file it produces.

This information is in the link I previously posted:

see the section starting "To get a full diff" here.

To save you having to go there and read everything, here's a quick summary:

To get a full diff, use one of the following commands (or just use Arcanist to upload your patch):

git show HEAD -U999999 > mypatch.patch
git format-patch -U999999 @{u}
svn diff --diff-cmd=diff -x -U999999
sidneym updated this revision to Diff 176979.Dec 6 2018, 8:12 AM

Updated the patch to include full context.

Added the contents of the header blocks to the test.

jhenderson added inline comments.Dec 7 2018, 1:17 AM
test/tools/llvm-readobj/headers.test
1 ↗(On Diff #176979)

I've been thinking a bit more about the test for this, and I think I have a better suggestion, after all that:

  1. Run llvm-readelf with --headers, store the output in a file.
  2. Run llvm-readelf with -e, store the output in a file.
  3. Run llvm-readelf with --file-headers, --program-headers and --section-headers, store in a different file.
  4. Compare the two files from 1) and 2) against the file from 3) using cmp.

That would avoid the need for testing the detailed contents, whilst also providing what we really want, i.e. that --headers produces file, program and section header output. How does that sound?

sidneym updated this revision to Diff 177236.Dec 7 2018, 9:24 AM

Output each option test result to different files and verify that each of the files contains identical content.

This revision is now accepted and ready to land.Dec 10 2018, 1:52 AM
This revision was automatically updated to reflect the committed changes.