[PATCH] D80876: [clang] Default to windows response files when running on windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

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).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Mateusz Mikuła via Phabricator via cfe-commits]

mati865 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a subscriber: mati865.
mstorsjo added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Reid "Away June-Sep" Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Paul Robinson via Phabricator via cfe-commits]

probinson added a subscriber: rnk.
probinson added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Sam Clegg via Phabricator via cfe-commits]

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



Comment at: clang/tools/driver/driver.cpp:391
+if (ClangCLMode ||
+llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows())
+  Tokenizer = ::cl::TokenizeWindowsCommandLine;

probinson wrote:
> 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".
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?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Paul Robinson via Phabricator via cfe-commits]

probinson added inline comments.



Comment at: clang/tools/driver/driver.cpp:391
+if (ClangCLMode ||
+llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows())
+  Tokenizer = ::cl::TokenizeWindowsCommandLine;

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".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80876



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80876: [clang] Default to windows response files when running on windows

[Sam Clegg via Phabricator via cfe-commits]

sbc100 created this revision.
Herald added subscribers: cfe-commits, sunfish, aheejin.
Herald added a project: clang.
sbc100 added a reviewer: ruiu.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80876

Files:
  clang/tools/driver/driver.cpp


Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -385,10 +385,22 @@
   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 = ::cl::TokenizeWindowsCommandLine;
+else
+  Tokenizer = ::cl::TokenizeGNUCommandLine;
+break;
+  }
+  case Windows:
 Tokenizer = ::cl::TokenizeWindowsCommandLine;
-  else
+break;
+  case POSIX:
 Tokenizer = ::cl::TokenizeGNUCommandLine;
+break;
+  }
 
   if (MarkEOLs && argv.size() > 1 && StringRef(argv[1]).startswith("-cc1"))
 MarkEOLs = false;


Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -385,10 +385,22 @@
   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 = ::cl::TokenizeWindowsCommandLine;
+else
+  Tokenizer = ::cl::TokenizeGNUCommandLine;
+break;
+  }
+  case Windows:
 Tokenizer = ::cl::TokenizeWindowsCommandLine;
-  else
+break;
+  case POSIX:
 Tokenizer = ::cl::TokenizeGNUCommandLine;
+break;
+  }
 
   if (MarkEOLs && argv.size() > 1 && StringRef(argv[1]).startswith("-cc1"))
 MarkEOLs = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits