Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+35 −11
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions clang/lib/Driver/Driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines +1258 to +1259
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

TargetTriple = T.str();
}

// Try parsing configuration file.
if (!ContainsError)
ContainsError = loadConfigFiles();
Expand Down Expand Up @@ -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();
Comment on lines +1303 to +1309
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@Bo98 Bo98 Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

}
}
}

// Check for working directory option before accessing any files
Expand Down Expand Up @@ -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()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mode have the same issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Build TargetTriple from target_profile option for clang-dxc.
if (const Arg *A = Args.getLastArg(options::OPT_target_profile)) {
StringRef TargetProfile = A->getValue();
Expand Down
Empty file added clang/test/Driver/Inputs/config-cl/arm64ec-pc-windows-msvc.cfg
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions clang/test/Driver/Inputs/config-cl/config-arm64ec-arg.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/arm64EC
10 changes: 10 additions & 0 deletions clang/test/Driver/config-file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading