Fix Response Files on Windows #22985
Conversation
tools/driver/driver.cpp
Outdated
| #ifdef _WIN32 | ||
| // PROGRAM_START/InitLLVM overwrites the passed arguments with the unexpanded | ||
| // commandline arguments. Since we already handle getting utf-8 arguments, we | ||
| // overwrite the changed arguments again. |
There was a problem hiding this comment.
This seems wrong for a few reasons:
- the Swift-driver-written response files are going to be in UTF-8, aren't they?
- the PrettyStackTraceProgram inside InitLLVM isn't going to have the expanded arguments, which we were very much trying to support
- this is now doing redundant work
Maybe we shouldn't be using InitLLVM.
There was a problem hiding this comment.
- The response files are expected to be in UTF-8. This converts the file name argument to UTF16 but leaves the expanded response files alone.
- This does actually register the right arguments, you get the expanded arguments in "Program Arguments:". InitLLVM overwrites the arguments you pass it, but not the ones it registers with the StackPrinter. I'm unsure why.
- Yup, it's completely redundant. Now that I think about it, it would be slightly better to make copies of the expanded arguments and pass those to InitLLVM. But alternatively, it wouldn't be difficult to have an
InitSwiftinstead. Any preference?
There was a problem hiding this comment.
The less we can diverge from LLVM the better, so if "just don't use the replacements InitLLVM gives us" is the right answer, we can just make extra Argc and Argv variables that we then throw away.
There was a problem hiding this comment.
Seems reasonable to me!
tools/driver/driver.cpp
Outdated
| PROGRAM_START(ExpandedArgc, ExpandedArgv); | ||
| #endif |
There was a problem hiding this comment.
I think you can make this even simpler by renaming ExpandedArgc/v to ThrowawayArgc/v, and then initializing the argv ArrayRef below from ExpandedArgs instead. It's a good comment, though.
There was a problem hiding this comment.
Good call, +1 to one less #ifdef _WIN32.
| @@ -0,0 +1,8 @@ | |||
| // Windows returns a negative exit code from -debug-crash-immediately so not | |||
| // considers that to be a crash (requiring --crash) instead of an abnormal exit | |||
There was a problem hiding this comment.
Hm. The driver is supposed to be catching crashes when not doing an exec. Is it possible the decision of whether to exec or not is different on Windows vs. other platforms?
There was a problem hiding this comment.
What do you mean by catching crashes when not doing an exec?
There was a problem hiding this comment.
The TaskQueue implementation has a callback for when a process crashes, and is supposed to exit with a normal error when that happens.
There was a problem hiding this comment.
From what I've seen, on Unix, if a process exits with an uncaught signal, the driver catches that and exits with a positive exit code (254 it seems).
On Windows, signals aren't really a thing, so the signal handler only gets called if llvm:sys::Wait fails due to say, a malformed argument. Otherwise, it simply bubbles up the return code to the driver and the driver exits with it. I think the closest Windows thing would be checking the high bits of the 32bit exit code. 10 is a warning, 11 is an error which llvm respects. However, that's not really a signal and I'm not sure if it makes too much sense to have that call the signalled callback.
There was a problem hiding this comment.
I think the "signalled" callback really is supposed to be "crashed". It controls things like whether temporary files are cleaned up or left in place for debugging purposes.
| // MERGE: -emit-module-doc-path [[PARTIAL_MODULE_B:[^ ]+]].swiftdoc | ||
| // MERGE: bin/swift{{c?}} | ||
| // MERGE: @{{[^ ]*}}arguments-{{[0-9a-zA-Z]+}}.resp # -frontend -merge-modules -emit-module [[PARTIAL_MODULE_A]].swiftmodule [[PARTIAL_MODULE_B]].swiftmodule | ||
| // MERGE: bin{{(/|\\\\)}}swift{{c?}} |
There was a problem hiding this comment.
Personal opinion: {{[/\\]}} is a little easier to read.
There was a problem hiding this comment.
There are two backslashes here that need to be matched. ..Though the parens there are not needed.
There was a problem hiding this comment.
Oh, bah, I always forget that. Thanks.
|
With #23018 (comment), we don't need to have multiple tests anymore! |
This prevents response files on Windows from being overridden after they are read.
|
@swift-ci Please smoke test |
This prevents response files on Windows from being overridden after they
are read.
Note that this is #20885 but without the unicode changes. The patch upstream has been taking a while and response files would help a lot with building Foundation on Windows.