Explicitly materialize debuginfo tests for all debuggers

src/tools/compiletest/src/debuggers.rs

@@ -1,59 +1,7 @@
-use std::env;
 use std::process::Command;
-use std::sync::Arc;
 
 use camino::Utf8Path;
 
-use crate::common::{Config, Debugger};
-
-pub(crate) fn configure_cdb(config: &Config) -> Option<Arc<Config>> {
-    config.cdb.as_ref()?;
-
-    Some(Arc::new(Config { debugger: Some(Debugger::Cdb), ..config.clone() }))
-}
-
-pub(crate) fn configure_gdb(config: &Config) -> Option<Arc<Config>> {
-    config.gdb_version?;
-
-    if config.matches_env("msvc") {
-        return None;
-    }
-
-    if config.remote_test_client.is_some() && !config.target.contains("android") {
-        println!(
-            "WARNING: debuginfo tests are not available when \
-             testing with remote"
-        );
-        return None;
-    }
-
-    if config.target.contains("android") {
-        println!(
-            "{} debug-info test uses tcp 5039 port.\
-             please reserve it",
-            config.target
-        );
-
-        // android debug-info test uses remote debugger so, we test 1 thread
-        // at once as they're all sharing the same TCP port to communicate
-        // over.
-        //
-        // we should figure out how to lift this restriction! (run them all
-        // on different ports allocated dynamically).
-        //
-        // SAFETY: at this point we are still single-threaded.
-        unsafe { env::set_var("RUST_TEST_THREADS", "1") };
-    }
-
-    Some(Arc::new(Config { debugger: Some(Debugger::Gdb), ..config.clone() }))
-}
-
-pub(crate) fn configure_lldb(config: &Config) -> Option<Arc<Config>> {
-    config.lldb.as_ref()?;
-
-    Some(Arc::new(Config { debugger: Some(Debugger::Lldb), ..config.clone() }))
-}
-
 pub(crate) fn query_cdb_version(cdb: &Utf8Path) -> Option<[u16; 4]> {
     let mut version = None;
     if let Ok(output) = Command::new(cdb).arg("/version").output() {

src/tools/compiletest/src/directives.rs

@@ -1,3 +1,4 @@
+use std::borrow::Cow;
 use std::collections::HashSet;
 use std::process::Command;
 use std::{env, fs};
@@ -895,57 +896,85 @@ pub(crate) fn make_test_description(
     poisoned: &mut bool,
     aux_props: &mut AuxProps,
 ) -> CollectedTestDesc {
-    let mut ignore = false;
-    let mut ignore_message = None;
+    let mut ignore_message: Option<Cow<'static, str>> = None;
     let mut should_fail = false;
 
-    // Scan through the test file to handle `ignore-*`, `only-*`, and `needs-*` directives.
-    iter_directives(config, file_directives, &mut |ln @ &DirectiveLine { line_number, .. }| {
-        if !ln.applies_to_test_revision(test_revision) {
-            return;
+    // Perform a per-file (rather than per-line) ignore decision to skip running debuginfo tests
+    // if we don't have a debugger for them available.
+    // This is needed because we duplicate the Config once for each debugger.
+    if config.mode == TestMode::DebugInfo {
+        match &config.debugger {
+            Some(Debugger::Cdb) => {
+                if let Some(msg) = check_cdb_support(config) {
+                    ignore_message = Some(Cow::Owned(msg));
+                }
+            }
+            Some(Debugger::Gdb) => {
+                if let Some(msg) = check_gdb_support(config) {
+                    ignore_message = Some(Cow::Owned(msg));
+                }
+            }
+            Some(Debugger::Lldb) => {
+                if let Some(msg) = check_lldb_support(config) {
+                    ignore_message = Some(Cow::Owned(msg));
+                }
+            }
+            None => {}
         }
+    }
 
-        // Parse `aux-*` directives, for use by up-to-date checks.
-        parse_and_update_aux(config, ln, aux_props);
+    if ignore_message.is_none() {
+        // Scan through the test file to handle `ignore-*`, `only-*`, and `needs-*` directives.
+        iter_directives(
+            config,
+            file_directives,
+            &mut |ln @ &DirectiveLine { line_number, .. }| {
+                if !ln.applies_to_test_revision(test_revision) {
+                    return;
+                }
 
-        macro_rules! decision {
-            ($e:expr) => {
-                match $e {
-                    IgnoreDecision::Ignore { reason } => {
-                        ignore = true;
-                        ignore_message = Some(reason.into());
-                    }
-                    IgnoreDecision::Error { message } => {
-                        error!("{path}:{line_number}: {message}");
-                        *poisoned = true;
-                        return;
-                    }
-                    IgnoreDecision::Continue => {}
+                // Parse `aux-*` directives, for use by up-to-date checks.
+                parse_and_update_aux(config, ln, aux_props);
+
+                macro_rules! decision {
+                    ($e:expr) => {
+                        match $e {
+                            IgnoreDecision::Ignore { reason } => {
+                                ignore_message = Some(reason.into());
+                            }
+                            IgnoreDecision::Error { message } => {
+                                error!("{path}:{line_number}: {message}");
+                                *poisoned = true;
+                                return;
+                            }
+                            IgnoreDecision::Continue => {}
+                        }
+                    };
                 }
-            };
-        }
 
-        decision!(cfg::handle_ignore(&cache.cfg_conditions, ln));
-        decision!(cfg::handle_only(&cache.cfg_conditions, ln));
-        decision!(needs::handle_needs(&cache.needs, config, ln));
-        decision!(ignore_llvm(config, ln));
-        decision!(ignore_backends(config, ln));
-        decision!(needs_backends(config, ln));
-        decision!(ignore_cdb(config, ln));
-        decision!(ignore_gdb(config, ln));
-        decision!(ignore_lldb(config, ln));
-        decision!(ignore_parallel_frontend(config, ln));
-
-        if config.target == "wasm32-unknown-unknown"
-            && config.parse_name_directive(ln, directives::CHECK_RUN_RESULTS)
-        {
-            decision!(IgnoreDecision::Ignore {
-                reason: "ignored on WASM as the run results cannot be checked there".into(),
-            });
-        }
+                decision!(cfg::handle_ignore(&cache.cfg_conditions, ln));
+                decision!(cfg::handle_only(&cache.cfg_conditions, ln));
+                decision!(needs::handle_needs(&cache.needs, config, ln));
+                decision!(ignore_llvm(config, ln));
+                decision!(ignore_backends(config, ln));
+                decision!(needs_backends(config, ln));
+                decision!(ignore_cdb(config, ln));
+                decision!(ignore_gdb(config, ln));
+                decision!(ignore_lldb(config, ln));
+                decision!(ignore_parallel_frontend(config, ln));
+
+                if config.target == "wasm32-unknown-unknown"
+                    && config.parse_name_directive(ln, directives::CHECK_RUN_RESULTS)
+                {
+                    decision!(IgnoreDecision::Ignore {
+                        reason: "ignored on WASM as the run results cannot be checked there".into(),
+                    });
+                }
 
-        should_fail |= config.parse_name_directive(ln, "should-fail");
-    });
+                should_fail |= config.parse_name_directive(ln, "should-fail");
+            },
+        );
+    }
 
     // The `should-fail` annotation doesn't apply to pretty tests,
     // since we run the pretty printer across all tests by default.
@@ -959,12 +988,37 @@ pub(crate) fn make_test_description(
     CollectedTestDesc {
         name,
         filterable_path: filterable_path.to_owned(),
-        ignore,
         ignore_message,
         should_fail,
     }
 }
 
+/// Returns `None` if CDB is available, otherwise returns an ignore message.
+fn check_cdb_support(config: &Config) -> Option<String> {
+    if config.cdb.is_none() { Some("cdb is not available".to_string()) } else { None }
+}
+
+/// Returns `None` if GDB is available, otherwise returns an ignore message.
+fn check_gdb_support(config: &Config) -> Option<String> {
+    if config.gdb_version.is_none() {
+        return Some("gdb is not available".to_string());
+    }
+
+    if config.matches_env("msvc") {
+        return Some("gdb tests do not run on msvc".to_string());
+    }
+
+    if config.remote_test_client.is_some() && !config.target.contains("android") {
+        return Some("gdb tests are not available when testing with remote".to_string());
+    }
+    None
+}
+
+/// Returns `None` if LLDB is available, otherwise returns an ignore message.
+fn check_lldb_support(config: &Config) -> Option<String> {
+    if config.lldb.is_none() { Some("lldb is not available".to_string()) } else { None }
+}
+
 fn ignore_cdb(config: &Config, line: &DirectiveLine<'_>) -> IgnoreDecision {
     if config.debugger != Some(Debugger::Cdb) {
         return IgnoreDecision::Continue;

src/tools/compiletest/src/directives/tests.rs

@@ -284,7 +284,7 @@ fn check_ignore(config: &Config, contents: &str) -> bool {
     let tn = String::new();
     let p = Utf8Path::new("a.rs");
     let d = make_test_description(&config, tn, p, p, contents, None);
-    d.ignore
+    d.is_ignored()
 }
 
 #[test]

src/tools/compiletest/src/executor.rs

@@ -100,7 +100,7 @@ fn spawn_test_thread(
     test: &CollectedTest,
     completion_sender: mpsc::Sender<TestCompletion>,
 ) -> Option<thread::JoinHandle<()>> {
-    if test.desc.ignore && !test.config.run_ignored {
+    if test.desc.is_ignored() && !test.config.run_ignored {
         completion_sender
             .send(TestCompletion { id, outcome: TestOutcome::Ignored, stdout: None })
             .unwrap();
@@ -336,11 +336,16 @@ pub(crate) struct CollectedTest {
 pub(crate) struct CollectedTestDesc {
     pub(crate) name: String,
     pub(crate) filterable_path: Utf8PathBuf,
-    pub(crate) ignore: bool,
     pub(crate) ignore_message: Option<Cow<'static, str>>,
     pub(crate) should_fail: ShouldFail,
 }
 
+impl CollectedTestDesc {
+    pub(crate) fn is_ignored(&self) -> bool {
+        self.ignore_message.is_some()
+    }
+}
+
 /// Tests with `//@ should-fail` are tests of compiletest itself, and should
 /// be reported as successful if and only if they would have _failed_.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]

src/tools/compiletest/src/lib.rs

@@ -561,19 +561,28 @@ fn run_tests(config: Arc<Config>) {
     if let TestMode::DebugInfo = config.mode {
         // Debugging emscripten code doesn't make sense today
         if !config.target.contains("emscripten") {
-            match config.debugger {
-                Some(Debugger::Cdb) => configs.extend(debuggers::configure_cdb(&config)),
-                Some(Debugger::Gdb) => configs.extend(debuggers::configure_gdb(&config)),
-                Some(Debugger::Lldb) => configs.extend(debuggers::configure_lldb(&config)),
-                // FIXME: the *implicit* debugger discovery makes it really difficult to control
-                // which {`cdb`, `gdb`, `lldb`} are used. These should **not** be implicitly
-                // discovered by `compiletest`; these should be explicit `bootstrap` configuration
-                // options that are passed to `compiletest`!
-                None => {
-                    configs.extend(debuggers::configure_cdb(&config));
-                    configs.extend(debuggers::configure_gdb(&config));
-                    configs.extend(debuggers::configure_lldb(&config));
-                }
+            // FIXME: ideally, we would just have one config, and then have some mechanism of
+            // generating multiple variants of a test, one for each debugger (something like
+            // debuginfo revisions). But for now, we just create three configs.
+            configs.extend([
+                Arc::new(Config { debugger: Some(Debugger::Cdb), ..config.as_ref().clone() }),
+                Arc::new(Config { debugger: Some(Debugger::Gdb), ..config.as_ref().clone() }),
+                Arc::new(Config { debugger: Some(Debugger::Lldb), ..config.as_ref().clone() }),
+            ]);
+
+            // FIXME: this should ideally happen somewhere else..
+            if config.target.contains("android") {
+                println!("{} debug-info test uses tcp 5039 port. please reserve it", config.target);
+
+                // android debug-info test uses remote debugger so, we test 1 thread
+                // at once as they're all sharing the same TCP port to communicate
+                // over.
+                //
+                // we should figure out how to lift this restriction! (run them all
+                // on different ports allocated dynamically).
+                //
+                // SAFETY: at this point we are still single-threaded.
+                unsafe { env::set_var("RUST_TEST_THREADS", "1") };
             }
         }
     } else {
@@ -946,11 +955,10 @@ fn make_test(cx: &TestCollectorCx, collector: &mut TestCollector, testpaths: &Te
 
         // If a test's inputs haven't changed since the last time it ran,
         // mark it as ignored so that the executor will skip it.
-        if !desc.ignore
+        if !desc.is_ignored()
             && !cx.config.force_rerun
             && is_up_to_date(cx, testpaths, &aux_props, revision)
         {
-            desc.ignore = true;
             // Keep this in sync with the "up-to-date" message detected by bootstrap.
             // FIXME(Zalathar): Now that we are no longer tied to libtest, we could
             // find a less fragile way to communicate this status to bootstrap.