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

[Mike Pozulp via Phabricator via llvm-commits]

mmpozulp marked an inline comment as done.
mmpozulp added inline comments.


================
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));
----------------
jhenderson wrote:
> Is this change related? It looks like it's just formatting, so you shouldn't change it as part of this change.
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 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65372