Page MenuHomePhabricator

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

Authored by tpimh on Sep 17 2018, 12:44 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

tpimh created this revision.Sep 17 2018, 12:44 AM

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

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

alexshap requested changes to this revision.Sep 17 2018, 1:35 AM
alexshap added inline comments.
tools/llvm-objcopy/ObjcopyOpts.td
70 ↗(On Diff #165716)

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 ?

This revision now requires changes to proceed.Sep 17 2018, 1:35 AM

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 this revision to Diff 165723.Sep 17 2018, 1:55 AM

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

srhines added inline comments.Sep 17 2018, 1:59 AM
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 updated this revision to Diff 165726.Sep 17 2018, 2:04 AM

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

alexshap accepted this revision.Sep 17 2018, 2:05 AM
This revision is now accepted and ready to land.Sep 17 2018, 2:05 AM

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 marked 2 inline comments as done.Sep 17 2018, 2:10 AM

Please, go ahead and commit it.

srhines accepted this revision.Sep 17 2018, 2:22 AM

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.).

This revision was automatically updated to reflect the committed changes.

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.

-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.