Auto merge of #117484 - Zalathar:tests, r=cjgillot

coverage: Unify `tests/coverage-map` and `tests/run-coverage` into `tests/coverage`

Ever since the introduction of the `coverage-map` suite, it's been awkward to have to manage two separate coverage test directories containing dozens of mostly-identical files.

However, those two suites were separate for good reasons. They have very different requirements (since only one of them requires actually running the test program), running only one suite is noticeably faster than running both, and having separate suites allows them to be blessed separately if desired. So while unifying them was an obvious idea, actually doing so was non-trivial.

---

Nevertheless, this PR finds a way to merge the two suites into one directory while retaining almost all of the developer-experience benefits of having two suites. This required non-trivial implementations of `Step`, but the end result works very smoothly.

---

The first 5 commits are a copy of #117340, which has been closed in favour of this PR.

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

@@ -1305,6 +1305,47 @@ macro_rules! test_definitions {
     };
 }
 
+/// Declares an alias for running the [`Coverage`] tests in only one mode.
+/// Adapted from [`test_definitions`].
+macro_rules! coverage_test_alias {
+    ($name:ident {
+        alias_and_mode: $alias_and_mode:expr,
+        default: $default:expr,
+        only_hosts: $only_hosts:expr $(,)?
+    }) => {
+        #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
+        pub struct $name {
+            pub compiler: Compiler,
+            pub target: TargetSelection,
+        }
+
+        impl $name {
+            const MODE: &'static str = $alias_and_mode;
+        }
+
+        impl Step for $name {
+            type Output = ();
+            const DEFAULT: bool = $default;
+            const ONLY_HOSTS: bool = $only_hosts;
+
+            fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
+                run.alias($alias_and_mode)
+            }
+
+            fn make_run(run: RunConfig<'_>) {
+                let compiler = run.builder.compiler(run.builder.top_stage, run.build_triple());
+
+                run.builder.ensure($name { compiler, target: run.target });
+            }
+
+            fn run(self, builder: &Builder<'_>) {
+                Coverage { compiler: self.compiler, target: self.target }
+                    .run_unified_suite(builder, Self::MODE)
+            }
+        }
+    };
+}
+
 default_test!(Ui { path: "tests/ui", mode: "ui", suite: "ui" });
 
 default_test!(RunPassValgrind {
@@ -1349,13 +1390,66 @@ host_test!(RunMakeFullDeps {
 
 default_test!(Assembly { path: "tests/assembly", mode: "assembly", suite: "assembly" });
 
-default_test!(CoverageMap {
-    path: "tests/coverage-map",
-    mode: "coverage-map",
-    suite: "coverage-map"
+/// Custom test step that is responsible for running the coverage tests
+/// in multiple different modes.
+///
+/// Each individual mode also has its own alias that will run the tests in
+/// just that mode.
+#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
+pub struct Coverage {
+    pub compiler: Compiler,
+    pub target: TargetSelection,
+}
+
+impl Coverage {
+    const PATH: &'static str = "tests/coverage";
+    const SUITE: &'static str = "coverage";
+
+    fn run_unified_suite(&self, builder: &Builder<'_>, mode: &'static str) {
+        builder.ensure(Compiletest {
+            compiler: self.compiler,
+            target: self.target,
+            mode,
+            suite: Self::SUITE,
+            path: Self::PATH,
+            compare_mode: None,
+        })
+    }
+}
+
+impl Step for Coverage {
+    type Output = ();
+    const DEFAULT: bool = false;
+    const ONLY_HOSTS: bool = false;
+
+    fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
+        run.suite_path(Self::PATH)
+    }
+
+    fn make_run(run: RunConfig<'_>) {
+        let compiler = run.builder.compiler(run.builder.top_stage, run.build_triple());
+
+        run.builder.ensure(Coverage { compiler, target: run.target });
+    }
+
+    fn run(self, builder: &Builder<'_>) {
+        self.run_unified_suite(builder, CoverageMap::MODE);
+        self.run_unified_suite(builder, RunCoverage::MODE);
+    }
+}
+
+// Aliases for running the coverage tests in only one mode.
+coverage_test_alias!(CoverageMap {
+    alias_and_mode: "coverage-map",
+    default: true,
+    only_hosts: false,
+});
+coverage_test_alias!(RunCoverage {
+    alias_and_mode: "run-coverage",
+    default: true,
+    only_hosts: true,
 });
 
-host_test!(RunCoverage { path: "tests/run-coverage", mode: "run-coverage", suite: "run-coverage" });
 host_test!(RunCoverageRustdoc {
     path: "tests/run-coverage-rustdoc",
     mode: "run-coverage",

src/bootstrap/src/core/builder.rs

@@ -727,6 +727,7 @@ impl<'a> Builder<'a> {
                 test::Tidy,
                 test::Ui,
                 test::RunPassValgrind,
+                test::Coverage,
                 test::CoverageMap,
                 test::RunCoverage,
                 test::MirOpt,

src/tools/compiletest/src/common.rs

@@ -78,7 +78,7 @@ impl Default for Mode {
 }
 
 impl Mode {
-    pub fn disambiguator(self) -> &'static str {
+    pub fn aux_dir_disambiguator(self) -> &'static str {
         // Pretty-printing tests could run concurrently, and if they do,
         // they need to keep their output segregated.
         match self {
@@ -86,6 +86,15 @@ impl Mode {
             _ => "",
         }
     }
+
+    pub fn output_dir_disambiguator(self) -> &'static str {
+        // Coverage tests use the same test files for multiple test modes,
+        // so each mode should have a separate output directory.
+        match self {
+            CoverageMap | RunCoverage => self.to_str(),
+            _ => "",
+        }
+    }
 }
 
 string_enum! {
@@ -699,6 +708,7 @@ pub fn output_testname_unique(
     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(config.mode.output_dir_disambiguator())
         .with_extra_extension(revision.unwrap_or(""))
         .with_extra_extension(mode)
         .with_extra_extension(debugger)

src/tools/compiletest/src/runtest.rs

@@ -2193,7 +2193,7 @@ impl<'test> TestCx<'test> {
             || self.is_vxworks_pure_static()
             || self.config.target.contains("bpf")
             || !self.config.target_cfg().dynamic_linking
-            || self.config.mode == RunCoverage
+            || matches!(self.config.mode, CoverageMap | RunCoverage)
         {
             // We primarily compile all auxiliary libraries as dynamic libraries
             // to avoid code size bloat and large binaries as much as possible
@@ -2481,9 +2481,9 @@ impl<'test> TestCx<'test> {
             RunCoverage => {
                 rustc.arg("-Cinstrument-coverage");
                 // Coverage reports are sometimes sensitive to optimizations,
-                // and the current snapshots assume no optimization unless
+                // and the current snapshots assume `opt-level=2` unless
                 // overridden by `compile-flags`.
-                rustc.arg("-Copt-level=0");
+                rustc.arg("-Copt-level=2");
             }
             RunPassValgrind | Pretty | DebugInfo | Codegen | Rustdoc | RustdocJson | RunMake
             | CodegenUnits | JsDocTest | Assembly => {
@@ -2720,7 +2720,7 @@ impl<'test> TestCx<'test> {
     fn aux_output_dir_name(&self) -> PathBuf {
         self.output_base_dir()
             .join("auxiliary")
-            .with_extra_extension(self.config.mode.disambiguator())
+            .with_extra_extension(self.config.mode.aux_dir_disambiguator())
     }
 
     /// Generates a unique name for the test, such as `testname.revision.mode`.

tests/coverage-map/README.md

@@ -1,13 +0,0 @@
-The tests in `./status-quo` were copied from `tests/run-coverage` in order to
-capture the current behavior of the instrumentor on non-trivial programs.
-The actual mappings have not been closely inspected.
-
-## Maintenance note
-
-These tests can be sensitive to small changes in MIR spans or MIR control flow,
-especially in HIR-to-MIR lowering or MIR optimizations.
-
-If you haven't touched the coverage code directly, and the `run-coverage` test
-suite still works, then it should usually be OK to just `--bless` these
-coverage mapping tests as necessary, without worrying too much about the exact
-changes.

tests/coverage-map/if.cov-map

@@ -1,15 +0,0 @@
-Function name: if::main
-Raw bytes (28): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 03, 01, 02, 0c, 05, 02, 0d, 02, 06, 02, 02, 06, 00, 07, 07, 01, 05, 01, 02]
-Number of files: 1
-- file 0 => global file 1
-Number of expressions: 2
-- expression 0 operands: lhs = Counter(0), rhs = Counter(1)
-- expression 1 operands: lhs = Counter(1), rhs = Expression(0, Sub)
-Number of file 0 mappings: 4
-- Code(Counter(0)) at (prev + 3, 1) to (start + 2, 12)
-- Code(Counter(1)) at (prev + 2, 13) to (start + 2, 6)
-- Code(Expression(0, Sub)) at (prev + 2, 6) to (start + 0, 7)
-    = (c0 - c1)
-- Code(Expression(1, Add)) at (prev + 1, 5) to (start + 1, 2)
-    = (c1 + (c0 - c1))
-

tests/coverage-map/if.rs

@@ -1,9 +0,0 @@
-// compile-flags: --edition=2021
-
-fn main() {
-    let cond = std::env::args().len() == 1;
-    if cond {
-        println!("true");
-    }
-    println!("done");
-}

tests/coverage-map/status-quo/overflow.rs

@@ -1,63 +0,0 @@
-#![allow(unused_assignments)]
-// failure-status: 101
-
-fn might_overflow(to_add: u32) -> u32 {
-    if to_add > 5 {
-        println!("this will probably overflow");
-    }
-    let add_to = u32::MAX - 5;
-    println!("does {} + {} overflow?", add_to, to_add);
-    let result = to_add + add_to;
-    println!("continuing after overflow check");
-    result
-}
-
-fn main() -> Result<(), u8> {
-    let mut countdown = 10;
-    while countdown > 0 {
-        if countdown == 1 {
-            let result = might_overflow(10);
-            println!("Result: {}", result);
-        } else if countdown < 5 {
-            let result = might_overflow(1);
-            println!("Result: {}", result);
-        }
-        countdown -= 1;
-    }
-    Ok(())
-}
-
-// Notes:
-//   1. Compare this program and its coverage results to those of the very similar test `assert.rs`,
-//      and similar tests `panic_unwind.rs`, abort.rs` and `try_error_result.rs`.
-//   2. This test confirms the coverage generated when a program passes or fails a
-//      compiler-generated `TerminatorKind::Assert` (based on an overflow check, in this case).
-//   3. Similar to how the coverage instrumentation handles `TerminatorKind::Call`,
-//      compiler-generated assertion failures are assumed to be a symptom of a program bug, not
-//      expected behavior. To simplify the coverage graphs and keep instrumented programs as
-//      small and fast as possible, `Assert` terminators are assumed to always succeed, and
-//      therefore are considered "non-branching" terminators. So, an `Assert` terminator does not
-//      get its own coverage counter.
-//   4. After an unhandled panic or failed Assert, coverage results may not always be intuitive.
-//      In this test, the final count for the statements after the `if` block in `might_overflow()`
-//      is 4, even though the lines after `to_add + add_to` were executed only 3 times. Depending
-//      on the MIR graph and the structure of the code, this count could have been 3 (which might
-//      have been valid for the overflowed add `+`, but should have been 4 for the lines before
-//      the overflow. The reason for this potential uncertainty is, a `CounterKind` is incremented
-//      via StatementKind::Counter at the end of the block, but (as in the case in this test),
-//      a CounterKind::Expression is always evaluated. In this case, the expression was based on
-//      a `Counter` incremented as part of the evaluation of the `if` expression, which was
-//      executed, and counted, 4 times, before reaching the overflow add.
-
-// If the program did not overflow, the coverage for `might_overflow()` would look like this:
-//
-//     4|       |fn might_overflow(to_add: u32) -> u32 {
-//     5|      4|    if to_add > 5 {
-//     6|      0|        println!("this will probably overflow");
-//     7|      4|    }
-//     8|      4|    let add_to = u32::MAX - 5;
-//     9|      4|    println!("does {} + {} overflow?", add_to, to_add);
-//    10|      4|    let result = to_add + add_to;
-//    11|      4|    println!("continuing after overflow check");
-//    12|      4|    result
-//    13|      4|}

tests/coverage-map/status-quo/sort_groups.rs

@@ -1,23 +0,0 @@
-// compile-flags: --edition=2021
-
-// Demonstrate that `sort_subviews.py` can sort instantiation groups into a
-// predictable order, while preserving their heterogeneous contents.
-
-fn main() {
-    let cond = std::env::args().len() > 1;
-    generic_fn::<()>(cond);
-    generic_fn::<&'static str>(!cond);
-    if false {
-        generic_fn::<char>(cond);
-    }
-    generic_fn::<i32>(cond);
-    other_fn();
-}
-
-fn generic_fn<T>(cond: bool) {
-    if cond {
-        println!("{}", std::any::type_name::<T>());
-    }
-}
-
-fn other_fn() {}

tests/coverage/README.md

@@ -0,0 +1,16 @@
+The tests in this directory are shared by two different test modes, and can be
+run in multiple different ways:
+
+- `./x.py test coverage-map` (compiles to LLVM IR and checks coverage mappings)
+- `./x.py test run-coverage` (runs a test binary and checks its coverage report)
+- `./x.py test coverage` (runs both `coverage-map` and `run-coverage`)
+
+## Maintenance note
+
+These tests can be sensitive to small changes in MIR spans or MIR control flow,
+especially in HIR-to-MIR lowering or MIR optimizations.
+
+If you haven't touched the coverage code directly, and the tests still pass in
+`run-coverage` mode, then it should usually be OK to just re-bless the mappings
+as necessary with `./x.py test coverage-map --bless`, without worrying too much
+about the exact changes.

tests/coverage/issue-85461.cov-map

@@ -0,0 +1,8 @@
+Function name: issue_85461::main
+Raw bytes (9): 0x[01, 01, 00, 01, 01, 08, 01, 03, 02]
+Number of files: 1
+- file 0 => global file 1
+Number of expressions: 0
+Number of file 0 mappings: 1
+- Code(Counter(0)) at (prev + 8, 1) to (start + 3, 2)
+

tests/coverage/long_and_wide.coverage

@@ -0,0 +1,151 @@
+   LL|       |// compile-flags: --edition=2021
+   LL|       |// ignore-tidy-linelength
+   LL|       |
+   LL|       |// This file deliberately contains line and column numbers larger than 127,
+   LL|       |// to verify that `coverage-dump`'s ULEB128 parser can handle them.
+   LL|       |
+   LL|      1|fn main() {
+   LL|      1|    wide_function();
+   LL|      1|    long_function();
+   LL|      1|    far_function();
+   LL|      1|}
+   LL|       |
+   LL|       |#[rustfmt::skip]
+   LL|      1|fn wide_function() { /*                                                                                                           */ (); }
+   LL|       |
+   LL|      1|fn long_function() {
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|    //
+   LL|      1|}
+   LL|       |
+   LL|      1|fn far_function() {}
+