[clang][Driver] Fix triple config loading for clang-cl #111397

Bo98 · 2024-10-07T16:07:52Z

clang-cl sets the default triple to <arch>-pc-windows-msvc. The host triple was however being used instead when loading default configs. As a result, <triple>.cfg files could break clang-cl by applying flags that aren't compatible with Windows/MSVC.

Move the default triple calculation earlier to handle this and correctly load <arch>-pc-windows-msvc.cfg instead.

github-actions · 2024-10-07T16:08:13Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-10-07T16:08:45Z

@llvm/pr-subscribers-clang-driver

Author: Bo Anderson (Bo98)

Changes

clang-cl sets the default triple to <arch>-pc-windows-msvc. The host triple was however being used instead when loading default configs.

Move the default triple calculation earlier to handle this.

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

4 Files Affected:

(modified) clang/lib/Driver/Driver.cpp (+24-11)
(added) clang/test/Driver/Inputs/config-cl/arm64ec-pc-windows-msvc.cfg ()
(added) clang/test/Driver/Inputs/config-cl/config-arm64ec-arg.cfg (+1)
(modified) clang/test/Driver/config-file.c (+10)

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index a5d43bdac23735..541e79ec97056b 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1247,6 +1247,19 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   CLOptions = std::make_unique<InputArgList>(
       ParseArgStrings(ArgList.slice(1), /*UseDriverMode=*/true, ContainsError));
 
+  // We want to determine the triple early so that we load the correct config.
+  if (IsCLMode()) {
+    // clang-cl targets MSVC-style Win32.
+    llvm::Triple T(TargetTriple);
+    T.setOS(llvm::Triple::Win32);
+    T.setVendor(llvm::Triple::PC);
+    T.setEnvironment(llvm::Triple::MSVC);
+    T.setObjectFormat(llvm::Triple::COFF);
+    if (CLOptions->hasArg(options::OPT__SLASH_arm64EC))
+      T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);
+    TargetTriple = T.str();
+  }
+
   // Try parsing configuration file.
   if (!ContainsError)
     ContainsError = loadConfigFiles();
@@ -1286,6 +1299,16 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
           appendOneArg(Args, Opt, nullptr);
         }
     }
+
+    // The config file may have changed the architecture so apply it.
+    if (HasConfigFile && Args.hasArg(options::OPT__SLASH_arm64EC)) {
+      llvm::Triple T(TargetTriple);
+      if (T.getArch() != llvm::Triple::aarch64 ||
+          T.getSubArch() != llvm::Triple::AArch64SubArch_arm64ec) {
+        T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);
+        TargetTriple = T.str();
+      }
+    }
   }
 
   // Check for working directory option before accessing any files
@@ -1336,17 +1359,7 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
 
   // FIXME: TargetTriple is used by the target-prefixed calls to as/ld
   // and getToolChain is const.
-  if (IsCLMode()) {
-    // clang-cl targets MSVC-style Win32.
-    llvm::Triple T(TargetTriple);
-    T.setOS(llvm::Triple::Win32);
-    T.setVendor(llvm::Triple::PC);
-    T.setEnvironment(llvm::Triple::MSVC);
-    T.setObjectFormat(llvm::Triple::COFF);
-    if (Args.hasArg(options::OPT__SLASH_arm64EC))
-      T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);
-    TargetTriple = T.str();
-  } else if (IsDXCMode()) {
+  if (IsDXCMode()) {
     // Build TargetTriple from target_profile option for clang-dxc.
     if (const Arg *A = Args.getLastArg(options::OPT_target_profile)) {
       StringRef TargetProfile = A->getValue();
diff --git a/clang/test/Driver/Inputs/config-cl/arm64ec-pc-windows-msvc.cfg b/clang/test/Driver/Inputs/config-cl/arm64ec-pc-windows-msvc.cfg
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/config-cl/config-arm64ec-arg.cfg b/clang/test/Driver/Inputs/config-cl/config-arm64ec-arg.cfg
new file mode 100644
index 00000000000000..2eb4402ddf6cd7
--- /dev/null
+++ b/clang/test/Driver/Inputs/config-cl/config-arm64ec-arg.cfg
@@ -0,0 +1 @@
+/arm64EC
diff --git a/clang/test/Driver/config-file.c b/clang/test/Driver/config-file.c
index 9df830ca4c7538..cbad0e408b9ab4 100644
--- a/clang/test/Driver/config-file.c
+++ b/clang/test/Driver/config-file.c
@@ -82,3 +82,13 @@
 // CHECK-TWO-CONFIGS: -isysroot
 // CHECK-TWO-CONFIGS-SAME: /opt/data
 // CHECK-TWO-CONFIGS-SAME: -Wall
+
+
+//--- clang-cl loads the correct triple
+// RUN: env -u CLANG_NO_DEFAULT_CONFIG %clang_cl /arm64EC --config-system-dir=%S/Inputs/config-cl --config-user-dir= -v 2>&1 | FileCheck %s -check-prefix CHECK-CL --implicit-check-not 'Configuration file:'
+// CHECK-CL: Configuration file: {{.*}}Inputs{{.}}config-cl{{.}}arm64ec-pc-windows-msvc.cfg
+
+//--- clang-cl configs support setting /arm64EC
+// RUN: %clang_cl --config-system-dir=%S/Inputs/config-cl --config-user-dir= --config=config-arm64ec-arg.cfg -v 2>&1 | FileCheck %s -check-prefix CHECK-CL-FLAG --implicit-check-not 'Configuration file:'
+// CHECK-CL-FLAG: Target: arm64ec-pc-windows-msvc
+// CHECK-CL-FLAG: Configuration file: {{.*}}Inputs{{.}}config-cl{{.}}config-arm64ec-arg.cfg

llvmbot · 2024-10-07T16:08:46Z

@llvm/pr-subscribers-clang

Author: Bo Anderson (Bo98)

Changes

clang-cl sets the default triple to <arch>-pc-windows-msvc. The host triple was however being used instead when loading default configs.

Move the default triple calculation earlier to handle this.

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

4 Files Affected:

(modified) clang/lib/Driver/Driver.cpp (+24-11)
(added) clang/test/Driver/Inputs/config-cl/arm64ec-pc-windows-msvc.cfg ()
(added) clang/test/Driver/Inputs/config-cl/config-arm64ec-arg.cfg (+1)
(modified) clang/test/Driver/config-file.c (+10)

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index a5d43bdac23735..541e79ec97056b 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1247,6 +1247,19 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   CLOptions = std::make_unique<InputArgList>(
       ParseArgStrings(ArgList.slice(1), /*UseDriverMode=*/true, ContainsError));
 
+  // We want to determine the triple early so that we load the correct config.
+  if (IsCLMode()) {
+    // clang-cl targets MSVC-style Win32.
+    llvm::Triple T(TargetTriple);
+    T.setOS(llvm::Triple::Win32);
+    T.setVendor(llvm::Triple::PC);
+    T.setEnvironment(llvm::Triple::MSVC);
+    T.setObjectFormat(llvm::Triple::COFF);
+    if (CLOptions->hasArg(options::OPT__SLASH_arm64EC))
+      T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);
+    TargetTriple = T.str();
+  }
+
   // Try parsing configuration file.
   if (!ContainsError)
     ContainsError = loadConfigFiles();
@@ -1286,6 +1299,16 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
           appendOneArg(Args, Opt, nullptr);
         }
     }
+
+    // The config file may have changed the architecture so apply it.
+    if (HasConfigFile && Args.hasArg(options::OPT__SLASH_arm64EC)) {
+      llvm::Triple T(TargetTriple);
+      if (T.getArch() != llvm::Triple::aarch64 ||
+          T.getSubArch() != llvm::Triple::AArch64SubArch_arm64ec) {
+        T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);
+        TargetTriple = T.str();
+      }
+    }
   }
 
   // Check for working directory option before accessing any files
@@ -1336,17 +1359,7 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
 
   // FIXME: TargetTriple is used by the target-prefixed calls to as/ld
   // and getToolChain is const.
-  if (IsCLMode()) {
-    // clang-cl targets MSVC-style Win32.
-    llvm::Triple T(TargetTriple);
-    T.setOS(llvm::Triple::Win32);
-    T.setVendor(llvm::Triple::PC);
-    T.setEnvironment(llvm::Triple::MSVC);
-    T.setObjectFormat(llvm::Triple::COFF);
-    if (Args.hasArg(options::OPT__SLASH_arm64EC))
-      T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);
-    TargetTriple = T.str();
-  } else if (IsDXCMode()) {
+  if (IsDXCMode()) {
     // Build TargetTriple from target_profile option for clang-dxc.
     if (const Arg *A = Args.getLastArg(options::OPT_target_profile)) {
       StringRef TargetProfile = A->getValue();
diff --git a/clang/test/Driver/Inputs/config-cl/arm64ec-pc-windows-msvc.cfg b/clang/test/Driver/Inputs/config-cl/arm64ec-pc-windows-msvc.cfg
new file mode 100644
index 00000000000000..e69de29bb2d1d6
diff --git a/clang/test/Driver/Inputs/config-cl/config-arm64ec-arg.cfg b/clang/test/Driver/Inputs/config-cl/config-arm64ec-arg.cfg
new file mode 100644
index 00000000000000..2eb4402ddf6cd7
--- /dev/null
+++ b/clang/test/Driver/Inputs/config-cl/config-arm64ec-arg.cfg
@@ -0,0 +1 @@
+/arm64EC
diff --git a/clang/test/Driver/config-file.c b/clang/test/Driver/config-file.c
index 9df830ca4c7538..cbad0e408b9ab4 100644
--- a/clang/test/Driver/config-file.c
+++ b/clang/test/Driver/config-file.c
@@ -82,3 +82,13 @@
 // CHECK-TWO-CONFIGS: -isysroot
 // CHECK-TWO-CONFIGS-SAME: /opt/data
 // CHECK-TWO-CONFIGS-SAME: -Wall
+
+
+//--- clang-cl loads the correct triple
+// RUN: env -u CLANG_NO_DEFAULT_CONFIG %clang_cl /arm64EC --config-system-dir=%S/Inputs/config-cl --config-user-dir= -v 2>&1 | FileCheck %s -check-prefix CHECK-CL --implicit-check-not 'Configuration file:'
+// CHECK-CL: Configuration file: {{.*}}Inputs{{.}}config-cl{{.}}arm64ec-pc-windows-msvc.cfg
+
+//--- clang-cl configs support setting /arm64EC
+// RUN: %clang_cl --config-system-dir=%S/Inputs/config-cl --config-user-dir= --config=config-arm64ec-arg.cfg -v 2>&1 | FileCheck %s -check-prefix CHECK-CL-FLAG --implicit-check-not 'Configuration file:'
+// CHECK-CL-FLAG: Target: arm64ec-pc-windows-msvc
+// CHECK-CL-FLAG: Configuration file: {{.*}}Inputs{{.}}config-cl{{.}}config-arm64ec-arg.cfg

Bo98 · 2024-10-23T05:57:21Z

Ping?

carlocab

Needs conflict resolution, but otherwise LGTM

zmodem

I didn't even realize people are using config files with clang-cl (since IIRC it doesn't support the config file command-line options).

zmodem · 2024-12-12T12:29:32Z

clang/lib/Driver/Driver.cpp

+    // The config file may have changed the architecture so apply it.
+    if (HasConfigFile && Args.hasArg(options::OPT__SLASH_arm64EC)) {
+      llvm::Triple T(TargetTriple);
+      if (T.getArch() != llvm::Triple::aarch64 ||
+          T.getSubArch() != llvm::Triple::AArch64SubArch_arm64ec) {
+        T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);
+        TargetTriple = T.str();


The PR description only mentions moving the default triple calculation earlier, but not this part.

The config file can presumably change many things, so why does the target triple need special handling? Also what about flags that affect x86 vs. x86_64 (-m32 and -m64), do we need to handle those too?

There's two triple systems in the driver: TargetTriple and the triple fed to ToolChain (computed by computeTargetTriple). The former is feeds into the latter but is otherwise separate.

-m32 and -m64 are already handled in computeTargetTriple and so never affect TargetTriple. The code here was to keep the existing behaviour of /arm64EC affecting TargetTriple.

To be honest: I have no idea why there is two triples. It would be simpler if TargetTriple became DefaultTriple (and remains immutable), Driver.getTargetTriple() was removed and we just moved all this to computeTargetTriple but I was hesitant to change something I don't understand the history of.

Thanks for explaining. I haven't looked at this code in a long time, and TargetTriple vs. computeTargetTriple() is pretty confusing.

It seems /arm64EC is not handled in computeTargetTriple()(?) so I suppose it needs to be handled somewhere :)

zmodem · 2024-12-12T12:31:09Z

clang/lib/Driver/Driver.cpp

-      T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);
-    TargetTriple = T.str();
-  } else if (IsDXCMode()) {
+  if (IsDXCMode()) {


Doesn't this mode have the same issue?

It does but it relies on argument parsing so is trickier to move and I didn't have a DXC build on hand. I can have a look again though and see if I can get a build going.

I guess this is the same problem as above. It would be trivial to move this to computeTargetTriple but it seems this changes TargetTriple instead.

carlocab · 2024-12-12T13:45:14Z

I didn't even realize people are using config files with clang-cl (since IIRC it doesn't support the config file command-line options).

You don't need to use config files with clang-cl to need this patch -- you need it if you use triple-based config files and also use clang-cl. The issue is that if you have, for example, a x86_64-unknown-linux-gnu.cfg in a location that clang loads by default, then this breaks clang-cl because running clang-cl also loads x86_64-unknown-linux-gnu.cfg (which will generically contain incompatible flags).

zmodem · 2024-12-13T15:04:29Z

You don't need to use config files with clang-cl to need this patch -- you need it if you use triple-based config files and also use clang-cl. The issue is that if you have, for example, a x86_64-unknown-linux-gnu.cfg in a location that clang loads by default, then this breaks clang-cl because running clang-cl also loads x86_64-unknown-linux-gnu.cfg (which will generically contain incompatible flags).

Okay, but that sounds like a case where clang-cl users do not want to use config files. Maybe the fix is to always ignore config files in clang-cl mode, since there is no corresponding concept in MSVC cl, and it's not commonly used for the embedded/cross compiling scenarios where config files are used?

zmodem · 2024-12-13T14:51:20Z

clang/lib/Driver/Driver.cpp

+    // The config file may have changed the architecture so apply it.
+    if (HasConfigFile && Args.hasArg(options::OPT__SLASH_arm64EC)) {
+      llvm::Triple T(TargetTriple);
+      if (T.getArch() != llvm::Triple::aarch64 ||
+          T.getSubArch() != llvm::Triple::AArch64SubArch_arm64ec) {
+        T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);
+        TargetTriple = T.str();


Thanks for explaining. I haven't looked at this code in a long time, and TargetTriple vs. computeTargetTriple() is pretty confusing.

It seems /arm64EC is not handled in computeTargetTriple()(?) so I suppose it needs to be handled somewhere :)

zmodem · 2024-12-13T15:00:57Z

clang/lib/Driver/Driver.cpp

+    if (CLOptions->hasArg(options::OPT__SLASH_arm64EC))
+      T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);


I thought normally the arguments in the config file should get processed before the command-line arguments? What are the rules for this really?

carlocab · 2024-12-13T17:14:05Z

Maybe the fix is to always ignore config files in clang-cl mode

I'd be fine with doing this too -- I just wasn't aware that clang-cl did not support config files (since this fact doesn't seem to be documented anywhere, unless I missed it).

zmodem · 2024-12-16T12:22:37Z

Looks like I was wrong. clang-cl does support --config since daebf2c, so I suppose we can't just ignore these files.

Your change to update the triple based on clang-cl mode earlier makes sense to me.

I'm less sure about manually inspecting the /arm64EC flag in that code. Driver::loadDefaultConfigFiles() has logic to figure out the triple, is it not handled there?

I'm even less sure about the "The config file may have changed the architecture so apply it" part. It's not clear to me that we need it at all, and if we do need it, why is it specific to the /arm64EC flag?

carlocab · 2025-01-23T06:16:06Z

LLVM 20 is branching soon -- would be good to try to get this in before then.

zmodem · 2025-01-27T14:14:23Z

Okay. I think most of my previous comments (except for "maybe the fix is to always ignore config files in clang-cl mode) still apply though.

carlocab · 2025-01-28T14:31:03Z

Yep, was just trying to ping @Bo98 for an update here.

PaulWagener · 2025-07-31T09:29:23Z

I've resolved the conflicts in commit http://github.com/PaulWagener/llvm-project/commit/40a8c7c0ff3f688b690e4c74db734de67f0f89e9 (Bo98#1)

I don't know much about the technical details of this PR but I can confirm that this correctly builds and fixes the issue with cross compiling with homebrew clang-cl that I've been having.

Would be happy to see this upstreamed.

skoulik · 2025-09-17T02:53:51Z

Hello guys,

Can you check/confirm if this fix is in any way related to #159229 that I observe?

[clang][Driver] Fix triple config loading for clang-cl

a3e8b86

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

carlocab requested review from AaronBallman, MaskRay, mgorny and zmodem December 12, 2024 12:10

carlocab approved these changes Dec 12, 2024

View reviewed changes

zmodem reviewed Dec 12, 2024

View reviewed changes

zmodem reviewed Dec 13, 2024

View reviewed changes

		if (CLOptions->hasArg(options::OPT__SLASH_arm64EC))
		T.setArch(llvm::Triple::aarch64, llvm::Triple::AArch64SubArch_arm64ec);

[clang][Driver] Fix triple config loading for clang-cl #111397

Are you sure you want to change the base?

[clang][Driver] Fix triple config loading for clang-cl #111397

Uh oh!

Conversation

Bo98 commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 7, 2024

Uh oh!

llvmbot commented Oct 7, 2024

Uh oh!

llvmbot commented Oct 7, 2024

Uh oh!

Bo98 commented Oct 23, 2024

Uh oh!

carlocab left a comment

Choose a reason for hiding this comment

Uh oh!

zmodem left a comment

Choose a reason for hiding this comment

Uh oh!

zmodem Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

Bo98 Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmodem Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

zmodem Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

Bo98 Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

carlocab commented Dec 12, 2024

Uh oh!

zmodem commented Dec 13, 2024

Uh oh!

zmodem Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

zmodem Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

carlocab commented Dec 13, 2024

Uh oh!

zmodem commented Dec 16, 2024

Uh oh!

carlocab commented Jan 23, 2025

Uh oh!

zmodem commented Jan 27, 2025

Uh oh!

carlocab commented Jan 28, 2025

Uh oh!

PaulWagener commented Jul 31, 2025

Uh oh!

skoulik commented Sep 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Bo98 commented Oct 7, 2024 •

edited

Loading

Bo98 Dec 12, 2024 •

edited

Loading