coverage: Hoist the removal of unwanted macro expansion spans

Author: Zalathar

compiler/rustc_mir_transform/src/coverage/spans.rs

@@ -292,32 +292,9 @@ impl<'a> CoverageSpansGenerator<'a> {
             } else if curr.is_closure {
                 self.carve_out_span_for_closure();
             } else if self.prev_original_span == curr.span {
-                // Note that this compares the new (`curr`) span to `prev_original_span`.
-                // In this branch, the actual span byte range of `prev_original_span` is not
-                // important. What is important is knowing whether the new `curr` span was
-                // **originally** the same as the original span of `prev()`. The original spans
-                // reflect their original sort order, and for equal spans, conveys a partial
-                // ordering based on CFG dominator priority.
-                if prev.is_macro_expansion() && curr.is_macro_expansion() {
-                    // Macros that expand to include branching (such as
-                    // `assert_eq!()`, `assert_ne!()`, `info!()`, `debug!()`, or
-                    // `trace!()`) typically generate callee spans with identical
-                    // ranges (typically the full span of the macro) for all
-                    // `BasicBlocks`. This makes it impossible to distinguish
-                    // the condition (`if val1 != val2`) from the optional
-                    // branched statements (such as the call to `panic!()` on
-                    // assert failure). In this case it is better (or less
-                    // worse) to drop the optional branch bcbs and keep the
-                    // non-conditional statements, to count when reached.
-                    debug!(
-                        "  curr and prev are part of a macro expansion, and curr has the same span \
-                        as prev, but is in a different bcb. Drop curr and keep prev for next iter. \
-                        prev={prev:?}",
-                    );
-                    self.take_curr(); // Discards curr.
-                } else {
-                    self.update_pending_dups();
-                }
+                // `prev` and `curr` have the same span, or would have had the
+                // same span before `prev` was modified by other spans.
+                self.update_pending_dups();
             } else {
                 self.cutoff_prev_at_overlapping_curr();
                 self.maybe_push_macro_name_span();

compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs

@@ -1,4 +1,5 @@
 use rustc_data_structures::captures::Captures;
+use rustc_data_structures::fx::FxHashSet;
 use rustc_middle::mir::{
     self, AggregateKind, FakeReadCause, Rvalue, Statement, StatementKind, Terminator,
     TerminatorKind,
@@ -36,6 +37,9 @@ pub(super) fn mir_to_initial_sorted_coverage_spans(
 
     initial_spans.push(CoverageSpan::for_fn_sig(fn_sig_span));
 
+    initial_spans.sort_by(|a, b| basic_coverage_blocks.cmp_in_dominator_order(a.bcb, b.bcb));
+    remove_unwanted_macro_spans(&mut initial_spans);
+
     initial_spans.sort_by(|a, b| {
         // First sort by span start.
         Ord::cmp(&a.span.lo(), &b.span.lo())
@@ -56,6 +60,26 @@ pub(super) fn mir_to_initial_sorted_coverage_spans(
     initial_spans
 }
 
+/// Macros that expand into branches (e.g. `assert!`, `trace!`) tend to generate
+/// multiple condition/consequent blocks that have the span of the whole macro
+/// invocation, which is unhelpful. Keeping only the first such span seems to
+/// give better mappings, so remove the others.
+///
+/// (The input spans should be sorted in BCB dominator order, so that the
+/// retained "first" span is likely to dominate the others.)
+fn remove_unwanted_macro_spans(initial_spans: &mut Vec<CoverageSpan>) {
+    let mut seen_spans = FxHashSet::default();
+    initial_spans.retain(|covspan| {
+        // Ignore (retain) closure spans and non-macro-expansion spans.
+        if covspan.is_closure || !covspan.is_macro_expansion() {
+            return true;
+        }
+
+        // Retain only the first macro-expanded covspan with this span.
+        seen_spans.insert(covspan.span)
+    });
+}
+
 // Generate a set of `CoverageSpan`s from the filtered set of `Statement`s and `Terminator`s of
 // the `BasicBlock`(s) in the given `BasicCoverageBlockData`. One `CoverageSpan` is generated
 // for each `Statement` and `Terminator`. (Note that subsequent stages of coverage analysis will

tests/coverage/closure.cov-map

@@ -81,77 +81,63 @@ Number of file 0 mappings: 1
 - Code(Zero) at (prev + 171, 13) to (start + 2, 14)
 
 Function name: closure::main::{closure#14}
-Raw bytes (36): 0x[01, 01, 03, 05, 0a, 01, 05, 01, 05, 05, 03, b2, 01, 0d, 00, 15, 01, 01, 11, 01, 1b, 05, 01, 1e, 00, 25, 0a, 00, 2f, 00, 33, 03, 01, 0d, 00, 0e]
+Raw bytes (29): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, b2, 01, 0d, 02, 1b, 05, 02, 1e, 00, 25, 02, 00, 2f, 00, 33, 07, 01, 0d, 00, 0e]
 Number of files: 1
 - file 0 => global file 1
-Number of expressions: 3
-- expression 0 operands: lhs = Counter(1), rhs = Expression(2, Sub)
-- expression 1 operands: lhs = Counter(0), rhs = Counter(1)
-- expression 2 operands: lhs = Counter(0), rhs = Counter(1)
-Number of file 0 mappings: 5
-- Code(Expression(0, Add)) at (prev + 178, 13) to (start + 0, 21)
-    = (c1 + (c0 - c1))
-- Code(Counter(0)) at (prev + 1, 17) to (start + 1, 27)
-- Code(Counter(1)) at (prev + 1, 30) to (start + 0, 37)
-- Code(Expression(2, Sub)) at (prev + 0, 47) to (start + 0, 51)
+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 + 178, 13) to (start + 2, 27)
+- Code(Counter(1)) at (prev + 2, 30) to (start + 0, 37)
+- Code(Expression(0, Sub)) at (prev + 0, 47) to (start + 0, 51)
     = (c0 - c1)
-- Code(Expression(0, Add)) at (prev + 1, 13) to (start + 0, 14)
+- Code(Expression(1, Add)) at (prev + 1, 13) to (start + 0, 14)
     = (c1 + (c0 - c1))
 
 Function name: closure::main::{closure#15}
-Raw bytes (41): 0x[01, 01, 03, 05, 0a, 01, 05, 01, 05, 06, 01, ba, 01, 09, 00, 0a, 03, 01, 0d, 00, 15, 01, 01, 11, 01, 1b, 05, 01, 1e, 00, 25, 0a, 00, 2f, 00, 33, 03, 02, 09, 00, 0a]
+Raw bytes (29): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, ba, 01, 09, 03, 1b, 05, 03, 1e, 00, 25, 02, 00, 2f, 00, 33, 07, 02, 09, 00, 0a]
 Number of files: 1
 - file 0 => global file 1
-Number of expressions: 3
-- expression 0 operands: lhs = Counter(1), rhs = Expression(2, Sub)
-- expression 1 operands: lhs = Counter(0), rhs = Counter(1)
-- expression 2 operands: lhs = Counter(0), rhs = Counter(1)
-Number of file 0 mappings: 6
-- Code(Counter(0)) at (prev + 186, 9) to (start + 0, 10)
-- Code(Expression(0, Add)) at (prev + 1, 13) to (start + 0, 21)
-    = (c1 + (c0 - c1))
-- Code(Counter(0)) at (prev + 1, 17) to (start + 1, 27)
-- Code(Counter(1)) at (prev + 1, 30) to (start + 0, 37)
-- Code(Expression(2, Sub)) at (prev + 0, 47) to (start + 0, 51)
+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 + 186, 9) to (start + 3, 27)
+- Code(Counter(1)) at (prev + 3, 30) to (start + 0, 37)
+- Code(Expression(0, Sub)) at (prev + 0, 47) to (start + 0, 51)
     = (c0 - c1)
-- Code(Expression(0, Add)) at (prev + 2, 9) to (start + 0, 10)
+- Code(Expression(1, Add)) at (prev + 2, 9) to (start + 0, 10)
     = (c1 + (c0 - c1))
 
 Function name: closure::main::{closure#16}
-Raw bytes (36): 0x[01, 01, 03, 05, 0a, 01, 05, 01, 05, 05, 03, c4, 01, 0d, 00, 15, 01, 01, 11, 01, 1b, 05, 01, 1e, 00, 25, 0a, 00, 2f, 00, 33, 03, 01, 0d, 00, 0e]
+Raw bytes (29): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, c4, 01, 0d, 02, 1b, 05, 02, 1e, 00, 25, 02, 00, 2f, 00, 33, 07, 01, 0d, 00, 0e]
 Number of files: 1
 - file 0 => global file 1
-Number of expressions: 3
-- expression 0 operands: lhs = Counter(1), rhs = Expression(2, Sub)
-- expression 1 operands: lhs = Counter(0), rhs = Counter(1)
-- expression 2 operands: lhs = Counter(0), rhs = Counter(1)
-Number of file 0 mappings: 5
-- Code(Expression(0, Add)) at (prev + 196, 13) to (start + 0, 21)
-    = (c1 + (c0 - c1))
-- Code(Counter(0)) at (prev + 1, 17) to (start + 1, 27)
-- Code(Counter(1)) at (prev + 1, 30) to (start + 0, 37)
-- Code(Expression(2, Sub)) at (prev + 0, 47) to (start + 0, 51)
+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 + 196, 13) to (start + 2, 27)
+- Code(Counter(1)) at (prev + 2, 30) to (start + 0, 37)
+- Code(Expression(0, Sub)) at (prev + 0, 47) to (start + 0, 51)
     = (c0 - c1)
-- Code(Expression(0, Add)) at (prev + 1, 13) to (start + 0, 14)
+- Code(Expression(1, Add)) at (prev + 1, 13) to (start + 0, 14)
     = (c1 + (c0 - c1))
 
 Function name: closure::main::{closure#17}
-Raw bytes (41): 0x[01, 01, 03, 05, 0a, 01, 05, 01, 05, 06, 01, cc, 01, 09, 00, 0a, 03, 01, 0d, 00, 15, 01, 01, 11, 01, 1b, 05, 01, 1e, 00, 25, 0a, 00, 2f, 00, 33, 03, 02, 09, 00, 0a]
+Raw bytes (29): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, cc, 01, 09, 03, 1b, 05, 03, 1e, 00, 25, 02, 00, 2f, 00, 33, 07, 02, 09, 00, 0a]
 Number of files: 1
 - file 0 => global file 1
-Number of expressions: 3
-- expression 0 operands: lhs = Counter(1), rhs = Expression(2, Sub)
-- expression 1 operands: lhs = Counter(0), rhs = Counter(1)
-- expression 2 operands: lhs = Counter(0), rhs = Counter(1)
-Number of file 0 mappings: 6
-- Code(Counter(0)) at (prev + 204, 9) to (start + 0, 10)
-- Code(Expression(0, Add)) at (prev + 1, 13) to (start + 0, 21)
-    = (c1 + (c0 - c1))
-- Code(Counter(0)) at (prev + 1, 17) to (start + 1, 27)
-- Code(Counter(1)) at (prev + 1, 30) to (start + 0, 37)
-- Code(Expression(2, Sub)) at (prev + 0, 47) to (start + 0, 51)
+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 + 204, 9) to (start + 3, 27)
+- Code(Counter(1)) at (prev + 3, 30) to (start + 0, 37)
+- Code(Expression(0, Sub)) at (prev + 0, 47) to (start + 0, 51)
     = (c0 - c1)
-- Code(Expression(0, Add)) at (prev + 2, 9) to (start + 0, 10)
+- Code(Expression(1, Add)) at (prev + 2, 9) to (start + 0, 10)
     = (c1 + (c0 - c1))
 
 Function name: closure::main::{closure#18}