[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Martin Storsjö via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcbd73574e43e: Reapply: [Driver] Use forward slashes in most 
linker arguments (authored by mstorsjo).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D53066?vs=171253=223478#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D53066

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp

Index: clang/lib/Driver/ToolChains/Gnu.cpp
===
--- clang/lib/Driver/ToolChains/Gnu.cpp
+++ clang/lib/Driver/ToolChains/Gnu.cpp
@@ -1699,7 +1699,7 @@
 if (GCCToolchainDir.back() == '/')
   GCCToolchainDir = GCCToolchainDir.drop_back(); // remove the /
 
-Prefixes.push_back(GCCToolchainDir);
+Prefixes.push_back(llvm::sys::path::convert_to_slash(GCCToolchainDir));
   } else {
 // If we have a SysRoot, try that first.
 if (!D.SysRoot.empty()) {
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -163,7 +163,7 @@
 
 // Add filenames immediately.
 if (II.isFilename()) {
-  CmdArgs.push_back(II.getFilename());
+  CmdArgs.push_back(Args.MakeArgString(TC.normalizePath(II.getFilename(;
   continue;
 }
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3570,7 +3570,8 @@
 for (const auto  : Inputs) {
   addDashXForInput(Args, II, CmdArgs);
   if (II.isFilename())
-CmdArgs.push_back(II.getFilename());
+CmdArgs.push_back(
+Args.MakeArgString(TC.normalizePath(II.getFilename(;
   else
 II.getInputArg().renderAsInput(Args, CmdArgs);
 }
@@ -4963,7 +4964,8 @@
 // Handled with other dependency code.
   } else if (Output.isFilename()) {
 CmdArgs.push_back("-o");
-CmdArgs.push_back(Output.getFilename());
+CmdArgs.push_back(
+Args.MakeArgString(TC.normalizePath(Output.getFilename(;
   } else {
 assert(Output.isNothing() && "Invalid output.");
   }
@@ -4978,7 +4980,8 @@
 
   for (const InputInfo  : FrontendInputs) {
 if (Input.isFilename())
-  CmdArgs.push_back(Input.getFilename());
+  CmdArgs.push_back(
+  Args.MakeArgString(TC.normalizePath(Input.getFilename(;
 else
   Input.getInputArg().renderAsInput(Args, CmdArgs);
   }
@@ -5671,9 +5674,10 @@
   assert(Inputs.size() == 1 && "Unexpected number of inputs.");
   const InputInfo  = Inputs[0];
 
-  const llvm::Triple  = getToolChain().getEffectiveTriple();
+  const ToolChain  = getToolChain();
+  const llvm::Triple  = TC.getEffectiveTriple();
   const std::string  = Triple.getTriple();
-  const auto  = getToolChain().getDriver();
+  const auto  = TC.getDriver();
 
   // Don't warn about "clang -w -c foo.s"
   Args.ClaimAllArgs(options::OPT_w);
@@ -5847,7 +5851,7 @@
 
   assert(Output.isFilename() && "Unexpected lipo output.");
   CmdArgs.push_back("-o");
-  CmdArgs.push_back(Output.getFilename());
+  CmdArgs.push_back(Args.MakeArgString(TC.normalizePath(Output.getFilename(;
 
   const llvm::Triple  = getToolChain().getTriple();
   if (Args.hasArg(options::OPT_gsplit_dwarf) &&
@@ -5857,7 +5861,7 @@
   }
 
   assert(Input.isFilename() && "Invalid input.");
-  CmdArgs.push_back(Input.getFilename());
+  CmdArgs.push_back(Args.MakeArgString(TC.normalizePath(Input.getFilename(;
 
   const char *Exec = getToolChain().getDriver().getClangProgramPath();
   C.addCommand(llvm::make_unique(JA, *this, Exec, CmdArgs, Inputs));
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -376,9 +376,10 @@
 
   for (const auto  : getLibraryPaths()) {
 SmallString<128> P(LibPath);
-llvm::sys::path::append(P, Prefix + Twine("clang_rt.") + Component + Suffix);
+llvm::sys::path::append(P,
+Prefix + Twine("clang_rt.") + Component + Suffix);
 if (getVFS().exists(P))
-  return P.str();
+  return normalizePath(P);
   }
 
   StringRef Arch = getArchNameForCompilerRTLib(*this, Args);
@@ -386,7 +387,7 @@
   SmallString<128> Path(getCompilerRTPath());
   llvm::sys::path::append(Path, Prefix + Twine("clang_rt.") + Component + "-" +
 Arch + Env + Suffix);
-  return Path.str();
+  return 

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D53066



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

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Jordan Rupprecht via Phabricator via cfe-commits]

rupprecht resigned from this revision.
rupprecht added a comment.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D53066



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

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo reopened this revision.
mstorsjo added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D53066



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

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Martin Storsjö via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL345370: Reapply: [Driver] Use forward slashes in most linker 
arguments (authored by mstorsjo, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53066?vs=170989=171253#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53066

Files:
  cfe/trunk/include/clang/Driver/ToolChain.h
  cfe/trunk/lib/Driver/Driver.cpp
  cfe/trunk/lib/Driver/ToolChain.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
  cfe/trunk/lib/Driver/ToolChains/Gnu.cpp

Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -3570,7 +3570,8 @@
 for (const auto  : Inputs) {
   addDashXForInput(Args, II, CmdArgs);
   if (II.isFilename())
-CmdArgs.push_back(II.getFilename());
+CmdArgs.push_back(
+Args.MakeArgString(TC.normalizePath(II.getFilename(;
   else
 II.getInputArg().renderAsInput(Args, CmdArgs);
 }
@@ -4963,7 +4964,8 @@
 // Handled with other dependency code.
   } else if (Output.isFilename()) {
 CmdArgs.push_back("-o");
-CmdArgs.push_back(Output.getFilename());
+CmdArgs.push_back(
+Args.MakeArgString(TC.normalizePath(Output.getFilename(;
   } else {
 assert(Output.isNothing() && "Invalid output.");
   }
@@ -4978,7 +4980,8 @@
 
   for (const InputInfo  : FrontendInputs) {
 if (Input.isFilename())
-  CmdArgs.push_back(Input.getFilename());
+  CmdArgs.push_back(
+  Args.MakeArgString(TC.normalizePath(Input.getFilename(;
 else
   Input.getInputArg().renderAsInput(Args, CmdArgs);
   }
@@ -5671,9 +5674,10 @@
   assert(Inputs.size() == 1 && "Unexpected number of inputs.");
   const InputInfo  = Inputs[0];
 
-  const llvm::Triple  = getToolChain().getEffectiveTriple();
+  const ToolChain  = getToolChain();
+  const llvm::Triple  = TC.getEffectiveTriple();
   const std::string  = Triple.getTriple();
-  const auto  = getToolChain().getDriver();
+  const auto  = TC.getDriver();
 
   // Don't warn about "clang -w -c foo.s"
   Args.ClaimAllArgs(options::OPT_w);
@@ -5847,7 +5851,7 @@
 
   assert(Output.isFilename() && "Unexpected lipo output.");
   CmdArgs.push_back("-o");
-  CmdArgs.push_back(Output.getFilename());
+  CmdArgs.push_back(Args.MakeArgString(TC.normalizePath(Output.getFilename(;
 
   const llvm::Triple  = getToolChain().getTriple();
   if (Args.hasArg(options::OPT_gsplit_dwarf) &&
@@ -5857,7 +5861,7 @@
   }
 
   assert(Input.isFilename() && "Invalid input.");
-  CmdArgs.push_back(Input.getFilename());
+  CmdArgs.push_back(Args.MakeArgString(TC.normalizePath(Input.getFilename(;
 
   const char *Exec = getToolChain().getDriver().getClangProgramPath();
   C.addCommand(llvm::make_unique(JA, *this, Exec, CmdArgs, Inputs));
Index: cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
@@ -1699,7 +1699,7 @@
 if (GCCToolchainDir.back() == '/')
   GCCToolchainDir = GCCToolchainDir.drop_back(); // remove the /
 
-Prefixes.push_back(GCCToolchainDir);
+Prefixes.push_back(llvm::sys::path::convert_to_slash(GCCToolchainDir));
   } else {
 // If we have a SysRoot, try that first.
 if (!D.SysRoot.empty()) {
Index: cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
+++ cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp
@@ -163,7 +163,7 @@
 
 // Add filenames immediately.
 if (II.isFilename()) {
-  CmdArgs.push_back(II.getFilename());
+  CmdArgs.push_back(Args.MakeArgString(TC.normalizePath(II.getFilename(;
   continue;
 }
 
Index: cfe/trunk/lib/Driver/ToolChain.cpp
===
--- cfe/trunk/lib/Driver/ToolChain.cpp
+++ cfe/trunk/lib/Driver/ToolChain.cpp
@@ -376,17 +376,18 @@
 
   for (const auto  : getLibraryPaths()) {
 SmallString<128> P(LibPath);
-llvm::sys::path::append(P, Prefix + Twine("clang_rt.") + Component + Suffix);
+llvm::sys::path::append(P,
+Prefix + Twine("clang_rt.") + Component + Suffix);
 if (getVFS().exists(P))
-  return P.str();
+  return normalizePath(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 normalizePath(Path);
 }
 
 const char *ToolChain::getCompilerRTArgString(const llvm::opt::ArgList 

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.

lgtm


https://reviews.llvm.org/D53066



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

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 170989.
mstorsjo added a comment.

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.


https://reviews.llvm.org/D53066

Files:
  include/clang/Driver/ToolChain.h
  lib/Driver/Driver.cpp
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/CommonArgs.cpp
  lib/Driver/ToolChains/Gnu.cpp

Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -1699,7 +1699,7 @@
 if (GCCToolchainDir.back() == '/')
   GCCToolchainDir = GCCToolchainDir.drop_back(); // remove the /
 
-Prefixes.push_back(GCCToolchainDir);
+Prefixes.push_back(llvm::sys::path::convert_to_slash(GCCToolchainDir));
   } else {
 // If we have a SysRoot, try that first.
 if (!D.SysRoot.empty()) {
Index: lib/Driver/ToolChains/CommonArgs.cpp
===
--- lib/Driver/ToolChains/CommonArgs.cpp
+++ lib/Driver/ToolChains/CommonArgs.cpp
@@ -163,7 +163,7 @@
 
 // Add filenames immediately.
 if (II.isFilename()) {
-  CmdArgs.push_back(II.getFilename());
+  CmdArgs.push_back(Args.MakeArgString(TC.normalizePath(II.getFilename(;
   continue;
 }
 
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3496,7 +3496,8 @@
 for (const auto  : Inputs) {
   addDashXForInput(Args, II, CmdArgs);
   if (II.isFilename())
-CmdArgs.push_back(II.getFilename());
+CmdArgs.push_back(
+Args.MakeArgString(TC.normalizePath(II.getFilename(;
   else
 II.getInputArg().renderAsInput(Args, CmdArgs);
 }
@@ -4878,7 +4879,8 @@
 // Handled with other dependency code.
   } else if (Output.isFilename()) {
 CmdArgs.push_back("-o");
-CmdArgs.push_back(Output.getFilename());
+CmdArgs.push_back(
+Args.MakeArgString(TC.normalizePath(Output.getFilename(;
   } else {
 assert(Output.isNothing() && "Invalid output.");
   }
@@ -4893,7 +4895,8 @@
 
   for (const InputInfo  : FrontendInputs) {
 if (Input.isFilename())
-  CmdArgs.push_back(Input.getFilename());
+  CmdArgs.push_back(
+  Args.MakeArgString(TC.normalizePath(Input.getFilename(;
 else
   Input.getInputArg().renderAsInput(Args, CmdArgs);
   }
@@ -5586,9 +5589,10 @@
   assert(Inputs.size() == 1 && "Unexpected number of inputs.");
   const InputInfo  = Inputs[0];
 
-  const llvm::Triple  = getToolChain().getEffectiveTriple();
+  const ToolChain  = getToolChain();
+  const llvm::Triple  = TC.getEffectiveTriple();
   const std::string  = Triple.getTriple();
-  const auto  = getToolChain().getDriver();
+  const auto  = TC.getDriver();
 
   // Don't warn about "clang -w -c foo.s"
   Args.ClaimAllArgs(options::OPT_w);
@@ -5762,7 +5766,7 @@
 
   assert(Output.isFilename() && "Unexpected lipo output.");
   CmdArgs.push_back("-o");
-  CmdArgs.push_back(Output.getFilename());
+  CmdArgs.push_back(Args.MakeArgString(TC.normalizePath(Output.getFilename(;
 
   const llvm::Triple  = getToolChain().getTriple();
   if (Args.hasArg(options::OPT_gsplit_dwarf) &&
@@ -5772,7 +5776,7 @@
   }
 
   assert(Input.isFilename() && "Invalid input.");
-  CmdArgs.push_back(Input.getFilename());
+  CmdArgs.push_back(Args.MakeArgString(TC.normalizePath(Input.getFilename(;
 
   const char *Exec = getToolChain().getDriver().getClangProgramPath();
   C.addCommand(llvm::make_unique(JA, *this, Exec, CmdArgs, Inputs));
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -376,17 +376,18 @@
 
   for (const auto  : getLibraryPaths()) {
 SmallString<128> P(LibPath);
-llvm::sys::path::append(P, Prefix + Twine("clang_rt.") + Component + Suffix);
+llvm::sys::path::append(P,
+Prefix + Twine("clang_rt.") + Component + Suffix);
 if (getVFS().exists(P))
-  return P.str();
+  return normalizePath(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 normalizePath(Path);
 }
 
 const char *ToolChain::getCompilerRTArgString(const llvm::opt::ArgList ,
@@ -425,7 +426,7 @@
 }
 
 std::string ToolChain::GetFilePath(const char *Name) const {
-  return D.GetFilePath(Name, *this);
+  return 

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In https://reviews.llvm.org/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?


https://reviews.llvm.org/D53066



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

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Zachary Turner via Phabricator via cfe-commits]

zturner added a subscriber: mstorsjo.
zturner added a comment.

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?


https://reviews.llvm.org/D53066



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

Re: [PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Zachary Turner via cfe-commits]

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?
On Wed, Oct 24, 2018 at 12:16 AM Martin Storsjö via Phabricator <
revi...@reviews.llvm.org> wrote:

> mstorsjo added inline comments.
>
>
> 
> Comment at: lib/Driver/Driver.cpp:1013-1014
>}
> +  for (auto *Str : {, , , ,
> })
> +*Str = llvm::sys::path::convert_to_slash(*Str);
>
> 
> rnk wrote:
> > zturner wrote:
> > > Is this going to affect the cl driver as well?
> > 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.
> 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.
>
>
> https://reviews.llvm.org/D53066
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added inline comments.



Comment at: lib/Driver/Driver.cpp:1013-1014
   }
+  for (auto *Str : {, , , , })
+*Str = llvm::sys::path::convert_to_slash(*Str);
 

rnk wrote:
> zturner wrote:
> > Is this going to affect the cl driver as well?
> 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.
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.


https://reviews.llvm.org/D53066



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

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: lib/Driver/Driver.cpp:1013-1014
   }
+  for (auto *Str : {, , , , })
+*Str = llvm::sys::path::convert_to_slash(*Str);
 

zturner wrote:
> Is this going to affect the cl driver as well?
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.


https://reviews.llvm.org/D53066



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

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Zachary Turner via Phabricator via cfe-commits]

zturner added inline comments.



Comment at: lib/Driver/Driver.cpp:1013-1014
   }
+  for (auto *Str : {, , , , })
+*Str = llvm::sys::path::convert_to_slash(*Str);
 

Is this going to affect the cl driver as well?



Comment at: lib/Driver/ToolChain.cpp:381
 if (getVFS().exists(P))
-  return P.str();
+  return llvm::sys::path::convert_to_slash(P);
   }

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


https://reviews.llvm.org/D53066



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

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo updated this revision to Diff 170755.
mstorsjo retitled this revision from "[RFC] [Driver] Use forward slashes in 
most linker arguments" to "[Driver] Use forward slashes in most linker 
arguments".
mstorsjo added a comment.

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


https://reviews.llvm.org/D53066

Files:
  lib/Driver/Driver.cpp
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Driver/ToolChains/Gnu.cpp

Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -1655,7 +1655,7 @@
   llvm::StringRef SysRoot) {
   const Arg *A = Args.getLastArg(clang::driver::options::OPT_gcc_toolchain);
   if (A)
-return A->getValue();
+return llvm::sys::path::convert_to_slash(A->getValue());
 
   // If we have a SysRoot, ignore GCC_INSTALL_PREFIX.
   // GCC_INSTALL_PREFIX specifies the gcc installation for the default
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3488,15 +3488,17 @@
   // Handled with other dependency code.
 } else if (Output.isFilename()) {
   CmdArgs.push_back("-o");
-  CmdArgs.push_back(Output.getFilename());
+  CmdArgs.push_back(Args.MakeArgString(
+  llvm::sys::path::convert_to_slash(Output.getFilename(;
 } else {
   assert(Output.isNothing() && "Input output.");
 }
 
 for (const auto  : Inputs) {
   addDashXForInput(Args, II, CmdArgs);
   if (II.isFilename())
-CmdArgs.push_back(II.getFilename());
+CmdArgs.push_back(Args.MakeArgString(
+llvm::sys::path::convert_to_slash(II.getFilename(;
   else
 II.getInputArg().renderAsInput(Args, CmdArgs);
 }
@@ -4893,7 +4895,8 @@
 
   for (const InputInfo  : FrontendInputs) {
 if (Input.isFilename())
-  CmdArgs.push_back(Input.getFilename());
+  CmdArgs.push_back(Args.MakeArgString(
+  llvm::sys::path::convert_to_slash(Input.getFilename(;
 else
   Input.getInputArg().renderAsInput(Args, CmdArgs);
   }
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -378,15 +378,15 @@
 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 ,
@@ -425,7 +425,7 @@
 }
 
 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 {
@@ -774,12 +774,14 @@
 void ToolChain::AddFilePathLibArgs(const ArgList ,
ArgStringList ) const {
   for (const auto  : 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  : 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 ,
Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -1010,6 +1010,8 @@
 .Case("obj", SaveTempsObj)
 .Default(SaveTempsCwd);
   }
+  for (auto *Str : {, , , , })
+*Str = llvm::sys::path::convert_to_slash(*Str);
 
   setLTOMode(Args);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits