Page MenuHomePhabricator

[llvm-objcopy] [COFF] Implmement --strip-unneeded and -x/--discard-all for symbols
ClosedPublic

Authored by mstorsjo on Wed, Jan 9, 1:39 AM.

Diff Detail

Event Timeline

mstorsjo created this revision.Wed, Jan 9, 1:39 AM
jhenderson added inline comments.Thu, Jan 10, 2:23 AM
test/tools/llvm-objcopy/COFF/strip-unneeded.yaml
1

The ELF side of llvm-objcopy also strips undefined local symbols that are unreferenced with --strip-unneeded. Should that be the same in COFF?

8–9

Just to confirm, this is what GNU objcopy does? The ELF side attempts to strip local symbols in this case whether they're referenced or not.

Another difference between the two is that undefined locals are stripped by strip-unneeded, but not discard-all.

11

I have a marginal preference to use a different output file name for each case. This allows a user to inspect the corresponding output file, should the need arise, without having to mess about with the test.

mstorsjo marked 2 inline comments as done.Fri, Jan 11, 2:37 AM
mstorsjo added inline comments.
test/tools/llvm-objcopy/COFF/strip-unneeded.yaml
8–9

GNU objcopy doesn't strip referenced local symbols, but undefined locals (which I wasn't even aware of being a thing) does indeed seem to have the difference you mention. I'll implement that.

11

Yes, especially as the output actually should be different in this case, after adding a testcase of undefined local symbols.

mstorsjo updated this revision to Diff 181231.Fri, Jan 11, 2:52 AM
mstorsjo edited the summary of this revision. (Show Details)

Did the suggested changes to the test, added the distinction that --discard-all keeps undefined local symbols.

jhenderson added inline comments.Fri, Jan 11, 3:02 AM
test/tools/llvm-objcopy/COFF/strip-unneeded.yaml
8–9

Okay, as there's now a behaviour difference, please split discard-all into a separate test.

mstorsjo updated this revision to Diff 181236.Fri, Jan 11, 3:40 AM

Split testes into two separate files, with one shared yaml source in the Inputs directory.

jhenderson added inline comments.Fri, Jan 11, 5:34 AM
test/tools/llvm-objcopy/COFF/Inputs/discard-locals.yaml
15 ↗(On Diff #181236)

Thinking about it, it's probably worth having an undefined external too.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp
53–54

GNU objcopy (probably) doesn't have a "Config.DiscardAll" member. Change this and the StripUnneeded reference to switch names (i.e. --discard-all/--strip-unneeded).

mstorsjo updated this revision to Diff 181254.Fri, Jan 11, 5:41 AM
mstorsjo marked 2 inline comments as done.

Added an undefined external to the tests, changed the code comment to use CLI parameter names.

This revision is now accepted and ready to land.Fri, Jan 11, 6:06 AM
This revision was automatically updated to reflect the committed changes.