From a8b210dc75025bac0961e4874961d09d25ce2a8e Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Thu, 24 Nov 2022 20:03:02 -0500 Subject: [PATCH] address review comments --- src/bootstrap/compile.rs | 14 ++++++-------- src/bootstrap/config.rs | 5 ++++- src/bootstrap/dist.rs | 15 ++++----------- src/bootstrap/lib.rs | 17 ++++++++++++++++- src/bootstrap/test.rs | 2 ++ 5 files changed, 32 insertions(+), 21 deletions(-) diff --git a/src/bootstrap/compile.rs b/src/bootstrap/compile.rs index 21a2faeb953e7..54b026ade133c 100644 --- a/src/bootstrap/compile.rs +++ b/src/bootstrap/compile.rs @@ -310,17 +310,15 @@ pub fn std_cargo(builder: &Builder<'_>, target: TargetSelection, stage: u32, car // `compiler-builtins` crate is enabled and it's configured to learn where // `compiler-rt` is located. let compiler_builtins_c_feature = if builder.config.optimized_compiler_builtins { - if !builder.is_rust_llvm(target) { - panic!( - "LLVM must have the Rust project's patched sources to support using it for `compiler-builtins`; unset `llvm-config` or `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. + // + // Using system llvm is not supported. 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() { + if !compiler_builtins_root.exists() || builder.is_system_llvm(target) { panic!( - "needed LLVM sources available to build `compiler-rt`, but they weren't present; consider enabling `build.submodules = true`" + "needed LLVM sources available to build `compiler-rt`, but they weren't present; consider enabling `build.submodules = true`, disabling `optimized-compiler-builtins`, or unsetting `llvm-config`" ); } // Note that `libprofiler_builtins/build.rs` also computes this so if diff --git a/src/bootstrap/config.rs b/src/bootstrap/config.rs index 94a48a79b0939..9e9ea5eb29aa8 100644 --- a/src/bootstrap/config.rs +++ b/src/bootstrap/config.rs @@ -1229,7 +1229,10 @@ impl Config { target.llvm_config = Some(config.src.join(s)); } if let Some(patches) = cfg.llvm_has_rust_patches { - assert!(config.submodules.is_some(), "cannot set `llvm-hash-rust-patches` for a managed submodule"); + assert!( + config.submodules.is_some(), + "cannot set `llvm-has-rust-patches` for a managed submodule" + ); target.llvm_has_rust_patches = Some(patches); } if let Some(ref s) = cfg.llvm_filecheck { diff --git a/src/bootstrap/dist.rs b/src/bootstrap/dist.rs index f039e89c06eb7..7c3e4eb148366 100644 --- a/src/bootstrap/dist.rs +++ b/src/bootstrap/dist.rs @@ -22,7 +22,6 @@ use crate::builder::{Builder, Kind, RunConfig, ShouldRun, Step}; use crate::cache::{Interned, INTERNER}; use crate::channel; use crate::compile; -use crate::config::Target; use crate::config::TargetSelection; use crate::doc::DocumentationFormat; use crate::tarball::{GeneratedTarball, OverlayKind, Tarball}; @@ -1904,16 +1903,10 @@ fn maybe_install_llvm(builder: &Builder<'_>, target: TargetSelection, dst_libdir // // 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. - let should_install_llvm = match builder.config.target_config.get(&target) { - // If the LLVM is coming from ourselves (just from CI) though, we - // still want to install it, as it otherwise won't be available. - Some(Target { llvm_config: Some(_), .. }) => { - builder.config.llvm_from_ci && target == builder.config.build - } - Some(Target { llvm_config: None, .. }) | None => true, - }; - - if !should_install_llvm { + // + // 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; } diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index 693cd7b9c2dba..c1e1d7af35cbf 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -877,12 +877,27 @@ impl Build { INTERNER.intern_path(self.out.join(&*target.triple).join("md-doc")) } + /// 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. + /// + /// 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(_), .. }) => { + !self.config.llvm_from_ci || target != self.config.build + } + Some(Target { llvm_config: None, .. }) | 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 pre-built version of LLVM, but the user has promised that pre-built version has our patches. + // 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, // We're using pre-built LLVM and the user hasn't promised the patches match. // This only has our patches if it's our managed, CI-built LLVM. diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 38873cee93bd9..f10028caf3880 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -1551,6 +1551,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"); }