Page MenuHomePhabricator

[objcopy] make objcopy follow program header standards
ClosedPublic

Authored by amontanez on Sep 11 2018, 4:50 PM.

Details

Summary

Objects with unused program headers copied by objcopy would always have nonzero values for program header offset and program header entry size. While technically valid, this atypical behavior triggers warnings in some tools. This change sets the two fields to zero when the program header is unused, better fitting the general expectations for unused program header data.

Section headers behaved somewhat similarly (though only with the entry size), and are fixed in this revision as well.

Diff Detail

Repository
rL LLVM

Event Timeline

amontanez created this revision.Sep 11 2018, 4:50 PM

A couple nits but this LGTM.

llvm/test/tools/llvm-objcopy/relocatable-phdr.test
5 ↗(On Diff #164997)

You'll want to use llvm-readobj instead of llvm-readelf. It produces a more machine readable format.

llvm/tools/llvm-objcopy/Object.cpp
1053 ↗(On Diff #164997)

I'd use phnum here instead of recalling size.

Thanks for fixing this!

amontanez updated this revision to Diff 165003.Sep 11 2018, 5:55 PM

Now using llvm-readobj instead of llvm-readelf for test, updated patch to use Ehdr.e_phnum instead of calling size() repeatedly.

amontanez marked 2 inline comments as done.Sep 11 2018, 5:57 PM

Done, good points.

When you write your diff use git diff -U<some big number> origin/master so that the diff is relative to what's upstream not your last change locally. I think git show might be understood by fabricator as well but I'm not 100% sure.

llvm/test/tools/llvm-objcopy/relocatable-phdr.test
16 ↗(On Diff #165003)

nit: add a space here

jakehehrlich accepted this revision.Sep 11 2018, 6:07 PM

Oh also LGTM

This revision is now accepted and ready to land.Sep 11 2018, 6:07 PM
mcgrathr added inline comments.Sep 11 2018, 6:40 PM
llvm/tools/llvm-objcopy/Object.cpp
1057 ↗(On Diff #165003)

While you're at it, don't set shentsize when there are no sections.

jakehehrlich added inline comments.Sep 11 2018, 6:53 PM
llvm/tools/llvm-objcopy/Object.cpp
1057 ↗(On Diff #165003)

You'll want to use !WriteSectionHeaders || size(Obj.sections()) == 0 as the condition for that BTW. WriteSectionHeaders is normally true but is false when --strip-sections is used. WriteSectionHeaders isn't set to false when size(Obj.sections()) == 0 is true

Thinking about it now the if (WriteSectionHeaders) below should probably be WriteSectionHeaders && size(Obj.sections()) != 0 and then you can just set e_shentsize inside each branch of that if statement. That way if a sectionless object is input to llvm-objcopy it will output it with all those fields set to zero even though --strip-sections wasn't used.

jakehehrlich requested changes to this revision.Sep 11 2018, 6:59 PM

I'll LGTM after Roland's fixes are done since I agree, that would be nice to also fix.

This revision now requires changes to proceed.Sep 11 2018, 6:59 PM
amontanez edited the summary of this revision. (Show Details)

Fixes affected test cases, now includes fix for similar behavior from section headers.

amontanez marked 3 inline comments as done.Sep 12 2018, 10:12 AM

Done! I ran all the tests for llvm-objcopy and updated everything that was failing from these changes.

This revision is now accepted and ready to land.Sep 12 2018, 10:25 AM
This revision was automatically updated to reflect the committed changes.