address review comments

Author: jyn514

src/bootstrap/compile.rs

@@ -350,17 +350,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

src/bootstrap/config.rs

@@ -1515,7 +1515,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 {

src/bootstrap/dist.rs

@@ -25,7 +25,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::llvm;
@@ -1978,16 +1977,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;
     }
 

src/bootstrap/lib.rs

@@ -863,12 +863,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.

src/bootstrap/test.rs

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