Page MenuHomePhabricator

[llvm-objdump] Add --file-headers (-f) option
ClosedPublic

Authored by paulsemel on Jul 1 2018, 11:30 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

paulsemel created this revision.Jul 1 2018, 11:30 AM
jhenderson added inline comments.
tools/llvm-objdump/llvm-objdump.cpp
2135

Nit: Is there a need for the trailing whitespace in the output?

paulsemel updated this revision to Diff 153732.Jul 2 2018, 9:12 AM
paulsemel marked an inline comment as done.

Fixing trailing white space.

I was going to ask for a documentation update, but it turns out that there is no documentation for llvm-objdump outside the help text...!

It looks to me like GNU objdump also prints some flags when -f is used. Is this something you plan to add later?

lib/Object/COFFObjectFile.cpp
916

It looks like GNU objdump prints 0 rather than failing in this case.

test/tools/llvm-objdump/file-headers.test
1 ↗(On Diff #153732)

I must admit that I'm surprised we're not using yaml2obj to test here. I am not familiar with llvm-objdump, and I see that using pre-canned binaries is normal for its tests, but how do you feel about using yaml2obj instead?

Assuming you'd prefer to use pre-canned binaries, is it normal in the tests to use files that aren't specifically for that test?

There should also probably be separate elf and coff test cases.

tools/llvm-objdump/llvm-objdump.cpp
2312

Assuming this is what clang-format does, I suppose that this is right, but it definitely does make it a lot harder to follow :-(

About the other informations: I think they are more "format specific" and thus I wanted to add an other patch for it. Do you want me to do it in this patch ?

lib/Object/COFFObjectFile.cpp
916

Yes, I figured it out this night.. I really don't know why. Do you think we might follow the behavior ?

test/tools/llvm-objdump/file-headers.test
1 ↗(On Diff #153732)

Yes, I anyway definitely need to add a COFF test. I can use yaml2obj here, this is not a problem for me, but as a lot of tests in this tool are doing this way, I just didn't want to break the move :)

tools/llvm-objdump/llvm-objdump.cpp
2135

Absolutely not, fixing it !

2312

Woops, shouldn't have pushed this... will go back on this modification no worries :)

About the other informations: I think they are more "format specific" and thus I wanted to add an other patch for it. Do you want me to do it in this patch ?

No, I don't think it's that important, and I'm happy for that to be added later, when someone wants to add it.

lib/Object/COFFObjectFile.cpp
916

I think it makes sense to. The other information in the file-headers dump is potentially useful, so preventing it being printed is bad (though if there's a clean way of not printing the address at all for COFF object files, I'm all for it not being printed in this case).

test/tools/llvm-objdump/file-headers.test
1 ↗(On Diff #153732)

I think when we can do better, there's no reason to stick to the same pattern. Somebody needs to break the mould at some point!

paulsemel updated this revision to Diff 153919.Jul 3 2018, 8:11 AM

Applied Jame's suggestions

Looks fine to me, apart from a couple of test-related comments.

test/tools/llvm-objdump/file-headers-coff.test
13

Does yaml2obj have support for PE-COFF too? If so, could you add a third test case, with a non-zero start address, please.

test/tools/llvm-objdump/file-headers-elf.test
13

Could you provide a non-zero start address, please, to show that the address is correctly read.

paulsemel updated this revision to Diff 153961.Jul 3 2018, 1:38 PM
paulsemel marked 2 inline comments as done.

Added Jame's suggestions

paulsemel added a comment.EditedJul 3 2018, 1:38 PM
This comment has been deleted.
test/tools/llvm-objdump/file-headers-coff.test
13

Well, as you will see, I've done what I could about it.. There is a lot of useless flags to set to be able to build the PE file, but I didn't find another way to do it.

Some inline comment nits, and one thing I noticed regarding the "not implemented" part.

test/tools/llvm-objdump/file-headers-pe.test
10

those -> these

Also add a full stop at the end of the line.

11–12

All ... picked. They can't interfere ... here.

(i.e. remove extra "values", change comma to full stop, fix typo, add trailing full stop).

28

May be worth moving this particular field above the comment, since it is an important part of the test.

tools/llvm-objdump/llvm-objdump.cpp
2135

Hmmm... Should this use report_error? Or is not implemented normally reported on stdout?

Also, how easy is to get a test that hits this case? If it's simple, I think it should have one. If not, no need to write a complicated one.

jhenderson added inline comments.Jul 4 2018, 2:18 AM
test/tools/llvm-objdump/file-headers-coff.test
3

One more thing: since --file-headers is an alias of -f, I'd like to see both tested, to show they do the same thing. It should just be a case of duplicating this line in each of the tests, but with the other version of the switch (the same goes with other similar changes, such as D48904).

paulsemel updated this revision to Diff 154087.Jul 4 2018, 3:23 AM
paulsemel marked 5 inline comments as done.

Added Jame's suggestions

jhenderson accepted this revision.Jul 4 2018, 3:49 AM

LGTM, with the remaining two nits fixed.

test/tools/llvm-objdump/file-headers-error.test
1 ↗(On Diff #154087)

I'd rename this file to file-headers-unsupported to better convey what is tested here.

test/tools/llvm-objdump/file-headers-pe.test
11–12

Still got a typo: interfer -> interfere

This revision is now accepted and ready to land.Jul 4 2018, 3:49 AM
paulsemel updated this revision to Diff 154090.Jul 4 2018, 4:41 AM
paulsemel marked an inline comment as done.

Should I land ?

Should I land ?

I don't see why not. It's not controversial, as far as I can tell.

paulsemel closed this revision.Jul 4 2018, 8:30 AM

Forgot to add Differential Revision in the commit, closing the revision