Auto merge of #68391 - tmiasko:compiletest-debuginfo, r=alexcrichton

compiletest: Simplify multi-debugger support

Previous implementation used a single mode type to store various pieces
of otherwise loosely related information:

* Whether debuginfo mode is in use or not.
* Which debuggers should run in general.
* Which debuggers are enabled for particular test case.

The new implementation introduces a separation between those aspects.
There is a single debuginfo mode parametrized by a debugger type.
The debugger detection is performed first and a separate configuration
is created for each detected debugger. The test cases are gathered
independently for each debugger which makes it trivial to implement
support for `ignore` / `only` conditions.

Functional changes:

* A single `debuginfo` entry point (rather than `debuginfo-cdb`, `debuginfo-gdb+lldb`, etc.).
* Debugger name is included in the test name.
* Test outputs are placed in per-debugger directory.
* Fixed spurious hash mismatch. Previously, the config mode would change
  from `DebugInfoGdbLldb` (when collecting tests) to `DebugInfoGdb` or
  `DebugInfoLldb` (when running them) which would affect hash computation.
* PYTHONPATH is additionally included in gdb hash.
* lldb-python and lldb-python-dir are additionally included in lldb hash.

Author: bors

src/bootstrap/test.rs

@@ -957,14 +957,6 @@ impl Step for Compiletest {
         }
 
         if suite == "debuginfo" {
-            let msvc = builder.config.build.contains("msvc");
-            if mode == "debuginfo" {
-                return builder.ensure(Compiletest {
-                    mode: if msvc { "debuginfo-cdb" } else { "debuginfo-gdb+lldb" },
-                    ..self
-                });
-            }
-
             builder
                 .ensure(dist::DebuggerScripts { sysroot: builder.sysroot(compiler), host: target });
         }

src/tools/compiletest/src/common.rs

@@ -14,10 +14,7 @@ pub enum Mode {
     RunFail,
     RunPassValgrind,
     Pretty,
-    DebugInfoCdb,
-    DebugInfoGdbLldb,
-    DebugInfoGdb,
-    DebugInfoLldb,
+    DebugInfo,
     Codegen,
     Rustdoc,
     CodegenUnits,
@@ -32,13 +29,9 @@ pub enum Mode {
 impl Mode {
     pub fn disambiguator(self) -> &'static str {
         // Pretty-printing tests could run concurrently, and if they do,
-        // they need to keep their output segregated. Same is true for debuginfo tests that
-        // can be run on cdb, gdb, and lldb.
+        // they need to keep their output segregated.
         match self {
             Pretty => ".pretty",
-            DebugInfoCdb => ".cdb",
-            DebugInfoGdb => ".gdb",
-            DebugInfoLldb => ".lldb",
             _ => "",
         }
     }
@@ -52,10 +45,7 @@ impl FromStr for Mode {
             "run-fail" => Ok(RunFail),
             "run-pass-valgrind" => Ok(RunPassValgrind),
             "pretty" => Ok(Pretty),
-            "debuginfo-cdb" => Ok(DebugInfoCdb),
-            "debuginfo-gdb+lldb" => Ok(DebugInfoGdbLldb),
-            "debuginfo-lldb" => Ok(DebugInfoLldb),
-            "debuginfo-gdb" => Ok(DebugInfoGdb),
+            "debuginfo" => Ok(DebugInfo),
             "codegen" => Ok(Codegen),
             "rustdoc" => Ok(Rustdoc),
             "codegen-units" => Ok(CodegenUnits),
@@ -77,10 +67,7 @@ impl fmt::Display for Mode {
             RunFail => "run-fail",
             RunPassValgrind => "run-pass-valgrind",
             Pretty => "pretty",
-            DebugInfoCdb => "debuginfo-cdb",
-            DebugInfoGdbLldb => "debuginfo-gdb+lldb",
-            DebugInfoGdb => "debuginfo-gdb",
-            DebugInfoLldb => "debuginfo-lldb",
+            DebugInfo => "debuginfo",
             Codegen => "codegen",
             Rustdoc => "rustdoc",
             CodegenUnits => "codegen-units",
@@ -155,6 +142,29 @@ impl CompareMode {
     }
 }
 
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub enum Debugger {
+    Cdb,
+    Gdb,
+    Lldb,
+}
+
+impl Debugger {
+    fn to_str(&self) -> &'static str {
+        match self {
+            Debugger::Cdb => "cdb",
+            Debugger::Gdb => "gdb",
+            Debugger::Lldb => "lldb",
+        }
+    }
+}
+
+impl fmt::Display for Debugger {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        fmt::Display::fmt(self.to_str(), f)
+    }
+}
+
 /// Configuration for compiletest
 #[derive(Clone)]
 pub struct Config {
@@ -208,6 +218,9 @@ pub struct Config {
     /// The test mode, compile-fail, run-fail, ui
     pub mode: Mode,
 
+    /// The debugger to use in debuginfo mode. Unset otherwise.
+    pub debugger: Option<Debugger>,
+
     /// Run ignored tests
     pub run_ignored: bool,
 
@@ -362,9 +375,11 @@ pub fn output_testname_unique(
     revision: Option<&str>,
 ) -> PathBuf {
     let mode = config.compare_mode.as_ref().map_or("", |m| m.to_str());
+    let debugger = config.debugger.as_ref().map_or("", |m| m.to_str());
     PathBuf::from(&testpaths.file.file_stem().unwrap())
         .with_extra_extension(revision.unwrap_or(""))
         .with_extra_extension(mode)
+        .with_extra_extension(debugger)
 }
 
 /// Absolute path to the directory where all output for the given

src/tools/compiletest/src/header.rs

@@ -6,69 +6,26 @@ use std::path::{Path, PathBuf};
 
 use log::*;
 
-use crate::common::{self, CompareMode, Config, FailMode, Mode, PassMode};
+use crate::common::{CompareMode, Config, Debugger, FailMode, Mode, PassMode};
 use crate::extract_gdb_version;
 use crate::util;
 
 #[cfg(test)]
 mod tests;
 
-/// Whether to ignore the test.
-#[derive(Clone, Copy, PartialEq, Debug)]
-pub enum Ignore {
-    /// Runs it.
-    Run,
-    /// Ignore it totally.
-    Ignore,
-    /// Ignore only the gdb test, but run the lldb test.
-    IgnoreGdb,
-    /// Ignore only the lldb test, but run the gdb test.
-    IgnoreLldb,
-}
-
-impl Ignore {
-    pub fn can_run_gdb(&self) -> bool {
-        *self == Ignore::Run || *self == Ignore::IgnoreLldb
-    }
-
-    pub fn can_run_lldb(&self) -> bool {
-        *self == Ignore::Run || *self == Ignore::IgnoreGdb
-    }
-
-    pub fn no_gdb(&self) -> Ignore {
-        match *self {
-            Ignore::Run => Ignore::IgnoreGdb,
-            Ignore::IgnoreGdb => Ignore::IgnoreGdb,
-            _ => Ignore::Ignore,
-        }
-    }
-
-    pub fn no_lldb(&self) -> Ignore {
-        match *self {
-            Ignore::Run => Ignore::IgnoreLldb,
-            Ignore::IgnoreLldb => Ignore::IgnoreLldb,
-            _ => Ignore::Ignore,
-        }
-    }
-}
-
 /// The result of parse_cfg_name_directive.
 #[derive(Clone, Copy, PartialEq, Debug)]
 enum ParsedNameDirective {
     /// No match.
     NoMatch,
     /// Match.
     Match,
-    /// Mode was DebugInfoGdbLldb and this matched gdb.
-    MatchGdb,
-    /// Mode was DebugInfoGdbLldb and this matched lldb.
-    MatchLldb,
 }
 
 /// Properties which must be known very early, before actually running
 /// the test.
 pub struct EarlyProps {
-    pub ignore: Ignore,
+    pub ignore: bool,
     pub should_fail: bool,
     pub aux: Vec<String>,
     pub aux_crate: Vec<(String, String)>,
@@ -78,84 +35,61 @@ pub struct EarlyProps {
 impl EarlyProps {
     pub fn from_file(config: &Config, testfile: &Path) -> Self {
         let mut props = EarlyProps {
-            ignore: Ignore::Run,
+            ignore: false,
             should_fail: false,
             aux: Vec::new(),
             aux_crate: Vec::new(),
             revisions: vec![],
         };
 
-        if config.mode == common::DebugInfoGdbLldb {
-            if config.lldb_python_dir.is_none() {
-                props.ignore = props.ignore.no_lldb();
-            }
-            if config.gdb_version.is_none() {
-                props.ignore = props.ignore.no_gdb();
-            }
-        } else if config.mode == common::DebugInfoCdb {
-            if config.cdb.is_none() {
-                props.ignore = Ignore::Ignore;
-            }
-        }
-
         let rustc_has_profiler_support = env::var_os("RUSTC_PROFILER_SUPPORT").is_some();
         let rustc_has_sanitizer_support = env::var_os("RUSTC_SANITIZER_SUPPORT").is_some();
 
         iter_header(testfile, None, &mut |ln| {
             // we should check if any only-<platform> exists and if it exists
             // and does not matches the current platform, skip the test
-            if props.ignore != Ignore::Ignore {
+            if !props.ignore {
                 props.ignore = match config.parse_cfg_name_directive(ln, "ignore") {
-                    ParsedNameDirective::Match => Ignore::Ignore,
+                    ParsedNameDirective::Match => true,
                     ParsedNameDirective::NoMatch => props.ignore,
-                    ParsedNameDirective::MatchGdb => props.ignore.no_gdb(),
-                    ParsedNameDirective::MatchLldb => props.ignore.no_lldb(),
                 };
 
                 if config.has_cfg_prefix(ln, "only") {
                     props.ignore = match config.parse_cfg_name_directive(ln, "only") {
                         ParsedNameDirective::Match => props.ignore,
-                        ParsedNameDirective::NoMatch => Ignore::Ignore,
-                        ParsedNameDirective::MatchLldb => props.ignore.no_gdb(),
-                        ParsedNameDirective::MatchGdb => props.ignore.no_lldb(),
+                        ParsedNameDirective::NoMatch => true,
                     };
                 }
 
                 if ignore_llvm(config, ln) {
-                    props.ignore = Ignore::Ignore;
+                    props.ignore = true;
                 }
 
                 if config.run_clang_based_tests_with.is_none()
                     && config.parse_needs_matching_clang(ln)
                 {
-                    props.ignore = Ignore::Ignore;
+                    props.ignore = true;
                 }
 
                 if !rustc_has_profiler_support && config.parse_needs_profiler_support(ln) {
-                    props.ignore = Ignore::Ignore;
+                    props.ignore = true;
                 }
 
                 if !rustc_has_sanitizer_support && config.parse_needs_sanitizer_support(ln) {
-                    props.ignore = Ignore::Ignore;
+                    props.ignore = true;
                 }
 
                 if config.target == "wasm32-unknown-unknown" && config.parse_check_run_results(ln) {
-                    props.ignore = Ignore::Ignore;
+                    props.ignore = true;
                 }
-            }
 
-            if (config.mode == common::DebugInfoGdb || config.mode == common::DebugInfoGdbLldb)
-                && props.ignore.can_run_gdb()
-                && ignore_gdb(config, ln)
-            {
-                props.ignore = props.ignore.no_gdb();
-            }
+                if config.debugger == Some(Debugger::Gdb) && ignore_gdb(config, ln) {
+                    props.ignore = true;
+                }
 
-            if (config.mode == common::DebugInfoLldb || config.mode == common::DebugInfoGdbLldb)
-                && props.ignore.can_run_lldb()
-                && ignore_lldb(config, ln)
-            {
-                props.ignore = props.ignore.no_lldb();
+                if config.debugger == Some(Debugger::Lldb) && ignore_lldb(config, ln) {
+                    props.ignore = true;
+                }
             }
 
             if let Some(s) = config.parse_aux_build(ln) {
@@ -881,70 +815,37 @@ impl Config {
     /// Parses a name-value directive which contains config-specific information, e.g., `ignore-x86`
     /// or `normalize-stderr-32bit`.
     fn parse_cfg_name_directive(&self, line: &str, prefix: &str) -> ParsedNameDirective {
-        if line.starts_with(prefix) && line.as_bytes().get(prefix.len()) == Some(&b'-') {
-            let name = line[prefix.len() + 1..].split(&[':', ' '][..]).next().unwrap();
-
-            if name == "test" ||
-                &self.target == name ||                             // triple
-                util::matches_os(&self.target, name) ||             // target
-                util::matches_env(&self.target, name) ||            // env
-                name == util::get_arch(&self.target) ||             // architecture
-                name == util::get_pointer_width(&self.target) ||    // pointer width
-                name == self.stage_id.split('-').next().unwrap() || // stage
-                (self.target != self.host && name == "cross-compile") ||
-                match self.compare_mode {
-                    Some(CompareMode::Nll) => name == "compare-mode-nll",
-                    Some(CompareMode::Polonius) => name == "compare-mode-polonius",
-                    None => false,
-                } ||
-                (cfg!(debug_assertions) && name == "debug")
-            {
-                ParsedNameDirective::Match
-            } else {
-                match self.mode {
-                    common::DebugInfoGdbLldb => {
-                        if name == "gdb" {
-                            ParsedNameDirective::MatchGdb
-                        } else if name == "lldb" {
-                            ParsedNameDirective::MatchLldb
-                        } else {
-                            ParsedNameDirective::NoMatch
-                        }
-                    }
-                    common::DebugInfoCdb => {
-                        if name == "cdb" {
-                            ParsedNameDirective::Match
-                        } else {
-                            ParsedNameDirective::NoMatch
-                        }
-                    }
-                    common::DebugInfoGdb => {
-                        if name == "gdb" {
-                            ParsedNameDirective::Match
-                        } else {
-                            ParsedNameDirective::NoMatch
-                        }
-                    }
-                    common::DebugInfoLldb => {
-                        if name == "lldb" {
-                            ParsedNameDirective::Match
-                        } else {
-                            ParsedNameDirective::NoMatch
-                        }
-                    }
-                    common::Pretty => {
-                        if name == "pretty" {
-                            ParsedNameDirective::Match
-                        } else {
-                            ParsedNameDirective::NoMatch
-                        }
-                    }
-                    _ => ParsedNameDirective::NoMatch,
-                }
-            }
-        } else {
-            ParsedNameDirective::NoMatch
+        if !line.as_bytes().starts_with(prefix.as_bytes()) {
+            return ParsedNameDirective::NoMatch;
+        }
+        if line.as_bytes().get(prefix.len()) != Some(&b'-') {
+            return ParsedNameDirective::NoMatch;
         }
+
+        let name = line[prefix.len() + 1..].split(&[':', ' '][..]).next().unwrap();
+
+        let is_match = name == "test" ||
+            &self.target == name ||                             // triple
+            util::matches_os(&self.target, name) ||             // target
+            util::matches_env(&self.target, name) ||            // env
+            name == util::get_arch(&self.target) ||             // architecture
+            name == util::get_pointer_width(&self.target) ||    // pointer width
+            name == self.stage_id.split('-').next().unwrap() || // stage
+            (self.target != self.host && name == "cross-compile") ||
+            match self.compare_mode {
+                Some(CompareMode::Nll) => name == "compare-mode-nll",
+                Some(CompareMode::Polonius) => name == "compare-mode-polonius",
+                None => false,
+            } ||
+            (cfg!(debug_assertions) && name == "debug") ||
+            match self.debugger {
+                Some(Debugger::Cdb) => name == "cdb",
+                Some(Debugger::Gdb) => name == "gdb",
+                Some(Debugger::Lldb) => name == "lldb",
+                None => false,
+            };
+
+        if is_match { ParsedNameDirective::Match } else { ParsedNameDirective::NoMatch }
     }
 
     fn has_cfg_prefix(&self, line: &str, prefix: &str) -> bool {