Skip to content

Commit

Permalink
Revert to always passing --target to Clang
Browse files Browse the repository at this point in the history
  • Loading branch information
madsmtm committed Feb 5, 2025
1 parent 353566c commit bfdf3d5
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 32 deletions.
69 changes: 45 additions & 24 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2194,21 +2194,34 @@ impl Build {

// Pass `--target` with the LLVM target to configure Clang for cross-compiling.
//
// NOTE: In the past, we passed this, along with the deployment version in here
// on Apple targets, but versioned targets were found to have poor compatibility
// with older versions of Clang, especially around comes to configuration files.
// This is **required** for cross-compilation, as it's the only flag that
// consistently forces Clang to change the "toolchain" that is responsible for
// parsing target-specific flags:
// https://github.com/rust-lang/cc-rs/issues/1388
// https://github.com/llvm/llvm-project/blob/llvmorg-19.1.7/clang/lib/Driver/Driver.cpp#L1359-L1360
// https://github.com/llvm/llvm-project/blob/llvmorg-19.1.7/clang/lib/Driver/Driver.cpp#L6347-L6532
//
// Instead, we specify `-arch` along with `-mmacosx-version-min=`, `-mtargetos=`
// and similar flags in `.apple_flags()`.
// This can be confusing, because on e.g. host macOS, you can usually get by
// with `-arch` and `-mtargetos=`. But that only works because the _default_
// toolchain is `Darwin`, which enables parsing of darwin-specific options.
//
// Note that Clang errors when both `-mtargetos=` and `-target` are specified,
// so we omit this entirely on Apple targets (it's redundant when specifying
// both the `-arch` and the deployment target / OS flag) (in theory we _could_
// specify this on some of the Apple targets that use the older
// `-m*-version-min=`, but for consistency we omit it entirely).
if target.vendor != "apple" {
cmd.push_cc_arg(format!("--target={}", target.llvm_target).into());
}
// NOTE: In the past, we passed the deployment version in here on all Apple
// targets, but versioned targets were found to have poor compatibility with
// older versions of Clang, especially when it comes to configuration files:
// https://github.com/rust-lang/cc-rs/issues/1278
//
// So instead, we pass the deployment target with `-m*-version-min=`, and only
// pass it here on visionOS and Mac Catalyst where that option does not exist:
// https://github.com/rust-lang/cc-rs/issues/1383
let clang_target = if target.os == "visionos" || target.abi == "macabi" {
Cow::Owned(
target.versioned_llvm_target(&self.apple_deployment_target(target)),
)
} else {
Cow::Borrowed(target.llvm_target)
};

cmd.push_cc_arg(format!("--target={clang_target}").into());
}
}
ToolFamily::Msvc { clang_cl } => {
Expand Down Expand Up @@ -2648,21 +2661,29 @@ impl Build {
fn apple_flags(&self, cmd: &mut Tool) -> Result<(), Error> {
let target = self.get_target()?;

// Add `-arch` on all compilers. This is a Darwin/Apple-specific flag
// that works both on GCC and Clang.
// This is a Darwin/Apple-specific flag that works both on GCC and Clang, but it is only
// necessary on GCC since we specify `-target` on Clang.
// https://gcc.gnu.org/onlinedocs/gcc/Darwin-Options.html#:~:text=arch
// https://clang.llvm.org/docs/CommandGuide/clang.html#cmdoption-arch
let arch = map_darwin_target_from_rust_to_compiler_architecture(&target);
cmd.args.push("-arch".into());
cmd.args.push(arch.into());
if cmd.is_like_gnu() {
let arch = map_darwin_target_from_rust_to_compiler_architecture(&target);
cmd.args.push("-arch".into());
cmd.args.push(arch.into());
}

// Pass the deployment target via `-mmacosx-version-min=`, `-mtargetos=` and similar.
//
// It is also necessary on GCC, as it forces a compilation error if the compiler is not
// Pass the deployment target via `-mmacosx-version-min=`, `-miphoneos-version-min=` and
// similar. Also necessary on GCC, as it forces a compilation error if the compiler is not
// configured for Darwin: https://gcc.gnu.org/onlinedocs/gcc/Darwin-Options.html
let min_version = self.apple_deployment_target(&target);
cmd.args
.push(target.apple_version_flag(&min_version).into());
//
// On visionOS and Mac Catalyst, there is no -m*-version-min= flag:
// https://github.com/llvm/llvm-project/issues/88271
// And the workaround to use `-mtargetos=` cannot be used with the `--target` flag that we
// otherwise specify. So we avoid emitting that, and put the version in `--target` instead.
if cmd.is_like_gnu() || !(target.os == "visionos" || target.abi == "macabi") {
let min_version = self.apple_deployment_target(&target);
cmd.args
.push(target.apple_version_flag(&min_version).into());
}

// AppleClang sometimes requires sysroot even on macOS
if cmd.is_xctoolchain_clang() || target.os != "macos" {
Expand Down
23 changes: 23 additions & 0 deletions src/target/llvm.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,26 @@
use super::TargetInfo;

impl TargetInfo<'_> {
/// The versioned LLVM/Clang target triple.
pub(crate) fn versioned_llvm_target(&self, version: &str) -> String {
// Only support versioned Apple targets for now.
assert_eq!(self.vendor, "apple");

let mut components = self.llvm_target.split("-");
let arch = components.next().expect("llvm_target should have arch");
let vendor = components.next().expect("llvm_target should have vendor");
let os = components.next().expect("LLVM target should have os");
let environment = components.next();
assert_eq!(components.next(), None, "too many LLVM target components");

if let Some(env) = environment {
format!("{arch}-{vendor}-{os}{version}-{env}")
} else {
format!("{arch}-{vendor}-{os}{version}")
}
}
}

/// Rust and Clang don't really agree on naming, so do a best-effort
/// conversion to support out-of-tree / custom target-spec targets.
pub(crate) fn guess_llvm_target_triple(
Expand Down
15 changes: 7 additions & 8 deletions tests/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ fn clang_apple_tvos() {
.file("foo.c")
.compile("foo");

test.cmd(0).must_have_in_order("-arch", "arm64");
test.cmd(0).must_have("--target=arm64-apple-tvos");
test.cmd(0).must_have("-mappletvos-version-min=9.0");
}

Expand All @@ -640,10 +640,9 @@ fn clang_apple_mac_catalyst() {
.compile("foo");
let execution = test.cmd(0);

execution.must_have_in_order("-arch", "arm64");
execution.must_have("--target=arm64-apple-ios15.0-macabi");
// --target and -mtargetos= don't mix
execution.must_not_have("--target=arm64-apple-ios-macabi");
execution.must_have("-mtargetos=ios15.0-macabi");
execution.must_not_have("-mtargetos=");
execution.must_have_in_order("-isysroot", sdkroot);
execution.must_have_in_order(
"-isystem",
Expand Down Expand Up @@ -672,7 +671,8 @@ fn clang_apple_tvsimulator() {
.file("foo.c")
.compile("foo");

test.cmd(0).must_have_in_order("-arch", "x86_64");
test.cmd(0)
.must_have("--target=x86_64-apple-tvos-simulator");
test.cmd(0).must_have("-mappletvsimulator-version-min=9.0");
}

Expand All @@ -697,10 +697,9 @@ fn clang_apple_visionos() {

dbg!(test.cmd(0).args);

test.cmd(0).must_have_in_order("-arch", "arm64");
test.cmd(0).must_have("--target=arm64-apple-xros1.0");
// --target and -mtargetos= don't mix.
test.cmd(0).must_not_have("--target=arm64-apple-xros");
test.cmd(0).must_have("-mtargetos=xros1.0");
test.cmd(0).must_not_have("-mtargetos=");

// Flags that don't exist.
test.cmd(0).must_not_have("-mxros-version-min=1.0");
Expand Down

0 comments on commit bfdf3d5

Please sign in to comment.