This is an archive of the discontinued LLVM Phabricator instance.

[llvm-strip] Expose --discard-all option
ClosedPublic

Authored by alexander-shaposhnikov on Jun 4 2018, 6:10 PM.

Details

Summary

Expose objcopy's --discard-all option in llvm-strip.
Test plan: make check-all

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson added inline comments.Jun 5 2018, 2:08 AM
test/tools/llvm-objcopy/discard-all.test
14

Why is the -d here?

Could you also test -x with llvm-strip, please?

test/tools/llvm-objcopy/discard-all.test
14

Why is the -d here?

@jhenderson - otherwise strip will remove the symbol table and we won't be able to test how it's getting modified.

First, thanks for the patch :)
I think @jhenderson pointed out something important. If you do strip -x foo -o bar, you will notice that it will only discard local symbols from the binary.
This is not really what's happening with this behavior.. (as you mentioned, this removes symbol table, and that's why you can't test the -x option alone)
Maybe we should fix this ? What do you think ? :)

tools/llvm-objcopy/llvm-objcopy.cpp
588–592

Maybe you should do something like (see my comment above for the reason):

Config.DiscardAll = InputArgs.hasArg(STRIP_discard_all);
if (!Config.StripDebug && !Config.DiscardAll)
  Config.StripAll = true;
594–595

And thus remove those lines :)

right, I'll update the patch & tests

Fix the behavior of strip -x

jhenderson accepted this revision.Jun 6 2018, 1:44 AM

LGTM. I think the updated version makes a lot more sense (why discard the local symbols if you are going to remove the symbol table anyway?).

This revision is now accepted and ready to land.Jun 6 2018, 1:44 AM
paulsemel accepted this revision.Jun 6 2018, 1:56 AM

LGTM too :)

This revision was automatically updated to reflect the committed changes.