Rollup merge of #148099 - Zalathar:debuggers, r=jieyouxu

Prepare to move debugger discovery from compiletest to bootstrap

For a while I've been wanting to move debugger discovery out of compiletest and into bootstrap, so that bootstrap would be responsible for deciding which debugger to use, and compiletest would faithfully use that debugger with no extra magic (and no horrible config-duplicating hacks).

Making that change is complicated, and eventually I had so many intertwined preparatory changes that I split them off into this PR, so that it can be reviewed and tested as a smaller chunk.

---

To avoid scope creep, the changes in this PR try to move code as-is as much as possible, even if the moved code could arguably benefit from further cleanups. And in many cases, that code will need to be further overhauled anyway when discovery steps are actually moved out of compiletest.

Author: GuillaumeGomez

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

@@ -29,6 +29,7 @@ use crate::core::builder::{
 };
 use crate::core::config::TargetSelection;
 use crate::core::config::flags::{Subcommand, get_completion, top_level_help};
+use crate::core::debuggers;
 use crate::utils::build_stamp::{self, BuildStamp};
 use crate::utils::exec::{BootstrapCommand, command};
 use crate::utils::helpers::{
@@ -38,8 +39,6 @@ use crate::utils::helpers::{
 use crate::utils::render_tests::{add_flags_and_try_run_tests, try_run_tests};
 use crate::{CLang, CodegenBackendKind, DocTests, GitRepo, Mode, PathSet, envify};
 
-const ADB_TEST_DIR: &str = "/data/local/tmp/work";
-
 /// Runs `cargo test` on various internal tools used by bootstrap.
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct CrateBootstrap {
@@ -2078,27 +2077,28 @@ Please disable assertions with `rust.debug-assertions = false`.
 
         cmd.arg("--python").arg(builder.python());
 
-        if let Some(ref gdb) = builder.config.gdb {
-            cmd.arg("--gdb").arg(gdb);
-        }
-
-        let lldb_exe = builder.config.lldb.clone().unwrap_or_else(|| PathBuf::from("lldb"));
-        let lldb_version = command(&lldb_exe)
-            .allow_failure()
-            .arg("--version")
-            .run_capture(builder)
-            .stdout_if_ok()
-            .and_then(|v| if v.trim().is_empty() { None } else { Some(v) });
-        if let Some(ref vers) = lldb_version {
-            cmd.arg("--lldb-version").arg(vers);
-            let lldb_python_dir = command(&lldb_exe)
-                .allow_failure()
-                .arg("-P")
-                .run_capture_stdout(builder)
-                .stdout_if_ok()
-                .map(|p| p.lines().next().expect("lldb Python dir not found").to_string());
-            if let Some(ref dir) = lldb_python_dir {
-                cmd.arg("--lldb-python-dir").arg(dir);
+        // FIXME(#148099): Currently we set these Android-related flags in all
+        // modes, even though they should only be needed in "debuginfo" mode,
+        // because the GDB-discovery code in compiletest currently assumes that
+        // `--android-cross-path` is always set for Android targets.
+        if let Some(debuggers::Android { adb_path, adb_test_dir, android_cross_path }) =
+            debuggers::discover_android(builder, target)
+        {
+            cmd.arg("--adb-path").arg(adb_path);
+            cmd.arg("--adb-test-dir").arg(adb_test_dir);
+            cmd.arg("--android-cross-path").arg(android_cross_path);
+        }
+
+        if mode == "debuginfo" {
+            if let Some(debuggers::Gdb { gdb }) = debuggers::discover_gdb(builder) {
+                cmd.arg("--gdb").arg(gdb);
+            }
+
+            if let Some(debuggers::Lldb { lldb_version, lldb_python_dir }) =
+                debuggers::discover_lldb(builder)
+            {
+                cmd.arg("--lldb-version").arg(lldb_version);
+                cmd.arg("--lldb-python-dir").arg(lldb_python_dir);
             }
         }
 
@@ -2332,16 +2332,6 @@ Please disable assertions with `rust.debug-assertions = false`.
 
         cmd.env("RUST_TEST_TMPDIR", builder.tempdir());
 
-        cmd.arg("--adb-path").arg("adb");
-        cmd.arg("--adb-test-dir").arg(ADB_TEST_DIR);
-        if target.contains("android") && !builder.config.dry_run() {
-            // Assume that cc for this target comes from the android sysroot
-            cmd.arg("--android-cross-path")
-                .arg(builder.cc(target).parent().unwrap().parent().unwrap());
-        } else {
-            cmd.arg("--android-cross-path").arg("");
-        }
-
         if builder.config.cmd.rustfix_coverage() {
             cmd.arg("--rustfix-coverage");
         }

src/bootstrap/src/core/debuggers/android.rs

@@ -0,0 +1,24 @@
+use std::path::PathBuf;
+
+use crate::core::builder::Builder;
+use crate::core::config::TargetSelection;
+
+pub(crate) struct Android {
+    pub(crate) adb_path: &'static str,
+    pub(crate) adb_test_dir: &'static str,
+    pub(crate) android_cross_path: PathBuf,
+}
+
+pub(crate) fn discover_android(builder: &Builder<'_>, target: TargetSelection) -> Option<Android> {
+    let adb_path = "adb";
+    // See <https://github.com/rust-lang/rust/pull/102755>.
+    let adb_test_dir = "/data/local/tmp/work";
+
+    let android_cross_path = if target.contains("android") && !builder.config.dry_run() {
+        builder.cc(target).parent().unwrap().parent().unwrap().to_owned()
+    } else {
+        PathBuf::new()
+    };
+
+    Some(Android { adb_path, adb_test_dir, android_cross_path })
+}

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

@@ -0,0 +1,13 @@
+use std::path::Path;
+
+use crate::core::builder::Builder;
+
+pub(crate) struct Gdb<'a> {
+    pub(crate) gdb: &'a Path,
+}
+
+pub(crate) fn discover_gdb<'a>(builder: &'a Builder<'_>) -> Option<Gdb<'a>> {
+    let gdb = builder.config.gdb.as_deref()?;
+
+    Some(Gdb { gdb })
+}

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

@@ -0,0 +1,32 @@
+use std::path::PathBuf;
+
+use crate::core::builder::Builder;
+use crate::utils::exec::command;
+
+pub(crate) struct Lldb {
+    pub(crate) lldb_version: String,
+    pub(crate) lldb_python_dir: String,
+}
+
+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"));
+
+    let lldb_version = command(&lldb_exe)
+        .allow_failure()
+        .arg("--version")
+        .run_capture(builder)
+        .stdout_if_ok()
+        .and_then(|v| if v.trim().is_empty() { None } else { Some(v) })?;
+
+    let lldb_python_dir = command(&lldb_exe)
+        .allow_failure()
+        .arg("-P")
+        .run_capture_stdout(builder)
+        .stdout_if_ok()
+        .map(|p| p.lines().next().expect("lldb Python dir not found").to_string())?;
+
+    Some(Lldb { lldb_version, lldb_python_dir })
+}

src/bootstrap/src/core/debuggers/mod.rs

@@ -0,0 +1,10 @@
+//! Code for discovering debuggers and debugger-related configuration, so that
+//! it can be passed to compiletest when running debuginfo tests.
+
+pub(crate) use self::android::{Android, discover_android};
+pub(crate) use self::gdb::{Gdb, discover_gdb};
+pub(crate) use self::lldb::{Lldb, discover_lldb};
+
+mod android;
+mod gdb;
+mod lldb;

src/bootstrap/src/core/mod.rs

@@ -1,6 +1,7 @@
 pub(crate) mod build_steps;
 pub(crate) mod builder;
 pub(crate) mod config;
+pub(crate) mod debuggers;
 pub(crate) mod download;
 pub(crate) mod metadata;
 pub(crate) mod sanity;

src/tools/compiletest/src/debuggers.rs

@@ -103,22 +103,19 @@ fn find_cdb(target: &str) -> Option<Utf8PathBuf> {
 }
 
 /// Returns Path to CDB
-pub(crate) fn analyze_cdb(
-    cdb: Option<String>,
-    target: &str,
-) -> (Option<Utf8PathBuf>, Option<[u16; 4]>) {
+pub(crate) fn discover_cdb(cdb: Option<String>, target: &str) -> Option<Utf8PathBuf> {
     let cdb = cdb.map(Utf8PathBuf::from).or_else(|| find_cdb(target));
+    cdb
+}
 
+pub(crate) fn query_cdb_version(cdb: &Utf8Path) -> Option<[u16; 4]> {
     let mut version = None;
-    if let Some(cdb) = cdb.as_ref() {
-        if let Ok(output) = Command::new(cdb).arg("/version").output() {
-            if let Some(first_line) = String::from_utf8_lossy(&output.stdout).lines().next() {
-                version = extract_cdb_version(&first_line);
-            }
+    if let Ok(output) = Command::new(cdb).arg("/version").output() {
+        if let Some(first_line) = String::from_utf8_lossy(&output.stdout).lines().next() {
+            version = extract_cdb_version(&first_line);
         }
     }
-
-    (cdb, version)
+    version
 }
 
 pub(crate) fn extract_cdb_version(full_version_line: &str) -> Option<[u16; 4]> {
@@ -132,12 +129,11 @@ pub(crate) fn extract_cdb_version(full_version_line: &str) -> Option<[u16; 4]> {
     Some([major, minor, patch, build])
 }
 
-/// Returns (Path to GDB, GDB Version)
-pub(crate) fn analyze_gdb(
+pub(crate) fn discover_gdb(
     gdb: Option<String>,
     target: &str,
     android_cross_path: &Utf8Path,
-) -> (Option<String>, Option<u32>) {
+) -> Option<String> {
     #[cfg(not(windows))]
     const GDB_FALLBACK: &str = "gdb";
     #[cfg(windows)]
@@ -159,6 +155,10 @@ pub(crate) fn analyze_gdb(
         Some(ref s) => s.to_owned(),
     };
 
+    Some(gdb)
+}
+
+pub(crate) fn query_gdb_version(gdb: &str) -> Option<u32> {
     let mut version_line = None;
     if let Ok(output) = Command::new(&gdb).arg("--version").output() {
         if let Some(first_line) = String::from_utf8_lossy(&output.stdout).lines().next() {
@@ -168,10 +168,10 @@ pub(crate) fn analyze_gdb(
 
     let version = match version_line {
         Some(line) => extract_gdb_version(&line),
-        None => return (None, None),
+        None => return None,
     };
 
-    (Some(gdb), version)
+    version
 }
 
 pub(crate) fn extract_gdb_version(full_version_line: &str) -> Option<u32> {

src/tools/compiletest/src/lib.rs

@@ -258,10 +258,11 @@ fn parse_config(args: Vec<String>) -> Config {
     let target = opt_str2(matches.opt_str("target"));
     let android_cross_path = opt_path(matches, "android-cross-path");
     // FIXME: `cdb_version` is *derived* from cdb, but it's *not* technically a config!
-    let (cdb, cdb_version) = debuggers::analyze_cdb(matches.opt_str("cdb"), &target);
+    let cdb = debuggers::discover_cdb(matches.opt_str("cdb"), &target);
+    let cdb_version = cdb.as_deref().and_then(debuggers::query_cdb_version);
     // FIXME: `gdb_version` is *derived* from gdb, but it's *not* technically a config!
-    let (gdb, gdb_version) =
-        debuggers::analyze_gdb(matches.opt_str("gdb"), &target, &android_cross_path);
+    let gdb = debuggers::discover_gdb(matches.opt_str("gdb"), &target, &android_cross_path);
+    let gdb_version = gdb.as_deref().and_then(debuggers::query_gdb_version);
     // FIXME: `lldb_version` is *derived* from lldb, but it's *not* technically a config!
     let lldb_version =
         matches.opt_str("lldb-version").as_deref().and_then(debuggers::extract_lldb_version);