Auto merge of #159455 - Kobzol:bootstrap-discover-lldb, r=jieyouxu

Implement opt-in debugger discovery for GDB and LLDB in bootstrap



GDB and LLDB is no longer automatically auto-discovered, you can opt into the previous behavior using `build.gdb/lldb = "discover"`.

Note: this means that unless you opt into using a specific debugger, you won't run any `debuginfo` tests by default (except for `cdb`, which is still automatically detected).

r? @jieyouxu

bootstrap.example.toml

@@ -343,10 +343,12 @@
 
 # The path to (or name of) the GDB executable to use. This is only used for
 # executing the debuginfo test suite.
+# Set this to "discover" to automatically discover GDB from the environment.
 #build.gdb = "gdb"
 
 # The path to (or name of) the LLDB executable to use. This is only used for
 # executing the debuginfo test suite.
+# Set this to "discover" to automatically discover LLDB from the environment.
 #build.lldb = "lldb"
 
 # The node.js executable to use. Note that this is only used for the emscripten

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

@@ -2544,7 +2544,7 @@ Please disable assertions with `rust.debug-assertions = false`.
 
             if let Some(debuggers::Gdb { gdb }) = debuggers::discover_gdb(builder, android.as_ref())
             {
-                cmd.arg("--gdb").arg(gdb.as_ref());
+                cmd.arg("--gdb").arg(gdb);
             }
 
             if let Some(debuggers::Lldb { lldb_exe, lldb_version }) =

src/bootstrap/src/core/config/config.rs

@@ -49,8 +49,9 @@ use crate::core::config::toml::target::{
     DefaultLinuxLinkerOverride, Target, TomlTarget, default_linux_linker_overrides,
 };
 use crate::core::config::{
-    CompilerBuiltins, CompressDebuginfo, DebuginfoLevel, DryRun, GccCiMode, LlvmLibunwind, Merge,
-    OverrideAllocator, ReplaceOpt, RustcLto, SplitDebuginfo, StringOrBool, threads_from_config,
+    CompilerBuiltins, CompressDebuginfo, DebuggerPath, DebuginfoLevel, DryRun, GccCiMode,
+    LlvmLibunwind, Merge, OverrideAllocator, ReplaceOpt, RustcLto, SplitDebuginfo, StringOrBool,
+    threads_from_config,
 };
 use crate::core::download::{
     DownloadContext, download_beta_toolchain, is_download_ci_available, maybe_download_rustfmt,
@@ -284,8 +285,8 @@ pub struct Config {
     pub codegen_tests: bool,
     pub nodejs: Option<PathBuf>,
     pub yarn: Option<PathBuf>,
-    pub gdb: Option<PathBuf>,
-    pub lldb: Option<PathBuf>,
+    pub gdb: Option<DebuggerPath>,
+    pub lldb: Option<DebuggerPath>,
     pub python: Option<PathBuf>,
     pub windows_rc: Option<PathBuf>,
     pub reuse: Option<PathBuf>,
@@ -1457,7 +1458,7 @@ NOTE: Please add `--stage 2` to your command line, or if you're sure you want to
             free_args: flags_free_args,
             full_bootstrap: build_full_bootstrap.unwrap_or(false),
             gcc_ci_mode,
-            gdb: build_gdb.map(PathBuf::from),
+            gdb: build_gdb,
             host_target,
             hosts,
             in_tree_gcc_info,
@@ -1478,7 +1479,7 @@ NOTE: Please add `--stage 2` to your command line, or if you're sure you want to
             libgccjit_libs_dir: gcc_libgccjit_libs_dir,
             library_docs_private_items: build_library_docs_private_items.unwrap_or(false),
             lld_enabled,
-            lldb: build_lldb.map(PathBuf::from),
+            lldb: build_lldb,
             llvm_allow_old_toolchain: llvm_allow_old_toolchain.unwrap_or(false),
             llvm_assertions,
             llvm_bitcode_linker_enabled: rust_llvm_bitcode_linker.unwrap_or(false),

src/bootstrap/src/core/config/mod.rs

@@ -508,6 +508,27 @@ pub enum GccCiMode {
     DownloadFromCi,
 }
 
+#[derive(Clone, Debug, PartialEq)]
+pub enum DebuggerPath {
+    /// Use a debugger at this path
+    Path(PathBuf),
+    /// Try to automatically discover a version of a debugger from the environment
+    Discover,
+}
+
+impl<'d> Deserialize<'d> for DebuggerPath {
+    fn deserialize<D>(deserializer: D) -> Result<Self, <D as serde::Deserializer<'d>>::Error>
+    where
+        D: serde::Deserializer<'d>,
+    {
+        let value = String::deserialize(deserializer)?;
+        match value.as_str() {
+            "discover" => Ok(Self::Discover),
+            path => Ok(Self::Path(PathBuf::from(path))),
+        }
+    }
+}
+
 pub fn threads_from_config(v: u32) -> u32 {
     match v {
         0 => std::thread::available_parallelism().map_or(1, std::num::NonZeroUsize::get) as u32,

src/bootstrap/src/core/config/tests.rs

@@ -11,7 +11,7 @@ use clap::CommandFactory;
 use super::flags::Flags;
 use super::toml::change_id::ChangeIdWrapper;
 use super::toml::rust::parse_codegen_backends;
-use super::{Config, RUSTC_IF_UNCHANGED_ALLOWED_PATHS};
+use super::{Config, DebuggerPath, RUSTC_IF_UNCHANGED_ALLOWED_PATHS};
 use crate::ChangeId;
 use crate::core::build_steps::clippy::{LintConfig, get_clippy_rules_in_order};
 use crate::core::build_steps::llvm::LLVM_INVALIDATION_PATHS;
@@ -111,7 +111,11 @@ fn override_toml() {
         crate::core::config::RustcLto::Fat,
         "setting string value without quotes"
     );
-    assert_eq!(config.gdb, Some("bar".into()), "setting string value with quotes");
+    assert_eq!(
+        config.gdb,
+        Some(DebuggerPath::Path("bar".into())),
+        "setting string value with quotes"
+    );
     assert!(!config.deny_warnings, "setting boolean value");
     assert_eq!(
         config.optimized_compiler_builtins,

src/bootstrap/src/core/config/toml/build.rs

@@ -11,7 +11,7 @@ use std::collections::HashMap;
 use serde::{Deserialize, Deserializer};
 
 use crate::core::config::toml::ReplaceOpt;
-use crate::core::config::{CompilerBuiltins, Merge, StringOrBool};
+use crate::core::config::{CompilerBuiltins, DebuggerPath, Merge, StringOrBool};
 use crate::{HashSet, PathBuf, define_config, exit};
 
 define_config! {
@@ -33,8 +33,8 @@ define_config! {
         library_docs_private_items: Option<bool> = "library-docs-private-items",
         docs_minification: Option<bool> = "docs-minification",
         submodules: Option<bool> = "submodules",
-        gdb: Option<String> = "gdb",
-        lldb: Option<String> = "lldb",
+        gdb: Option<DebuggerPath> = "gdb",
+        lldb: Option<DebuggerPath> = "lldb",
         nodejs: Option<String> = "nodejs",
         npm: Option<String> = "npm", // unused, present for compatibility
         yarn: Option<String> = "yarn",

src/bootstrap/src/core/debuggers/gdb.rs

@@ -1,39 +1,36 @@
-use std::borrow::Cow;
-use std::path::Path;
+use std::path::PathBuf;
 
 use crate::core::android;
 use crate::core::builder::Builder;
+use crate::core::config::DebuggerPath;
 use crate::utils::exec::BootstrapCommand;
 
-pub(crate) struct Gdb<'a> {
-    pub(crate) gdb: Cow<'a, Path>,
+pub(crate) struct Gdb {
+    pub(crate) gdb: PathBuf,
 }
 
-pub(crate) fn discover_gdb<'a>(
-    builder: &'a Builder<'_>,
+pub(crate) fn discover_gdb(
+    builder: &Builder<'_>,
     android: Option<&android::Android>,
-) -> Option<Gdb<'a>> {
+) -> Option<Gdb> {
     // If there's an explicitly-configured gdb, use that.
-    if let Some(gdb) = builder.config.gdb.as_deref() {
-        // FIXME(Zalathar): Consider returning None if gdb is an empty string,
-        // as a way to explicitly disable ambient gdb discovery.
-        let gdb = Cow::Borrowed(gdb);
-        return Some(Gdb { gdb });
+    match &builder.config.gdb {
+        Some(DebuggerPath::Path(path)) => {
+            return Some(Gdb { gdb: path.clone() });
+        }
+        Some(DebuggerPath::Discover) => {}
+        None => return None,
     }
 
     // Otherwise, fall back to whatever gdb is sitting around in PATH.
-    // (That's the historical behavior, but maybe we should require opt-in?)
-
-    let gdb: Cow<'_, Path> = match android {
-        Some(android::Android { android_cross_path, .. }) => {
-            android_cross_path.join("bin/gdb").into()
-        }
-        None => Path::new("gdb").into(),
+    let gdb = match android {
+        Some(android::Android { android_cross_path, .. }) => android_cross_path.join("bin/gdb"),
+        None => PathBuf::from("gdb"),
     };
 
     // Check whether an ambient gdb exists, by running `gdb --version`.
     let output = {
-        let mut gdb_command = BootstrapCommand::new(gdb.as_ref()).allow_failure();
+        let mut gdb_command = BootstrapCommand::new(&gdb).allow_failure();
         gdb_command.arg("--version");
         gdb_command.run_capture(builder)
     };

src/bootstrap/src/core/debuggers/lldb.rs

@@ -1,6 +1,7 @@
 use std::path::PathBuf;
 
 use crate::core::builder::Builder;
+use crate::core::config::DebuggerPath;
 use crate::utils::exec::command;
 
 pub(crate) struct Lldb {
@@ -9,17 +10,17 @@ pub(crate) struct Lldb {
 }
 
 pub(crate) fn discover_lldb(builder: &Builder<'_>) -> Option<Lldb> {
-    // FIXME(#148361): We probably should not be picking up whatever arbitrary
-    // lldb happens to be in the user's path, and instead require some kind of
-    // explicit opt-in or configuration.
-    let lldb_exe = builder.config.lldb.clone().unwrap_or_else(|| PathBuf::from("lldb"));
+    // If a path to a LLDB binary was provided, it has to exist and return some version, to avoid
+    // silent failures.
+    let (lldb_exe, explicitly_set_lldb) = match &builder.config.lldb {
+        Some(DebuggerPath::Path(path)) => (path.clone(), true),
+        Some(DebuggerPath::Discover) => (PathBuf::from("lldb"), false),
+        None => return None,
+    };
 
     let mut cmd = command(&lldb_exe);
     cmd.arg("--version");
 
-    // If a path to a LLDB binary was provided, it has to exist and return some version, to avoid
-    // silent failures.
-    let explicitly_set_lldb = builder.config.lldb.is_some();
     if !explicitly_set_lldb {
         cmd = cmd.allow_failure();
     }

src/bootstrap/src/core/sanity.rs

@@ -16,7 +16,7 @@ use std::{env, fs};
 
 use crate::builder::{Builder, Kind};
 use crate::core::build_steps::tool;
-use crate::core::config::{CompilerBuiltins, Target};
+use crate::core::config::{CompilerBuiltins, DebuggerPath, Target};
 use crate::utils::exec::command;
 use crate::{Build, Subcommand, t};
 
@@ -187,12 +187,10 @@ than building it.
         .map(|p| cmd_finder.must_have(p))
         .or_else(|| cmd_finder.maybe_have("yarn"));
 
-    build.config.gdb = build
-        .config
-        .gdb
-        .take()
-        .map(|p| cmd_finder.must_have(p))
-        .or_else(|| cmd_finder.maybe_have("gdb"));
+    build.config.gdb = build.config.gdb.take().map(|p| match p {
+        DebuggerPath::Discover => DebuggerPath::Discover,
+        DebuggerPath::Path(path) => DebuggerPath::Path(cmd_finder.must_have(path)),
+    });
 
     build.config.reuse = build
         .config

src/bootstrap/src/utils/change_tracker.rs

@@ -651,4 +651,9 @@ pub const CONFIG_CHANGE_HISTORY: &[ChangeInfo] = &[
         severity: ChangeSeverity::Info,
         summary: "A new `build.sde` configuration option has been added to support intrinsic-test.",
     },
+    ChangeInfo {
+        change_id: 159455,
+        severity: ChangeSeverity::Warning,
+        summary: "GDB and LLDB are no longer automatically discovered from the environment. If you want to use path discovery for them, you can opt in using `build.gdb = \"discover\"` or `build.lldb = \"discover\"`.",
+    },
 ];

src/ci/run.sh

@@ -140,6 +140,11 @@ else
   # No need to compress debuginfo for tests, and we do not want to depend on zlib
   RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set rust.compress-debuginfo=off"
 
+  # Enable GDB/LLDB automatic discovery on CI
+  # Ideally, we should later change this to provide the paths explicitly
+  RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set build.gdb=discover"
+  RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set build.lldb=discover"
+
   # We almost always want debug assertions enabled, but sometimes this takes too
   # long for too little benefit, so we just turn them off.
   if [ "$NO_DEBUG_ASSERTIONS" = "" ]; then