This is an archive of the discontinued LLVM Phabricator instance.

Differential D52163

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

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

Details

Reviewers
alexander-shaposhnikov
srhines
Commits
rG1de445c71c2f: [llvm-objcopy] Add missing alias for --strip-all-gnu
rL342364: [llvm-objcopy] Add missing alias for --strip-all-gnu
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
Herald added a reviewer: alexander-shaposhnikov. · View Herald TranscriptSep 17 2018, 12:44 AM
Herald added subscribers: llvm-commits, rupprecht, jakehehrlich. · View Herald Transcript
alexander-shaposhnikov added a comment.Sep 17 2018, 1:19 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

alexander-shaposhnikov added a comment.Sep 17 2018, 1:31 AM

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

alexander-shaposhnikov requested changes to this revision.Sep 17 2018, 1:35 AM
alexander-shaposhnikov added inline comments.
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 ?

This revision now requires changes to proceed.Sep 17 2018, 1:35 AM
srhines added a subscriber: srhines.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

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.

alexander-shaposhnikov accepted this revision.Sep 17 2018, 2:05 AM
This revision is now accepted and ready to land.Sep 17 2018, 2:05 AM
alexander-shaposhnikov added a comment.Sep 17 2018, 2:09 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.).

Closed by commit rL342364: [llvm-objcopy] Add missing alias for --strip-all-gnu (authored by alexander-shaposhnikov). · Explain WhySep 17 2018, 2:46 AM
This revision was automatically updated to reflect the committed changes.
jhenderson added a subscriber: jhenderson.Sep 17 2018, 3:09 AM

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 added a comment.Sep 17 2018, 11:06 AM

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

Revision Contents

PathSize
test/
tools/
llvm-objcopy/
2 lines
tools/
llvm-objcopy/
2 lines

Diff 165723

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

# RUN: yaml2obj %s > %t # RUN: yaml2obj %s > %t
# RUN: llvm-objcopy --strip-all-gnu %t %t2 # RUN: llvm-objcopy --strip-all-gnu %t %t2
# RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s # RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s
# RUN: llvm-objcopy -S %t %t2
srhinesUnsubmitted
Done
ReplyInline Actions

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.

srhines: There's already a %t2, so you shouldn't reuse it. I also suggest you just cmp against %t2…
# RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s
!ELF !ELF
FileHeader: FileHeader:
Class: ELFCLASS64 Class: ELFCLASS64
Data: ELFDATA2LSB Data: ELFDATA2LSB
Type: ET_REL Type: ET_REL
Machine: EM_X86_64 Machine: EM_X86_64
Sections: Sections:
▲ Show 20 Lines • Show All 43 Lines • Show Last 20 Lines

tools/llvm-objcopy/ObjcopyOpts.td

Show First 20 Lines • Show All 61 Lines • ▼ Show 20 Linesdef j : JoinedOrSeparate<["-"], "j">,
Alias<only_keep>; Alias<only_keep>;
defm add_section : Eq<"add-section">, defm add_section : Eq<"add-section">,
MetaVarName<"section=file">, MetaVarName<"section=file">,
HelpText<"Make a section named <section> with the contents of <file>.">; HelpText<"Make a section named <section> with the contents of <file>.">;
def strip_all : Flag<["-", "--"], "strip-all">, def strip_all : Flag<["-", "--"], "strip-all">,
HelpText<"Remove non-allocated sections other than .gnu.warning* sections">; HelpText<"Remove non-allocated sections other than .gnu.warning* sections">;
def strip_all_gnu : Flag<["-", "--"], "strip-all-gnu">, def strip_all_gnu : Flag<["-", "--"], "strip-all-gnu">,
HelpText<"Compaitable with GNU objcopy's --strip-all">; HelpText<"Compaitable with GNU objcopy's --strip-all">;
def S : JoinedOrSeparate<["-"], "S">,
alexander-shaposhnikovUnsubmitted
Done
ReplyInline Actions

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 ?

alexander-shaposhnikov: btw - it probably should not be JoinedOrSeparate (since it doesn't take any argument's values)…
Alias<strip_all_gnu>;
def strip_debug : Flag<["-", "--"], "strip-debug">, def strip_debug : Flag<["-", "--"], "strip-debug">,
HelpText<"Remove all debug information">; HelpText<"Remove all debug information">;
def strip_dwo : Flag<["-", "--"], "strip-dwo">, def strip_dwo : Flag<["-", "--"], "strip-dwo">,
HelpText<"Remove all DWARF .dwo sections from file">; HelpText<"Remove all DWARF .dwo sections from file">;
def strip_sections : Flag<["-", "--"], "strip-sections">, def strip_sections : Flag<["-", "--"], "strip-sections">,
HelpText<"Remove all section headers">; HelpText<"Remove all section headers">;
def strip_non_alloc : Flag<["-", "--"], "strip-non-alloc">, def strip_non_alloc : Flag<["-", "--"], "strip-non-alloc">,
HelpText<"Remove all non-allocated sections">; HelpText<"Remove all non-allocated sections">;
▲ Show 20 Lines • Show All 63 Lines • Show Last 20 Lines