Rollup merge of #84875 - richkadel:no-coverage-dont-check-unused, r=tmandry

Removes unneeded check of `#[no_coverage]` in mapgen

There is an anticipated feature request to support a compiler flag that
only adds coverage for specific files (or perhaps mods). As I thought
about where that change would need to be supported, I realized that
checking the attribute in mapgen (for unused functions) was unnecessary.
The unused functions are only synthesized if they have MIR coverage, and
functions with the `no_coverage` attribute will not have been
instrumented with MIR coverage statements in the first place.

New tests confirm this.

Also, while adding tests, I updated resolved comments and FIXMEs in
other tests, and expanded comments and tests on one remaining issue that
is still not resolved.

r? `@tmandry`
cc: `@wesleywiser`

Author: JohnTitor

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

@@ -8,7 +8,6 @@ use rustc_codegen_ssa::traits::{ConstMethods, CoverageInfoMethods};
 use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet};
 use rustc_hir::def_id::{DefId, DefIdSet, LOCAL_CRATE};
 use rustc_llvm::RustString;
-use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
 use rustc_middle::mir::coverage::CodeRegion;
 use rustc_span::Symbol;
 
@@ -281,11 +280,8 @@ fn add_unused_functions<'ll, 'tcx>(cx: &CodegenCx<'ll, 'tcx>) {
 
     let mut unused_def_ids_by_file: FxHashMap<Symbol, Vec<DefId>> = FxHashMap::default();
     for &non_codegenned_def_id in all_def_ids.difference(codegenned_def_ids) {
-        let codegen_fn_attrs = tcx.codegen_fn_attrs(non_codegenned_def_id);
-        if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NO_COVERAGE) {
-            continue;
-        }
-        // Make sure the non-codegenned (unused) function has a file_name
+        // Make sure the non-codegenned (unused) function has at least one MIR
+        // `Coverage` statement with a code region, and return its file name.
         if let Some(non_codegenned_file_name) = tcx.covered_file_name(non_codegenned_def_id) {
             let def_ids =
                 unused_def_ids_by_file.entry(*non_codegenned_file_name).or_insert_with(Vec::new);

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.async2.txt

@@ -14,9 +14,9 @@
    14|      1|    }
    15|      1|}
    16|       |
-   17|       |// FIXME(#83985): The auto-generated closure in an async function is failing to include
-   18|       |// the println!() and `let` assignment lines in the coverage code region(s), as it does in the
-   19|       |// non-async function above, unless the `println!()` is inside a covered block.
+   17|       |
+   18|       |
+   19|       |
    20|      1|async fn async_func() {
    21|      1|    println!("async_func was covered");
    22|      1|    let b = true;
@@ -26,9 +26,9 @@
                    ^0
    26|      1|}
    27|       |
-   28|       |// FIXME(#83985): As above, this async function only has the `println!()` macro call, which is not
-   29|       |// showing coverage, so the entire async closure _appears_ uncovered; but this is not exactly true.
-   30|       |// It's only certain kinds of lines and/or their context that results in missing coverage.
+   28|       |
+   29|       |
+   30|       |
    31|      1|async fn async_func_just_println() {
    32|      1|    println!("async_func_just_println was covered");
    33|      1|}

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.no_cov_crate.txt

@@ -11,8 +11,27 @@
    11|       |    println!("called but not covered");
    12|       |}
    13|       |
-   14|      1|fn main() {
-   15|      1|    do_not_add_coverage_1();
-   16|      1|    do_not_add_coverage_2();
-   17|      1|}
+   14|       |#[no_coverage]
+   15|       |fn do_not_add_coverage_not_called() {
+   16|       |    println!("not called and not covered");
+   17|       |}
+   18|       |
+   19|      1|fn add_coverage_1() {
+   20|      1|    println!("called and covered");
+   21|      1|}
+   22|       |
+   23|      1|fn add_coverage_2() {
+   24|      1|    println!("called and covered");
+   25|      1|}
+   26|       |
+   27|      0|fn add_coverage_not_called() {
+   28|      0|    println!("not called but covered");
+   29|      0|}
+   30|       |
+   31|      1|fn main() {
+   32|      1|    do_not_add_coverage_1();
+   33|      1|    do_not_add_coverage_2();
+   34|      1|    add_coverage_1();
+   35|      1|    add_coverage_2();
+   36|      1|}
 

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.panic_unwind.txt

@@ -29,22 +29,4 @@
    29|       |//   2. Since the `panic_unwind.rs` test is allowed to unwind, it is also allowed to execute the
    30|       |//      normal program exit cleanup, including writing out the current values of the coverage
    31|       |//      counters.
-   32|       |//   3. The coverage results show (interestingly) that the `panic!()` call did execute, but it does
-   33|       |//      not show coverage of the `if countdown == 1` branch in `main()` that calls
-   34|       |//      `might_panic(true)` (causing the call to `panic!()`).
-   35|       |//   4. The reason `main()`s `if countdown == 1` branch, calling `might_panic(true)`, appears
-   36|       |//      "uncovered" is, InstrumentCoverage (intentionally) treats `TerminatorKind::Call` terminators
-   37|       |//      as non-branching, because when a program executes normally, they always are. Errors handled
-   38|       |//      via the try `?` operator produce error handling branches that *are* treated as branches in
-   39|       |//      coverage results. By treating calls without try `?` operators as non-branching (assumed to
-   40|       |//      return normally and continue) the coverage graph can be simplified, producing smaller,
-   41|       |//      faster binaries, and cleaner coverage results.
-   42|       |//   5. The reason the coverage results actually show `panic!()` was called is most likely because
-   43|       |//      `panic!()` is a macro, not a simple function call, and there are other `Statement`s and/or
-   44|       |//      `Terminator`s that execute with a coverage counter before the panic and unwind occur.
-   45|       |//   6. Since the common practice is not to use `panic!()` for error handling, the coverage
-   46|       |//      implementation avoids incurring an additional cost (in program size and execution time) to
-   47|       |//      improve coverage results for an event that is generally not "supposed" to happen.
-   48|       |//   7. FIXME(#78544): This issue describes a feature request for a proposed option to enable
-   49|       |//      more accurate coverage results for tests that intentionally panic.
 

src/test/run-make-fulldeps/coverage/async2.rs

@@ -14,9 +14,9 @@ fn non_async_func() {
     }
 }
 
-// FIXME(#83985): The auto-generated closure in an async function is failing to include
-// the println!() and `let` assignment lines in the coverage code region(s), as it does in the
-// non-async function above, unless the `println!()` is inside a covered block.
+
+
+
 async fn async_func() {
     println!("async_func was covered");
     let b = true;
@@ -25,9 +25,9 @@ async fn async_func() {
     }
 }
 
-// FIXME(#83985): As above, this async function only has the `println!()` macro call, which is not
-// showing coverage, so the entire async closure _appears_ uncovered; but this is not exactly true.
-// It's only certain kinds of lines and/or their context that results in missing coverage.
+
+
+
 async fn async_func_just_println() {
     println!("async_func_just_println was covered");
 }

src/test/run-make-fulldeps/coverage/no_cov_crate.rs

@@ -11,7 +11,26 @@ fn do_not_add_coverage_2() {
     println!("called but not covered");
 }
 
+#[no_coverage]
+fn do_not_add_coverage_not_called() {
+    println!("not called and not covered");
+}
+
+fn add_coverage_1() {
+    println!("called and covered");
+}
+
+fn add_coverage_2() {
+    println!("called and covered");
+}
+
+fn add_coverage_not_called() {
+    println!("not called but covered");
+}
+
 fn main() {
     do_not_add_coverage_1();
     do_not_add_coverage_2();
+    add_coverage_1();
+    add_coverage_2();
 }

src/test/run-make-fulldeps/coverage/panic_unwind.rs

@@ -29,21 +29,3 @@ fn main() -> Result<(), u8> {
 //   2. Since the `panic_unwind.rs` test is allowed to unwind, it is also allowed to execute the
 //      normal program exit cleanup, including writing out the current values of the coverage
 //      counters.
-//   3. The coverage results show (interestingly) that the `panic!()` call did execute, but it does
-//      not show coverage of the `if countdown == 1` branch in `main()` that calls
-//      `might_panic(true)` (causing the call to `panic!()`).
-//   4. The reason `main()`s `if countdown == 1` branch, calling `might_panic(true)`, appears
-//      "uncovered" is, InstrumentCoverage (intentionally) treats `TerminatorKind::Call` terminators
-//      as non-branching, because when a program executes normally, they always are. Errors handled
-//      via the try `?` operator produce error handling branches that *are* treated as branches in
-//      coverage results. By treating calls without try `?` operators as non-branching (assumed to
-//      return normally and continue) the coverage graph can be simplified, producing smaller,
-//      faster binaries, and cleaner coverage results.
-//   5. The reason the coverage results actually show `panic!()` was called is most likely because
-//      `panic!()` is a macro, not a simple function call, and there are other `Statement`s and/or
-//      `Terminator`s that execute with a coverage counter before the panic and unwind occur.
-//   6. Since the common practice is not to use `panic!()` for error handling, the coverage
-//      implementation avoids incurring an additional cost (in program size and execution time) to
-//      improve coverage results for an event that is generally not "supposed" to happen.
-//   7. FIXME(#78544): This issue describes a feature request for a proposed option to enable
-//      more accurate coverage results for tests that intentionally panic.