Rollup merge of #79818 - richkadel:llvm-coverage-counters-2.1.0, r=tmandry

Fixes to Rust coverage

Fixes: #79725

Some macros can create a situation where `fn_sig_span` and `body_span`
map to different files.

New documentation on coverage tests incorrectly assumed multiple test
binaries could just be listed at the end of the `llvm-cov` command,
but it turns out each binary needs a `--object` prefix.

This PR fixes the bug and updates the documentation to correct that
issue. It also fixes a few other minor issues in internal implementation
comments, and adds documentation on getting coverage results for doc
tests.

Author: tmandry

compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs

@@ -241,7 +241,7 @@ fn save_function_record(
 /// (functions referenced by other "used" or public items). Any other functions considered unused,
 /// or "Unreachable" were still parsed and processed through the MIR stage.
 ///
-/// We can find the unreachable functions by the set different of all MIR `DefId`s (`tcx` query
+/// We can find the unreachable functions by the set difference of all MIR `DefId`s (`tcx` query
 /// `mir_keys`) minus the codegenned `DefId`s (`tcx` query `collect_and_partition_mono_items`).
 ///
 /// *HOWEVER* the codegenned `DefId`s are partitioned across multiple `CodegenUnit`s (CGUs), and

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

@@ -33,7 +33,7 @@ impl CoverageGraph {
         // Pre-transform MIR `BasicBlock` successors and predecessors into the BasicCoverageBlock
         // equivalents. Note that since the BasicCoverageBlock graph has been fully simplified, the
         // each predecessor of a BCB leader_bb should be in a unique BCB, and each successor of a
-        // BCB last_bb should bin in its own unique BCB. Therefore, collecting the BCBs using
+        // BCB last_bb should be in its own unique BCB. Therefore, collecting the BCBs using
         // `bb_to_bcb` should work without requiring a deduplication step.
 
         let successors = IndexVec::from_fn_n(
@@ -283,7 +283,9 @@ rustc_index::newtype_index! {
     }
 }
 
-/// A BasicCoverageBlockData (BCB) represents the maximal-length sequence of MIR BasicBlocks without
+/// `BasicCoverageBlockData` holds the data indexed by a `BasicCoverageBlock`.
+///
+/// A `BasicCoverageBlock` (BCB) represents the maximal-length sequence of MIR `BasicBlock`s without
 /// conditional branches, and form a new, simplified, coverage-specific Control Flow Graph, without
 /// altering the original MIR CFG.
 ///

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

@@ -88,6 +88,7 @@ struct Instrumentor<'a, 'tcx> {
     pass_name: &'a str,
     tcx: TyCtxt<'tcx>,
     mir_body: &'a mut mir::Body<'tcx>,
+    source_file: Lrc<SourceFile>,
     fn_sig_span: Span,
     body_span: Span,
     basic_coverage_blocks: CoverageGraph,
@@ -96,9 +97,13 @@ struct Instrumentor<'a, 'tcx> {
 
 impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
     fn new(pass_name: &'a str, tcx: TyCtxt<'tcx>, mir_body: &'a mut mir::Body<'tcx>) -> Self {
+        let source_map = tcx.sess.source_map();
         let (some_fn_sig, hir_body) = fn_sig_and_body(tcx, mir_body.source.def_id());
         let body_span = hir_body.value.span;
-        let fn_sig_span = match some_fn_sig {
+        let source_file = source_map.lookup_source_file(body_span.lo());
+        let fn_sig_span = match some_fn_sig.filter(|fn_sig| {
+            Lrc::ptr_eq(&source_file, &source_map.lookup_source_file(fn_sig.span.hi()))
+        }) {
             Some(fn_sig) => fn_sig.span.with_hi(body_span.lo()),
             None => body_span.shrink_to_lo(),
         };
@@ -108,6 +113,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
             pass_name,
             tcx,
             mir_body,
+            source_file,
             fn_sig_span,
             body_span,
             basic_coverage_blocks,
@@ -268,8 +274,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
         let tcx = self.tcx;
         let source_map = tcx.sess.source_map();
         let body_span = self.body_span;
-        let source_file = source_map.lookup_source_file(body_span.lo());
-        let file_name = Symbol::intern(&source_file.name.to_string());
+        let file_name = Symbol::intern(&self.source_file.name.to_string());
 
         let mut bcb_counters = IndexVec::from_elem_n(None, self.basic_coverage_blocks.num_nodes());
         for covspan in coverage_spans {
@@ -285,11 +290,20 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
                 bug!("Every BasicCoverageBlock should have a Counter or Expression");
             };
             graphviz_data.add_bcb_coverage_span_with_counter(bcb, &covspan, &counter_kind);
+
+            debug!(
+                "Calling make_code_region(file_name={}, source_file={:?}, span={}, body_span={})",
+                file_name,
+                self.source_file,
+                source_map.span_to_string(span),
+                source_map.span_to_string(body_span)
+            );
+
             inject_statement(
                 self.mir_body,
                 counter_kind,
                 self.bcb_last_bb(bcb),
-                Some(make_code_region(file_name, &source_file, span, body_span)),
+                Some(make_code_region(file_name, &self.source_file, span, body_span)),
             );
         }
     }

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

@@ -217,6 +217,27 @@ pub struct CoverageSpans<'a, 'tcx> {
 }
 
 impl<'a, 'tcx> CoverageSpans<'a, 'tcx> {
+    /// Generate a minimal set of `CoverageSpan`s, each representing a contiguous code region to be
+    /// counted.
+    ///
+    /// The basic steps are:
+    ///
+    /// 1. Extract an initial set of spans from the `Statement`s and `Terminator`s of each
+    ///    `BasicCoverageBlockData`.
+    /// 2. Sort the spans by span.lo() (starting position). Spans that start at the same position
+    ///    are sorted with longer spans before shorter spans; and equal spans are sorted
+    ///    (deterministically) based on "dominator" relationship (if any).
+    /// 3. Traverse the spans in sorted order to identify spans that can be dropped (for instance,
+    ///    if another span or spans are already counting the same code region), or should be merged
+    ///    into a broader combined span (because it represents a contiguous, non-branching, and
+    ///    uninterrupted region of source code).
+    ///
+    ///    Closures are exposed in their enclosing functions as `Assign` `Rvalue`s, and since
+    ///    closures have their own MIR, their `Span` in their enclosing function should be left
+    ///    "uncovered".
+    ///
+    /// Note the resulting vector of `CoverageSpan`s may not be fully sorted (and does not need
+    /// to be).
     pub(super) fn generate_coverage_spans(
         mir_body: &'a mir::Body<'tcx>,
         fn_sig_span: Span,
@@ -247,27 +268,6 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> {
         coverage_spans.to_refined_spans()
     }
 
-    /// Generate a minimal set of `CoverageSpan`s, each representing a contiguous code region to be
-    /// counted.
-    ///
-    /// The basic steps are:
-    ///
-    /// 1. Extract an initial set of spans from the `Statement`s and `Terminator`s of each
-    ///    `BasicCoverageBlockData`.
-    /// 2. Sort the spans by span.lo() (starting position). Spans that start at the same position
-    ///    are sorted with longer spans before shorter spans; and equal spans are sorted
-    ///    (deterministically) based on "dominator" relationship (if any).
-    /// 3. Traverse the spans in sorted order to identify spans that can be dropped (for instance,
-    ///    if another span or spans are already counting the same code region), or should be merged
-    ///    into a broader combined span (because it represents a contiguous, non-branching, and
-    ///    uninterrupted region of source code).
-    ///
-    ///    Closures are exposed in their enclosing functions as `Assign` `Rvalue`s, and since
-    ///    closures have their own MIR, their `Span` in their enclosing function should be left
-    ///    "uncovered".
-    ///
-    /// Note the resulting vector of `CoverageSpan`s does may not be fully sorted (and does not need
-    /// to be).
     fn mir_to_initial_sorted_coverage_spans(&self) -> Vec<CoverageSpan> {
         let mut initial_spans = Vec::<CoverageSpan>::with_capacity(self.mir_body.num_nodes() * 2);
         for (bcb, bcb_data) in self.basic_coverage_blocks.iter_enumerated() {

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

@@ -213,19 +213,102 @@ Then run the `cov` tool, with the `profdata` file and all test binaries:
 $ cargo cov -- report \
     --use-color --ignore-filename-regex='/.cargo/registry' \
     --instr-profile=json5format.profdata \
-    target/debug/deps/lib-30768f9c53506dc5 \
-    target/debug/deps/json5format-fececd4653271682
+    --object target/debug/deps/lib-30768f9c53506dc5 \
+    --object target/debug/deps/json5format-fececd4653271682
 $ cargo cov -- show \
     --use-color --ignore-filename-regex='/.cargo/registry' \
     --instr-profile=json5format.profdata \
-    target/debug/deps/lib-30768f9c53506dc5 \
-    target/debug/deps/json5format-fececd4653271682 \
+    --object target/debug/deps/lib-30768f9c53506dc5 \
+    --object target/debug/deps/json5format-fececd4653271682 \
     --show-instantiations --show-line-counts-or-regions \
     --Xdemangler=rustfilt | less -R
 ```
 
 _Note the command line option `--ignore-filename-regex=/.cargo/registry`, which excludes the sources for dependencies from the coverage results._
 
+### Tips for listing the binaries automatically
+
+For `bash` users, one suggested way to automatically complete the `cov` command with the list of binaries is with a command like:
+
+```bash
+$ cargo cov -- report \
+    $( \
+      for file in \
+        $( \
+          RUSTFLAGS="-Zinstrument-coverage" \
+            cargo test --tests --no-run --message-format=json \
+              | jq -r "select(.profile.test == true) | .filenames[]" \
+              | grep -v dSYM - \
+        ); \
+      do \
+        printf "%s %s " -object $file; \
+      done \
+    ) \
+  --instr-profile=json5format.profdata --summary-only # and/or other options
+```
+
+Adding `--no-run --message-format=json` to the _same_ `cargo test` command used to run
+the tests (including the same environment variables and flags) generates output in a JSON
+format that `jq` can easily query.
+
+The `printf` command takes this list and generates the `--object <binary>` arguments
+for each listed test binary.
+
+### Including doc tests
+
+The previous examples run `cargo test` with `--tests`, which excludes doc tests.[^79417]
+
+To include doc tests in the coverage results, drop the `--tests` flag, and apply the
+`-Zinstrument-coverage` flag, and some doc-test-specific options in the
+`RUSTDOCFLAGS` environment variable. (The `cargo profdata` command does not change.)
+
+```bash
+$ RUSTFLAGS="-Zinstrument-coverage" \
+  RUSTDOCFLAGS="-Zinstrument-coverage -Zunstable-options --persist-doctests target/debug/doctestbins" \
+  LLVM_PROFILE_FILE="json5format-%m.profraw" \
+    cargo test
+$ cargo profdata -- merge \
+    -sparse json5format-*.profraw -o json5format.profdata
+```
+
+The `-Zunstable-options --persist-doctests` flag is required, to save the test binaries
+(with their coverage maps) for `llvm-cov`.
+
+```bash
+$ cargo cov -- report \
+    $( \
+      for file in \
+        $( \
+          RUSTFLAGS="-Zinstrument-coverage" \
+          RUSTDOCFLAGS="-Zinstrument-coverage -Zunstable-options --persist-doctests target/debug/doctestbins" \
+            cargo test --no-run --message-format=json \
+              | jq -r "select(.profile.test == true) | .filenames[]" \
+              | grep -v dSYM - \
+        ) \
+        target/debug/doctestbins/*/rust_out; \
+      do \
+        [[ -x $file ]] && printf "%s %s " -object $file; \
+      done \
+    ) \
+  --instr-profile=json5format.profdata --summary-only # and/or other options
+```
+
+Note, the differences in this `cargo cov` command, compared with the version without
+doc tests, include:
+
+* The `cargo test ... --no-run` command is updated with the same environment variables
+  and flags used to _build_ the tests, _including_ the doc tests. (`LLVM_PROFILE_FILE`
+  is only used when _running_ the tests.)
+* The file glob pattern `target/debug/doctestbins/*/rust_out` adds the `rust_out`
+  binaries generated for doc tests (note, however, that some `rust_out` files may not
+  be executable binaries).
+* `[[ -x $file ]] &&` filters the files passed on to the `printf`, to include only
+  executable binaries.
+
+[^79417]: There is ongoing work to resolve a known issue
+[(#79417)](https://github.com/rust-lang/rust/issues/79417) that doc test coverage
+generates incorrect source line numbers in `llvm-cov show` results.
+
 ## Other references
 
 Rust's implementation and workflow for source-based code coverage is based on the same library and tools used to implement [source-based code coverage in Clang]. (This document is partially based on the Clang guide.)

src/test/run-make-fulldeps/coverage-reports/Makefile

@@ -147,13 +147,19 @@ else
 	# Note `llvm-cov show` output for some programs can vary, but can be ignored
 	# by inserting `// ignore-llvm-cov-show-diffs` at the top of the source file.
 	#
-	# FIXME(richkadel): It looks like most past variations seem to have been mitigated. None of the
-	# Rust test source samples have the `// ignore-llvm-cov-show-diffs` anymore. The main variation
-	# I had seen (and is still present in the new `coverage/lib/used_crate.rs`) is the `llvm-cov show`
-	# reporting of multiple instantiations of a generic function with different type substitutions.
-	# For some reason, `llvm-cov show` can report these in a non-deterministic order, breaking the
-	# `diff` comparision. I was able to work around the problem with `diff --ignore-matching-lines=RE`
+	# FIXME(richkadel): None of the Rust test source samples have the
+	# `// ignore-llvm-cov-show-diffs` anymore. This directive exists to work around a limitation
+	# with `llvm-cov show`. When reporting coverage for multiple instantiations of a generic function,
+	# with different type substitutions, `llvm-cov show` prints these in a non-deterministic order,
+	# breaking the `diff` comparision.
+	#
+	# A partial workaround is implemented below, with `diff --ignore-matching-lines=RE`
 	# to ignore each line prefixing each generic instantiation coverage code region.
+	#
+	# This workaround only works if the coverage counts are identical across all reported
+	# instantiations. If there is no way to ensure this, you may need to apply the
+	# `// ignore-llvm-cov-show-diffs` directive, and rely on the `.json` and counter
+	# files for validating results have not changed.
 
 	$(DIFF) --ignore-matching-lines='::<.*>.*:$$' \
 		[email protected] "$(TMPDIR)"/[email protected] || \
@@ -190,10 +196,6 @@ endif
 			$(call BIN,"$(TMPDIR)"/$@) \
 		| "$(PYTHON)" $(BASEDIR)/prettify_json.py \
 		> "$(TMPDIR)"/[email protected]
-	# FIXME(richkadel): With the addition of `--ignore-matching-lines=RE` to ignore the
-	# non-deterministically-ordered coverage results for multiple instantiations of generics with
-	# differing type substitutions, I probably don't need the `.json` files anymore (and may not
-	# need `prettify_json.py` either).
 
 ifdef RUSTC_BLESS_TEST
 	cp "$(TMPDIR)"/[email protected] [email protected]

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

@@ -19,12 +19,12 @@
    18|      2|    println!("used_only_from_bin_crate_generic_function with {:?}", arg);
    19|      2|}
   ------------------
-  | used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
+  | used_crate::used_only_from_bin_crate_generic_function::<&str>:
   |   17|      1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
   |   18|      1|    println!("used_only_from_bin_crate_generic_function with {:?}", arg);
   |   19|      1|}
   ------------------
-  | used_crate::used_only_from_bin_crate_generic_function::<&str>:
+  | used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec<i32>>:
   |   17|      1|pub fn used_only_from_bin_crate_generic_function<T: Debug>(arg: T) {
   |   18|      1|    println!("used_only_from_bin_crate_generic_function with {:?}", arg);
   |   19|      1|}

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

@@ -35,9 +35,6 @@ Counter in file 0 11:1 -> 11:2, (#2 + (#1 - #2))
 Counter in file 0 21:1 -> 21:23, #1
 Counter in file 0 67:5 -> 67:23, #1
 Counter in file 0 38:1 -> 38:19, #1
-Counter in file 0 29:1 -> 29:22, #1
-Counter in file 0 93:1 -> 101:2, #1
-Counter in file 0 91:1 -> 91:25, #1
 Counter in file 0 38:19 -> 42:12, #1
 Counter in file 0 43:9 -> 43:10, #3
 Counter in file 0 43:14 -> 43:18, (#1 + 0)
@@ -49,11 +46,14 @@ Counter in file 0 44:27 -> 44:32, #8
 Counter in file 0 44:36 -> 44:38, (#6 + 0)
 Counter in file 0 45:14 -> 45:16, #7
 Counter in file 0 47:1 -> 47:2, (#5 + (#6 + #7))
+Counter in file 0 29:1 -> 29:22, #1
+Counter in file 0 93:1 -> 101:2, #1
+Counter in file 0 91:1 -> 91:25, #1
 Counter in file 0 51:5 -> 52:18, #1
 Counter in file 0 53:13 -> 53:14, #2
 Counter in file 0 63:13 -> 63:14, (#1 - #2)
 Counter in file 0 65:5 -> 65:6, (#2 + (#1 - #2))
-Counter in file 0 13:20 -> 13:21, #1
+Counter in file 0 17:20 -> 17:21, #1
 Counter in file 0 49:1 -> 68:12, #1
 Counter in file 0 69:9 -> 69:10, #2
 Counter in file 0 69:14 -> 69:27, (#1 + 0)
@@ -69,8 +69,8 @@ Counter in file 0 86:14 -> 86:16, #2
 Counter in file 0 87:14 -> 87:16, #3
 Counter in file 0 89:1 -> 89:2, (#3 + (#2 + (#1 - (#3 + #2))))
 Counter in file 0 17:1 -> 17:20, #1
-Counter in file 0 17:20 -> 17:21, #1
 Counter in file 0 66:5 -> 66:23, #1
+Counter in file 0 13:20 -> 13:21, #1
 Counter in file 0 17:9 -> 17:10, #1
 Counter in file 0 17:9 -> 17:10, #1
 Counter in file 0 117:17 -> 117:19, #1

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage_counters.generics.txt

@@ -32,12 +32,12 @@ Combined regions:
   10:5 -> 12:6 (count=1)
 Segment at 10:5 (count = 1), RegionEntry
 Segment at 12:6 (count = 0), Skipped
-Emitting segments for function: _RNvXs_Cs4fqI2P2rA04_8genericsINtB4_8FireworklENtNtNtCs6HRHKMTmAen_4core3ops4drop4Drop4dropB4_
+Emitting segments for function: _RNvXs_Cs4fqI2P2rA04_8genericsINtB4_8FireworklENtNtNtCs3rFBWs28XFJ_4core3ops4drop4Drop4dropB4_
 Combined regions:
   17:5 -> 19:6 (count=1)
 Segment at 17:5 (count = 1), RegionEntry
 Segment at 19:6 (count = 0), Skipped
-Emitting segments for function: _RNvXs_Cs4fqI2P2rA04_8genericsINtB4_8FireworkdENtNtNtCs6HRHKMTmAen_4core3ops4drop4Drop4dropB4_
+Emitting segments for function: _RNvXs_Cs4fqI2P2rA04_8genericsINtB4_8FireworkdENtNtNtCs3rFBWs28XFJ_4core3ops4drop4Drop4dropB4_
 Combined regions:
   17:5 -> 19:6 (count=1)
 Segment at 17:5 (count = 1), RegionEntry

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage_counters.uses_crate.txt

@@ -1,5 +1,5 @@
-Counter in file 0 25:1 -> 27:2, #1
 Counter in file 0 17:1 -> 19:2, #1
+Counter in file 0 25:1 -> 27:2, #1
 Counter in file 0 17:1 -> 19:2, #1
 Counter in file 0 5:1 -> 12:2, #1
 Counter in file 0 17:1 -> 19:2, 0
@@ -78,17 +78,17 @@ Segment at 51:1 (count = 0), RegionEntry
 Segment at 51:2 (count = 0), Skipped
 Segment at 53:1 (count = 1), RegionEntry
 Segment at 61:2 (count = 0), Skipped
-Emitting segments for function: _RINvCsbDqzXfLQacH_10used_crate41used_only_from_bin_crate_generic_functionRINtNtCsFAjihUSTht_5alloc3vec3VeclEECs4fqI2P2rA04_10uses_crate
+Emitting segments for function: _RINvCsbDqzXfLQacH_10used_crate41used_only_from_bin_crate_generic_functionReECs4fqI2P2rA04_10uses_crate
 Combined regions:
   17:1 -> 19:2 (count=1)
 Segment at 17:1 (count = 1), RegionEntry
 Segment at 19:2 (count = 0), Skipped
-Emitting segments for function: _RINvCsbDqzXfLQacH_10used_crate41used_only_from_bin_crate_generic_functionReECs4fqI2P2rA04_10uses_crate
+Emitting segments for function: _RINvCsbDqzXfLQacH_10used_crate41used_only_from_bin_crate_generic_functionRINtNtCs3QflaznQylx_5alloc3vec3VeclEECs4fqI2P2rA04_10uses_crate
 Combined regions:
   17:1 -> 19:2 (count=1)
 Segment at 17:1 (count = 1), RegionEntry
 Segment at 19:2 (count = 0), Skipped
-Emitting segments for function: _RINvCsbDqzXfLQacH_10used_crate46used_only_from_this_lib_crate_generic_functionINtNtCsFAjihUSTht_5alloc3vec3VeclEEB2_
+Emitting segments for function: _RINvCsbDqzXfLQacH_10used_crate46used_only_from_this_lib_crate_generic_functionINtNtCs3QflaznQylx_5alloc3vec3VeclEEB2_
 Combined regions:
   21:1 -> 23:2 (count=1)
 Segment at 21:1 (count = 1), RegionEntry
@@ -98,7 +98,7 @@ Combined regions:
   21:1 -> 23:2 (count=1)
 Segment at 21:1 (count = 1), RegionEntry
 Segment at 23:2 (count = 0), Skipped
-Emitting segments for function: _RINvCsbDqzXfLQacH_10used_crate50used_from_bin_crate_and_lib_crate_generic_functionINtNtCsFAjihUSTht_5alloc3vec3VeclEECs4fqI2P2rA04_10uses_crate
+Emitting segments for function: _RINvCsbDqzXfLQacH_10used_crate50used_from_bin_crate_and_lib_crate_generic_functionINtNtCs3QflaznQylx_5alloc3vec3VeclEECs4fqI2P2rA04_10uses_crate
 Combined regions:
   25:1 -> 27:2 (count=1)
 Segment at 25:1 (count = 1), RegionEntry