[clang] [Driver] Ensure we error on lto with link.exe and target `*-windows-msvc` on non cl driver modes #109607

MaxEW707 · 2024-09-23T04:41:17Z

Similar to previous PRs I've done to change some IsCLMode checks to isWindowsMSVCEnvironment.
I stumbled into this one accidentally last week. I did some greps and I think this is the last one for now. All the remaining IsCLMode checks are only valid for the cl driver mode.

For the *-windows-msvc target MSVC link.exe and lld are supported. LTO isn't supported with MSVC link.exe and so we error when lto is enabled but MSVC link.exe is being used. However we only error if the driver mode is cl.
If we are using the clang driver with the *-windows-msvc target then ensure an error is also emitted when LTO and MSVC link.exe are used together.

llvmbot · 2024-09-23T04:41:48Z

@llvm/pr-subscribers-clang-driver

Author: Max Winkler (MaxEW707)

Changes

Similar to previous PRs I've done to change some IsCLMode checks to isWindowsMSVCEnvironment.
I stumbled into this one accidentally last week. I did some greps and I think this is the last one for now. All the remaining IsCLMode checks are only valid for the cl driver mode.

For the *-windows-msvc target MSVC link.exe and lld are supported. LTO isn't supported with MSVC link.exe and so we error when lto is enabled but MSVC link.exe is being used. However we only error if the driver mode is cl.
If we are using the clang driver with the *-windows-msvc target then ensure an error is also emitted when LTO and MSVC link.exe are used together.

Full diff: https://github.com/llvm/llvm-project/pull/109607.diff

6 Files Affected:

(modified) clang/lib/Driver/Driver.cpp (+2-1)
(modified) clang/test/Driver/cl-options.c (+2)
(modified) clang/test/Driver/clang_f_opts.c (+1-1)
(modified) clang/test/Driver/lto.c (+1-1)
(modified) clang/test/Driver/thinlto.c (+1-1)
(modified) clang/test/Driver/thinlto.cu (+1-1)

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 44548fa9d706fb..a9506cf911b950 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -4011,7 +4011,8 @@ void Driver::handleArguments(Compilation &C, DerivedArgList &Args,
     // Emitting LLVM while linking disabled except in HIPAMD Toolchain
     if (Args.hasArg(options::OPT_emit_llvm) && !Args.hasArg(options::OPT_hip_link))
       Diag(clang::diag::err_drv_emit_llvm_link);
-    if (IsCLMode() && LTOMode != LTOK_None &&
+    if (C.getDefaultToolChain().getTriple().isWindowsMSVCEnvironment() &&
+        LTOMode != LTOK_None &&
         !Args.getLastArgValue(options::OPT_fuse_ld_EQ)
              .equals_insensitive("lld"))
       Diag(clang::diag::err_drv_lto_without_lld);
diff --git a/clang/test/Driver/cl-options.c b/clang/test/Driver/cl-options.c
index a6f338533ad763..f0de6289d95223 100644
--- a/clang/test/Driver/cl-options.c
+++ b/clang/test/Driver/cl-options.c
@@ -626,6 +626,8 @@
 // LTO-THIN: -flto=thin
 
 // RUN: not %clang_cl -### -Fe%t.exe -entry:main -flto -- %s 2>&1 | FileCheck -check-prefix=LTO-WITHOUT-LLD %s
+// RUN: not %clang_cl -### -fuse-ld=link -Fe%t.exe -entry:main -flto -- %s 2>&1 | FileCheck -check-prefix=LTO-WITHOUT-LLD %s
+// RUN: not %clang -### --target=x86_64-windows-msvc -fuse-ld=link -Fe%t.exe -entry:main -flto -- %s 2>&1 | FileCheck -check-prefix=LTO-WITHOUT-LLD %s
 // LTO-WITHOUT-LLD: LTO requires -fuse-ld=lld
 
 // RUN: %clang_cl  -### -- %s 2>&1 | FileCheck -check-prefix=NOCFGUARD %s
diff --git a/clang/test/Driver/clang_f_opts.c b/clang/test/Driver/clang_f_opts.c
index adb6f075b6c15d..5dfd86f7515236 100644
--- a/clang/test/Driver/clang_f_opts.c
+++ b/clang/test/Driver/clang_f_opts.c
@@ -605,7 +605,7 @@
 // RUN: %clang -### -S -fjmc -g -fno-jmc -target x86_64-pc-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC %s
 // RUN: %clang -### -S -fjmc -g -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_JMC %s
 // RUN: %clang -### -S -fjmc -g -fno-jmc -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC %s
-// RUN: %clang -### -fjmc -g -flto -target x86_64-pc-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC_LTO %s
+// RUN: %clang -### -fjmc -g -flto -fuse-ld=lld -target x86_64-pc-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC_LTO %s
 // RUN: %clang -### -fjmc -g -flto -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_JMC_LTO %s
 // RUN: %clang -### -fjmc -g -flto -fno-jmc -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC_LTO %s
 // CHECK_JMC_WARN: -fjmc requires debug info. Use -g or debug options that enable debugger's stepping function; option ignored
diff --git a/clang/test/Driver/lto.c b/clang/test/Driver/lto.c
index b6c89eb99e2741..d3e22d9a1196b2 100644
--- a/clang/test/Driver/lto.c
+++ b/clang/test/Driver/lto.c
@@ -5,7 +5,7 @@
 // CHECK-COMPILE-ACTIONS: 2: compiler, {1}, ir
 // CHECK-COMPILE-ACTIONS: 3: backend, {2}, lto-bc
 
-// RUN: %clang -ccc-print-phases %s -flto 2> %t
+// RUN: %clang -ccc-print-phases -fuse-ld=lld %s -flto 2> %t
 // RUN: FileCheck -check-prefix=CHECK-COMPILELINK-ACTIONS < %t %s
 //
 // CHECK-COMPILELINK-ACTIONS: 0: input, "{{.*}}lto.c", c
diff --git a/clang/test/Driver/thinlto.c b/clang/test/Driver/thinlto.c
index e08435f71cc0b0..b54ef307628851 100644
--- a/clang/test/Driver/thinlto.c
+++ b/clang/test/Driver/thinlto.c
@@ -5,7 +5,7 @@
 // CHECK-COMPILE-ACTIONS: 2: compiler, {1}, ir
 // CHECK-COMPILE-ACTIONS: 3: backend, {2}, lto-bc
 
-// RUN: %clang -ccc-print-phases %s -flto=thin 2> %t
+// RUN: %clang -fuse-ld=lld -ccc-print-phases %s -flto=thin 2> %t
 // RUN: FileCheck -check-prefix=CHECK-COMPILELINK-ACTIONS < %t %s
 //
 // CHECK-COMPILELINK-ACTIONS: 0: input, "{{.*}}thinlto.c", c
diff --git a/clang/test/Driver/thinlto.cu b/clang/test/Driver/thinlto.cu
index c175aae37c718d..b7875884a24aa8 100644
--- a/clang/test/Driver/thinlto.cu
+++ b/clang/test/Driver/thinlto.cu
@@ -6,7 +6,7 @@
 // CHECK-COMPILE-ACTIONS-NOT: lto-bc
 // CHECK-COMPILE-ACTIONS: 12: backend, {11}, lto-bc, (host-cuda)
 
-// RUN: %clangxx -ccc-print-phases --no-offload-new-driver -nocudainc -nocudalib %s -flto=thin 2> %t
+// RUN: %clangxx -ccc-print-phases -fuse-ld=lld --no-offload-new-driver -nocudainc -nocudalib %s -flto=thin 2> %t
 // RUN: FileCheck -check-prefix=CHECK-COMPILELINK-ACTIONS < %t %s
 //
 // CHECK-COMPILELINK-ACTIONS: 0: input, "{{.*}}thinlto.cu", cuda, (host-cuda)

llvmbot · 2024-09-23T04:41:49Z

@llvm/pr-subscribers-clang

Author: Max Winkler (MaxEW707)

Changes

Similar to previous PRs I've done to change some IsCLMode checks to isWindowsMSVCEnvironment.
I stumbled into this one accidentally last week. I did some greps and I think this is the last one for now. All the remaining IsCLMode checks are only valid for the cl driver mode.

For the *-windows-msvc target MSVC link.exe and lld are supported. LTO isn't supported with MSVC link.exe and so we error when lto is enabled but MSVC link.exe is being used. However we only error if the driver mode is cl.
If we are using the clang driver with the *-windows-msvc target then ensure an error is also emitted when LTO and MSVC link.exe are used together.

Full diff: https://github.com/llvm/llvm-project/pull/109607.diff

6 Files Affected:

(modified) clang/lib/Driver/Driver.cpp (+2-1)
(modified) clang/test/Driver/cl-options.c (+2)
(modified) clang/test/Driver/clang_f_opts.c (+1-1)
(modified) clang/test/Driver/lto.c (+1-1)
(modified) clang/test/Driver/thinlto.c (+1-1)
(modified) clang/test/Driver/thinlto.cu (+1-1)

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 44548fa9d706fb..a9506cf911b950 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -4011,7 +4011,8 @@ void Driver::handleArguments(Compilation &C, DerivedArgList &Args,
     // Emitting LLVM while linking disabled except in HIPAMD Toolchain
     if (Args.hasArg(options::OPT_emit_llvm) && !Args.hasArg(options::OPT_hip_link))
       Diag(clang::diag::err_drv_emit_llvm_link);
-    if (IsCLMode() && LTOMode != LTOK_None &&
+    if (C.getDefaultToolChain().getTriple().isWindowsMSVCEnvironment() &&
+        LTOMode != LTOK_None &&
         !Args.getLastArgValue(options::OPT_fuse_ld_EQ)
              .equals_insensitive("lld"))
       Diag(clang::diag::err_drv_lto_without_lld);
diff --git a/clang/test/Driver/cl-options.c b/clang/test/Driver/cl-options.c
index a6f338533ad763..f0de6289d95223 100644
--- a/clang/test/Driver/cl-options.c
+++ b/clang/test/Driver/cl-options.c
@@ -626,6 +626,8 @@
 // LTO-THIN: -flto=thin
 
 // RUN: not %clang_cl -### -Fe%t.exe -entry:main -flto -- %s 2>&1 | FileCheck -check-prefix=LTO-WITHOUT-LLD %s
+// RUN: not %clang_cl -### -fuse-ld=link -Fe%t.exe -entry:main -flto -- %s 2>&1 | FileCheck -check-prefix=LTO-WITHOUT-LLD %s
+// RUN: not %clang -### --target=x86_64-windows-msvc -fuse-ld=link -Fe%t.exe -entry:main -flto -- %s 2>&1 | FileCheck -check-prefix=LTO-WITHOUT-LLD %s
 // LTO-WITHOUT-LLD: LTO requires -fuse-ld=lld
 
 // RUN: %clang_cl  -### -- %s 2>&1 | FileCheck -check-prefix=NOCFGUARD %s
diff --git a/clang/test/Driver/clang_f_opts.c b/clang/test/Driver/clang_f_opts.c
index adb6f075b6c15d..5dfd86f7515236 100644
--- a/clang/test/Driver/clang_f_opts.c
+++ b/clang/test/Driver/clang_f_opts.c
@@ -605,7 +605,7 @@
 // RUN: %clang -### -S -fjmc -g -fno-jmc -target x86_64-pc-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC %s
 // RUN: %clang -### -S -fjmc -g -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_JMC %s
 // RUN: %clang -### -S -fjmc -g -fno-jmc -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC %s
-// RUN: %clang -### -fjmc -g -flto -target x86_64-pc-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC_LTO %s
+// RUN: %clang -### -fjmc -g -flto -fuse-ld=lld -target x86_64-pc-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC_LTO %s
 // RUN: %clang -### -fjmc -g -flto -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_JMC_LTO %s
 // RUN: %clang -### -fjmc -g -flto -fno-jmc -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC_LTO %s
 // CHECK_JMC_WARN: -fjmc requires debug info. Use -g or debug options that enable debugger's stepping function; option ignored
diff --git a/clang/test/Driver/lto.c b/clang/test/Driver/lto.c
index b6c89eb99e2741..d3e22d9a1196b2 100644
--- a/clang/test/Driver/lto.c
+++ b/clang/test/Driver/lto.c
@@ -5,7 +5,7 @@
 // CHECK-COMPILE-ACTIONS: 2: compiler, {1}, ir
 // CHECK-COMPILE-ACTIONS: 3: backend, {2}, lto-bc
 
-// RUN: %clang -ccc-print-phases %s -flto 2> %t
+// RUN: %clang -ccc-print-phases -fuse-ld=lld %s -flto 2> %t
 // RUN: FileCheck -check-prefix=CHECK-COMPILELINK-ACTIONS < %t %s
 //
 // CHECK-COMPILELINK-ACTIONS: 0: input, "{{.*}}lto.c", c
diff --git a/clang/test/Driver/thinlto.c b/clang/test/Driver/thinlto.c
index e08435f71cc0b0..b54ef307628851 100644
--- a/clang/test/Driver/thinlto.c
+++ b/clang/test/Driver/thinlto.c
@@ -5,7 +5,7 @@
 // CHECK-COMPILE-ACTIONS: 2: compiler, {1}, ir
 // CHECK-COMPILE-ACTIONS: 3: backend, {2}, lto-bc
 
-// RUN: %clang -ccc-print-phases %s -flto=thin 2> %t
+// RUN: %clang -fuse-ld=lld -ccc-print-phases %s -flto=thin 2> %t
 // RUN: FileCheck -check-prefix=CHECK-COMPILELINK-ACTIONS < %t %s
 //
 // CHECK-COMPILELINK-ACTIONS: 0: input, "{{.*}}thinlto.c", c
diff --git a/clang/test/Driver/thinlto.cu b/clang/test/Driver/thinlto.cu
index c175aae37c718d..b7875884a24aa8 100644
--- a/clang/test/Driver/thinlto.cu
+++ b/clang/test/Driver/thinlto.cu
@@ -6,7 +6,7 @@
 // CHECK-COMPILE-ACTIONS-NOT: lto-bc
 // CHECK-COMPILE-ACTIONS: 12: backend, {11}, lto-bc, (host-cuda)
 
-// RUN: %clangxx -ccc-print-phases --no-offload-new-driver -nocudainc -nocudalib %s -flto=thin 2> %t
+// RUN: %clangxx -ccc-print-phases -fuse-ld=lld --no-offload-new-driver -nocudainc -nocudalib %s -flto=thin 2> %t
 // RUN: FileCheck -check-prefix=CHECK-COMPILELINK-ACTIONS < %t %s
 //
 // CHECK-COMPILELINK-ACTIONS: 0: input, "{{.*}}thinlto.cu", cuda, (host-cuda)

MaskRay · 2024-09-23T06:10:38Z

clang/test/Driver/clang_f_opts.c

@@ -605,7 +605,7 @@
 // RUN: %clang -### -S -fjmc -g -fno-jmc -target x86_64-pc-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC %s
 // RUN: %clang -### -S -fjmc -g -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_JMC %s
 // RUN: %clang -### -S -fjmc -g -fno-jmc -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC %s
-// RUN: %clang -### -fjmc -g -flto -target x86_64-pc-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC_LTO %s
+// RUN: %clang -### -fjmc -g -flto -fuse-ld=lld -target x86_64-pc-windows-msvc %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC_LTO %s


-fuse-ld=lld would lead to a failure if lld is not found in program paths. This failure happens on Linux, not sure about Windows.

Ahh the Windows MSVC linker code uses GetProgramPath and not GetLinkerPath so this error won't happen on the windows-msvc target. I'll see if there is a better way to do this anyhow.

I did some more digging here. Unit tests testing -fuse-ld=lld for *-windows-msvc already assumes that we won't error if lld-link.exe cannot be found. We would have to fix quite a few unit tests if the *-windows-msvc target ever starts erroring if lld-link.exe cannot be found. Example, https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/cl-link.c#L64.

MaskRay · 2024-09-23T06:11:24Z

clang/test/Driver/lto.c

@@ -5,7 +5,7 @@
 // CHECK-COMPILE-ACTIONS: 2: compiler, {1}, ir
 // CHECK-COMPILE-ACTIONS: 3: backend, {2}, lto-bc

-// RUN: %clang -ccc-print-phases %s -flto 2> %t
+// RUN: %clang -ccc-print-phases -fuse-ld=lld %s -flto 2> %t


This requirement is really non-obvious. The default target triple could be windows-msvc and without -fuse-ld=lld there would be an error?

I don't think this is reasonable for other people that write -ccc-print-phases tests.

Understood. Would having these tests do --target=x86_64-unknown-linux-gnu instead be sufficient or would you rather have users not worry about any target triple for -ccc-print-phases tests?

Looking around at other unit tests there doesn't seem to be a way to handle this without an explicit --target triple with my limited knowledge.

Otherwise I can close this PR out and keep the error behind the cl driver mode.
It isn't a super big deal since later on MSVC link.exe will fail with LNK1107: invalid or corrupt file which is just a little more obtuse if you are unaware of what to look out for.

MaxEW707 · 2024-10-01T00:13:26Z

clang/test/Driver/lto.c

@@ -5,7 +5,7 @@
 // CHECK-COMPILE-ACTIONS: 2: compiler, {1}, ir
 // CHECK-COMPILE-ACTIONS: 3: backend, {2}, lto-bc

-// RUN: %clang -ccc-print-phases %s -flto 2> %t
+// RUN: %clang -ccc-print-phases %if target={{.*-windows-msvc.*}} %{ -fuse-ld=lld %} -flto %s 2> %t


@MaskRay I went the %if route here.

Let me know if you prefer the sysroot route to handle systems that may not have lld installed as was done here and here.

As noted above the *-windows-msvc target currently does not error if lld-link.exe does not exist but let me know if you would rather I go the sysroot route for the -ccc-print-phases tests here.

MaxEW707 requested review from nico, MaskRay and zmodem September 23, 2024 04:41

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Sep 23, 2024

MaskRay reviewed Sep 23, 2024

View reviewed changes

MaxEW707 commented Oct 1, 2024

View reviewed changes

MaxEW707 added 2 commits September 30, 2024 17:20

Ensure we error on lto with link.exe on non-cl mode driver

a6ff2d1

Wrap windows-msvc target in %if for lit tests

0e58998

MaxEW707 force-pushed the mew/error-lto-link-msvc-target branch from 8eda299 to 0e58998 Compare October 1, 2024 01:25

MaskRay approved these changes Oct 1, 2024

View reviewed changes

MaxEW707 merged commit c1343a2 into llvm:main Oct 3, 2024
8 checks passed

MaxEW707 deleted the mew/error-lto-link-msvc-target branch October 3, 2024 02:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] [Driver] Ensure we error on lto with link.exe and target `*-windows-msvc` on non cl driver modes #109607

[clang] [Driver] Ensure we error on lto with link.exe and target `*-windows-msvc` on non cl driver modes #109607

MaxEW707 commented Sep 23, 2024

llvmbot commented Sep 23, 2024

llvmbot commented Sep 23, 2024

MaskRay Sep 23, 2024

MaxEW707 Sep 24, 2024

MaxEW707 Oct 1, 2024

MaskRay Sep 23, 2024 •

edited

Loading

MaxEW707 Sep 24, 2024

MaxEW707 Oct 1, 2024

[clang] [Driver] Ensure we error on lto with link.exe and target *-windows-msvc on non cl driver modes #109607

[clang] [Driver] Ensure we error on lto with link.exe and target *-windows-msvc on non cl driver modes #109607

Conversation

MaxEW707 commented Sep 23, 2024

llvmbot commented Sep 23, 2024

llvmbot commented Sep 23, 2024

MaskRay Sep 23, 2024

Choose a reason for hiding this comment

MaxEW707 Sep 24, 2024

Choose a reason for hiding this comment

MaxEW707 Oct 1, 2024

Choose a reason for hiding this comment

MaskRay Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

MaxEW707 Sep 24, 2024

Choose a reason for hiding this comment

MaxEW707 Oct 1, 2024

Choose a reason for hiding this comment

[clang] [Driver] Ensure we error on lto with link.exe and target `*-windows-msvc` on non cl driver modes #109607

[clang] [Driver] Ensure we error on lto with link.exe and target `*-windows-msvc` on non cl driver modes #109607

MaskRay Sep 23, 2024 •

edited

Loading