Rollup merge of #121261 - Zalathar:pending-dups, r=oli-obk

coverage: Remove `pending_dups` from the span refiner

When extracting coverage spans from a function's MIR, we need to decide how to handle spans that are associated with more than one node (BCB) in the coverage control flow graph.

The existing code for managing those duplicate spans is very subtle and difficult to modify. But by eagerly deduplicating those extracted spans in a much simpler way, we can remove a massive chunk of complexity from the span refiner.

There is a tradeoff here, in that we no longer try to retain *all* nondominating BCBs that have the same span, only the last one in the (semi-arbitrary) dominance ordering. But in practice, this produces very little difference in our coverage tests, and the simplification is so significant that I think it's worthwhile.

``@rustbot`` label +A-code-coverage

compiler/rustc_mir_transform/src/coverage/spans.rs

@@ -61,7 +61,7 @@ pub(super) fn generate_coverage_spans(
             hir_info,
             basic_coverage_blocks,
         );
-        let coverage_spans = SpansRefiner::refine_sorted_spans(basic_coverage_blocks, sorted_spans);
+        let coverage_spans = SpansRefiner::refine_sorted_spans(sorted_spans);
         mappings.extend(coverage_spans.into_iter().map(|RefinedCovspan { bcb, span, .. }| {
             // Each span produced by the generator represents an ordinary code region.
             BcbMapping { kind: BcbMappingKind::Code(bcb), span }
@@ -88,8 +88,6 @@ pub(super) fn generate_coverage_spans(
 
 #[derive(Debug)]
 struct CurrCovspan {
-    /// This is used as the basis for [`PrevCovspan::original_span`], so it must
-    /// not be modified.
     span: Span,
     bcb: BasicCoverageBlock,
     is_closure: bool,
@@ -102,7 +100,7 @@ impl CurrCovspan {
 
     fn into_prev(self) -> PrevCovspan {
         let Self { span, bcb, is_closure } = self;
-        PrevCovspan { original_span: span, span, bcb, merged_spans: vec![span], is_closure }
+        PrevCovspan { span, bcb, merged_spans: vec![span], is_closure }
     }
 
     fn into_refined(self) -> RefinedCovspan {
@@ -115,7 +113,6 @@ impl CurrCovspan {
 
 #[derive(Debug)]
 struct PrevCovspan {
-    original_span: Span,
     span: Span,
     bcb: BasicCoverageBlock,
     /// List of all the original spans from MIR that have been merged into this
@@ -135,42 +132,17 @@ impl PrevCovspan {
         self.merged_spans.push(other.span);
     }
 
-    fn cutoff_statements_at(&mut self, cutoff_pos: BytePos) {
+    fn cutoff_statements_at(mut self, cutoff_pos: BytePos) -> Option<RefinedCovspan> {
         self.merged_spans.retain(|span| span.hi() <= cutoff_pos);
         if let Some(max_hi) = self.merged_spans.iter().map(|span| span.hi()).max() {
             self.span = self.span.with_hi(max_hi);
         }
-    }
-
-    fn into_dup(self) -> DuplicateCovspan {
-        let Self { original_span, span, bcb, merged_spans: _, is_closure } = self;
-        // Only unmodified spans end up in `pending_dups`.
-        debug_assert_eq!(original_span, span);
-        DuplicateCovspan { span, bcb, is_closure }
-    }
-
-    fn refined_copy(&self) -> RefinedCovspan {
-        let &Self { original_span: _, span, bcb, merged_spans: _, is_closure } = self;
-        RefinedCovspan { span, bcb, is_closure }
-    }
 
-    fn into_refined(self) -> RefinedCovspan {
-        self.refined_copy()
+        if self.merged_spans.is_empty() { None } else { Some(self.into_refined()) }
     }
-}
-
-#[derive(Debug)]
-struct DuplicateCovspan {
-    span: Span,
-    bcb: BasicCoverageBlock,
-    is_closure: bool,
-}
 
-impl DuplicateCovspan {
-    /// Returns a copy of this covspan, as a [`RefinedCovspan`].
-    /// Should only be called in places that would otherwise clone this covspan.
     fn refined_copy(&self) -> RefinedCovspan {
-        let &Self { span, bcb, is_closure } = self;
+        let &Self { span, bcb, merged_spans: _, is_closure } = self;
         RefinedCovspan { span, bcb, is_closure }
     }
 
@@ -205,10 +177,7 @@ impl RefinedCovspan {
 ///  * Merge spans that represent continuous (both in source code and control flow), non-branching
 ///    execution
 ///  * Carve out (leave uncovered) any span that will be counted by another MIR (notably, closures)
-struct SpansRefiner<'a> {
-    /// The BasicCoverageBlock Control Flow Graph (BCB CFG).
-    basic_coverage_blocks: &'a CoverageGraph,
-
+struct SpansRefiner {
     /// The initial set of coverage spans, sorted by `Span` (`lo` and `hi`) and by relative
     /// dominance between the `BasicCoverageBlock`s of equal `Span`s.
     sorted_spans_iter: std::vec::IntoIter<SpanFromMir>,
@@ -223,36 +192,22 @@ struct SpansRefiner<'a> {
     /// If that `curr` was discarded, `prev` retains its value from the previous iteration.
     some_prev: Option<PrevCovspan>,
 
-    /// One or more coverage spans with the same `Span` but different `BasicCoverageBlock`s, and
-    /// no `BasicCoverageBlock` in this list dominates another `BasicCoverageBlock` in the list.
-    /// If a new `curr` span also fits this criteria (compared to an existing list of
-    /// `pending_dups`), that `curr` moves to `prev` before possibly being added to
-    /// the `pending_dups` list, on the next iteration. As a result, if `prev` and `pending_dups`
-    /// have the same `Span`, the criteria for `pending_dups` holds for `prev` as well: a `prev`
-    /// with a matching `Span` does not dominate any `pending_dup` and no `pending_dup` dominates a
-    /// `prev` with a matching `Span`)
-    pending_dups: Vec<DuplicateCovspan>,
-
     /// The final coverage spans to add to the coverage map. A `Counter` or `Expression`
     /// will also be injected into the MIR for each BCB that has associated spans.
     refined_spans: Vec<RefinedCovspan>,
 }
 
-impl<'a> SpansRefiner<'a> {
+impl SpansRefiner {
     /// Takes the initial list of (sorted) spans extracted from MIR, and "refines"
     /// them by merging compatible adjacent spans, removing redundant spans,
     /// and carving holes in spans when they overlap in unwanted ways.
-    fn refine_sorted_spans(
-        basic_coverage_blocks: &'a CoverageGraph,
-        sorted_spans: Vec<SpanFromMir>,
-    ) -> Vec<RefinedCovspan> {
+    fn refine_sorted_spans(sorted_spans: Vec<SpanFromMir>) -> Vec<RefinedCovspan> {
+        let sorted_spans_len = sorted_spans.len();
         let this = Self {
-            basic_coverage_blocks,
             sorted_spans_iter: sorted_spans.into_iter(),
             some_curr: None,
             some_prev: None,
-            pending_dups: Vec::new(),
-            refined_spans: Vec::with_capacity(basic_coverage_blocks.num_nodes() * 2),
+            refined_spans: Vec::with_capacity(sorted_spans_len),
         };
 
         this.to_refined_spans()
@@ -292,21 +247,11 @@ impl<'a> SpansRefiner<'a> {
                 self.take_curr(); // Discards curr.
             } else if curr.is_closure {
                 self.carve_out_span_for_closure();
-            } else if prev.original_span == prev.span && prev.span == curr.span {
-                // Prev and curr have the same span, and prev's span hasn't
-                // been modified by other spans.
-                self.update_pending_dups();
             } else {
                 self.cutoff_prev_at_overlapping_curr();
             }
         }
 
-        // Drain any remaining dups into the output.
-        for dup in self.pending_dups.drain(..) {
-            debug!("    ...adding at least one pending dup={:?}", dup);
-            self.refined_spans.push(dup.into_refined());
-        }
-
         // There is usually a final span remaining in `prev` after the loop ends,
         // so add it to the output as well.
         if let Some(prev) = self.some_prev.take() {
@@ -359,36 +304,6 @@ impl<'a> SpansRefiner<'a> {
         self.some_prev.take().unwrap_or_else(|| bug!("some_prev is None (take_prev)"))
     }
 
-    /// If there are `pending_dups` but `prev` is not a matching dup (`prev.span` doesn't match the
-    /// `pending_dups` spans), then one of the following two things happened during the previous
-    /// iteration:
-    ///   * the previous `curr` span (which is now `prev`) was not a duplicate of the pending_dups
-    ///     (in which case there should be at least two spans in `pending_dups`); or
-    ///   * the `span` of `prev` was modified by `curr_mut().merge_from(prev)` (in which case
-    ///     `pending_dups` could have as few as one span)
-    /// In either case, no more spans will match the span of `pending_dups`, so
-    /// add the `pending_dups` if they don't overlap `curr`, and clear the list.
-    fn maybe_flush_pending_dups(&mut self) {
-        let Some(last_dup) = self.pending_dups.last() else { return };
-        if last_dup.span == self.prev().span {
-            return;
-        }
-
-        debug!(
-            "    SAME spans, but pending_dups are NOT THE SAME, so BCBs matched on \
-            previous iteration, or prev started a new disjoint span"
-        );
-        if last_dup.span.hi() <= self.curr().span.lo() {
-            for dup in self.pending_dups.drain(..) {
-                debug!("    ...adding at least one pending={:?}", dup);
-                self.refined_spans.push(dup.into_refined());
-            }
-        } else {
-            self.pending_dups.clear();
-        }
-        assert!(self.pending_dups.is_empty());
-    }
-
     /// Advance `prev` to `curr` (if any), and `curr` to the next coverage span in sorted order.
     fn next_coverage_span(&mut self) -> bool {
         if let Some(curr) = self.some_curr.take() {
@@ -408,7 +323,6 @@ impl<'a> SpansRefiner<'a> {
                 );
             } else {
                 self.some_curr = Some(CurrCovspan::new(curr.span, curr.bcb, curr.is_closure));
-                self.maybe_flush_pending_dups();
                 return true;
             }
         }
@@ -433,13 +347,6 @@ impl<'a> SpansRefiner<'a> {
             let mut pre_closure = self.prev().refined_copy();
             pre_closure.span = pre_closure.span.with_hi(left_cutoff);
             debug!("  prev overlaps a closure. Adding span for pre_closure={:?}", pre_closure);
-
-            for mut dup in self.pending_dups.iter().map(DuplicateCovspan::refined_copy) {
-                dup.span = dup.span.with_hi(left_cutoff);
-                debug!("    ...and at least one pre_closure dup={:?}", dup);
-                self.refined_spans.push(dup);
-            }
-
             self.refined_spans.push(pre_closure);
         }
 
@@ -448,58 +355,9 @@ impl<'a> SpansRefiner<'a> {
             self.prev_mut().span = self.prev().span.with_lo(right_cutoff);
             debug!("  Mutated prev.span to start after the closure. prev={:?}", self.prev());
 
-            for dup in &mut self.pending_dups {
-                debug!("    ...and at least one overlapping dup={:?}", dup);
-                dup.span = dup.span.with_lo(right_cutoff);
-            }
-
             // Prevent this curr from becoming prev.
             let closure_covspan = self.take_curr().into_refined();
             self.refined_spans.push(closure_covspan); // since self.prev() was already updated
-        } else {
-            self.pending_dups.clear();
-        }
-    }
-
-    /// Called if `curr.span` equals `prev.original_span` (and potentially equal to all
-    /// `pending_dups` spans, if any). Keep in mind, `prev.span()` may have been changed.
-    /// If prev.span() was merged into other spans (with matching BCB, for instance),
-    /// `prev.span.hi()` will be greater than (further right of) `prev.original_span.hi()`.
-    /// If prev.span() was split off to the right of a closure, prev.span().lo() will be
-    /// greater than prev.original_span.lo(). The actual span of `prev.original_span` is
-    /// not as important as knowing that `prev()` **used to have the same span** as `curr()`,
-    /// which means their sort order is still meaningful for determining the dominator
-    /// relationship.
-    ///
-    /// When two coverage spans have the same `Span`, dominated spans can be discarded; but if
-    /// neither coverage span dominates the other, both (or possibly more than two) are held,
-    /// until their disposition is determined. In this latter case, the `prev` dup is moved into
-    /// `pending_dups` so the new `curr` dup can be moved to `prev` for the next iteration.
-    fn update_pending_dups(&mut self) {
-        let prev_bcb = self.prev().bcb;
-        let curr_bcb = self.curr().bcb;
-
-        // Equal coverage spans are ordered by dominators before dominated (if any), so it should be
-        // impossible for `curr` to dominate any previous coverage span.
-        debug_assert!(!self.basic_coverage_blocks.dominates(curr_bcb, prev_bcb));
-
-        // `prev` is a duplicate of `curr`, so add it to the list of pending dups.
-        // If it dominates `curr`, it will be removed by the subsequent discard step.
-        let prev = self.take_prev().into_dup();
-        debug!(?prev, "adding prev to pending dups");
-        self.pending_dups.push(prev);
-
-        let initial_pending_count = self.pending_dups.len();
-        if initial_pending_count > 0 {
-            self.pending_dups
-                .retain(|dup| !self.basic_coverage_blocks.dominates(dup.bcb, curr_bcb));
-
-            let n_discarded = initial_pending_count - self.pending_dups.len();
-            if n_discarded > 0 {
-                debug!(
-                    "  discarded {n_discarded} of {initial_pending_count} pending_dups that dominated curr",
-                );
-            }
         }
     }
 
@@ -516,19 +374,13 @@ impl<'a> SpansRefiner<'a> {
             if it has statements that end before curr; prev={:?}",
             self.prev()
         );
-        if self.pending_dups.is_empty() {
-            let curr_span = self.curr().span;
-            self.prev_mut().cutoff_statements_at(curr_span.lo());
-            if self.prev().merged_spans.is_empty() {
-                debug!("  ... no non-overlapping statements to add");
-            } else {
-                debug!("  ... adding modified prev={:?}", self.prev());
-                let prev = self.take_prev().into_refined();
-                self.refined_spans.push(prev);
-            }
+
+        let curr_span = self.curr().span;
+        if let Some(prev) = self.take_prev().cutoff_statements_at(curr_span.lo()) {
+            debug!("after cutoff, adding {prev:?}");
+            self.refined_spans.push(prev);
         } else {
-            // with `pending_dups`, `prev` cannot have any statements that don't overlap
-            self.pending_dups.clear();
+            debug!("prev was eliminated by cutoff");
         }
     }
 }

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

@@ -52,14 +52,19 @@ pub(super) fn mir_to_initial_sorted_coverage_spans(
             // - Span A extends further left, or
             // - Both have the same start and span A extends further right
             .then_with(|| Ord::cmp(&a.span.hi(), &b.span.hi()).reverse())
-            // If both spans are equal, sort the BCBs in dominator order,
-            // so that dominating BCBs come before other BCBs they dominate.
-            .then_with(|| basic_coverage_blocks.cmp_in_dominator_order(a.bcb, b.bcb))
-            // If two spans are otherwise identical, put closure spans first,
-            // as this seems to be what the refinement step expects.
+            // If two spans have the same lo & hi, put closure spans first,
+            // as they take precedence over non-closure spans.
             .then_with(|| Ord::cmp(&a.is_closure, &b.is_closure).reverse())
+            // After deduplication, we want to keep only the most-dominated BCB.
+            .then_with(|| basic_coverage_blocks.cmp_in_dominator_order(a.bcb, b.bcb).reverse())
     });
 
+    // Among covspans with the same span, keep only one. Closure spans take
+    // precedence, otherwise keep the one with the most-dominated BCB.
+    // (Ideally we should try to preserve _all_ non-dominating BCBs, but that
+    // requires a lot more complexity in the span refiner, for little benefit.)
+    initial_spans.dedup_by(|b, a| a.span.source_equal(b.span));
+
     initial_spans
 }
 

tests/coverage/closure_macro.cov-map

@@ -7,18 +7,17 @@ Number of file 0 mappings: 1
 - Code(Counter(0)) at (prev + 29, 1) to (start + 2, 2)
 
 Function name: closure_macro::main
-Raw bytes (43): 0x[01, 01, 02, 01, 05, 05, 02, 07, 01, 21, 01, 01, 21, 02, 02, 09, 00, 0f, 05, 00, 12, 00, 13, 02, 00, 12, 00, 13, 05, 00, 54, 00, 55, 02, 02, 09, 02, 0b, 07, 03, 01, 00, 02]
+Raw bytes (38): 0x[01, 01, 02, 01, 05, 05, 02, 06, 01, 21, 01, 01, 21, 02, 02, 09, 00, 12, 02, 00, 0f, 00, 54, 05, 00, 54, 00, 55, 02, 02, 09, 02, 0b, 07, 03, 01, 00, 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: 7
+Number of file 0 mappings: 6
 - Code(Counter(0)) at (prev + 33, 1) to (start + 1, 33)
-- Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 0, 15)
+- Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 0, 18)
     = (c0 - c1)
-- Code(Counter(1)) at (prev + 0, 18) to (start + 0, 19)
-- Code(Expression(0, Sub)) at (prev + 0, 18) to (start + 0, 19)
+- Code(Expression(0, Sub)) at (prev + 0, 15) to (start + 0, 84)
     = (c0 - c1)
 - Code(Counter(1)) at (prev + 0, 84) to (start + 0, 85)
 - Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 2, 11)

tests/coverage/closure_macro_async.cov-map

@@ -15,18 +15,17 @@ Number of file 0 mappings: 1
 - Code(Counter(0)) at (prev + 35, 1) to (start + 0, 43)
 
 Function name: closure_macro_async::test::{closure#0}
-Raw bytes (43): 0x[01, 01, 02, 01, 05, 05, 02, 07, 01, 23, 2b, 01, 21, 02, 02, 09, 00, 0f, 05, 00, 12, 00, 13, 02, 00, 12, 00, 13, 05, 00, 54, 00, 55, 02, 02, 09, 02, 0b, 07, 03, 01, 00, 02]
+Raw bytes (38): 0x[01, 01, 02, 01, 05, 05, 02, 06, 01, 23, 2b, 01, 21, 02, 02, 09, 00, 12, 02, 00, 0f, 00, 54, 05, 00, 54, 00, 55, 02, 02, 09, 02, 0b, 07, 03, 01, 00, 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: 7
+Number of file 0 mappings: 6
 - Code(Counter(0)) at (prev + 35, 43) to (start + 1, 33)
-- Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 0, 15)
+- Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 0, 18)
     = (c0 - c1)
-- Code(Counter(1)) at (prev + 0, 18) to (start + 0, 19)
-- Code(Expression(0, Sub)) at (prev + 0, 18) to (start + 0, 19)
+- Code(Expression(0, Sub)) at (prev + 0, 15) to (start + 0, 84)
     = (c0 - c1)
 - Code(Counter(1)) at (prev + 0, 84) to (start + 0, 85)
 - Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 2, 11)