Addressed feedback from 2020-12-01

Added one more test (two files) showing coverage of generics and unused
functions across crates.

Created and referenced new Issues, as requested.

Added comments.

Added a note about the possible effects of compiler options on LLVM
coverage maps.

Author: richkadel

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

@@ -262,6 +262,10 @@ fn add_unreachable_coverage<'tcx>(
     tcx: TyCtxt<'tcx>,
     function_coverage_map: &mut FxHashMap<Instance<'tcx>, FunctionCoverage<'tcx>>,
 ) {
+    // FIXME(#79622): Can this solution be simplified and/or improved? Are there other sources
+    // of compiler state data that might help (or better sources that could be exposed, but
+    // aren't yet)?
+
     // Note: If the crate *only* defines generic functions, there are no codegenerated non-generic
     // functions to add any unreachable code to. In this case, the unreachable code regions will
     // have no coverage, instead of having coverage with zero executions.
@@ -359,6 +363,21 @@ fn add_unreachable_coverage<'tcx>(
             for def_id in
                 unreachable_def_ids_by_file.remove(&covered_file_name).into_iter().flatten()
             {
+                // Note, this loop adds an unreachable code regions for each MIR-derived region.
+                // Alternatively, we could add a single code region for the maximum span of all
+                // code regions here.
+                //
+                // Observed downsides of this approach are:
+                //
+                // 1. The coverage results will appear inconsistent compared with the same (or
+                //    similar) code in a function that is reached.
+                // 2. If the function is unreachable from one crate but reachable when compiling
+                //    another referencing crate (such as a cross-crate reference to a
+                //    generic function or inlined function), actual coverage regions overlaid
+                //    on a single larger code span of `Zero` coverage can appear confusing or
+                //    wrong. Chaning the unreachable coverage from a `code_region` to a
+                //    `gap_region` can help, but still can look odd with `0` line counts for
+                //    lines between executed (> 0) lines (such as for blank lines or comments).
                 for &region in tcx.covered_code_regions(def_id) {
                     function_coverage.add_unreachable_region(region.clone());
                 }

compiler/rustc_mir/src/transform/coverage/graph.rs

@@ -140,7 +140,9 @@ impl CoverageGraph {
 
                 // The following `TerminatorKind`s are either not expected outside an unwind branch,
                 // or they should not (under normal circumstances) branch. Coverage graphs are
-                // simplified by assuring coverage results are accurate for well-behaved programs.
+                // simplified by assuring coverage results are accurate for program executions that
+                // don't panic.
+                //
                 // Programs that panic and unwind may record slightly inaccurate coverage results
                 // for a coverage region containing the `Terminator` that began the panic. This
                 // is as intended. (See Issue #78544 for a possible future option to support

compiler/rustc_mir/src/transform/coverage/mod.rs

@@ -499,6 +499,8 @@ fn fn_sig_and_body<'tcx>(
     tcx: TyCtxt<'tcx>,
     def_id: DefId,
 ) -> (Option<&'tcx rustc_hir::FnSig<'tcx>>, &'tcx rustc_hir::Body<'tcx>) {
+    // FIXME(#79625): Consider improving MIR to provide the information needed, to avoid going back
+    // to HIR for it.
     let hir_node = tcx.hir().get_if_local(def_id).expect("expected DefId is local");
     let fn_body_id = hir::map::associated_body(hir_node).expect("HIR node is a function with body");
     (hir::map::fn_sig(hir_node), tcx.hir().body(fn_body_id))

compiler/rustc_mir/src/transform/coverage/spans.rs

@@ -359,12 +359,12 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> {
         }
 
         // Async functions wrap a closure that implements the body to be executed. The enclosing
-        // function is initially called, posts the closure to the executor, and returns. To avoid
-        // showing the return from the enclosing function as a "covered" return from the closure,
-        // the enclosing function's `TerminatorKind::Return`s `CoverageSpan` is excluded. The
-        // closure's `Return` is the only one that will be counted. This provides adequate
-        // coverage, and more intuitive counts. (Avoids double-counting the closing brace of the
-        // function body.)
+        // function is called and returns an `impl Future` without initially executing any of the
+        // body. To avoid showing the return from the enclosing function as a "covered" return from
+        // the closure, the enclosing function's `TerminatorKind::Return`s `CoverageSpan` is
+        // excluded. The closure's `Return` is the only one that will be counted. This provides
+        // adequate coverage, and more intuitive counts. (Avoids double-counting the closing brace
+        // of the function body.)
         let body_ends_with_closure = if let Some(last_covspan) = refined_spans.last() {
             last_covspan.is_closure && last_covspan.span.hi() == self.body_span.hi()
         } else {

src/doc/unstable-book/src/compiler-flags/source-based-code-coverage.md

@@ -76,6 +76,8 @@ $ RUSTC=$HOME/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc \
     cargo build --example formatjson5
 ```
 
+Note that some compiler options, combined with `-Zinstrument-coverage`, can produce LLVM IR and/or linked binaries that are incompatible with LLVM coverage maps. For example, coverage requires references to actual functions in LLVM IR. If any covered function is optimized out, the coverage tools may not be able to process the coverage results. If you need to pass additional options, with coverage enabled, test them early, to confirm you will get the coverage results you expect.
+
 ## Running the instrumented binary to generate raw coverage profiling data
 
 In the previous example, `cargo` generated the coverage-instrumented binary `formatjson5`:

src/test/run-make-fulldeps/coverage-reports/expected_export_coverage.async.json

@@ -6,50 +6,50 @@
           "filename": "../coverage/async.rs",
           "summary": {
             "functions": {
-              "count": 16,
-              "covered": 15,
-              "percent": 93.75
+              "count": 17,
+              "covered": 16,
+              "percent": 94.11764705882352
             },
             "instantiations": {
-              "count": 16,
-              "covered": 15,
-              "percent": 93.75
+              "count": 17,
+              "covered": 16,
+              "percent": 94.11764705882352
             },
             "lines": {
-              "count": 102,
-              "covered": 75,
-              "percent": 73.52941176470588
+              "count": 105,
+              "covered": 77,
+              "percent": 73.33333333333333
             },
             "regions": {
-              "count": 76,
-              "covered": 35,
-              "notcovered": 41,
-              "percent": 46.05263157894737
+              "count": 78,
+              "covered": 36,
+              "notcovered": 42,
+              "percent": 46.15384615384615
             }
           }
         }
       ],
       "totals": {
         "functions": {
-          "count": 16,
-          "covered": 15,
-          "percent": 93.75
+          "count": 17,
+          "covered": 16,
+          "percent": 94.11764705882352
         },
         "instantiations": {
-          "count": 16,
-          "covered": 15,
-          "percent": 93.75
+          "count": 17,
+          "covered": 16,
+          "percent": 94.11764705882352
         },
         "lines": {
-          "count": 102,
-          "covered": 75,
-          "percent": 73.52941176470588
+          "count": 105,
+          "covered": 77,
+          "percent": 73.33333333333333
         },
         "regions": {
-          "count": 76,
-          "covered": 35,
-          "notcovered": 41,
-          "percent": 46.05263157894737
+          "count": 78,
+          "covered": 36,
+          "notcovered": 42,
+          "percent": 46.15384615384615
         }
       }
     }

src/test/run-make-fulldeps/coverage-reports/expected_export_coverage.uses_crate.json

@@ -0,0 +1,85 @@
+{
+  "data": [
+    {
+      "files": [
+        {
+          "filename": "../coverage/used_crate/mod.rs",
+          "summary": {
+            "functions": {
+              "count": 3,
+              "covered": 3,
+              "percent": 100
+            },
+            "instantiations": {
+              "count": 4,
+              "covered": 4,
+              "percent": 100
+            },
+            "lines": {
+              "count": 31,
+              "covered": 14,
+              "percent": 45.16129032258064
+            },
+            "regions": {
+              "count": 16,
+              "covered": 6,
+              "notcovered": 10,
+              "percent": 37.5
+            }
+          }
+        },
+        {
+          "filename": "../coverage/uses_crate.rs",
+          "summary": {
+            "functions": {
+              "count": 1,
+              "covered": 1,
+              "percent": 100
+            },
+            "instantiations": {
+              "count": 1,
+              "covered": 1,
+              "percent": 100
+            },
+            "lines": {
+              "count": 6,
+              "covered": 6,
+              "percent": 100
+            },
+            "regions": {
+              "count": 1,
+              "covered": 1,
+              "notcovered": 0,
+              "percent": 100
+            }
+          }
+        }
+      ],
+      "totals": {
+        "functions": {
+          "count": 4,
+          "covered": 4,
+          "percent": 100
+        },
+        "instantiations": {
+          "count": 5,
+          "covered": 5,
+          "percent": 100
+        },
+        "lines": {
+          "count": 37,
+          "covered": 20,
+          "percent": 54.054054054054056
+        },
+        "regions": {
+          "count": 17,
+          "covered": 7,
+          "notcovered": 10,
+          "percent": 41.17647058823529
+        }
+      }
+    }
+  ],
+  "type": "llvm.coverage.json.export",
+  "version": "2.0.1"
+}

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

@@ -93,39 +93,43 @@
    88|       |    }
    89|      1|}
    90|       |
-   91|      1|fn main() {
-   92|      1|    let _ = g(10);
-   93|      1|    let _ = h(9);
-   94|      1|    let mut future = Box::pin(i(8));
-   95|      1|    j(7);
-   96|      1|    l(6);
-   97|      1|    executor::block_on(future.as_mut());
-   98|      1|}
-   99|       |
-  100|       |mod executor {
-  101|       |    use core::{
-  102|       |        future::Future,
-  103|       |        pin::Pin,
-  104|       |        task::{Context, Poll, RawWaker, RawWakerVTable, Waker},
-  105|       |    };
-  106|       |
-  107|      1|    pub fn block_on<F: Future>(mut future: F) -> F::Output {
-  108|      1|        let mut future = unsafe { Pin::new_unchecked(&mut future) };
-  109|      1|
-  110|      1|        static VTABLE: RawWakerVTable = RawWakerVTable::new(
-  111|      1|            |_| unimplemented!("clone"),
-  112|      1|            |_| unimplemented!("wake"),
-  113|      1|            |_| unimplemented!("wake_by_ref"),
-  114|      1|            |_| (),
-  115|      1|        );
-  116|      1|        let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) };
-  117|      1|        let mut context = Context::from_waker(&waker);
-  118|       |
-  119|       |        loop {
-  120|      1|            if let Poll::Ready(val) = future.as_mut().poll(&mut context) {
-  121|      1|                break val;
-  122|      0|            }
-  123|       |        }
-  124|      1|    }
-  125|       |}
+   91|      1|async fn m(x: u8) -> u8 { x - 1 }
+                                      ^0
+   92|       |
+   93|      1|fn main() {
+   94|      1|    let _ = g(10);
+   95|      1|    let _ = h(9);
+   96|      1|    let mut future = Box::pin(i(8));
+   97|      1|    j(7);
+   98|      1|    l(6);
+   99|      1|    let _ = m(5);
+  100|      1|    executor::block_on(future.as_mut());
+  101|      1|}
+  102|       |
+  103|       |mod executor {
+  104|       |    use core::{
+  105|       |        future::Future,
+  106|       |        pin::Pin,
+  107|       |        task::{Context, Poll, RawWaker, RawWakerVTable, Waker},
+  108|       |    };
+  109|       |
+  110|      1|    pub fn block_on<F: Future>(mut future: F) -> F::Output {
+  111|      1|        let mut future = unsafe { Pin::new_unchecked(&mut future) };
+  112|      1|
+  113|      1|        static VTABLE: RawWakerVTable = RawWakerVTable::new(
+  114|      1|            |_| unimplemented!("clone"),
+  115|      1|            |_| unimplemented!("wake"),
+  116|      1|            |_| unimplemented!("wake_by_ref"),
+  117|      1|            |_| (),
+  118|      1|        );
+  119|      1|        let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) };
+  120|      1|        let mut context = Context::from_waker(&waker);
+  121|       |
+  122|       |        loop {
+  123|      1|            if let Poll::Ready(val) = future.as_mut().poll(&mut context) {
+  124|      1|                break val;
+  125|      0|            }
+  126|       |        }
+  127|      1|    }
+  128|       |}
 

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

@@ -45,4 +45,19 @@
    44|       |`function_source_hash` without a code region, if necessary.
    45|       |
    46|       |*/
+   47|       |
+   48|       |// FIXME(#79626): The derived traits get coverage, which is great, but some of the traits appear
+   49|       |// to get two coverage execution counts at different positions:
+   50|       |//
+   51|       |// ```text
+   52|       |//    4|      2|#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
+   53|       |//                       ^0            ^0      ^0 ^0  ^1       ^0 ^0^0
+   54|       |// ```text
+   55|       |//
+   56|       |// `PartialEq`, `PartialOrd`, and `Ord` (and possibly `Eq`, if the trait name was longer than 2
+   57|       |// characters) have counts at their first and last characters.
+   58|       |//
+   59|       |// Why is this? Why does `PartialOrd` have two values (1 and 0)? This must mean we are checking
+   60|       |// distinct coverages, so maybe we don't want to eliminate one of them. Should we merge them?
+   61|       |// If merged, do we lose some information?