[llvm-objcopy] Add support for response files in llvm-strip and llvm-objcopy

[mmpozulp]

Summary

Addresses https://bugs.llvm.org/show_bug.cgi?id=42671

[mmpozulp]

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

316–326 ↗

(On Diff #212074)

I copied these lines from lib/Support/CommandLine.cpp where they appear as

bool CommandLineParser::ParseCommandLineOptions(int argc,
                                                const char *const *argv,
                                                StringRef Overview,
                                                raw_ostream *Errs,
                                                bool LongOptionsUseDoubleDash) {
  assert(hasOptions() && "No options specified!");

  // Expand response files.
  SmallVector<const char *, 20> newArgv(argv, argv + argc);
  BumpPtrAllocator A;
  StringSaver Saver(A);
  ExpandResponseFiles(Saver,
         Triple(sys::getProcessTriple()).isOSWindows() ?
         cl::TokenizeWindowsCommandLine : cl::TokenizeGNUCommandLine,
         newArgv);
  argv = &newArgv[0];
  argc = static_cast<int>(newArgv.size());
  ...

Should these lines be a separate function in the CommandLine library, or is it better to be repetitive here?

[jhenderson]

llvm/test/tools/llvm-objcopy/ELF/response-file.test

3 ↗

(On Diff #212074)

I see what you're trying to do here, but I don't think this is a good way of testing response files, because an invalid command-line might lead to the tool printing its help/usage text, which could include "@FILE" in it. I'd recommend showing that a basic operation can be performed (e.g. a copy with --strip-debug).

It might be worth a test showing that "@FILE" appears when --help is explicitly passed on the command-line however.

llvm/tools/llvm-objcopy/CopyConfig.cpp

411 ↗

(On Diff #212074)

name -> Name (or possibly even "ToolName").

412 ↗

(On Diff #212074)

I'd pass them in as llvm-objcopy and llvm-strip here.

412–413 ↗

(On Diff #212074)

Twines are only intended to be used for temporaries essentially (or as function parameters which are passed in as temporaries). I believe what you are doing here may not be safe. You should do it inline at the usage point, since you only use them once..

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

316–326 ↗

(On Diff #212074)

It looks like there are quite a few tools that call cl::ExpandResponseFiles directly themselves. It seems logical to refactor them all at once to use the same version. You might want to see if you can do that, rather than just copying this code here.

329–331 ↗

(On Diff #212074)

Is this change related? It looks like it's just formatting, so you shouldn't change it as part of this change.

[mmpozulp]

Incorporate feedback from @jhenderson. I still need to try refactoring tools which call cl::ExpandResponseFiles to avoid code duplication. If successful then we won't have to continue the code duplication trend here.

[mmpozulp]

Capitalize variable newArgv -> NewArgv

[mmpozulp]

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

329–331 ↗

(On Diff #212074)

Yes. Without it a bunch of tests fail with the message error: too many positional arguments. In the calls to parseStripOptions and parseObjcopyOptions we were passing argc instead of argc - 1 which was a bug. It didn't manifest because argv[argc] was a null pointer. Now argv[argc] is junk after we set argv:

   311 	int main(int argc, char **argv) {
-> 312 	  InitLLVM X(argc, argv);
   313 	  ToolName = argv[0];
   314 	  bool IsStrip = sys::path::stem(ToolName).contains("strip");
   315 	
(lldb) p argv[argc]
(char *) $0 = 0x0000000000000000
(lldb) cont
Process 8459 resuming
Process 8459 stopped
* thread #1, name = 'llvm-objcopy', stop reason = breakpoint 2.1
    frame #0: 0x00005555555f3bcf llvm-objcopy`main(argc=9, argv=0x00007fffffffe400) at llvm-objcopy.cpp:326:39
   323 	                              : cl::TokenizeGNUCommandLine,
   324 	                          NewArgv);
   325 	  argv = const_cast<char **>(&NewArgv[0]);
-> 326 	  argc = static_cast<int>(NewArgv.size());
   327 	
   328 	  Expected<DriverConfig> DriverConfig =
   329 	      IsStrip
(lldb) p argv[argc]
(char *) $1 = 0x00007fffffffe470 "\xa8\xaa=VUU"

We see the junk and think its another positional argument... unless the junk string starts with a dash in which case the tool might complain that it doesn't understand the extra flag :)

[jhenderson]

Hi @mmpozulp,

When you've addressed review comments, it's helpful if you mark the "Done" box in the relevant comment. That way it's obvious what hasn't been addressed. If you do it before you upload, then they will get submitted when the patch gets updated.

It might be worth a test showing that "@FILE" appears when --help is explicitly passed on the command-line however.

Having thought about it, I definitely think I'd like this please. If it's not in the help text, it might as well not exist. You also need to update the documentation to show that it is supported (see similar changes I made recently to other tools).

llvm/test/tools/llvm-objcopy/ELF/response-file.test

7–8 ↗

(On Diff #214297)

This is a minor point, but we've tended to use llvm-readelf/llvm-readobj to check sections, IIRC. Also, it's probably worth doing a cmp to show that the two output files are identical.

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

11 ↗

(On Diff #214297)

Could you do this header reordering in a separate NFC commit please (no need to get a review).

[mmpozulp]

Rebaseline to get the new llvm-strip.rst docs page.
Incorporate feedback from @jhenderson to

1. Click "Done" boxes in phabricator. Thanks :)
2. Add @FILE check to help-message.test
3. Update llvm-strip.rst and llvm-objcopy.rst docs with @FILE option.
4. Remove the header re-ordering. I like it the way it was before.
5. Change response-file.test to a) use llvm-readobj to check sections,

and b) run cmp to show the output files are identical.
TODO: Try refactoring tools which call cl::ExpandResponseFiles to
avoid copy-pasting code from lib/Support/CommandLine.cpp like I did here.

[jhenderson]

LGTM, with a couple of nits. I'm okay with the refactoring being left to a different change, as long as you don't leave it a long time.

llvm/test/tools/llvm-objcopy/ELF/response-file.test

8 ↗

(On Diff #214439)

Strictly, you no longer need the second llvm-readobj line, since the cmp makes it redundant.

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

316 ↗

(On Diff #214439)

Perhaps worth adding a TODO note saying that this is duplicated code.

[rupprecht]

llvm/tools/llvm-objcopy/CopyConfig.cpp

414 ↗

(On Diff #214439)

This method is a bit of a hack, as the older library has a cl::extrahelp class that's usually used to support this [1], but libOption doesn't have yet. We should probably add one. Can you add a comment here to remove this once something like that is supported?

[1] e.g. https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-size/llvm-size.cpp#L102

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

325–331 ↗

(On Diff #214439)

Instead of manipulating argc/argv directly, you can convert NewArgv directly to an array ref to pass to parse*Options, e.g.

auto Args = makeArrayRef(NewArgv).drop_front();
Expected<DriverConfig> DriverConfig =
    IsStrip ? parseStripOptions(Args, reportWarning)
            : parseObjcopyOptions(Args);

[MaskRay]

Ping

[mmpozulp]

In D65372#1664237, @MaskRay wrote:

Ping

Thanks for the ping :) I'll try to update the patch this weekend.

[mmpozulp]

Incorporate feedback from @jhenderson and @rupprecht

llvm/trunk/docs/CommandGuide/llvm-objcopy.rst

 
  Remove from the output all local or undefined symbols that are not required by
  relocations. Also remove all debug sections.
 
 .. option:: --version, -V
 
   Display the version of this program.
 
+.. option:: @<FILE>
+
+  Read command-line options and commands from response file `<FILE>`.
+
 COFF-SPECIFIC OPTIONS
 ---------------------
 
 The following options are implemented only for COFF objects. If used with other
 objects, :program:`llvm-objcopy` will either emit an error or silently ignore
 them.
 
 .. option:: --only-keep-debug

llvm/trunk/docs/CommandGuide/llvm-strip.rst

 
  Remove from the output all local or undefined symbols that are not required by
  relocations. Also remove all debug sections.
 
 .. option:: --version, -V
 
  Display the version of this program.
 
+.. option:: @<FILE>
+
+  Read command-line options and commands from response file `<FILE>`.
+
 COFF-SPECIFIC OPTIONS
 ---------------------
 
 The following options are implemented only for COFF objects. If used with other
 objects, :program:`llvm-strip` will either emit an error or silently ignore
 them.
 
 .. option:: --only-keep-debug

llvm/trunk/test/tools/llvm-objcopy/ELF/help-message.test

 # RUN: llvm-objcopy -h | FileCheck --check-prefix=OBJCOPY-USAGE %s
 # RUN: llvm-objcopy --help | FileCheck --check-prefix=OBJCOPY-USAGE %s
 # RUN: not llvm-objcopy 2>&1 | FileCheck --check-prefix=OBJCOPY-USAGE %s
 # RUN: not llvm-objcopy -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-objcopy --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-objcopy --strip-debug 2>&1 | FileCheck %s --check-prefix=NO-INPUT-FILES
 
 # RUN: llvm-strip -h | FileCheck --check-prefix=STRIP-USAGE %s
 # RUN: llvm-strip --help | FileCheck --check-prefix=STRIP-USAGE %s
 # RUN: not llvm-strip 2>&1 | FileCheck --check-prefix=STRIP-USAGE %s
 # RUN: not llvm-strip -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-strip --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
 # RUN: not llvm-strip --strip-debug 2>&1 | FileCheck %s --check-prefix=NO-INPUT-FILES
 
 
 # OBJCOPY-USAGE:  USAGE: llvm-objcopy
+# OBJCOPY-USAGE:  @FILE
+
 # STRIP-USAGE:    USAGE: llvm-strip
+# STRIP-USAGE:    @FILE
+
 # UNKNOWN-ARG:    unknown argument '{{-+}}abcabc'
 # NO-INPUT-FILES: no input file specified

llvm/trunk/test/tools/llvm-objcopy/ELF/response-file.test

+## Check that we support response files.
+# RUN: yaml2obj %s -o %t.o
+# RUN: echo "--strip-debug %t.o" > %t-response
+# RUN: llvm-objcopy @%t-response %t2.o
+# RUN: llvm-strip @%t-response
+
+# RUN: llvm-readobj -S %t.o | FileCheck %s
+# RUN: cmp %t.o %t2.o
+
+# CHECK-NOT: .debug_foo
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: .debug_foo
+    Type: SHT_PROGBITS

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

 
 template <class T> static ErrorOr<T> getAsInteger(StringRef Val) {
   T Result;
   if (Val.getAsInteger(0, Result))
     return errc::invalid_argument;
   return Result;
 }
 
+static void printHelp(const opt::OptTable &OptTable, raw_ostream &OS,
+                      StringRef ToolName) {
+  OptTable.PrintHelp(OS, (ToolName + " input [output]").str().c_str(),
+                     (ToolName + " tool").str().c_str());
+  // TODO: Replace this with libOption call once it adds extrahelp support.
+  // The CommandLine library has a cl::extrahelp class to support this,
+  // but libOption does not have that yet.
+  OS << "\nPass @FILE as argument to read options from FILE.\n";
+}
+
 // ParseObjcopyOptions returns the config and sets the input arguments. If a
 // help flag is set then ParseObjcopyOptions will print the help messege and
 // exit.
 Expected<DriverConfig> parseObjcopyOptions(ArrayRef<const char *> ArgsArr) {
   DriverConfig DC;
   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");
+    printHelp(T, errs(), "llvm-objcopy");
     exit(1);
   }
 
   if (InputArgs.hasArg(OBJCOPY_help)) {
-    T.PrintHelp(outs(), "llvm-objcopy input [output]", "objcopy tool");
+    printHelp(T, outs(), "llvm-objcopy");
     exit(0);
   }
 
   if (InputArgs.hasArg(OBJCOPY_version)) {
     outs() << "llvm-objcopy, compatible with GNU objcopy\n";
     cl::PrintVersionMessage();
     exit(0);
   }

 parseStripOptions(ArrayRef<const char *> ArgsArr,
                   std::function<Error(Error)> ErrorCallback) {
   StripOptTable T;
   unsigned MissingArgumentIndex, MissingArgumentCount;
   llvm::opt::InputArgList InputArgs =
       T.ParseArgs(ArgsArr, MissingArgumentIndex, MissingArgumentCount);
 
   if (InputArgs.size() == 0) {
-    T.PrintHelp(errs(), "llvm-strip [options] file...", "strip tool");
+    printHelp(T, errs(), "llvm-strip");
     exit(1);
   }
 
   if (InputArgs.hasArg(STRIP_help)) {
-    T.PrintHelp(outs(), "llvm-strip [options] file...", "strip tool");
+    printHelp(T, outs(), "llvm-strip");
     exit(0);
   }
 
   if (InputArgs.hasArg(STRIP_version)) {
     outs() << "llvm-strip, compatible with GNU strip\n";
     cl::PrintVersionMessage();
     exit(0);
   }

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

 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Object/Error.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Memory.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/StringSaver.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdlib>
 #include <memory>
 #include <string>
 #include <system_error>
static Error executeObjcopy(const CopyConfig &Config) {
 
   return Error::success();
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];
   bool IsStrip = sys::path::stem(ToolName).contains("strip");
+
+  // Expand response files.
+  // TODO: Move these lines, which are copied from lib/Support/CommandLine.cpp,
+  // into a separate function in the CommandLine library and call that function
+  // here. This is duplicated code.
+  SmallVector<const char *, 20> NewArgv(argv, argv + argc);
+  BumpPtrAllocator A;
+  StringSaver Saver(A);
+  cl::ExpandResponseFiles(Saver,
+                          Triple(sys::getProcessTriple()).isOSWindows()
+                              ? cl::TokenizeWindowsCommandLine
+                              : cl::TokenizeGNUCommandLine,
+                          NewArgv);
+
+  auto Args = makeArrayRef(NewArgv).drop_front();
+
   Expected<DriverConfig> DriverConfig =
-      IsStrip ? parseStripOptions(makeArrayRef(argv + 1, argc), reportWarning)
-              : parseObjcopyOptions(makeArrayRef(argv + 1, argc));
+      IsStrip ? parseStripOptions(Args, reportWarning)
+              : parseObjcopyOptions(Args);
   if (!DriverConfig) {
     logAllUnhandledErrors(DriverConfig.takeError(),
                           WithColor::error(errs(), ToolName));
     return 1;
   }
   for (const CopyConfig &CopyConfig : DriverConfig->CopyConfigs) {
     if (Error E = executeObjcopy(CopyConfig)) {
       logAllUnhandledErrors(std::move(E), WithColor::error(errs(), ToolName));
       return 1;
     }
   }
 
   return 0;
 }