[llvm-strip] Support stripping multiple input files

[rupprecht]

Summary

Allow strip to be called on multiple input files, which is interpreted as stripping N files in place. Using multiple input files is incompatible with -o.

To allow this, create a DriverConfig struct which just wraps a list of CopyConfigs. objcopy will only ever have a single CopyConfig, but strip will have N (where N >= 1) CopyConfigs.

[rupprecht]

• Add test case for -o flag error

[alexander-shaposhnikov]

tools/llvm-objcopy/llvm-objcopy.cpp

185 ↗

(On Diff #163935)

nit: SmallVector<CopyConfig, 1>

[rupprecht]

• Use SmallVector for CopyConfigs

[alexander-shaposhnikov]

test/tools/llvm-objcopy/strip-multiple-files.test

9 ↗

(On Diff #163941)

not sure if these checks are particularly useful,
i'd probably reorganize and simplify (and, actually, speed up) this test a little bit:

1. run llvm-strip on one .o
2. run llvm-readobj on the output and verify that the output is correct (via FileCheck)
3. run llvm-strip on 3 .o and verify that the outputs are correct (via cmp ) (compare them with the result of (2))

What would you say to this approach ?

[rupprecht]

• Simplify test

[rupprecht]

test/tools/llvm-objcopy/strip-multiple-files.test

9 ↗

(On Diff #163941)

Thanks, that makes it look nicer than what I have here.

I left in the case where we just have two files, since I want to make sure that edge case is tested (e.g. "llvm-strip a b" isn't somehow interpreted as "llvm-strip a -o b")

[alexander-shaposhnikov]

To me looks good, but please wait for Jake.

[jakehehrlich]

Whoops...I meant to say the following already. I was confused why it was missing my LGTM.

Below no longer relevant:
I like the change, I'm quite satisfied with the coverage given by your test. You're welcome to optimize the test a bit as Alex has outlined (I would be satisfied with that testing strategy as well).

llvm/trunk/test/tools/llvm-objcopy/strip-multiple-files.test

+# RUN: yaml2obj %s > %t.o
+
+# Run llvm-strip normally. This will create a stripped object file for later
+# tests so we only have to run FileCheck on it once.
+# RUN: cp %t.o %t.1.o
+# RUN: llvm-strip --keep-symbol=foo %t.1.o -o %t.stripped.o
+# RUN: llvm-readobj -symbols %t.stripped.o | FileCheck %s
+
+# llvm-strip on two files:
+# RUN: cp %t.o %t.1.o
+# RUN: cp %t.o %t.2.o
+# RUN: llvm-strip --keep-symbol=foo %t.1.o %t.2.o
+# RUN: cmp %t.1.o %t.stripped.o
+# RUN: cmp %t.2.o %t.stripped.o
+
+# llvm-strip on three files:
+# RUN: cp %t.o %t.1.o
+# RUN: cp %t.o %t.2.o
+# RUN: cp %t.o %t.3.o
+# RUN: llvm-strip --keep-symbol=foo %t.1.o %t.2.o %t.3.o
+# RUN: cmp %t.1.o %t.stripped.o
+# RUN: cmp %t.2.o %t.stripped.o
+# RUN: cmp %t.3.o %t.stripped.o
+
+# -o cannot be used with multiple input files
+# RUN: cp %t.o %t.1.o
+# RUN: cp %t.o %t.2.o
+# RUN: not llvm-strip --keep-symbol=foo -o %t.stripped.o %t.1.o %t.2.o 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=BAD-O-FLAG
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+Symbols:
+  Local:
+    - Name:     foo
+      Type:     STT_FUNC
+      Section:  .text
+      Value:    0x1234
+      Size:     8
+    - Name:     bar
+      Type:     STT_FUNC
+      Section:  .text
+      Value:    0x5678
+      Size:     8
+
+# CHECK:      Symbols [
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name:
+# CHECK-NEXT:     Value: 0x0
+# CHECK-NEXT:     Size: 0
+# CHECK-NEXT:     Binding: Local
+# CHECK-NEXT:     Type: None
+# CHECK-NEXT:     Other: 0
+# CHECK-NEXT:     Section: Undefined
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Symbol {
+# CHECK-NEXT:     Name: foo
+# CHECK-NEXT:     Value: 0x1234
+# CHECK-NEXT:     Size: 8
+# CHECK-NEXT:     Binding: Local
+# CHECK-NEXT:     Type: Function
+# CHECK-NEXT:     Other: 0
+# CHECK-NEXT:     Section: .text
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+# BAD-O-FLAG: Multiple input files cannot be used in combination with -o.

llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp

 };
 
 struct SectionRename {
   StringRef OriginalName;
   StringRef NewName;
   Optional<uint64_t> NewFlags;
 };
 
+// Configuration for copying/stripping a single file.
 struct CopyConfig {
   // Main input/output options
   StringRef InputFilename;
   StringRef InputFormat;
   StringRef OutputFilename;
   StringRef OutputFormat;
 
   // Only applicable for --input-format=Binary
struct CopyConfig {
   bool StripDWO = false;
   bool StripDebug = false;
   bool StripNonAlloc = false;
   bool StripSections = false;
   bool StripUnneeded = false;
   bool Weaken = false;
 };
 
+// Configuration for the overall invocation of this tool. When invoked as
+// objcopy, will always contain exactly one CopyConfig. When invoked as strip,
+// will contain one or more CopyConfigs.
+struct DriverConfig {
+  SmallVector<CopyConfig, 1> CopyConfigs;
+};
+
 using SectionPred = std::function<bool(const SectionBase &Sec)>;
 
 enum SectionFlag {
   SecNone = 0,
   SecAlloc = 1 << 0,
   SecLoad = 1 << 1,
   SecNoload = 1 << 2,
   SecReadonly = 1 << 3,
for (StringRef Line : Lines) {
     if (!TrimmedLine.empty())
       Symbols.push_back(TrimmedLine.str());
   }
 }
 
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
-static CopyConfig parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
+static DriverConfig parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   ObjcopyOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
     T.PrintHelp(errs(), "llvm-objcopy <input> [ <output> ]", "objcopy tool");
     exit(1);
for (auto Arg : InputArgs.filtered(OBJCOPY_weaken_symbol))
     Config.SymbolsToWeaken.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_strip_symbol))
     Config.SymbolsToRemove.push_back(Arg->getValue());
   for (auto Arg : InputArgs.filtered(OBJCOPY_keep_symbol))
     Config.SymbolsToKeep.push_back(Arg->getValue());
 
   Config.PreserveDates = InputArgs.hasArg(OBJCOPY_preserve_dates);
 
-  return Config;
+  DriverConfig DC;
+  DC.CopyConfigs.push_back(std::move(Config));
+  return DC;
 }
 
 // ParseStripOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseStripOptions will print the help messege and
 // exit.
-static CopyConfig parseStripOptions(ArrayRef<const char *> ArgsArr) {
+static DriverConfig parseStripOptions(ArrayRef<const char *> ArgsArr) {
   StripOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
     T.PrintHelp(errs(), "llvm-strip", "strip tool");
     exit(1);
   }
 
   if (InputArgs.hasArg(STRIP_help)) {
     T.PrintHelp(outs(), "llvm-strip", "strip tool");
     exit(0);
   }
 
   SmallVector<const char *, 2> Positional;
   for (auto Arg : InputArgs.filtered(STRIP_UNKNOWN))
     error("unknown argument '" + Arg->getAsString(InputArgs) + "'");
   for (auto Arg : InputArgs.filtered(STRIP_INPUT))
     Positional.push_back(Arg->getValue());
 
   if (Positional.empty())
     error("No input file specified");
 
-  if (Positional.size() > 1)
-    error("Support for multiple input files is not implemented yet");
+  if (Positional.size() > 1 && InputArgs.hasArg(STRIP_output))
+    error("Multiple input files cannot be used in combination with -o");
 
   CopyConfig Config;
-  Config.InputFilename = Positional[0];
-  Config.OutputFilename =
-      InputArgs.getLastArgValue(STRIP_output, Positional[0]);
-
   Config.StripDebug = InputArgs.hasArg(STRIP_strip_debug);
 
   Config.DiscardAll = InputArgs.hasArg(STRIP_discard_all);
   Config.StripUnneeded = InputArgs.hasArg(STRIP_strip_unneeded);
   Config.StripAll = InputArgs.hasArg(STRIP_strip_all);
 
   if (!Config.StripDebug && !Config.StripUnneeded && !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());
 
   Config.PreserveDates = InputArgs.hasArg(STRIP_preserve_dates);
 
-  return Config;
+  DriverConfig DC;
+  if (Positional.size() == 1) {
+    Config.InputFilename = Positional[0];
+    Config.OutputFilename =
+        InputArgs.getLastArgValue(STRIP_output, Positional[0]);
+    DC.CopyConfigs.push_back(std::move(Config));
+  } else {
+    for (const char *Filename : Positional) {
+      Config.InputFilename = Filename;
+      Config.OutputFilename = Filename;
+      DC.CopyConfigs.push_back(Config);
+    }
+  }
+
+  return DC;
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];
-  CopyConfig Config;
+  DriverConfig DriverConfig;
   if (sys::path::stem(ToolName).endswith_lower("strip"))
-    Config = parseStripOptions(makeArrayRef(argv + 1, argc));
+    DriverConfig = parseStripOptions(makeArrayRef(argv + 1, argc));
   else
-    Config = parseObjcopyOptions(makeArrayRef(argv + 1, argc));
-  executeElfObjcopy(Config);
+    DriverConfig = parseObjcopyOptions(makeArrayRef(argv + 1, argc));
+  for (const CopyConfig &CopyConfig : DriverConfig.CopyConfigs)
+    executeElfObjcopy(CopyConfig);
 }