This is an archive of the discontinued LLVM Phabricator instance.

[llvm-strip] Support -s alias for --strip-all. Make both strip and objcopy case sensitive to support both -s (--strip-all) and -S (--strip-debug).
ClosedPublic

Authored by rupprecht on Oct 11 2018, 1:00 PM.

Details

Summary

GNU strip supports both -s and -S as aliases for --strip-all and --strip-debug, respectfully.

As part of this, it turns out that strip/objcopy were accepting case insensitive command line args. I'm not sure if there was an explicit reason for this. The only others uses of this are llvm-cvtres/llvm-mt/llvm-lib, which are all tools specific for windows support. Forcing case sensitivity allows both aliases to exist, but seems like a good idea anyway.

And as a surprise test case adjustment, the llvm-strip unit test was running with -keep=unavailable_symbol, despite keep not be a valid flag for strip. This is because there is a flag -K which, when case insensitivity is permitted, allows it to be interpreted as -K = eep=unavailable_symbol (e.g. to allow -Kfoo == --keep-symbol=foo).

Diff Detail

Repository
rL LLVM

Event Timeline

rupprecht created this revision.Oct 11 2018, 1:00 PM
rupprecht edited the summary of this revision. (Show Details)Oct 11 2018, 1:01 PM

LGTM with the inline comment fixed.

I agree that command-line flags shouldn't be case-sensitive in general. I would expect e.g. -k and -K to be two different switches, and if one did not exist, then I'd expect an error.

test/tools/llvm-objcopy/strip-all.test
42–44 ↗(On Diff #169341)

Nit: To reduce unnecessary changes in the diffs could you keep this as %t9, and move the llvm-strip -s case to below it, please.

rupprecht updated this revision to Diff 169915.Oct 16 2018, 4:09 PM
rupprecht marked an inline comment as done.
  • Reorder test cases for smaller diffs
rupprecht added inline comments.Oct 16 2018, 4:10 PM
test/tools/llvm-objcopy/strip-all.test
42–44 ↗(On Diff #169341)

Done -- I kept it here and bumped up the next case (--keep-symbol) since that seems to be a different enough test case.

nhaehnle removed a subscriber: nhaehnle.Oct 17 2018, 3:24 AM
jakehehrlich accepted this revision.Oct 23 2018, 11:37 AM

Looks like a strict improvement to me and a nice test case fix. Thanks!

LGTM.

This revision is now accepted and ready to land.Oct 23 2018, 11:37 AM
This revision was automatically updated to reflect the committed changes.