[llvm-strip] Add -o option

[alexander-shaposhnikov]

Summary

This diff implements -o <file> option https://sourceware.org/binutils/docs/binutils/strip.html .

Test plan: make check-all

[alexander-shaposhnikov]

fix typo + clang-format

[paulsemel]

Just two nits for me

tools/llvm-objcopy/StripOpts.td

11–12

what about :

MetaVarName<"file">,
HelpText<"Output stripped file into <file>">;

tools/llvm-objcopy/llvm-objcopy.cpp

581–583

I think it should be :

Config.OutputFilename = InputArgs.getLastArgValue(STRIP_output, Positional[0]);

[jhenderson]

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

6–7

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

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]

tools/llvm-objcopy/StripOpts.td

11–12

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

[alexander-shaposhnikov]

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

6–7

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.

[alexander-shaposhnikov]

Make use of getLastArgValue

[jhenderson]

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

6–7

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

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

Perhaps "Write output to <file>"?

[alexander-shaposhnikov]

reuse unchanged file, update the help message

[jhenderson]

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.

[paulsemel]

LGTM

[alexander-shaposhnikov]

@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 ?

[jhenderson]

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.

[alexander-shaposhnikov]

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

[jhenderson]

LGTM. Thanks!

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

 # RUN: yaml2obj %s > %t
+# RUN: cp %t %t3
 # RUN: llvm-objcopy --strip-all %t %t2
 # RUN: llvm-readobj -file-headers -sections %t2 | FileCheck %s
 
-# We run yaml2obj again rather than copy %t to avoid interfering
-# with llvm-objcopy's test (which potentially could have corrupted/updated the binary).
+# Verify that the previous llvm-objcopy's run has not modified the input.
+# RUN: cmp %t %t3
 
-# RUN: yaml2obj %s > %t3
 # RUN: llvm-strip %t3 
-# RUN: llvm-readobj -file-headers -sections %t3 | FileCheck %s
 # RUN: cmp %t2 %t3
 
+# RUN: cp %t %t4
+# RUN: llvm-strip %t4 -o %t5
+# RUN: cmp %t2 %t5
+
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:
   - Name:            .bss

tools/llvm-objcopy/StripOpts.td

 include "llvm/Option/OptParser.td"
 
 multiclass Eq<string name> {
   def NAME: Separate<["--", "-"], name>;
   def NAME # _eq: Joined<["--", "-"], name # "=">, Alias<!cast<Separate>(NAME)>;
 }
 
 def help : Flag<["-", "--"], "help">;
 
+defm output : Eq<"o">,
+              MetaVarName<"output">,
+              HelpText<"Output file">;
+
 def strip_debug : Flag<["-", "--"], "strip-debug">,
                   HelpText<"Remove debugging symbols only">;
 
 defm remove_section : Eq<"remove-section">,
                       MetaVarName<"section">,
                       HelpText<"Remove <section>">;
 
 def R : JoinedOrSeparate<["-"], "R">,
         Alias<remove_section>;
 
 defm keep_symbol : Eq<"keep-symbol">,
                    MetaVarName<"symbol">,
                    HelpText<"Do not remove symbol <symbol>">;
 
 def K : JoinedOrSeparate<["-"], "K">,
         Alias<keep_symbol>;

tools/llvm-objcopy/llvm-objcopy.cpp

CopyConfig ParseStripOptions(ArrayRef<const char *> ArgsArr) {
   if (Positional.empty())
     error("No input file specified");
 
   if (Positional.size() > 2)
     error("Support for multiple input files is not implemented yet");
 
   CopyConfig Config;
   Config.InputFilename = Positional[0];
-  Config.OutputFilename = Positional[0];
+  Config.OutputFilename = InputArgs.hasArg(STRIP_output)
+                              ? InputArgs.getLastArg(STRIP_output)->getValue()
+                              : Positional[0];
 
   // Strip debug info only.
   Config.StripDebug = InputArgs.hasArg(STRIP_strip_debug);
   if (!Config.StripDebug)
     Config.StripAll = true;
 
   for (auto Arg : InputArgs.filtered(STRIP_remove_section))
     Config.ToRemove.push_back(Arg->getValue());