From b3ebe50664bbc5be6db6dc6ddb320e28105d37ba Mon Sep 17 00:00:00 2001
From: jyn <github@jyn.dev>
Date: Mon, 25 Dec 2023 07:43:08 -0500
Subject: [PATCH 1/2] add and document a new `is_system_llvm` abstraction
---
src/bootstrap/src/core/build_steps/dist.rs | 35 +++++++++++-----------
src/bootstrap/src/core/build_steps/test.rs | 2 ++
src/bootstrap/src/core/config/config.rs | 9 +++++-
src/bootstrap/src/lib.rs | 32 +++++++++++++++-----
4 files changed, 52 insertions(+), 26 deletions(-)
diff --git a/src/bootstrap/src/core/build_steps/dist.rs b/src/bootstrap/src/core/build_steps/dist.rs
index 98e267713daf7..46b3bdb659130 100644
--- a/src/bootstrap/src/core/build_steps/dist.rs
+++ b/src/bootstrap/src/core/build_steps/dist.rs
@@ -2042,23 +2042,24 @@ fn install_llvm_file(builder: &Builder<'_>, source: &Path, destination: &Path) {
///
/// Returns whether the files were actually copied.
fn maybe_install_llvm(builder: &Builder<'_>, target: TargetSelection, dst_libdir: &Path) -> bool {
- if let Some(config) = builder.config.target_config.get(&target) {
- if config.llvm_config.is_some() && !builder.config.llvm_from_ci {
- // If the LLVM was externally provided, then we don't currently copy
- // artifacts into the sysroot. This is not necessarily the right
- // choice (in particular, it will require the LLVM dylib to be in
- // the linker's load path at runtime), but the common use case for
- // external LLVMs is distribution provided LLVMs, and in that case
- // they're usually in the standard search path (e.g., /usr/lib) and
- // copying them here is going to cause problems as we may end up
- // with the wrong files and isn't what distributions want.
- //
- // This behavior may be revisited in the future though.
- //
- // If the LLVM is coming from ourselves (just from CI) though, we
- // still want to install it, as it otherwise won't be available.
- return false;
- }
+ // If the LLVM was externally provided, then we don't currently copy
+ // artifacts into the sysroot. This is not necessarily the right
+ // choice (in particular, it will require the LLVM dylib to be in
+ // the linker's load path at runtime), but the common use case for
+ // external LLVMs is distribution provided LLVMs, and in that case
+ // they're usually in the standard search path (e.g., /usr/lib) and
+ // copying them here is going to cause problems as we may end up
+ // with the wrong files and isn't what distributions want.
+ //
+ // This behavior may be revisited in the future though.
+ //
+ // NOTE: this intentionally doesn't use `is_rust_llvm`; whether this is patched or not doesn't matter,
+ // we only care if the shared object itself is managed by bootstrap.
+ //
+ // If the LLVM is coming from ourselves (just from CI) though, we
+ // still want to install it, as it otherwise won't be available.
+ if builder.is_system_llvm(target) {
+ return false;
}
// On macOS, rustc (and LLVM tools) link to an unversioned libLLVM.dylib
diff --git a/src/bootstrap/src/core/build_steps/test.rs b/src/bootstrap/src/core/build_steps/test.rs
index d0f36f99342fe..f953b0f8c9178 100644
--- a/src/bootstrap/src/core/build_steps/test.rs
+++ b/src/bootstrap/src/core/build_steps/test.rs
@@ -1848,6 +1848,8 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
llvm_components_passed = true;
}
if !builder.is_rust_llvm(target) {
+ // FIXME: missing Rust patches is not the same as being system llvm; we should rename the flag at some point.
+ // Inspecting the tests with `// no-system-llvm` in src/test *looks* like this is doing the right thing, though.
cmd.arg("--system-llvm");
}
diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs
index f1e1b89d9ba71..9199d540767e0 100644
--- a/src/bootstrap/src/core/config/config.rs
+++ b/src/bootstrap/src/core/config/config.rs
@@ -1810,7 +1810,14 @@ impl Config {
}
target.llvm_config = Some(config.src.join(s));
}
- target.llvm_has_rust_patches = cfg.llvm_has_rust_patches;
+ if let Some(patches) = cfg.llvm_has_rust_patches {
+ assert_eq!(
+ config.submodules,
+ Some(false),
+ "cannot set `llvm-has-rust-patches` for a managed submodule (set `build.submodules = false` if you want to apply patches)"
+ );
+ target.llvm_has_rust_patches = Some(patches);
+ }
if let Some(ref s) = cfg.llvm_filecheck {
target.llvm_filecheck = Some(config.src.join(s));
}
diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs
index 871318de5955e..30824f5852283 100644
--- a/src/bootstrap/src/lib.rs
+++ b/src/bootstrap/src/lib.rs
@@ -823,18 +823,34 @@ impl Build {
INTERNER.intern_path(self.out.join(&*target.triple).join("md-doc"))
}
- /// Returns `true` if no custom `llvm-config` is set for the specified target.
+ /// Returns `true` if this is an external version of LLVM not managed by bootstrap.
+ /// In particular, we expect llvm sources to be available when this is false.
///
- /// If no custom `llvm-config` was specified then Rust's llvm will be used.
+ /// NOTE: this is not the same as `!is_rust_llvm` when `llvm_has_patches` is set.
+ fn is_system_llvm(&self, target: TargetSelection) -> bool {
+ match self.config.target_config.get(&target) {
+ Some(Target { llvm_config: Some(_), .. }) => {
+ let ci_llvm = self.config.llvm_from_ci && target == self.config.build;
+ !ci_llvm
+ }
+ // We're building from the in-tree src/llvm-project sources.
+ Some(Target { llvm_config: None, .. }) => false,
+ None => false,
+ }
+ }
+
+ /// Returns `true` if this is our custom, patched, version of LLVM.
+ ///
+ /// This does not necessarily imply that we're managing the `llvm-project` submodule.
fn is_rust_llvm(&self, target: TargetSelection) -> bool {
match self.config.target_config.get(&target) {
+ // We're using a user-controlled version of LLVM. The user has explicitly told us whether the version has our patches.
+ // (They might be wrong, but that's not a supported use-case.)
+ // In particular, this tries to support `submodules = false` and `patches = false`, for using a newer version of LLVM that's not through `rust-lang/llvm-project`.
Some(Target { llvm_has_rust_patches: Some(patched), .. }) => *patched,
- Some(Target { llvm_config, .. }) => {
- // If the user set llvm-config we assume Rust is not patched,
- // but first check to see if it was configured by llvm-from-ci.
- (self.config.llvm_from_ci && target == self.config.build) || llvm_config.is_none()
- }
- None => true,
+ // The user hasn't promised the patches match.
+ // This only has our patches if it's downloaded from CI or built from source.
+ _ => !self.is_system_llvm(target),
}
}
From a10b1be432865c5e1152dbb2e3fc0dabd6668335 Mon Sep 17 00:00:00 2001
From: jyn <github@jyn.dev>
Date: Mon, 25 Dec 2023 07:46:12 -0500
Subject: [PATCH 2/2] add a new `optimized_compiler_builtins` option
in particular, this makes the `c` feature for compiler-builtins an explicit opt-in, rather than silently detected by whether `llvm-project` is checked out on disk.
exposing this is necessary because the `cc` crate doesn't support cross-compiling to MSVC, and we want people to be able to run `x check --target foo` regardless of whether they have a c toolchain available.
this also uses the new option in CI, where we *do* want to optimize compiler_builtins.
the new option is off by default for the `dev` channel and on otherwise.
---
config.example.toml | 8 +++++++
src/bootstrap/src/core/build_steps/compile.rs | 21 ++++++++++++++-----
src/bootstrap/src/core/config/config.rs | 6 ++++++
src/bootstrap/src/utils/change_tracker.rs | 5 +++++
.../disabled/dist-x86_64-haiku/Dockerfile | 2 ++
.../host-x86_64/dist-various-2/Dockerfile | 2 ++
.../host-x86_64/x86_64-gnu-llvm-17/Dockerfile | 1 +
src/ci/run.sh | 9 ++++++++
8 files changed, 49 insertions(+), 5 deletions(-)
diff --git a/config.example.toml b/config.example.toml
index 4cf7c1e81990c..a45f50cd106b7 100644
--- a/config.example.toml
+++ b/config.example.toml
@@ -339,6 +339,14 @@
# on this runtime, such as `-C profile-generate` or `-C instrument-coverage`).
#profiler = false
+# Use the optimized LLVM C intrinsics for `compiler_builtins`, rather than Rust intrinsics.
+# Requires the LLVM submodule to be managed by bootstrap (i.e. not external) so that `compiler-rt`
+# sources are available.
+#
+# Setting this to `false` generates slower code, but removes the requirement for a C toolchain in
+# order to run `x check`.
+#optimized-compiler-builtins = if rust.channel == "dev" { false } else { true }
+
# Indicates whether the native libraries linked into Cargo will be statically
# linked or not.
#cargo-native-static = false
diff --git a/src/bootstrap/src/core/build_steps/compile.rs b/src/bootstrap/src/core/build_steps/compile.rs
index df4d1a43dabc7..57caf9b403198 100644
--- a/src/bootstrap/src/core/build_steps/compile.rs
+++ b/src/bootstrap/src/core/build_steps/compile.rs
@@ -382,9 +382,7 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car
// Determine if we're going to compile in optimized C intrinsics to
// the `compiler-builtins` crate. These intrinsics live in LLVM's
- // `compiler-rt` repository, but our `src/llvm-project` submodule isn't
- // always checked out, so we need to conditionally look for this. (e.g. if
- // an external LLVM is used we skip the LLVM submodule checkout).
+ // `compiler-rt` repository.
//
// Note that this shouldn't affect the correctness of `compiler-builtins`,
// but only its speed. Some intrinsics in C haven't been translated to Rust
@@ -395,8 +393,21 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car
// If `compiler-rt` is available ensure that the `c` feature of the
// `compiler-builtins` crate is enabled and it's configured to learn where
// `compiler-rt` is located.
- let compiler_builtins_root = builder.src.join("src/llvm-project/compiler-rt");
- let compiler_builtins_c_feature = if compiler_builtins_root.exists() {
+ let compiler_builtins_c_feature = if builder.config.optimized_compiler_builtins {
+ // NOTE: this interacts strangely with `llvm-has-rust-patches`. In that case, we enforce `submodules = false`, so this is a no-op.
+ // But, the user could still decide to manually use an in-tree submodule.
+ //
+ // NOTE: if we're using system llvm, we'll end up building a version of `compiler-rt` that doesn't match the LLVM we're linking to.
+ // That's probably ok? At least, the difference wasn't enforced before. There's a comment in
+ // the compiler_builtins build script that makes me nervous, though:
+ // https://github.com/rust-lang/compiler-builtins/blob/31ee4544dbe47903ce771270d6e3bea8654e9e50/build.rs#L575-L579
+ builder.update_submodule(&Path::new("src").join("llvm-project"));
+ let compiler_builtins_root = builder.src.join("src/llvm-project/compiler-rt");
+ if !compiler_builtins_root.exists() {
+ panic!(
+ "need LLVM sources available to build `compiler-rt`, but they weren't present; consider enabling `build.submodules = true` or disabling `optimized-compiler-builtins`"
+ );
+ }
// Note that `libprofiler_builtins/build.rs` also computes this so if
// you're changing something here please also change that.
cargo.env("RUST_COMPILER_RT_ROOT", &compiler_builtins_root);
diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs
index 9199d540767e0..7c1071212779e 100644
--- a/src/bootstrap/src/core/config/config.rs
+++ b/src/bootstrap/src/core/config/config.rs
@@ -178,6 +178,8 @@ pub struct Config {
pub patch_binaries_for_nix: Option<bool>,
pub stage0_metadata: Stage0Metadata,
pub android_ndk: Option<PathBuf>,
+ /// Whether to use the `c` feature of the `compiler_builtins` crate.
+ pub optimized_compiler_builtins: bool,
pub stdout_is_tty: bool,
pub stderr_is_tty: bool,
@@ -848,6 +850,7 @@ define_config! {
// NOTE: only parsed by bootstrap.py, `--feature build-metrics` enables metrics unconditionally
metrics: Option<bool> = "metrics",
android_ndk: Option<PathBuf> = "android-ndk",
+ optimized_compiler_builtins: Option<bool> = "optimized-compiler-builtins",
}
}
@@ -1396,6 +1399,7 @@ impl Config {
// This field is only used by bootstrap.py
metrics: _,
android_ndk,
+ optimized_compiler_builtins,
} = toml.build.unwrap_or_default();
if let Some(file_build) = build {
@@ -1916,6 +1920,8 @@ impl Config {
config.rust_debuginfo_level_std = with_defaults(debuginfo_level_std);
config.rust_debuginfo_level_tools = with_defaults(debuginfo_level_tools);
config.rust_debuginfo_level_tests = debuginfo_level_tests.unwrap_or(DebuginfoLevel::None);
+ config.optimized_compiler_builtins =
+ optimized_compiler_builtins.unwrap_or(config.channel != "dev");
let download_rustc = config.download_rustc_commit.is_some();
// See https://github.com/rust-lang/compiler-team/issues/326
diff --git a/src/bootstrap/src/utils/change_tracker.rs b/src/bootstrap/src/utils/change_tracker.rs
index 1eadc036b5e6e..2238fd61a35a9 100644
--- a/src/bootstrap/src/utils/change_tracker.rs
+++ b/src/bootstrap/src/utils/change_tracker.rs
@@ -101,4 +101,9 @@ pub const CONFIG_CHANGE_HISTORY: &[ChangeInfo] = &[
severity: ChangeSeverity::Warning,
summary: "rust-analyzer-proc-macro-srv is no longer enabled by default. To build it, you must either enable it in the configuration or explicitly invoke it with x.py.",
},
+ ChangeInfo {
+ change_id: 102579,
+ severity: ChangeSeverity::Warning,
+ summary: "A new `optimized-compiler-builtins` option has been introduced. Whether to build llvm's `compiler-rt` from source is no longer implicitly controlled by git state. See the PR for more details.",
+ },
];
diff --git a/src/ci/docker/host-x86_64/disabled/dist-x86_64-haiku/Dockerfile b/src/ci/docker/host-x86_64/disabled/dist-x86_64-haiku/Dockerfile
index 5ddd3f1803964..637b5fa22f974 100644
--- a/src/ci/docker/host-x86_64/disabled/dist-x86_64-haiku/Dockerfile
+++ b/src/ci/docker/host-x86_64/disabled/dist-x86_64-haiku/Dockerfile
@@ -47,4 +47,6 @@ ENV RUST_CONFIGURE_ARGS --disable-jemalloc \
--set=$TARGET.cc=x86_64-unknown-haiku-gcc \
--set=$TARGET.cxx=x86_64-unknown-haiku-g++ \
--set=$TARGET.llvm-config=/bin/llvm-config-haiku
+ENV EXTERNAL_LLVM 1
+
ENV SCRIPT python3 ../x.py dist --host=$HOST --target=$HOST
diff --git a/src/ci/docker/host-x86_64/dist-various-2/Dockerfile b/src/ci/docker/host-x86_64/dist-various-2/Dockerfile
index 5f1fec74bed54..380e716746b5b 100644
--- a/src/ci/docker/host-x86_64/dist-various-2/Dockerfile
+++ b/src/ci/docker/host-x86_64/dist-various-2/Dockerfile
@@ -139,4 +139,6 @@ ENV RUST_CONFIGURE_ARGS --enable-extended --enable-lld --disable-docs \
--set target.wasm32-wasi-preview1-threads.wasi-root=/wasm32-wasi-preview1-threads \
--musl-root-armv7=/musl-armv7
+ENV EXTERNAL_LLVM 1
+
ENV SCRIPT python3 ../x.py dist --host='' --target $TARGETS
diff --git a/src/ci/docker/host-x86_64/x86_64-gnu-llvm-17/Dockerfile b/src/ci/docker/host-x86_64/x86_64-gnu-llvm-17/Dockerfile
index f1d6b9a4ef26b..fe30a95344104 100644
--- a/src/ci/docker/host-x86_64/x86_64-gnu-llvm-17/Dockerfile
+++ b/src/ci/docker/host-x86_64/x86_64-gnu-llvm-17/Dockerfile
@@ -40,6 +40,7 @@ RUN sh /scripts/sccache.sh
# We are disabling CI LLVM since this builder is intentionally using a host
# LLVM, rather than the typical src/llvm-project LLVM.
ENV NO_DOWNLOAD_CI_LLVM 1
+ENV EXTERNAL_LLVM 1
# Using llvm-link-shared due to libffi issues -- see #34486
ENV RUST_CONFIGURE_ARGS \
diff --git a/src/ci/run.sh b/src/ci/run.sh
index 5700172fd3ec4..a979e3a2f099d 100755
--- a/src/ci/run.sh
+++ b/src/ci/run.sh
@@ -80,6 +80,15 @@ fi
# space required for CI artifacts.
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --dist-compression-formats=xz"
+# Enable the `c` feature for compiler_builtins, but only when the `compiler-rt` source is available
+# (to avoid spending a lot of time cloning llvm)
+if [ "$EXTERNAL_LLVM" = "" ]; then
+ RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set build.optimized-compiler-builtins"
+elif [ "$DEPLOY$DEPLOY_ALT" = "1" ]; then
+ echo "error: dist builds should always use optimized compiler-rt!" >&2
+ exit 1
+fi
+
if [ "$DIST_SRC" = "" ]; then
RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --disable-dist-src"
fi