Auto merge of #119556 - onur-ozkan:optimized-compiler-builtins, r=onur-ozkan

Reland optimized-compiler-builtins config

Copy of #102579 PR.

From #102579:

> No concerns on my side. Currently, Jyn isn't actively working on the project. I will close this PR; open another one to cherry-pick the commits, resolve conflicts, and then r+ it.

> Fixes https://github.com/rust-lang/rust/issues/102560. Fixes https://github.com/rust-lang/rust/issues/101172. Helps with https://github.com/rust-lang/rust/issues/105065 (although there's some weirdness there - it's still broken when optimized-compiler-builtins is set to true).

Fixes https://github.com/rust-lang/rust/issues/102560. Fixes https://github.com/rust-lang/rust/issues/101172. Helps with https://github.com/rust-lang/rust/issues/105065

r? ghost

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

src/bootstrap/src/core/build_steps/compile.rs

@@ -406,9 +406,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
@@ -419,8 +417,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);

src/bootstrap/src/core/build_steps/dist.rs

@@ -2032,23 +2032,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

src/bootstrap/src/core/build_steps/test.rs

@@ -1850,6 +1850,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");
             }
 

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 {
@@ -1810,7 +1814,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));
                 }
@@ -1909,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

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),
         }
     }
 

src/bootstrap/src/utils/change_tracker.rs

@@ -106,4 +106,9 @@ pub const CONFIG_CHANGE_HISTORY: &[ChangeInfo] = &[
         severity: ChangeSeverity::Info,
         summary: "The dist.missing-tools config option was deprecated, as it was unused. If you are using it, remove it from your config, it will be removed soon.",
     },
+    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.",
+    },
 ];

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

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 \

src/ci/run.sh

@@ -85,6 +85,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