-S as an alias for --strip-all-gnu

[tpimh]

Summary

This alias is needed for GNU binutils compatibility. This flag is used for instance by the Linux kernel build system. I do not know if it needs a test, if it does, it's easy to add one.

This is my first patch, so please, correct me if my submission is wrong.

[alexander-shaposhnikov]

the change looks fine, but it needs a test - simply have a look at the existing ones and add one more invocation of the tool (with this alias).
To run the tests there are special targets: check-llvm-tools, check-all, etc

[alexander-shaposhnikov]

also, pls, include the full context into the patch https://llvm.org/docs/Phabricator.html
svn diff --diff-cmd=diff -x -U999999

[alexander-shaposhnikov]

tools/llvm-objcopy/ObjcopyOpts.td

70

btw - it probably should not be JoinedOrSeparate (since it doesn't take any argument's values), instead, it should be simply Flag. Am I missing smth ?

[srhines]

Thanks for noticing this missing feature, as well as for the patch! You really should add a simple test to test/tools/llvm-objcopy/strip-all-gnu.test so that this doesn't ever regress. The test strip-all-and-keep-symbol.test is an example that has multiple RUN lines. Here is what you need to change the strip-all-gnu.test to (I believe) in order to get this working:

...

1. RUN: yaml2obj %s > %t
2. RUN: cp %t %t1
3. RUN: llvm-objcopy --strip-all-gnu %t %t2
4. RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s

1. RUN: llvm-objcopy -S %t1 %t3
2. RUN: cmp %t2 %t3

...

To check that everything works, you can run ninja check-all from your LLVM build directory (substitute whatever build system you used for ninja if you need to).

[tpimh]

Updated diff to include the whole modified file and added a test.

[srhines]

test/tools/llvm-objcopy/strip-all-gnu.test

4 ↗

(On Diff #165723)

There's already a %t2, so you shouldn't reuse it. I also suggest you just cmp against %t2, because the results should be identical/deterministic for this flag.

[tpimh]

Changed the test to a version suggested by @srhines, changed option type to Flag as it doesn't take an argument.

[alexander-shaposhnikov]

LG, thx. @tpimh, do you have commit access to LLVM or do you need me or @srhines commit this patch on behalf of you ?

[tpimh]

Please, go ahead and commit it.

[srhines]

Thanks again! @alexshap feel free to submit this or I can do it in the morning tomorrow (can't stay up any later to watch for unlikely breakage, etc.).

[jhenderson]

Thanks for the patch! Just one comment to add: please make sure with any llvm-objcopy changes to include myself as a reviewer, and probably also @jakehehrlich as he's the code owner.

[jakehehrlich]

-S should be an alias for --strip-all not --strip-all-gnu. There's seldom if ever going to be someone who rely's on the difference between the two. --strip-all-gnu exists as a way to allow people to hack llvm-objcopy into their system for entirely imagined use cases that we have not thus far actually seen. I'll be updating this patch.

tools/llvm-objcopy/ObjcopyOpts.td

                 HelpText<"Remove non-allocated sections other than .gnu.warning* sections">;
 def strip_all_gnu : Flag<["-", "--"], "strip-all-gnu">,
                     HelpText<"Compaitable with GNU objcopy's --strip-all">;
+def S : JoinedOrSeparate<["-"], "S">,
+        Alias<strip_all_gnu>;
 def strip_debug : Flag<["-", "--"], "strip-debug">,
                   HelpText<"Remove all debug information">;
 def strip_dwo : Flag<["-", "--"], "strip-dwo">,