[master-next] Fix up the driver's use of PROGRAM_START after PR #16362 #17494
Conversation
…lang#16362 On the master-next branch, the InitLLVM class used by the PROGRAM_START macro can modify the argc/argv values (currently only on Windows). Expanding response files before initializing the stack trace also modifies the arguments so use a separate SmallVector for each copy of the argument vector.
|
I'm going to go ahead and merge this to un-break the master-next build, but Jordan, please let me know if you have any comments or suggestions on this. |
| @@ -180,7 +180,7 @@ static int run_driver(StringRef ExecName, | |||
| } | |||
|
|
|||
| int main(int argc_, const char **argv_) { | |||
| SmallVector<const char *, 256> argv(&argv_[0], &argv_[argc_]); | |||
| SmallVector<const char *, 256> args(&argv_[0], &argv_[argc_]); | |||
There was a problem hiding this comment.
Can you give this a more descriptive name, then? It's not good to have args and argv both floating around in this function.
Jordan reviewed my change and suggested using a better variable name to avoid potential confusion between "args" and "argv".
[master-next] NFC: Rename variable from PR #17494
|
@jrose-apple I am confused by the changes made here. In #16362 we refactored |
|
The master-next branch mostly just needs to stay working to make sure upstream LLVM doesn't break us in some way. I guess I should have filed a bug to come back and clean this up. |
|
FWIW, I compared against other LLVM tools to see how they handle this. At least for the tools that I looked at (e.g., llvm-ar), the pretty stack trace info is initialized before expanding the response files. Austin's change in 3fe62b9 specifically moved the PROGRAM_START macro so that the stack trace info would be initialized after expanding the response files. Since that was clearly an intentional decision, I figured it was better to copy the arguments to preserve that behavior. Maybe there's a clever way to do it without a copy, but that seemed like the most straightforward solution. |
|
I think the point is that we need to copy them once to expand response files, but we shouldn't need to copy a second time for this. |
|
I understand that, but with recent versions of LLVM (i.e., master-next), the llvm::InitLLVM constructor used by the PROGRAM_START macro can rewrite the arguments on some platforms, so there's a separate copy for that. |
| const char **ExpandedArgv = args.data(); | ||
| PROGRAM_START(ExpandedArgc, ExpandedArgv); | ||
| SmallVector<const char *, 256> argv(&ExpandedArgv[0], | ||
| &ExpandedArgv[ExpandedArgc]); |
There was a problem hiding this comment.
There is specifically no need for this SmallVector. An ArrayRef would be fine.
There was a problem hiding this comment.
Oh. Yeah, of course. #17728
On the master-next branch, the InitLLVM class used by the PROGRAM_START
macro can modify the argc/argv values (currently only on Windows).
Expanding response files before initializing the stack trace also modifies
the arguments so use a separate SmallVector for each copy of the argument
vector.