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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/response-file.test:3
+# RUN: echo "--help" > %t-response
+# RUN: llvm-objcopy @%t-response | FileCheck %s
+# RUN: llvm-strip @%t-response | FileCheck %s
----------------
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.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:411
+static void printHelp(const opt::OptTable &OptTable, raw_ostream &OS,
+                      StringRef name) {
+  Twine Usage = "llvm-" + name + " input [output]";
----------------
name -> Name (or possibly even "ToolName").


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:412
+                      StringRef name) {
+  Twine Usage = "llvm-" + name + " input [output]";
+  Twine Title = name + " tool";
----------------
jhenderson wrote:
> 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..
I'd pass them in as llvm-objcopy and llvm-strip here.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:412-413
+                      StringRef name) {
+  Twine Usage = "llvm-" + name + " input [output]";
+  Twine Title = name + " tool";
+  OptTable.PrintHelp(OS, Usage.str().c_str(), Title.str().c_str());
----------------
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..


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:316-326
+  // Expand response files.
+  SmallVector<const char *, 20> newArgv(argv, argv + argc);
+  BumpPtrAllocator A;
+  StringSaver Saver(A);
+  cl::ExpandResponseFiles(Saver,
+                          Triple(sys::getProcessTriple()).isOSWindows()
+                              ? cl::TokenizeWindowsCommandLine
----------------
mmpozulp wrote:
> 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? 
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.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:329-331
+      IsStrip
+          ? parseStripOptions(makeArrayRef(argv + 1, argc - 1), reportWarning)
+          : parseObjcopyOptions(makeArrayRef(argv + 1, argc - 1));
----------------
Is this change related? It looks like it's just formatting, so you shouldn't change it as part of this change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65372/new/

https://reviews.llvm.org/D65372