[llvm-strip] Expose --discard-all option

[alexander-shaposhnikov]

Summary

Expose objcopy's --discard-all option in llvm-strip.
Test plan: make check-all

[alexander-shaposhnikov]

Fix typo

[jhenderson]

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

14

Why is the -d here?

Could you also test -x with llvm-strip, please?

[alexander-shaposhnikov]

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

14

Why is the -d here?

@jhenderson - otherwise strip will remove the symbol table and we won't be able to test how it's getting modified.

[paulsemel]

First, thanks for the patch :)
I think @jhenderson pointed out something important. If you do strip -x foo -o bar, you will notice that it will only discard local symbols from the binary.
This is not really what's happening with this behavior.. (as you mentioned, this removes symbol table, and that's why you can't test the -x option alone)
Maybe we should fix this ? What do you think ? :)

tools/llvm-objcopy/llvm-objcopy.cpp

588–592

Maybe you should do something like (see my comment above for the reason):

Config.DiscardAll = InputArgs.hasArg(STRIP_discard_all);
if (!Config.StripDebug && !Config.DiscardAll)
  Config.StripAll = true;

594–595

And thus remove those lines :)

[alexander-shaposhnikov]

right, I'll update the patch & tests

[alexander-shaposhnikov]

Fix the behavior of strip -x

[jhenderson]

LGTM. I think the updated version makes a lot more sense (why discard the local symbols if you are going to remove the symbol table anyway?).

[paulsemel]

LGTM too :)

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

 # RUN: yaml2obj %s > %t
+# RUN: cp %t %t1
 # RUN: llvm-objcopy --discard-all %t %t2
-# RUN: llvm-objcopy -x %t %t3
+# Verify that llvm-objcopy has not modified the input.
+# RUN: cmp %t %t1
 # RUN: llvm-readobj -symbols %t2 | FileCheck %s
-# RUN: diff %t2 %t3
+
+# RUN: llvm-objcopy -x %t %t3
+# Verify that llvm-objcopy has not modified the input.
+# RUN: cmp %t %t1
+# RUN: cmp %t2 %t3
+
+# Verify that llvm-strip modifies the symbol table the same way.
+
+# RUN: cp %t %t4
+# RUN: llvm-strip --discard-all %t4
+# RUN: cmp %t2 %t4
+
+# RUN: llvm-strip -x %t
+# RUN: cmp %t2 %t
 
 !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_REL
   Machine:         EM_X86_64
 Sections:

tools/llvm-objcopy/StripOpts.td

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>;
+
+def discard_all : Flag<["-", "--"], "discard-all">,
+                  HelpText<"Remove all local symbols except file and section symbols">;
+def x : Flag<["-"], "x">,
+        Alias<discard_all>;

tools/llvm-objcopy/llvm-objcopy.cpp

CopyConfig ParseStripOptions(ArrayRef<const char *> ArgsArr) {
 
   CopyConfig Config;
   Config.InputFilename = Positional[0];
   Config.OutputFilename =
       InputArgs.getLastArgValue(STRIP_output, Positional[0]);
 
   // Strip debug info only.
   Config.StripDebug = InputArgs.hasArg(STRIP_strip_debug);
-  if (!Config.StripDebug)
+  
+  Config.DiscardAll = InputArgs.hasArg(STRIP_discard_all);
+  
+  if (!Config.StripDebug && !Config.DiscardAll)
     Config.StripAll = true;
 
   for (auto Arg : InputArgs.filtered(STRIP_remove_section))
     Config.ToRemove.push_back(Arg->getValue());
 
   for (auto Arg : InputArgs.filtered(STRIP_keep_symbol))
     Config.SymbolsToKeep.push_back(Arg->getValue());
 
   return Config;
 }
 
 int main(int argc, char **argv) {