[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Igor Kudrin via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL326164: [Driver] Allow using a canonical form of 
-fuse-ld= when cross-compiling on… (authored by ikudrin, committed 
by ).

Changed prior to commit:
  https://reviews.llvm.org/D43621?vs=135888=136026#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43621

Files:
  cfe/trunk/lib/Driver/ToolChain.cpp
  cfe/trunk/test/Driver/Inputs/fuse_ld_windows/ld.foo.exe
  cfe/trunk/test/Driver/fuse-ld-windows.c


Index: cfe/trunk/test/Driver/fuse-ld-windows.c
===
--- cfe/trunk/test/Driver/fuse-ld-windows.c
+++ cfe/trunk/test/Driver/fuse-ld-windows.c
@@ -0,0 +1,25 @@
+// REQUIRES: system-windows
+
+// We used to require adding ".exe" suffix when cross-compiling on Windows.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -B %S/Inputs/fuse_ld_windows -fuse-ld=foo 2>&1 \
+// RUN:   | FileCheck %s
+
+// Check that the old variant still works.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -B %S/Inputs/fuse_ld_windows -fuse-ld=foo.exe 2>&1 \
+// RUN:   | FileCheck %s
+
+// With the full path, the extension can be omitted, too,
+// because Windows allows that.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -fuse-ld=%S/Inputs/fuse_ld_windows/ld.foo 2>&1 \
+// RUN:   | FileCheck %s
+
+// Check that the full path with the extension works too.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -fuse-ld=%S/Inputs/fuse_ld_windows/ld.foo.exe 2>&1 \
+// RUN:   | FileCheck %s
+
+// CHECK-NOT: invalid linker name
+// CHECK: /Inputs/fuse_ld_windows{{/|}}ld.foo
Index: cfe/trunk/lib/Driver/ToolChain.cpp
===
--- cfe/trunk/lib/Driver/ToolChain.cpp
+++ cfe/trunk/lib/Driver/ToolChain.cpp
@@ -412,7 +412,7 @@
   if (llvm::sys::path::is_absolute(UseLinker)) {
 // If we're passed what looks like an absolute path, don't attempt to
 // second-guess that.
-if (llvm::sys::fs::exists(UseLinker))
+if (llvm::sys::fs::can_execute(UseLinker))
   return UseLinker;
   } else if (UseLinker.empty() || UseLinker == "ld") {
 // If we're passed -fuse-ld= with no argument, or with the argument ld,
@@ -427,7 +427,7 @@
 LinkerName.append(UseLinker);
 
 std::string LinkerPath(GetProgramPath(LinkerName.c_str()));
-if (llvm::sys::fs::exists(LinkerPath))
+if (llvm::sys::fs::can_execute(LinkerPath))
   return LinkerPath;
   }
 


Index: cfe/trunk/test/Driver/fuse-ld-windows.c
===
--- cfe/trunk/test/Driver/fuse-ld-windows.c
+++ cfe/trunk/test/Driver/fuse-ld-windows.c
@@ -0,0 +1,25 @@
+// REQUIRES: system-windows
+
+// We used to require adding ".exe" suffix when cross-compiling on Windows.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -B %S/Inputs/fuse_ld_windows -fuse-ld=foo 2>&1 \
+// RUN:   | FileCheck %s
+
+// Check that the old variant still works.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -B %S/Inputs/fuse_ld_windows -fuse-ld=foo.exe 2>&1 \
+// RUN:   | FileCheck %s
+
+// With the full path, the extension can be omitted, too,
+// because Windows allows that.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -fuse-ld=%S/Inputs/fuse_ld_windows/ld.foo 2>&1 \
+// RUN:   | FileCheck %s
+
+// Check that the full path with the extension works too.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -fuse-ld=%S/Inputs/fuse_ld_windows/ld.foo.exe 2>&1 \
+// RUN:   | FileCheck %s
+
+// CHECK-NOT: invalid linker name
+// CHECK: /Inputs/fuse_ld_windows{{/|}}ld.foo
Index: cfe/trunk/lib/Driver/ToolChain.cpp
===
--- cfe/trunk/lib/Driver/ToolChain.cpp
+++ cfe/trunk/lib/Driver/ToolChain.cpp
@@ -412,7 +412,7 @@
   if (llvm::sys::path::is_absolute(UseLinker)) {
 // If we're passed what looks like an absolute path, don't attempt to
 // second-guess that.
-if (llvm::sys::fs::exists(UseLinker))
+if (llvm::sys::fs::can_execute(UseLinker))
   return UseLinker;
   } else if (UseLinker.empty() || UseLinker == "ld") {
 // If we're passed -fuse-ld= with no argument, or with the argument ld,
@@ -427,7 +427,7 @@
 LinkerName.append(UseLinker);
 
 std::string LinkerPath(GetProgramPath(LinkerName.c_str()));
-if (llvm::sys::fs::exists(LinkerPath))
+if (llvm::sys::fs::can_execute(LinkerPath))
   return LinkerPath;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Reid Kleckner via Phabricator via cfe-commits]

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

lgtm


https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

In https://reviews.llvm.org/D43621#1019724, @ruiu wrote:

> This change looks good to me, but I think we shouldn't check in an executable 
> binary file. Can you create `ld.foo.exe` in the test? Since you don't 
> actually run ld.foo.exe, creating that executable should be very easy.


If I read the diff correctly, it is an empty file, so this should just work.


https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

This change looks good to me, but I think we shouldn't check in an executable 
binary file. Can you create `ld.foo.exe` in the test? Since you don't actually 
run ld.foo.exe, creating that executable should be very easy.


https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Igor Kudrin via Phabricator via cfe-commits]

ikudrin updated this revision to Diff 135888.
ikudrin added a comment.

- Add a test.


https://reviews.llvm.org/D43621

Files:
  lib/Driver/ToolChain.cpp
  test/Driver/Inputs/fuse_ld_windows/ld.foo.exe
  test/Driver/fuse-ld-windows.c


Index: test/Driver/fuse-ld-windows.c
===
--- /dev/null
+++ test/Driver/fuse-ld-windows.c
@@ -0,0 +1,25 @@
+// REQUIRES: system-windows
+
+// We used to require adding ".exe" suffix when cross-compiling on Windows.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -B %S/Inputs/fuse_ld_windows -fuse-ld=foo 2>&1 \
+// RUN:   | FileCheck %s
+
+// Check that the old variant still works.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -B %S/Inputs/fuse_ld_windows -fuse-ld=foo.exe 2>&1 \
+// RUN:   | FileCheck %s
+
+// With the full path, the extension can be omitted, too,
+// because Windows allows that.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -fuse-ld=%S/Inputs/fuse_ld_windows/ld.foo 2>&1 \
+// RUN:   | FileCheck %s
+
+// Check that the full path with the extension works too.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -fuse-ld=%S/Inputs/fuse_ld_windows/ld.foo.exe 2>&1 \
+// RUN:   | FileCheck %s
+
+// CHECK-NOT: invalid linker name
+// CHECK: /Inputs/fuse_ld_windows{{/|}}ld.foo
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -412,7 +412,7 @@
   if (llvm::sys::path::is_absolute(UseLinker)) {
 // If we're passed what looks like an absolute path, don't attempt to
 // second-guess that.
-if (llvm::sys::fs::exists(UseLinker))
+if (llvm::sys::fs::can_execute(UseLinker))
   return UseLinker;
   } else if (UseLinker.empty() || UseLinker == "ld") {
 // If we're passed -fuse-ld= with no argument, or with the argument ld,
@@ -427,7 +427,7 @@
 LinkerName.append(UseLinker);
 
 std::string LinkerPath(GetProgramPath(LinkerName.c_str()));
-if (llvm::sys::fs::exists(LinkerPath))
+if (llvm::sys::fs::can_execute(LinkerPath))
   return LinkerPath;
   }
 


Index: test/Driver/fuse-ld-windows.c
===
--- /dev/null
+++ test/Driver/fuse-ld-windows.c
@@ -0,0 +1,25 @@
+// REQUIRES: system-windows
+
+// We used to require adding ".exe" suffix when cross-compiling on Windows.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -B %S/Inputs/fuse_ld_windows -fuse-ld=foo 2>&1 \
+// RUN:   | FileCheck %s
+
+// Check that the old variant still works.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -B %S/Inputs/fuse_ld_windows -fuse-ld=foo.exe 2>&1 \
+// RUN:   | FileCheck %s
+
+// With the full path, the extension can be omitted, too,
+// because Windows allows that.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -fuse-ld=%S/Inputs/fuse_ld_windows/ld.foo 2>&1 \
+// RUN:   | FileCheck %s
+
+// Check that the full path with the extension works too.
+// RUN: %clang %s -### -o %t.o -target i386-unknown-linux \
+// RUN: -fuse-ld=%S/Inputs/fuse_ld_windows/ld.foo.exe 2>&1 \
+// RUN:   | FileCheck %s
+
+// CHECK-NOT: invalid linker name
+// CHECK: /Inputs/fuse_ld_windows{{/|}}ld.foo
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -412,7 +412,7 @@
   if (llvm::sys::path::is_absolute(UseLinker)) {
 // If we're passed what looks like an absolute path, don't attempt to
 // second-guess that.
-if (llvm::sys::fs::exists(UseLinker))
+if (llvm::sys::fs::can_execute(UseLinker))
   return UseLinker;
   } else if (UseLinker.empty() || UseLinker == "ld") {
 // If we're passed -fuse-ld= with no argument, or with the argument ld,
@@ -427,7 +427,7 @@
 LinkerName.append(UseLinker);
 
 std::string LinkerPath(GetProgramPath(LinkerName.c_str()));
-if (llvm::sys::fs::exists(LinkerPath))
+if (llvm::sys::fs::can_execute(LinkerPath))
   return LinkerPath;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Igor Kudrin via Phabricator via cfe-commits]

ikudrin added a comment.

Not all toolchains call `ToolChain::GetLinkerPath`. For example, MSVC toolchain 
uses its own code:

  void visualstudio::Linker::ConstructJob(...) {
  ...
StringRef Linker = Args.getLastArgValue(options::OPT_fuse_ld_EQ, "link");
if (Linker.equals_lower("lld"))
  Linker = "lld-link";
  ...
  }

In my case, I am trying to cross-compile:

  > ...\clang.exe a.cpp -fuse-ld=lld -target i686-pc-linux-gnu
  clang.exe: error: invalid linker name in argument '-fuse-ld=lld'


Repository:
  rC Clang

https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

In https://reviews.llvm.org/D43621#1017027, @ruiu wrote:

> > That's weird, because lots of lldb tests compile and link test binaries on 
> > Windows with `-fuse-ld=lld` (without the `.exe`).  What makes you say the 
> > `.exe` is necessary?
>
> Maybe he is using clang (not clang-cl) and want to use ld.lld.exe instead of 
> lld-link?


The LLDB tests use clang, not clang-cl.  Here's a typical invocation:

  D:\src\llvm\build\ninja_release\bin\clang.exe  main.o -g -O0 -fno-builtin 
-m32  
-ID:\src\llvm\mono\llvm-project\lldb\packages\Python\lldbsuite\test\lang\c\conflicting-symbol/../../../make/../../../../../include
 
-ID:\src\llvm\mono\llvm-project\lldb\packages\Python\lldbsuite\test\lang\c\conflicting-symbol/
 -include 
D:\src\llvm\mono\llvm-project\lldb\packages\Python\lldbsuite\test\lang\c\conflicting-symbol/../../../make/test_common.h
 
-ID:\src\llvm\mono\llvm-project\lldb\packages\Python\lldbsuite\test\lang\c\conflicting-symbol/../../../make/
  -fno-limit-debug-info   -L. -LOne -lOne -LTwo -lTwo  -fuse-ld=lld 
--driver-mode=g++  -o "a.out"

Note that it's running `clang.exe` and passing `-fuse-ld=lld`.


Repository:
  rC Clang

https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Rui Ueyama via Phabricator via cfe-commits]

ruiu added a comment.

> That's weird, because lots of lldb tests compile and link test binaries on 
> Windows with `-fuse-ld=lld` (without the `.exe`).  What makes you say the 
> `.exe` is necessary?

Maybe he is using clang (not clang-cl) and want to use ld.lld.exe instead of 
lld-link?


Repository:
  rC Clang

https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

> Right now, we have to add an .exe suffix when using this switch, like 
> -fuse-ld=lld.exe.

That's weird, because lots of lldb tests compile and link test binaries on 
Windows with `-fuse-ld=lld` (without the `.exe`).  What makes you say the 
`.exe` is necessary?


Repository:
  rC Clang

https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added a comment.

In https://reviews.llvm.org/D43621#1015888, @amccarth wrote:

> This must be new-ish code, since I've routinely used `-fuse-ld=lld` (without 
> `.exe`) on Windows.


Well, since 12/2016: https://reviews.llvm.org/rC289668


Repository:
  rC Clang

https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

This must be new-ish code, since I've routinely used `-fuse-ld=lld` (without 
`.exe`) on Windows.


Repository:
  rC Clang

https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Igor Kudrin via Phabricator via cfe-commits]

ikudrin added inline comments.



Comment at: lib/Driver/ToolChain.cpp:415
 // second-guess that.
-if (llvm::sys::fs::exists(UseLinker))
+if (llvm::sys::fs::can_execute(UseLinker))
   return UseLinker;

Hahnfeld wrote:
> I don't think we should do this for absolute paths?
Why not? You can omit ".exe" suffix on Windows when calling an application, 
right?


Repository:
  rC Clang

https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Jonas Hahnfeld via Phabricator via cfe-commits]

Hahnfeld added inline comments.



Comment at: lib/Driver/ToolChain.cpp:415
 // second-guess that.
-if (llvm::sys::fs::exists(UseLinker))
+if (llvm::sys::fs::can_execute(UseLinker))
   return UseLinker;

I don't think we should do this for absolute paths?


Repository:
  rC Clang

https://reviews.llvm.org/D43621



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

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

[Igor Kudrin via Phabricator via cfe-commits]

ikudrin created this revision.
ikudrin added reviewers: phosek, Hahnfeld, logan, rnk.
ikudrin added a project: clang.

Right now, we have to add an `.exe` suffix when using this switch, like 
`-fuse-ld=lld.exe`.
If the suffix is omitted, `llvm::sys::fs::exists()` cannot find the file on 
Windows,
while `llvm::sys::fs::can_Execute()` automatically tries both variants.


Repository:
  rC Clang

https://reviews.llvm.org/D43621

Files:
  lib/Driver/ToolChain.cpp


Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -412,7 +412,7 @@
   if (llvm::sys::path::is_absolute(UseLinker)) {
 // If we're passed what looks like an absolute path, don't attempt to
 // second-guess that.
-if (llvm::sys::fs::exists(UseLinker))
+if (llvm::sys::fs::can_execute(UseLinker))
   return UseLinker;
   } else if (UseLinker.empty() || UseLinker == "ld") {
 // If we're passed -fuse-ld= with no argument, or with the argument ld,
@@ -427,7 +427,7 @@
 LinkerName.append(UseLinker);
 
 std::string LinkerPath(GetProgramPath(LinkerName.c_str()));
-if (llvm::sys::fs::exists(LinkerPath))
+if (llvm::sys::fs::can_execute(LinkerPath))
   return LinkerPath;
   }
 


Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -412,7 +412,7 @@
   if (llvm::sys::path::is_absolute(UseLinker)) {
 // If we're passed what looks like an absolute path, don't attempt to
 // second-guess that.
-if (llvm::sys::fs::exists(UseLinker))
+if (llvm::sys::fs::can_execute(UseLinker))
   return UseLinker;
   } else if (UseLinker.empty() || UseLinker == "ld") {
 // If we're passed -fuse-ld= with no argument, or with the argument ld,
@@ -427,7 +427,7 @@
 LinkerName.append(UseLinker);
 
 std::string LinkerPath(GetProgramPath(LinkerName.c_str()));
-if (llvm::sys::fs::exists(LinkerPath))
+if (llvm::sys::fs::can_execute(LinkerPath))
   return LinkerPath;
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits