Page MenuHomePhabricator

[llvm-strip] Add -o option
ClosedPublic

Authored by alexshap on May 29 2018, 4:40 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

alexshap created this revision.May 29 2018, 4:40 PM
alexshap updated this revision to Diff 149038.May 29 2018, 9:45 PM

fix typo + clang-format

Just two nits for me

tools/llvm-objcopy/StripOpts.td
11–12 ↗(On Diff #149038)

what about :

MetaVarName<"file">,
HelpText<"Output stripped file into <file>">;
tools/llvm-objcopy/llvm-objcopy.cpp
581–583 ↗(On Diff #149038)

I think it should be :

Config.OutputFilename = InputArgs.getLastArgValue(STRIP_output, Positional[0]);
jhenderson added inline comments.May 30 2018, 1:59 AM
test/tools/llvm-objcopy/strip-all.test
6–7 ↗(On Diff #149038)

I thought that we decided in a previous review not to do this, but instead to do what the old test is doing? Or am I misremembering?

tools/llvm-objcopy/StripOpts.td
11–12 ↗(On Diff #149038)

I think we should avoid mentioning "stripped" in the help text, because it's possible even now to use switches to prevent any actual stripping from occurring.

paulsemel added inline comments.May 30 2018, 3:56 AM
tools/llvm-objcopy/StripOpts.td
11–12 ↗(On Diff #149038)

Fair enough, but I think this might still be changed to be more verbose :)

alexshap added inline comments.May 30 2018, 8:18 AM
test/tools/llvm-objcopy/strip-all.test
6–7 ↗(On Diff #149038)

I think you are misremembering. We decided to avoid what the old test was doing.

The motivation was (if I'm not mistaken) that it's not really necessary to run yaml2obj twice and then do text matching twice.

alexshap updated this revision to Diff 149122.May 30 2018, 8:23 AM

Make use of getLastArgValue

jhenderson added inline comments.May 30 2018, 8:24 AM
test/tools/llvm-objcopy/strip-all.test
6–7 ↗(On Diff #149038)

Fair enough! I knew we'd decided to do it one way around, and not the other, so I was surprised to see a change.

12 ↗(On Diff #149038)

I wonder if we need this extra copy here? We could probably just use %t at this point, as we have checked that it hasn't been modified. I'm not bothered though if you prefer it.

tools/llvm-objcopy/StripOpts.td
11–12 ↗(On Diff #149038)

Perhaps "Write output to <file>"?

alexshap updated this revision to Diff 149126.May 30 2018, 8:31 AM

reuse unchanged file, update the help message

This is probably a bit nit-picky, but we should probably have a test to show that -o a.o -o b.o only results in one output file, namely b.o. Otherwise, looks fine to me.

This revision is now accepted and ready to land.May 30 2018, 8:51 AM

@jhenderson - in your opinion, what is the best portable way to check that a file doesn't exist - i mean what is your personal preference ?

I don't have a particular preference, and I don't know of an easy way to test a file doesn't exist in lit. However, one possibility might be to copy %t to some location (say %t-should-remain-the-same) and then do something like llvm-strip %t-whatever -o %t-should-remain-the-same -o %t-should-be-stripped, and then do a diff of the one that should not be changed against %t.

alexshap updated this revision to Diff 149140.May 30 2018, 9:24 AM

llvm-strip %t4 -o %t-should-remain-the-same -o %t-should-be-stripped

jhenderson accepted this revision.May 31 2018, 1:02 AM

LGTM. Thanks!

This revision was automatically updated to reflect the committed changes.