From 412c86cf033de5ad8e74b600892f3b9dea98a057 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 13 Feb 2024 12:27:31 +1100 Subject: [PATCH 1/5] coverage: When merging spans, keep `prev` and merge `curr` into it Swapping the direction of this merge produces the same results, but means that we never need to mutate `curr`. --- compiler/rustc_mir_transform/src/coverage/spans.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index d3d0c7bcc9501..a2ecf3d13ce92 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -221,11 +221,10 @@ impl<'a> SpansRefiner<'a> { let prev = self.prev(); let curr = self.curr(); - if curr.is_mergeable(prev) { + if prev.is_mergeable(curr) { debug!(" same bcb (and neither is a closure), merge with prev={prev:?}"); - let prev = self.take_prev(); - self.curr_mut().merge_from(&prev); - // Note that curr.span may now differ from curr_original_span + let curr = self.take_curr(); + self.prev_mut().merge_from(&curr); } else if prev.span.hi() <= curr.span.lo() { debug!( " different bcbs and disjoint spans, so keep curr for next iter, and add prev={prev:?}", @@ -286,11 +285,6 @@ impl<'a> SpansRefiner<'a> { self.some_curr.as_ref().unwrap_or_else(|| bug!("some_curr is None (curr)")) } - #[track_caller] - fn curr_mut(&mut self) -> &mut CoverageSpan { - self.some_curr.as_mut().unwrap_or_else(|| bug!("some_curr is None (curr_mut)")) - } - /// If called, then the next call to `next_coverage_span()` will *not* update `prev` with the /// `curr` coverage span. #[track_caller] From 5a569b1b8037f96ddaa9944547f1c3c1495034a1 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 13 Feb 2024 12:42:30 +1100 Subject: [PATCH 2/5] coverage: Don't track `curr_original_span` explicitly Now that we never mutate `curr.span`, we don't need to store its original span separately. --- compiler/rustc_mir_transform/src/coverage/spans.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index a2ecf3d13ce92..e2aee52cc214b 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -155,16 +155,11 @@ struct SpansRefiner<'a> { /// iteration. some_curr: Option, - /// The original `span` for `curr`, in case `curr.span()` is modified. The `curr_original_span` - /// **must not be mutated** (except when advancing to the next `curr`), even if `curr.span()` - /// is mutated. - curr_original_span: Span, - /// The CoverageSpan from a prior iteration; typically assigned from that iteration's `curr`. /// If that `curr` was discarded, `prev` retains its value from the previous iteration. some_prev: Option, - /// Assigned from `curr_original_span` from the previous iteration. The `prev_original_span` + /// Assigned from `curr.span` from the previous iteration. The `prev_original_span` /// **must not be mutated** (except when advancing to the next `prev`), even if `prev.span()` /// is mutated. prev_original_span: Span, @@ -196,7 +191,6 @@ impl<'a> SpansRefiner<'a> { basic_coverage_blocks, sorted_spans_iter: sorted_spans.into_iter(), some_curr: None, - curr_original_span: DUMMY_SP, some_prev: None, prev_original_span: DUMMY_SP, pending_dups: Vec::new(), @@ -340,8 +334,8 @@ impl<'a> SpansRefiner<'a> { /// Advance `prev` to `curr` (if any), and `curr` to the next `CoverageSpan` in sorted order. fn next_coverage_span(&mut self) -> bool { if let Some(curr) = self.some_curr.take() { + self.prev_original_span = curr.span; self.some_prev = Some(curr); - self.prev_original_span = self.curr_original_span; } while let Some(curr) = self.sorted_spans_iter.next() { debug!("FOR curr={:?}", curr); @@ -356,9 +350,6 @@ impl<'a> SpansRefiner<'a> { closure?); prev={prev:?}", ); } else { - // Save a copy of the original span for `curr` in case the `CoverageSpan` is changed - // by `self.curr_mut().merge_from(prev)`. - self.curr_original_span = curr.span; self.some_curr.replace(curr); self.maybe_flush_pending_dups(); return true; From a6183216d896e83feb89545d134902e801585042 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 10 Feb 2024 18:31:00 +1100 Subject: [PATCH 3/5] coverage: Split `CoverageSpan` into several distinct structs This requires some extra boilerplate, but in exchange it becomes much easier to see how each field and method is actually used. --- .../rustc_mir_transform/src/coverage/spans.rs | 198 ++++++++++++------ .../src/coverage/spans/from_mir.rs | 24 +-- 2 files changed, 139 insertions(+), 83 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index e2aee52cc214b..da08e9d9f9f4e 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -4,6 +4,7 @@ use rustc_middle::mir; use rustc_span::{BytePos, Span, DUMMY_SP}; use crate::coverage::graph::{BasicCoverageBlock, CoverageGraph, START_BCB}; +use crate::coverage::spans::from_mir::SpanFromMir; use crate::coverage::ExtractedHirInfo; mod from_mir; @@ -61,7 +62,7 @@ pub(super) fn generate_coverage_spans( basic_coverage_blocks, ); let coverage_spans = SpansRefiner::refine_sorted_spans(basic_coverage_blocks, sorted_spans); - mappings.extend(coverage_spans.into_iter().map(|CoverageSpan { bcb, span, .. }| { + 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 } })); @@ -85,18 +86,35 @@ pub(super) fn generate_coverage_spans( Some(CoverageSpans { bcb_has_mappings, mappings }) } -/// A BCB is deconstructed into one or more `Span`s. Each `Span` maps to a `CoverageSpan` that -/// references the originating BCB and one or more MIR `Statement`s and/or `Terminator`s. -/// Initially, the `Span`s come from the `Statement`s and `Terminator`s, but subsequent -/// transforms can combine adjacent `Span`s and `CoverageSpan` from the same BCB, merging the -/// `merged_spans` vectors, and the `Span`s to cover the extent of the combined `Span`s. -/// -/// Note: A span merged into another CoverageSpan may come from a `BasicBlock` that -/// is not part of the `CoverageSpan` bcb if the statement was included because it's `Span` matches -/// or is subsumed by the `Span` associated with this `CoverageSpan`, and it's `BasicBlock` -/// `dominates()` the `BasicBlock`s in this `CoverageSpan`. -#[derive(Debug, Clone)] -struct CoverageSpan { +#[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, +} + +impl CurrCovspan { + fn new(span: Span, bcb: BasicCoverageBlock, is_closure: bool) -> Self { + Self { span, bcb, is_closure } + } + + fn into_prev(self) -> PrevCovspan { + let Self { span, bcb, is_closure } = self; + PrevCovspan { span, bcb, merged_spans: vec![span], is_closure } + } + + fn into_refined(self) -> RefinedCovspan { + // This is only called in cases where `curr` is a closure span that has + // been carved out of `prev`. + debug_assert!(self.is_closure); + self.into_prev().into_refined() + } +} + +#[derive(Debug)] +struct PrevCovspan { span: Span, bcb: BasicCoverageBlock, /// List of all the original spans from MIR that have been merged into this @@ -105,37 +123,80 @@ struct CoverageSpan { is_closure: bool, } -impl CoverageSpan { - fn new(span: Span, bcb: BasicCoverageBlock, is_closure: bool) -> Self { - Self { span, bcb, merged_spans: vec![span], is_closure } +impl PrevCovspan { + fn is_mergeable(&self, other: &CurrCovspan) -> bool { + self.bcb == other.bcb && !self.is_closure && !other.is_closure } - pub fn merge_from(&mut self, other: &Self) { + fn merge_from(&mut self, other: &CurrCovspan) { debug_assert!(self.is_mergeable(other)); self.span = self.span.to(other.span); - self.merged_spans.extend_from_slice(&other.merged_spans); + self.merged_spans.push(other.span); } - pub fn cutoff_statements_at(&mut self, cutoff_pos: BytePos) { + fn cutoff_statements_at(&mut self, cutoff_pos: BytePos) { 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); } } - #[inline] - pub fn is_mergeable(&self, other: &Self) -> bool { - self.is_in_same_bcb(other) && !(self.is_closure || other.is_closure) + fn into_dup(self) -> DuplicateCovspan { + let Self { span, bcb, merged_spans: _, is_closure } = self; + DuplicateCovspan { span, bcb, is_closure } + } + + fn refined_copy(&self) -> RefinedCovspan { + let &Self { span, bcb, merged_spans: _, is_closure } = self; + RefinedCovspan { span, bcb, is_closure } + } + + fn into_refined(self) -> RefinedCovspan { + self.refined_copy() + } +} + +#[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; + RefinedCovspan { span, bcb, is_closure } + } + + fn into_refined(self) -> RefinedCovspan { + // Even though we consume self, we can just reuse the copying impl. + self.refined_copy() + } +} + +#[derive(Debug)] +struct RefinedCovspan { + span: Span, + bcb: BasicCoverageBlock, + is_closure: bool, +} + +impl RefinedCovspan { + fn is_mergeable(&self, other: &Self) -> bool { + self.bcb == other.bcb && !self.is_closure && !other.is_closure } - #[inline] - pub fn is_in_same_bcb(&self, other: &Self) -> bool { - self.bcb == other.bcb + fn merge_from(&mut self, other: &Self) { + debug_assert!(self.is_mergeable(other)); + self.span = self.span.to(other.span); } } -/// Converts the initial set of `CoverageSpan`s (one per MIR `Statement` or `Terminator`) into a -/// minimal set of `CoverageSpan`s, using the BCB CFG to determine where it is safe and useful to: +/// Converts the initial set of coverage spans (one per MIR `Statement` or `Terminator`) into a +/// minimal set of coverage spans, using the BCB CFG to determine where it is safe and useful to: /// /// * Remove duplicate source code coverage regions /// * Merge spans that represent continuous (both in source code and control flow), non-branching @@ -145,38 +206,38 @@ struct SpansRefiner<'a> { /// The BasicCoverageBlock Control Flow Graph (BCB CFG). basic_coverage_blocks: &'a CoverageGraph, - /// The initial set of `CoverageSpan`s, sorted by `Span` (`lo` and `hi`) and by relative + /// 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, + sorted_spans_iter: std::vec::IntoIter, - /// The current `CoverageSpan` to compare to its `prev`, to possibly merge, discard, force the + /// The current coverage span to compare to its `prev`, to possibly merge, discard, force the /// discard of the `prev` (and or `pending_dups`), or keep both (with `prev` moved to /// `pending_dups`). If `curr` is not discarded or merged, it becomes `prev` for the next /// iteration. - some_curr: Option, + some_curr: Option, - /// The CoverageSpan from a prior iteration; typically assigned from that iteration's `curr`. + /// The coverage span from a prior iteration; typically assigned from that iteration's `curr`. /// If that `curr` was discarded, `prev` retains its value from the previous iteration. - some_prev: Option, + some_prev: Option, /// Assigned from `curr.span` from the previous iteration. The `prev_original_span` /// **must not be mutated** (except when advancing to the next `prev`), even if `prev.span()` /// is mutated. prev_original_span: Span, - /// One or more `CoverageSpan`s with the same `Span` but different `BasicCoverageBlock`s, and + /// 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` `CoverageSpan` moves to `prev` before possibly being added to + /// `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, + pending_dups: Vec, - /// The final `CoverageSpan`s to add to the coverage map. A `Counter` or `Expression` - /// will also be injected into the MIR for each `CoverageSpan`. - refined_spans: Vec, + /// 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, } impl<'a> SpansRefiner<'a> { @@ -185,8 +246,8 @@ impl<'a> SpansRefiner<'a> { /// and carving holes in spans when they overlap in unwanted ways. fn refine_sorted_spans( basic_coverage_blocks: &'a CoverageGraph, - sorted_spans: Vec, - ) -> Vec { + sorted_spans: Vec, + ) -> Vec { let this = Self { basic_coverage_blocks, sorted_spans_iter: sorted_spans.into_iter(), @@ -200,9 +261,9 @@ impl<'a> SpansRefiner<'a> { this.to_refined_spans() } - /// Iterate through the sorted `CoverageSpan`s, and return the refined list of merged and - /// de-duplicated `CoverageSpan`s. - fn to_refined_spans(mut self) -> Vec { + /// Iterate through the sorted coverage spans, and return the refined list of merged and + /// de-duplicated spans. + fn to_refined_spans(mut self) -> Vec { while self.next_coverage_span() { // For the first span we don't have `prev` set, so most of the // span-processing steps don't make sense yet. @@ -223,7 +284,7 @@ impl<'a> SpansRefiner<'a> { debug!( " different bcbs and disjoint spans, so keep curr for next iter, and add prev={prev:?}", ); - let prev = self.take_prev(); + let prev = self.take_prev().into_refined(); self.refined_spans.push(prev); } else if prev.is_closure { // drop any equal or overlapping span (`curr`) and keep `prev` to test again in the @@ -246,14 +307,14 @@ impl<'a> SpansRefiner<'a> { // 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); + 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() { debug!(" AT END, adding last prev={prev:?}"); - self.refined_spans.push(prev); + self.refined_spans.push(prev.into_refined()); } // Do one last merge pass, to simplify the output. @@ -267,7 +328,7 @@ impl<'a> SpansRefiner<'a> { } }); - // Remove `CoverageSpan`s derived from closures, originally added to ensure the coverage + // Remove spans derived from closures, originally added to ensure the coverage // regions for the current function leave room for the closure's own coverage regions // (injected separately, from the closure's own MIR). self.refined_spans.retain(|covspan| !covspan.is_closure); @@ -275,29 +336,29 @@ impl<'a> SpansRefiner<'a> { } #[track_caller] - fn curr(&self) -> &CoverageSpan { + fn curr(&self) -> &CurrCovspan { self.some_curr.as_ref().unwrap_or_else(|| bug!("some_curr is None (curr)")) } /// If called, then the next call to `next_coverage_span()` will *not* update `prev` with the /// `curr` coverage span. #[track_caller] - fn take_curr(&mut self) -> CoverageSpan { + fn take_curr(&mut self) -> CurrCovspan { self.some_curr.take().unwrap_or_else(|| bug!("some_curr is None (take_curr)")) } #[track_caller] - fn prev(&self) -> &CoverageSpan { + fn prev(&self) -> &PrevCovspan { self.some_prev.as_ref().unwrap_or_else(|| bug!("some_prev is None (prev)")) } #[track_caller] - fn prev_mut(&mut self) -> &mut CoverageSpan { + fn prev_mut(&mut self) -> &mut PrevCovspan { self.some_prev.as_mut().unwrap_or_else(|| bug!("some_prev is None (prev_mut)")) } #[track_caller] - fn take_prev(&mut self) -> CoverageSpan { + fn take_prev(&mut self) -> PrevCovspan { self.some_prev.take().unwrap_or_else(|| bug!("some_prev is None (take_prev)")) } @@ -323,7 +384,7 @@ impl<'a> SpansRefiner<'a> { 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); + self.refined_spans.push(dup.into_refined()); } } else { self.pending_dups.clear(); @@ -331,11 +392,11 @@ impl<'a> SpansRefiner<'a> { assert!(self.pending_dups.is_empty()); } - /// Advance `prev` to `curr` (if any), and `curr` to the next `CoverageSpan` in sorted order. + /// 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() { self.prev_original_span = curr.span; - self.some_prev = Some(curr); + self.some_prev = Some(curr.into_prev()); } while let Some(curr) = self.sorted_spans_iter.next() { debug!("FOR curr={:?}", curr); @@ -350,7 +411,7 @@ impl<'a> SpansRefiner<'a> { closure?); prev={prev:?}", ); } else { - self.some_curr.replace(curr); + self.some_curr = Some(CurrCovspan::new(curr.span, curr.bcb, curr.is_closure)); self.maybe_flush_pending_dups(); return true; } @@ -373,11 +434,11 @@ impl<'a> SpansRefiner<'a> { let has_post_closure_span = prev.span.hi() > right_cutoff; if has_pre_closure_span { - let mut pre_closure = self.prev().clone(); + 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().cloned() { + 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); @@ -389,7 +450,7 @@ impl<'a> SpansRefiner<'a> { if has_post_closure_span { // Mutate `prev.span()` to start after the closure (and discard curr). // (**NEVER** update `prev_original_span` because it affects the assumptions - // about how the `CoverageSpan`s are ordered.) + // about how the coverage spans are ordered.) self.prev_mut().span = self.prev().span.with_lo(right_cutoff); debug!(" Mutated prev.span to start after the closure. prev={:?}", self.prev()); @@ -398,7 +459,8 @@ impl<'a> SpansRefiner<'a> { dup.span = dup.span.with_lo(right_cutoff); } - let closure_covspan = self.take_curr(); // Prevent this curr from becoming prev. + // 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(); @@ -415,8 +477,8 @@ impl<'a> SpansRefiner<'a> { /// which means their sort order is still meaningful for determining the dominator /// relationship. /// - /// When two `CoverageSpan`s have the same `Span`, dominated spans can be discarded; but if - /// neither `CoverageSpan` dominates the other, both (or possibly more than two) are held, + /// 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) { @@ -424,7 +486,7 @@ impl<'a> SpansRefiner<'a> { 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 `CoverageSpan`. + // impossible for `curr` to dominate any previous coverage span. debug_assert!(!self.basic_coverage_blocks.dominates(curr_bcb, prev_bcb)); let initial_pending_count = self.pending_dups.len(); @@ -448,7 +510,7 @@ impl<'a> SpansRefiner<'a> { self.cutoff_prev_at_overlapping_curr(); // If one span dominates the other, associate the span with the code from the dominated // block only (`curr`), and discard the overlapping portion of the `prev` span. (Note - // that if `prev.span` is wider than `prev_original_span`, a `CoverageSpan` will still + // that if `prev.span` is wider than `prev_original_span`, a coverage span will still // be created for `prev`s block, for the non-overlapping portion, left of `curr.span`.) // // For example: @@ -465,14 +527,14 @@ impl<'a> SpansRefiner<'a> { // an `Err`, and not counted as covered if the function always returns `Ok`. } else { // Save `prev` in `pending_dups`. (`curr` will become `prev` in the next iteration.) - // If the `curr` CoverageSpan is later discarded, `pending_dups` can be discarded as + // If the `curr` span is later discarded, `pending_dups` can be discarded as // well; but if `curr` is added to refined_spans, the `pending_dups` will also be added. debug!( " different bcbs but SAME spans, and neither dominates, so keep curr for \ next iter, and, pending upcoming spans (unless overlapping) add prev={:?}", self.prev() ); - let prev = self.take_prev(); + let prev = self.take_prev().into_dup(); self.pending_dups.push(prev); } } @@ -497,7 +559,7 @@ impl<'a> SpansRefiner<'a> { debug!(" ... no non-overlapping statements to add"); } else { debug!(" ... adding modified prev={:?}", self.prev()); - let prev = self.take_prev(); + let prev = self.take_prev().into_refined(); self.refined_spans.push(prev); } } else { diff --git a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs index 4ac8dde03a6cf..9517ede288f5d 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -9,7 +9,6 @@ use rustc_span::{ExpnKind, MacroKind, Span, Symbol}; use crate::coverage::graph::{ BasicCoverageBlock, BasicCoverageBlockData, CoverageGraph, START_BCB, }; -use crate::coverage::spans::CoverageSpan; use crate::coverage::ExtractedHirInfo; /// Traverses the MIR body to produce an initial collection of coverage-relevant @@ -22,7 +21,7 @@ pub(super) fn mir_to_initial_sorted_coverage_spans( mir_body: &mir::Body<'_>, hir_info: &ExtractedHirInfo, basic_coverage_blocks: &CoverageGraph, -) -> Vec { +) -> Vec { let &ExtractedHirInfo { body_span, .. } = hir_info; let mut initial_spans = vec![]; @@ -61,7 +60,7 @@ pub(super) fn mir_to_initial_sorted_coverage_spans( .then_with(|| Ord::cmp(&a.is_closure, &b.is_closure).reverse()) }); - initial_spans.into_iter().map(SpanFromMir::into_coverage_span).collect::>() + initial_spans } /// Macros that expand into branches (e.g. `assert!`, `trace!`) tend to generate @@ -119,10 +118,10 @@ fn split_visible_macro_spans(initial_spans: &mut Vec) { initial_spans.extend(extra_spans); } -// 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 +// Generate a set of coverage spans from the filtered set of `Statement`s and `Terminator`s of +// the `BasicBlock`(s) in the given `BasicCoverageBlockData`. One coverage span is generated // for each `Statement` and `Terminator`. (Note that subsequent stages of coverage analysis will -// merge some `CoverageSpan`s, at which point a `CoverageSpan` may represent multiple +// merge some coverage spans, at which point a coverage span may represent multiple // `Statement`s and/or `Terminator`s.) fn bcb_to_initial_coverage_spans<'a, 'tcx>( mir_body: &'a mir::Body<'tcx>, @@ -316,7 +315,7 @@ fn unexpand_into_body_span_with_prev( } #[derive(Debug)] -struct SpanFromMir { +pub(super) struct SpanFromMir { /// A span that has been extracted from MIR and then "un-expanded" back to /// within the current function's `body_span`. After various intermediate /// processing steps, this span is emitted as part of the final coverage @@ -324,10 +323,10 @@ struct SpanFromMir { /// /// With the exception of `fn_sig_span`, this should always be contained /// within `body_span`. - span: Span, + pub(super) span: Span, visible_macro: Option, - bcb: BasicCoverageBlock, - is_closure: bool, + pub(super) bcb: BasicCoverageBlock, + pub(super) is_closure: bool, } impl SpanFromMir { @@ -343,9 +342,4 @@ impl SpanFromMir { ) -> Self { Self { span, visible_macro, bcb, is_closure } } - - fn into_coverage_span(self) -> CoverageSpan { - let Self { span, visible_macro: _, bcb, is_closure } = self; - CoverageSpan::new(span, bcb, is_closure) - } } From 499609d8a4135bd69de7fb578d9179a82e4befd0 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 13 Feb 2024 12:54:26 +1100 Subject: [PATCH 4/5] coverage: Move `prev_original_span` into `PrevCovspan` Now that `prev` has its own dedicated struct, we can store the original span in that struct, instead of in a separate field in the refiner. --- .../rustc_mir_transform/src/coverage/spans.rs | 30 +++++++------------ 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index da08e9d9f9f4e..c1c991f2374d2 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -1,7 +1,7 @@ use rustc_data_structures::graph::WithNumNodes; use rustc_index::bit_set::BitSet; use rustc_middle::mir; -use rustc_span::{BytePos, Span, DUMMY_SP}; +use rustc_span::{BytePos, Span}; use crate::coverage::graph::{BasicCoverageBlock, CoverageGraph, START_BCB}; use crate::coverage::spans::from_mir::SpanFromMir; @@ -102,7 +102,7 @@ impl CurrCovspan { fn into_prev(self) -> PrevCovspan { let Self { span, bcb, is_closure } = self; - PrevCovspan { span, bcb, merged_spans: vec![span], is_closure } + PrevCovspan { original_span: span, span, bcb, merged_spans: vec![span], is_closure } } fn into_refined(self) -> RefinedCovspan { @@ -115,6 +115,7 @@ 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 @@ -142,12 +143,12 @@ impl PrevCovspan { } fn into_dup(self) -> DuplicateCovspan { - let Self { span, bcb, merged_spans: _, is_closure } = self; + let Self { original_span: _, span, bcb, merged_spans: _, is_closure } = self; DuplicateCovspan { span, bcb, is_closure } } fn refined_copy(&self) -> RefinedCovspan { - let &Self { span, bcb, merged_spans: _, is_closure } = self; + let &Self { original_span: _, span, bcb, merged_spans: _, is_closure } = self; RefinedCovspan { span, bcb, is_closure } } @@ -220,11 +221,6 @@ struct SpansRefiner<'a> { /// If that `curr` was discarded, `prev` retains its value from the previous iteration. some_prev: Option, - /// Assigned from `curr.span` from the previous iteration. The `prev_original_span` - /// **must not be mutated** (except when advancing to the next `prev`), even if `prev.span()` - /// is mutated. - prev_original_span: Span, - /// 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 @@ -253,7 +249,6 @@ impl<'a> SpansRefiner<'a> { sorted_spans_iter: sorted_spans.into_iter(), some_curr: None, some_prev: None, - prev_original_span: DUMMY_SP, pending_dups: Vec::new(), refined_spans: Vec::with_capacity(basic_coverage_blocks.num_nodes() * 2), }; @@ -295,7 +290,7 @@ impl<'a> SpansRefiner<'a> { self.take_curr(); // Discards curr. } else if curr.is_closure { self.carve_out_span_for_closure(); - } else if self.prev_original_span == curr.span { + } else if prev.original_span == curr.span { // `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(); @@ -395,7 +390,6 @@ impl<'a> SpansRefiner<'a> { /// 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() { - self.prev_original_span = curr.span; self.some_prev = Some(curr.into_prev()); } while let Some(curr) = self.sorted_spans_iter.next() { @@ -448,9 +442,7 @@ impl<'a> SpansRefiner<'a> { } if has_post_closure_span { - // Mutate `prev.span()` to start after the closure (and discard curr). - // (**NEVER** update `prev_original_span` because it affects the assumptions - // about how the coverage spans are ordered.) + // Mutate `prev.span` to start after the closure (and discard curr). self.prev_mut().span = self.prev().span.with_lo(right_cutoff); debug!(" Mutated prev.span to start after the closure. prev={:?}", self.prev()); @@ -467,12 +459,12 @@ impl<'a> SpansRefiner<'a> { } } - /// Called if `curr.span` equals `prev_original_span` (and potentially equal to all + /// 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()`. + /// `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 + /// 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. @@ -510,7 +502,7 @@ impl<'a> SpansRefiner<'a> { self.cutoff_prev_at_overlapping_curr(); // If one span dominates the other, associate the span with the code from the dominated // block only (`curr`), and discard the overlapping portion of the `prev` span. (Note - // that if `prev.span` is wider than `prev_original_span`, a coverage span will still + // that if `prev.span` is wider than `prev.original_span`, a coverage span will still // be created for `prev`s block, for the non-overlapping portion, left of `curr.span`.) // // For example: From e67db4c3b898a03a664d680f9d326f456169eb8f Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 13 Feb 2024 14:48:03 +1100 Subject: [PATCH 5/5] coverage: Simplify code for adding `prev` to pending dups If we only check for duplicate spans when `prev` is unmodified, we reduce the number of situations that `update_pending_dups` needs to handle. This could potentially change the coverage spans we produce in some unknown corner cases, but none of our current coverage tests indicate any change. --- .../rustc_mir_transform/src/coverage/spans.rs | 52 +++++-------------- 1 file changed, 12 insertions(+), 40 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index c1c991f2374d2..934e77e7deb0a 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -143,7 +143,9 @@ impl PrevCovspan { } fn into_dup(self) -> DuplicateCovspan { - let Self { original_span: _, span, bcb, merged_spans: _, is_closure } = self; + 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 } } @@ -290,9 +292,9 @@ 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 == curr.span { - // `prev` and `curr` have the same span, or would have had the - // same span before `prev` was modified by other spans. + } 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(); @@ -481,6 +483,12 @@ impl<'a> SpansRefiner<'a> { // 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 @@ -493,42 +501,6 @@ impl<'a> SpansRefiner<'a> { ); } } - - if self.basic_coverage_blocks.dominates(prev_bcb, curr_bcb) { - debug!( - " different bcbs but SAME spans, and prev dominates curr. Discard prev={:?}", - self.prev() - ); - self.cutoff_prev_at_overlapping_curr(); - // If one span dominates the other, associate the span with the code from the dominated - // block only (`curr`), and discard the overlapping portion of the `prev` span. (Note - // that if `prev.span` is wider than `prev.original_span`, a coverage span will still - // be created for `prev`s block, for the non-overlapping portion, left of `curr.span`.) - // - // For example: - // match somenum { - // x if x < 1 => { ... } - // }... - // - // The span for the first `x` is referenced by both the pattern block (every time it is - // evaluated) and the arm code (only when matched). The counter will be applied only to - // the dominated block. This allows coverage to track and highlight things like the - // assignment of `x` above, if the branch is matched, making `x` available to the arm - // code; and to track and highlight the question mark `?` "try" operator at the end of - // a function call returning a `Result`, so the `?` is covered when the function returns - // an `Err`, and not counted as covered if the function always returns `Ok`. - } else { - // Save `prev` in `pending_dups`. (`curr` will become `prev` in the next iteration.) - // If the `curr` span is later discarded, `pending_dups` can be discarded as - // well; but if `curr` is added to refined_spans, the `pending_dups` will also be added. - debug!( - " different bcbs but SAME spans, and neither dominates, so keep curr for \ - next iter, and, pending upcoming spans (unless overlapping) add prev={:?}", - self.prev() - ); - let prev = self.take_prev().into_dup(); - self.pending_dups.push(prev); - } } /// `curr` overlaps `prev`. If `prev`s span extends left of `curr`s span, keep _only_