Page MenuHomePhabricator

[llvm-objcopy] [COFF] Implement --strip-all[-gnu] for symbols
ClosedPublic

Authored by mstorsjo on Jan 9 2019, 1:44 AM.

Details

Summary

This operation removes all relocations, which is disastrous for object files, but that's probably expected.

Diff Detail

Repository
rL LLVM

Event Timeline

mstorsjo created this revision.Jan 9 2019, 1:44 AM
mstorsjo marked an inline comment as done.Jan 9 2019, 1:46 AM
mstorsjo added inline comments.
tools/llvm-objcopy/COFF/Writer.cpp
162 ↗(On Diff #180801)

The existing code here turned out to be subtly broken when writing an object file with no symbols, so I have to update this part of COFFWriter as part of this change.

jhenderson added inline comments.Jan 10 2019, 7:02 AM
test/tools/llvm-objcopy/COFF/strip-all.yaml
7–8 ↗(On Diff #180801)

These two could be folded together into one test.

10 ↗(On Diff #180801)

Do you anticipate --strip-all-gnu and and --strip-all doing the same thing? If so, it's probably clearer and easier to run cmp on the output of the two objcopy commands. Also, you don't appear to have any check for the short-form of the option (-S) or llvm-strip.

41–46 ↗(On Diff #180801)

What's the purpose of this symbol?

tools/llvm-objcopy/COFF/Writer.cpp
162 ↗(On Diff #180801)

If it's already broken, should this be a separate and previous change?

mstorsjo marked 7 inline comments as done.Jan 11 2019, 1:56 AM
mstorsjo added inline comments.
test/tools/llvm-objcopy/COFF/strip-all.yaml
7–8 ↗(On Diff #180801)

Ok, will do.

10 ↗(On Diff #180801)

As --strip-all-gnu vs --strip-all is for ELF specific details in how/what to strip, I don't think I'll need to make any difference between them, so I can just compare the output objects instead. I'll add tests for llvm-strip and -S as well.

41–46 ↗(On Diff #180801)

Just to provide a bit more test data, to show that both defined and undefined symbols are stripped.

tools/llvm-objcopy/COFF/Writer.cpp
162 ↗(On Diff #180801)

Done in D56584.

mstorsjo updated this revision to Diff 181225.Jan 11 2019, 1:58 AM
mstorsjo marked 3 inline comments as done.

Split out the general bugfix, did the suggested changes to the test.

jhenderson added inline comments.Jan 11 2019, 2:15 AM
test/tools/llvm-objcopy/COFF/strip-all.yaml
9 ↗(On Diff #181225)

--strip-out-gnu?

mstorsjo updated this revision to Diff 181230.Jan 11 2019, 2:29 AM
mstorsjo marked an inline comment as done.

Fixed the --strip-all-gnu typo in the test comment.

Okay, this LGTM, although I don't know enough about the characteristics stuff to be able to comment on that, so please get somebody else to review that bit.

mstorsjo edited the summary of this revision. (Show Details)Jan 11 2019, 2:53 AM

When doing this, GNU objcopy sets a few Characteristics flags as well - I'm only setting one of them as the other ones are documented to be deprecated. I'd like to have @rnk's opinion on this part.

My suggestion would be to continue setting the deprecated characteristics flags for --strip-all-gnu, but not for --strip-all, if I understand the meaning of the difference between the two flags. --strip-all-gnu should be for more extreme compatibility between gnu, --strip-all is allowed to be a bit more aggressive.
But I have never heard of characteristics before, maybe this is a case where we want to break GNU objcopy compatibility (we don't aim to be bit-for-bit compatible; even GNU-matching flags will different in minor aspects).
Probably best to let @rnk comment on characteristics.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp
41 ↗(On Diff #181230)

It looks like this is not always supposed to be added; have you seen https://sourceware.org/bugzilla/show_bug.cgi?id=11396?

43 ↗(On Diff #181230)

What about IMAGE_FILE_DEBUG_STRIPPED? Do you know if GNU objcopy sets that in any cases?

tools/llvm-objcopy/COFF/Writer.cpp
55 ↗(On Diff #181230)

IMO this if/else block is actually easier to read with a ternary:

S.Header.PointerToRelocations = (S.Header.NumberOfRelocations > 0) ? FileSize : 0;
mstorsjo marked 3 inline comments as done.Jan 11 2019, 3:04 PM

When doing this, GNU objcopy sets a few Characteristics flags as well - I'm only setting one of them as the other ones are documented to be deprecated. I'd like to have @rnk's opinion on this part.

My suggestion would be to continue setting the deprecated characteristics flags for --strip-all-gnu, but not for --strip-all, if I understand the meaning of the difference between the two flags. --strip-all-gnu should be for more extreme compatibility between gnu, --strip-all is allowed to be a bit more aggressive.
But I have never heard of characteristics before, maybe this is a case where we want to break GNU objcopy compatibility (we don't aim to be bit-for-bit compatible; even GNU-matching flags will different in minor aspects).
Probably best to let @rnk comment on characteristics.

Sounds sensible, but let's wait for his opinion.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp
41 ↗(On Diff #181230)

I haven't seen that one, thanks!

So if I understand it properly, GNU objcopy earlier set the "relocations stripped" flag if there were no base relocs (the other kind of relocs, on executables/DLLs) in the output binary, but the fix changed it to not do this if there were none in the input to begin with.

So far in these patches, I'm not touching base relocs at all though.

While waiting for @rnk's sentiment, I think these flags aren't strictly necessary wrt this patch at all, but I'm mostly setting them to be helpful.

43 ↗(On Diff #181230)

I haven't studied the GNU objcopy output closely enough wrt those stripping options, but I'll probably see that once I finish the patches about stripping sections and debug info. (I have it mostly functional since a long time, but it's missing finalizing according to earlier review comments and making more precise tests for each feature.)

tools/llvm-objcopy/COFF/Writer.cpp
55 ↗(On Diff #181230)

Ok, will change.

mstorsjo updated this revision to Diff 181375.Jan 11 2019, 3:07 PM

Using a ternary operator as suggested.

Alternatively, I could just skip setting the characteristics flags altogether in this patch, as I don't think anything actually reads them on object files, and the context here (removing relocations) only is relevant for object files - base relocs on executables/DLLs are untouched. We can always add them later if deemed necessary. WDYT?

mstorsjo updated this revision to Diff 181459.Jan 12 2019, 3:04 PM
mstorsjo edited the summary of this revision. (Show Details)

I came to the conclusion that setting the Characteristics flags is mostly orthogonal to the stripping of the symbols/relocs (the flag discussed before is actually only relevant for whether executables/DLLs are loadable at a different address than the default), so I split out that part of the patch and I might revisit it later, but it's not essential for the functionality of this patch.

The remainder has been LGTM'd by @jhenderson before, so unless there's other objections I guess that can be pushed at the beginning of next week.

jhenderson accepted this revision.Jan 14 2019, 2:48 AM

Latest version LGTM, but I'd like someone else to give a thumbs up too, as I don't know enough about COFF to be able to spot any potential issues necessarily.

This revision is now accepted and ready to land.Jan 14 2019, 2:48 AM

I don't see anything particularly COFF-specific here ... there were discussions about characteristics flags before, but that doesn't seem to be around in the latest version of the patch, and Martin's reasoning for that (that we're stripping relocations from object files, but the characteristics pertain to base relocations being stripped from image files, which aren't touched by this patch) makes sense. LGTM from that perspective.

rnk accepted this revision.Jan 14 2019, 2:11 PM

lgtm

rupprecht accepted this revision.Jan 14 2019, 8:14 PM
This revision was automatically updated to reflect the committed changes.