Auto merge of #148099 - Zalathar:debuggers, r=<try>

Prepare to move debugger discovery from compiletest to bootstrap

try-job: arm-android
try-job: aarch64-apple
try-job: x86_64-msvc-1
try-job: test-various

Author: rust-bors[bot]

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);