[Driver] Use forward slashes in most linker arguments

[mstorsjo]

Summary

libtool inspects the output of $CC -v to detect what object files and libraries are linked in by default. When clang is built as a native windows executable, all paths are formatted with backslashes, and the backslashes cause each argument to be enclosed in quotes. The backslashes and quotes break further processing within libtool (which is implemented in shell script, running in e.g. msys) pretty badly.

Between unix style pathes (that only work in tools that are linked to the msys runtime, essentially the same as cygwin) and proper windows style paths (with backslashes, that can easily break shell scripts and msys environments), the best compromise is to use windows style paths (starting with e.g. c:) but with forward slashes, which both msys based tools, shell scripts and native windows executables can cope with. This incidentally turns out to be the form of paths that GCC prints out when run with -v on windows as well.

This change potentially makes the output from clang -v a bit more inconsistent, but it is isn't necessarily very consistent to begin with.

[mstorsjo]

Ping @rnk or others with good ideas

[rnk]

lib/Driver/Job.cpp

104–105 ↗

(On Diff #168958)

This is blindly treating every argument as if it were a path. I can imagine someone wanting to define a macro to a string value containing backslashes, such as: -DAWESOME_MACRO="\"asdf\""
Given that this is used for -### and crash diagnostic printing, I'm hesitant to do this here. I think it might be better to, in a cygming environment, try to convert all our paths to forward slashes before we add them to the command line.

[mstorsjo]

lib/Driver/Job.cpp

104–105 ↗

(On Diff #168958)

A quick test of the concept is at http://martin.st/temp/clang-forward-slash.patch; this seems to be roughly enough to get forward slashes for all paths in the linker command at least. Does this look like what you have in mind?

For a plain "clang -v test.c", this takes care of everything except the temporary object file. It doesn't do anything for the compile command though. This should be roughly the minimal amount needed for this particular case of libtool, but then again makes the -v output wildly inconsistent.

Changing the path separator earlier would kinda require changing the preferred path separator throughout all callsites to any path api, or change the path utils to allow overriding the global default somehow.

[rnk]

lib/Driver/Job.cpp

104–105 ↗

(On Diff #168958)

I think our output is already pretty inconsistent. Most build systems use forward slash paths internally, so that's what the compiler sees on the command line. Then, we dutifully take those paths and join them with the "preferred" separator (\), and make paths with mixed slashes, which nobody wants.

I would go with the patch you put forward, if that solves the issue. Everything there is very gnu toolchain oriented, it makes sense to try to match their path style.

The one change I might object to is ToolChain::GetProgramPath, since cmd really wants the name of the program to have back slashes. As long as that comes out with backslashes in -### and crash report .sh files, I'd be OK with it.

[mstorsjo]

lib/Driver/Job.cpp

104–105 ↗

(On Diff #168958)

Ok, thanks - I'll test run things with that patch (minus the executable change) and update this review properly later then.

[mstorsjo]

Tested the earlier discussed solution, and it seems to work fine for my particular case with libtool.

[mstorsjo]

Ping @rnk

[rnk]

lgtm

[mstorsjo]

So, this failed due to some tests expecting to find the exact same path substring in different parts of the output (used with FileCheck captured variables): http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/763

What's the best course of action here - trying to extend the changes to forward slashes in more places, to make these tests pass, or adjust the tests to not break due to this? Or extend FileCheck to allow checks for "similar text, modulo path separator differences"? :-)

[mstorsjo]

Converting more path instances, enough to fix the tests that failed on windows bots. The patch is not very pretty, though...

[zturner]

lib/Driver/Driver.cpp

1013–1014 ↗

(On Diff #170755)

Is this going to affect the cl driver as well?

lib/Driver/ToolChain.cpp

381

Same questions here and for the rest of the changes in this file

[rnk]

lib/Driver/Driver.cpp

1013–1014 ↗

(On Diff #170755)

Yeah, if we change the resource dir, I think it's going to have knock-on affects to the debug info we emit to describe the locations of compiler intrinsic headers. I was imagining that this would be limited to flags which only get passed to GNU-ld-compatible linkers.

[mstorsjo]

lib/Driver/Driver.cpp

1013–1014 ↗

(On Diff #170755)

Well, the first minimal version that was committed in rL345004 was enough to produce paths that worked with libtool, but I guess that one also would cause some amount of changes for the cl driver.

Now in order for all the tests to pass, I had to change (almost) all of these as I have here. (After rechecking, I think I can spare one or two of them.)

But I do have to touch ResourceDir for test/Driver/linux-per-target-runtime-dir.c to pass, since it requires that a -resource-dir parameter matches a later -L linker flag, and I do need to change the -L linker flag for the libtool case.

So I guess it's back to the drawing board with this change then. What do you guys think is the path of least impact on e.g. the cl driver and PDB generation in general (and least messy scattered rewrites), without breaking a bunch of tests (e.g. test/Driver/linux-ld.c requires the --sysroot parameter to match -L), while still getting libtool compatible paths with forward slashes out of -v. At least when targeting mingw, but maybe ideally for any non-msvc target? For cross compiling for a linux target from windows, I would guess forward slashes would be preferred as well.

[zturner]

It seems like some combination of checking the target triple, host triple,
and driver mode and putting the conversions behind those checks could work?

For paths like resource dir that are going into debug info it should be
driver mode. For paths we pass to another tool it should probably be based
on target triple . This probably isn’t perfect, but WDYT?

[mstorsjo]

In D53066#1274288, @zturner wrote:

It seems like some combination of checking the target triple, host triple,
and driver mode and putting the conversions behind those checks could work?

Sounds ok, I can give it a shot. There's probably no need to explicitly check the host here, as the convert_to_slash function is a no-op on anything else than windows.

For paths like resource dir that are going into debug info it should be
driver mode.

Why is that? Wouldn't one want the same behaviour when building with the clang driver with an msvc target triple? Also, what kind of paths do you get in the PDB when crosscompiling from unix with clang-cl?

[mstorsjo]

Changed so that we only rewrite paths if targeting a non-windows OS, or cygwin/mingw. Since convert_to_slash is a no-op when running on anything else than windows, this should hit exactly the cases where converting to forward slashes should make sense.

[rnk]

lgtm

[mstorsjo]

Nope, still no really working properly with all the tests out there (I had only run the Driver subdirectory since I'm only testing in a very underpowered VM, and it had skipped the cuda/hip tests which also seemed to have a few issues):
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/13562
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/886
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/20994

I could potentially make all the tests in Driver work by continuing doing whack-a-mole for normalizing all paths in places where they are matched in tests, but this also failed some tests in CodeGen/thinlto which I haven't studied closer yet. It also seems to break tests in clang-tidy/clangd. So this doesn't really seem sustainable unless we do it completely thoroughly all over the place, but given cross-project dependencies, it looks like that is a much much larger scope change that I probably don't have time to pusue.

What other options are there? The actual original problem (libtool) I'm trying to solve only requires paths in this form in the output of clang -v (for linking). I guess that one could be worked around by scaling the change back to one of the original versions where I only changed a couple paths, and making that change conditional on -v being set (plus conditional on target as it is right now). Having that behave differently than -### as used in most tests, isn't really nice though, but if we change the output of -###, we're back at whack-a-mole.

[rupprecht]

I'm not familiar with this code or anything windows related, and it looks like @rnk is, so I'll remove myself and let him approve

[rnk]

We discussed adding a new path style that opts into a / preferred separator on Windows, something like path::Style::windows_forward. All of the absolute path handling routines would behave the same, but the preferred separator would now be /. We can make the default path style for mingw be mingw_forward.

lib/Driver/ToolChain.cpp

const char *Suffix = Shared ? (Triple.isOSWindows() ? ".lib" : ".so")
                               : (IsITANMSVCWindows ? ".lib" : ".a");
   if (Shared && Triple.isWindowsGNUEnvironment())
     Suffix = ".dll.a";
 
   for (const auto &LibPath : getLibraryPaths()) {
     SmallString<128> P(LibPath);
     llvm::sys::path::append(P, Prefix + Twine("clang_rt.") + Component + Suffix);
     if (getVFS().exists(P))
-      return P.str();
+      return llvm::sys::path::convert_to_slash(P);
   }
 
   StringRef Arch = getArchNameForCompilerRTLib(*this, Args);
   const char *Env = TT.isAndroid() ? "-android" : "";
   SmallString<128> Path(getCompilerRTPath());
   llvm::sys::path::append(Path, Prefix + Twine("clang_rt.") + Component + "-" +
                                     Arch + Env + Suffix);
-  return Path.str();
+  return llvm::sys::path::convert_to_slash(Path);
 }
 
 const char *ToolChain::getCompilerRTArgString(const llvm::opt::ArgList &Args,
                                               StringRef Component,
                                               bool Shared) const {
   return Args.MakeArgString(getCompilerRT(Args, Component, Shared));
 }
 
Tool *ToolChain::SelectTool(const JobAction &JA) const {
   if (getDriver().ShouldUseClangCompiler(JA)) return getClang();
   Action::ActionClass AC = JA.getKind();
   if (AC == Action::AssembleJobClass && useIntegratedAs())
     return getClangAs();
   return getTool(AC);
 }
 
 std::string ToolChain::GetFilePath(const char *Name) const {
-  return D.GetFilePath(Name, *this);
+  return llvm::sys::path::convert_to_slash(D.GetFilePath(Name, *this));
 }
 
 std::string ToolChain::GetProgramPath(const char *Name) const {
   return D.GetProgramPath(Name, *this);
 }
 
 std::string ToolChain::GetLinkerPath() const {
   const Arg* A = Args.getLastArg(options::OPT_fuse_ld_EQ);
case ToolChain::CST_Libstdcxx:
     CmdArgs.push_back("-lstdc++");
     break;
   }
 }
 
 void ToolChain::AddFilePathLibArgs(const ArgList &Args,
                                    ArgStringList &CmdArgs) const {
   for (const auto &LibPath : getLibraryPaths())
-    if(LibPath.length() > 0)
-      CmdArgs.push_back(Args.MakeArgString(StringRef("-L") + LibPath));
+    if (LibPath.length() > 0)
+      CmdArgs.push_back(Args.MakeArgString(
+          StringRef("-L") + llvm::sys::path::convert_to_slash(LibPath)));
 
   for (const auto &LibPath : getFilePaths())
-    if(LibPath.length() > 0)
-      CmdArgs.push_back(Args.MakeArgString(StringRef("-L") + LibPath));
+    if (LibPath.length() > 0)
+      CmdArgs.push_back(Args.MakeArgString(
+          StringRef("-L") + llvm::sys::path::convert_to_slash(LibPath)));
 }
 
 void ToolChain::AddCCKextLibArgs(const ArgList &Args,
                                  ArgStringList &CmdArgs) const {
   CmdArgs.push_back("-lcc_kext");
 }
 
 bool ToolChain::AddFastMathRuntimeIfAvailable(const ArgList &Args,