Skip to content

Commit

Permalink
Rollup merge of rust-lang#121492 - Zalathar:hole, r=fmease
Browse files Browse the repository at this point in the history 
coverage: Rename `is_closure` to `is_hole`

Extracted from rust-lang#121433, since I was having second thoughts about some of the other changes bundled in that PR, but these changes are still fine.

---

When refining covspans, we don't specifically care which ones represent closures; we just want to know which ones represent "holes" that should be carved out of other spans and then discarded.

(Closures are currently the only source of hole spans, but in the future we might want to also create hole spans for nested items and inactive `#[cfg(..)]` regions.)

``@rustbot`` label +A-code-coverage
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Feb 23, 2024
2 parents 4d5c4e5 + 44c8f55 commit 13b9bf5
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 65 deletions.
100 changes: 46 additions & 54 deletions compiler/rustc_mir_transform/src/coverage/spans.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,23 +90,23 @@ pub(super) fn generate_coverage_spans(
struct CurrCovspan {
span: Span,
bcb: BasicCoverageBlock,
is_closure: bool,
is_hole: bool,
}

impl CurrCovspan {
fn new(span: Span, bcb: BasicCoverageBlock, is_closure: bool) -> Self {
Self { span, bcb, is_closure }
fn new(span: Span, bcb: BasicCoverageBlock, is_hole: bool) -> Self {
Self { span, bcb, is_hole }
}

fn into_prev(self) -> PrevCovspan {
let Self { span, bcb, is_closure } = self;
PrevCovspan { span, bcb, merged_spans: vec![span], is_closure }
let Self { span, bcb, is_hole } = self;
PrevCovspan { span, bcb, merged_spans: vec![span], is_hole }
}

fn into_refined(self) -> RefinedCovspan {
// This is only called in cases where `curr` is a closure span that has
// This is only called in cases where `curr` is a hole span that has
// been carved out of `prev`.
debug_assert!(self.is_closure);
debug_assert!(self.is_hole);
self.into_prev().into_refined()
}
}
Expand All @@ -118,12 +118,12 @@ struct PrevCovspan {
/// List of all the original spans from MIR that have been merged into this
/// span. Mainly used to precisely skip over gaps when truncating a span.
merged_spans: Vec<Span>,
is_closure: bool,
is_hole: bool,
}

impl PrevCovspan {
fn is_mergeable(&self, other: &CurrCovspan) -> bool {
self.bcb == other.bcb && !self.is_closure && !other.is_closure
self.bcb == other.bcb && !self.is_hole && !other.is_hole
}

fn merge_from(&mut self, other: &CurrCovspan) {
Expand All @@ -142,8 +142,8 @@ impl PrevCovspan {
}

fn refined_copy(&self) -> RefinedCovspan {
let &Self { span, bcb, merged_spans: _, is_closure } = self;
RefinedCovspan { span, bcb, is_closure }
let &Self { span, bcb, merged_spans: _, is_hole } = self;
RefinedCovspan { span, bcb, is_hole }
}

fn into_refined(self) -> RefinedCovspan {
Expand All @@ -156,12 +156,12 @@ impl PrevCovspan {
struct RefinedCovspan {
span: Span,
bcb: BasicCoverageBlock,
is_closure: bool,
is_hole: bool,
}

impl RefinedCovspan {
fn is_mergeable(&self, other: &Self) -> bool {
self.bcb == other.bcb && !self.is_closure && !other.is_closure
self.bcb == other.bcb && !self.is_hole && !other.is_hole
}

fn merge_from(&mut self, other: &Self) {
Expand All @@ -176,16 +176,16 @@ impl RefinedCovspan {
/// * Remove duplicate source code coverage regions
/// * 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)
/// * Carve out (leave uncovered) any "hole" spans that need to be left blank
/// (e.g. closures that will be counted by their own MIR body)
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>,

/// 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.
/// The current coverage span to compare to its `prev`, to possibly merge, discard,
/// or cause `prev` to be modified or discarded.
/// If `curr` is not discarded or merged, it becomes `prev` for the next iteration.
some_curr: Option<CurrCovspan>,

/// The coverage span from a prior iteration; typically assigned from that iteration's `curr`.
Expand Down Expand Up @@ -229,7 +229,7 @@ impl SpansRefiner {
let curr = self.curr();

if prev.is_mergeable(curr) {
debug!(" same bcb (and neither is a closure), merge with prev={prev:?}");
debug!(?prev, "curr will be merged into prev");
let curr = self.take_curr();
self.prev_mut().merge_from(&curr);
} else if prev.span.hi() <= curr.span.lo() {
Expand All @@ -238,15 +238,13 @@ impl SpansRefiner {
);
let prev = self.take_prev().into_refined();
self.refined_spans.push(prev);
} else if prev.is_closure {
} else if prev.is_hole {
// drop any equal or overlapping span (`curr`) and keep `prev` to test again in the
// next iter
debug!(
" curr overlaps a closure (prev). Drop curr and keep prev for next iter. prev={prev:?}",
);
debug!(?prev, "prev (a hole) overlaps curr, so discarding curr");
self.take_curr(); // Discards curr.
} else if curr.is_closure {
self.carve_out_span_for_closure();
} else if curr.is_hole {
self.carve_out_span_for_hole();
} else {
self.cutoff_prev_at_overlapping_curr();
}
Expand All @@ -270,10 +268,9 @@ impl SpansRefiner {
}
});

// 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);
// Discard hole spans, since their purpose was to carve out chunks from
// other spans, but we don't want the holes themselves in the final mappings.
self.refined_spans.retain(|covspan| !covspan.is_hole);
self.refined_spans
}

Expand Down Expand Up @@ -316,48 +313,43 @@ impl SpansRefiner {
{
// Skip curr because prev has already advanced beyond the end of curr.
// This can only happen if a prior iteration updated `prev` to skip past
// a region of code, such as skipping past a closure.
debug!(
" prev.span starts after curr.span, so curr will be dropped (skipping past \
closure?); prev={prev:?}",
);
// a region of code, such as skipping past a hole.
debug!(?prev, "prev.span starts after curr.span, so curr will be dropped");
} else {
self.some_curr = Some(CurrCovspan::new(curr.span, curr.bcb, curr.is_closure));
self.some_curr = Some(CurrCovspan::new(curr.span, curr.bcb, curr.is_hole));
return true;
}
}
false
}

/// If `prev`s span extends left of the closure (`curr`), carve out the closure's span from
/// `prev`'s span. (The closure's coverage counters will be injected when processing the
/// closure's own MIR.) Add the portion of the span to the left of the closure; and if the span
/// extends to the right of the closure, update `prev` to that portion of the span. For any
/// `pending_dups`, repeat the same process.
fn carve_out_span_for_closure(&mut self) {
/// If `prev`s span extends left of the hole (`curr`), carve out the hole's span from
/// `prev`'s span. Add the portion of the span to the left of the hole; and if the span
/// extends to the right of the hole, update `prev` to that portion of the span.
fn carve_out_span_for_hole(&mut self) {
let prev = self.prev();
let curr = self.curr();

let left_cutoff = curr.span.lo();
let right_cutoff = curr.span.hi();
let has_pre_closure_span = prev.span.lo() < right_cutoff;
let has_post_closure_span = prev.span.hi() > right_cutoff;

if has_pre_closure_span {
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);
self.refined_spans.push(pre_closure);
let has_pre_hole_span = prev.span.lo() < right_cutoff;
let has_post_hole_span = prev.span.hi() > right_cutoff;

if has_pre_hole_span {
let mut pre_hole = prev.refined_copy();
pre_hole.span = pre_hole.span.with_hi(left_cutoff);
debug!(?pre_hole, "prev overlaps a hole; adding pre-hole span");
self.refined_spans.push(pre_hole);
}

if has_post_closure_span {
// Mutate `prev.span` to start after the closure (and discard curr).
if has_post_hole_span {
// Mutate `prev.span` to start after the hole (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());
debug!(prev=?self.prev(), "mutated prev to start after the hole");

// 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
let hole_covspan = self.take_curr().into_refined();
self.refined_spans.push(hole_covspan); // since self.prev() was already updated
}
}

Expand Down
27 changes: 16 additions & 11 deletions compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,14 @@ 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 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())
// If two spans have the same lo & hi, put hole spans first,
// as they take precedence over non-hole spans.
.then_with(|| Ord::cmp(&a.is_hole, &b.is_hole).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
// Among covspans with the same span, keep only one. Hole 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.)
Expand All @@ -78,8 +78,8 @@ pub(super) fn mir_to_initial_sorted_coverage_spans(
fn remove_unwanted_macro_spans(initial_spans: &mut Vec<SpanFromMir>) {
let mut seen_macro_spans = FxHashSet::default();
initial_spans.retain(|covspan| {
// Ignore (retain) closure spans and non-macro-expansion spans.
if covspan.is_closure || covspan.visible_macro.is_none() {
// Ignore (retain) hole spans and non-macro-expansion spans.
if covspan.is_hole || covspan.visible_macro.is_none() {
return true;
}

Expand All @@ -96,7 +96,7 @@ fn split_visible_macro_spans(initial_spans: &mut Vec<SpanFromMir>) {
let mut extra_spans = vec![];

initial_spans.retain(|covspan| {
if covspan.is_closure {
if covspan.is_hole {
return true;
}

Expand All @@ -112,7 +112,7 @@ fn split_visible_macro_spans(initial_spans: &mut Vec<SpanFromMir>) {
return true;
}

assert!(!covspan.is_closure);
assert!(!covspan.is_hole);
extra_spans.push(SpanFromMir::new(before, covspan.visible_macro, covspan.bcb, false));
extra_spans.push(SpanFromMir::new(after, covspan.visible_macro, covspan.bcb, false));
false // Discard the original covspan that we just split.
Expand Down Expand Up @@ -148,6 +148,8 @@ fn bcb_to_initial_coverage_spans<'a, 'tcx>(
let expn_span = filtered_statement_span(statement)?;
let (span, visible_macro) = unexpand(expn_span)?;

// A statement that looks like the assignment of a closure expression
// is treated as a "hole" span, to be carved out of other spans.
Some(SpanFromMir::new(span, visible_macro, bcb, is_closure_like(statement)))
});

Expand Down Expand Up @@ -336,7 +338,10 @@ pub(super) struct SpanFromMir {
pub(super) span: Span,
visible_macro: Option<Symbol>,
pub(super) bcb: BasicCoverageBlock,
pub(super) is_closure: bool,
/// If true, this covspan represents a "hole" that should be carved out
/// from other spans, e.g. because it represents a closure expression that
/// will be instrumented separately as its own function.
pub(super) is_hole: bool,
}

impl SpanFromMir {
Expand All @@ -348,8 +353,8 @@ impl SpanFromMir {
span: Span,
visible_macro: Option<Symbol>,
bcb: BasicCoverageBlock,
is_closure: bool,
is_hole: bool,
) -> Self {
Self { span, visible_macro, bcb, is_closure }
Self { span, visible_macro, bcb, is_hole }
}
}

0 comments on commit 13b9bf5

Please sign in to comment.