[clang] Default to windows response files when running on windows

[sbc100]

Summary

This matches other tools such as llvm-ar, lld, etc.

This consistency is useful when authoring downstream tools as it means
they can always use windows-style response files on windows, rather than
generate a different style for clang compared to the other tools.

This was discussed in the equivalent llvm-ar change:

https://reviews.llvm.org/D69665

wasm-ld behaviour was changed there:

https://reviews.llvm.org/D75577

[probinson]

clang/tools/driver/driver.cpp

391

Testing the triple is a functional change. I should think the test for CL mode is sufficient. Note that for PS4 we run on Windows but use a GNU-compatible command line and response-file style. If you want Windows-style on Windows, invoke the compiler as "clang-cl" instead of "clang".

[sbc100]

clang/tools/driver/driver.cpp

391

Yes this is a functional change. It changes the default when running on windows. I will see if I can write a (windows-only) test for it.

Some more background for why I think this change is right way to go:

In the emscripten toolchain we run clang as a cross compiler, we don't want cl.exe emulation.

We drive the compiler and other tools from a python script and we create response files whenever a command line is tool long.

It doesn't make sense to me that we should use windows response files for all other tools except for clang. In particular, llvm-ar, llc, opt, wasm-ld all default to windows response files on windows.

The function in question in our toolchain looks like this:

def generate_response_file(command):
    if is_windows:
       use_windows_style
    else:
       use_gnu_style

Without this change I have to write something like this:

def generate_response_file(command):
   # On windows all the tools we run use windows style response files.
   # Except for clang (for some reason?).
    if is_windows and 'clang.exe' not in command[0] :
       use_windows_style
    else:
       use_gnu_style

I don't see why clang would want to be different here. Can you think of any reason?

[probinson]

As a functional change it should definitely have a functional test.
Let me double-check on how our stuff behaves, I may be mis-remembering.

[probinson]

Indeed I am mis-remembering, sorry about that! We use gnu-style command-line options but we change RSPQuoting from Default to Windows; assuming getProcessTriple() is the host not the target, your change should have the same effect. I'll try removing our private patch and adding yours, and see what happens.

[probinson]

I was about to add @rnk who I remember has been in Windows driver behavior discussions in the past; except he has cleverly left a note that he's away June-Sept. Oh well.

[probinson]

It looks like your patch will allow us to remove a private patch that has a similar effect.
Incidentally if I apply this to an upstream checkout on Windows, I see clang/test/Driver/at_file.c fails, so you'd at least need to do something for that.
(UNSUPPORTED: system-windows at a minimum.)

[probinson]

Re testing, you could copy clang/test/Driver/at_file_win.c, which has an explicit --rsp-quoting=windows; remove that option and make it REQUIRES: system-windows. That should do it.

[rnk]

This seems wrong for mingw, so check with @mstorsjo.

[mstorsjo]

In D80876#2067532, @rnk wrote:

This seems wrong for mingw, so check with @mstorsjo.

Thanks for the headsup!

I agree that this change would make sense and would make things more consistent in general, but @rnk is also right that, within a mingw environment, the expectation would probably be for the old behaviour.

I did one build test with a large link command (done via libtool), where libtool produed a response file, containing unix-style-quoted windows paths, like some\\path\\to\\a\\file - and if I added --rsp-quoting=windows to it, it still built fine, so while it would be expanded to actually contain double backslashes in that case (I presume?) it seemed harmless. So as long as the response files contain only relative pathnames, misinterpreting a unix-quoted file as windows quoted seems harmless.

But as for keeping the old default in mingw environments, would it make sense to, if on windows, check the default target triple, and if that's a mingw target (Triple.isWindowsGNUEnvironment()), keep the unix style quoting as default?

[mstorsjo]

In D80876#2068428, @mstorsjo wrote:

But as for keeping the old default in mingw environments, would it make sense to, if on windows, check the default target triple, and if that's a mingw target (Triple.isWindowsGNUEnvironment()), keep the unix style quoting as default?

Then again... Would this defeat the purpose of this whole patch, for cases when building wasm e.g. in MSYS2, where the tools are built targeting mingw by default?

[sbc100]

If we want want to have the default on windows be dependent on mingw vs not-mingw then we should do it across the board so it applies to llvm-ar, lld, and other tools. Right now I don't think any of those other tools have any special mingw handling.

Would it make sense to expect mingw users to explicitly pass --rsp-quoting=gnu. I'm not very familiar with mingw use cases so I don't know if that is an unreadable request?

[sbc100]

The problem I ran into regarding the quoting style differences was trying to refer to filenames that contains single quote characters:

Here is our test case that I've currently disabled on windows pending the resolution of this issue:
https://github.com/emscripten-core/emscripten/blob/20ecfa2f8101fb21a9c0f9d75e3aa2dee6468e95/tests/test_other.py#L8379

[mstorsjo]

In D80876#2069418, @sbc100 wrote:

If we want want to have the default on windows be dependent on mingw vs not-mingw then we should do it across the board so it applies to llvm-ar, lld, and other tools. Right now I don't think any of those other tools have any special mingw handling.

That's a fair point

Would it make sense to expect mingw users to explicitly pass --rsp-quoting=gnu. I'm not very familiar with mingw use cases so I don't know if that is an unreadable request?

It'd be quite a bit of a hassle - one of the main points is to be able to build the vast majority of projects with unix style build systems, so anything that requires changes to the build systems is a hassle. That'd require either adding wrapper executables that pass such defaults (msys2 currently don't use anything such), or being able to configure custom defaults in the build (like one can set the default target or default linker etc).

If the practical failure cases only show up with response files with "tricky" filenames containing single quotes or similar, I guess it might be an option to just change the defaults (as you say, other tools already do it this way) and require changes only in projects that use problematic file names.

@mati865 What do you think, from the msys2 point of view?

[sbc100]

Do you know how gcc handles this case when running on mingw?

[mstorsjo]

In D80876#2069970, @sbc100 wrote:

Do you know how gcc handles this case when running on mingw?

It uses the gnu/unix quoting style - so that's the case for wanting clang to preserve compatibility with that behaviour.

The fact that any clang can be used for any target, blurs the distinction, as an msys2/mingw based clang both can be used for targeting wasm (where the build tools might expect to use windows style quoting) and for general mingw target things (where the build tools out of tradition expects to use unix style quoting in rsp files).

[mati865]

I don't have strong opinion here as I don't know this topic well (things just worked so far).
That said if Clang stops working as a drop-in replacement for GCC with commonly used build systems, MSYS2 will have to carry one more patch to bring back old behaviour.

[sbc100]

In D80876#2070233, @mstorsjo wrote:

In D80876#2069970, @sbc100 wrote:

Do you know how gcc handles this case when running on mingw?

It uses the gnu/unix quoting style - so that's the case for wanting clang to preserve compatibility with that behaviour.

Is there any case where gcc on windows will use the windows quoting style?

If the answer is no, and I assume GNU ar is the same, then perhaps the solution is revert https://reviews.llvm.org/D69665 instead and have all the llvm tools except clang-cl default to the GNU style.

The fact that any clang can be used for any target, blurs the distinction, as an msys2/mingw based clang both can be used for targeting wasm (where the build tools might expect to use windows style quoting) and for general mingw target things (where the build tools out of tradition expects to use unix style quoting in rsp files).

[mstorsjo]

In D80876#2071997, @sbc100 wrote:

In D80876#2070233, @mstorsjo wrote:

In D80876#2069970, @sbc100 wrote:

Do you know how gcc handles this case when running on mingw?

It uses the gnu/unix quoting style - so that's the case for wanting clang to preserve compatibility with that behaviour.

Is there any case where gcc on windows will use the windows quoting style?

Not that I'm aware of, no.

If the answer is no, and I assume GNU ar is the same, then perhaps the solution is revert https://reviews.llvm.org/D69665 instead and have all the llvm tools except clang-cl default to the GNU style.

Hmm, that might be at least one consistent strategy. For llvm-ar, it'd default to posix quoting, while llvm-lib does use windows style quoting. (It probably doesn't need a full revert, but keeping the rsp-quoting option, just changing the default.)

In one sense, it feels weird to stop doing the native thing in these tools, just for the sake of the mingw usecase - but on the other hand, if there's no other predecent for using windows style quoting with those particular tools, and there is predecent for keeping the posix quoting, I guess it should be fine. That would at least be a path forward without potentially breaking the msys2/mingw ecosystem.

clang/tools/driver/driver.cpp

int main(int argc_, const char **argv_) {
   }
 
   // Determines whether we want nullptr markers in argv to indicate response
   // files end-of-lines. We only use this for the /LINK driver argument with
   // clang-cl.exe on Windows.
   bool MarkEOLs = ClangCLMode;
 
   llvm::cl::TokenizerCallback Tokenizer;
-  if (RSPQuoting == Windows || (RSPQuoting == Default && ClangCLMode))
+  switch (RSPQuoting) {
+  case Default: {
+    if (ClangCLMode ||
+        llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows())
       Tokenizer = &llvm::cl::TokenizeWindowsCommandLine;
     else
       Tokenizer = &llvm::cl::TokenizeGNUCommandLine;
+    break;
+  }
+  case Windows:
+    Tokenizer = &llvm::cl::TokenizeWindowsCommandLine;
+    break;
+  case POSIX:
+    Tokenizer = &llvm::cl::TokenizeGNUCommandLine;
+    break;
+  }
 
   if (MarkEOLs && argv.size() > 1 && StringRef(argv[1]).startswith("-cc1"))
     MarkEOLs = false;
   llvm::cl::ExpandResponseFiles(Saver, Tokenizer, argv, MarkEOLs);
 
   // Handle -cc1 integrated tools, even if -cc1 was expanded from a response
   // file.
   auto FirstArg = std::find_if(argv.begin() + 1, argv.end(),