From 506b9f96893a56159334f05d85500313a783f199 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 22 Dec 2023 14:11:42 +1100 Subject: [PATCH 1/6] coverage: Avoid early returns from `mir_to_initial_sorted_coverage_spans` --- .../src/coverage/spans/from_mir.rs | 29 +++++++++---------- 1 file changed, 14 insertions(+), 15 deletions(-) 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 8f6592afe85cb..913819ef33d32 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -15,27 +15,26 @@ pub(super) fn mir_to_initial_sorted_coverage_spans( basic_coverage_blocks: &CoverageGraph, ) -> Vec { let &ExtractedHirInfo { is_async_fn, fn_sig_span, body_span, .. } = hir_info; + + let mut initial_spans = vec![CoverageSpan::for_fn_sig(fn_sig_span)]; + if is_async_fn { // An async function desugars into a function that returns a future, // with the user code wrapped in a closure. Any spans in the desugared - // outer function will be unhelpful, so just produce a single span - // associating the function signature with its entry BCB. - return vec![CoverageSpan::for_fn_sig(fn_sig_span)]; - } - - let mut initial_spans = Vec::with_capacity(mir_body.basic_blocks.len() * 2); - for (bcb, bcb_data) in basic_coverage_blocks.iter_enumerated() { - initial_spans.extend(bcb_to_initial_coverage_spans(mir_body, body_span, bcb, bcb_data)); - } + // outer function will be unhelpful, so just keep the signature span + // and ignore all of the spans in the MIR body. + } else { + for (bcb, bcb_data) in basic_coverage_blocks.iter_enumerated() { + initial_spans.extend(bcb_to_initial_coverage_spans(mir_body, body_span, bcb, bcb_data)); + } - if initial_spans.is_empty() { - // This can happen if, for example, the function is unreachable (contains only a - // `BasicBlock`(s) with an `Unreachable` terminator). - return initial_spans; + // If no spans were extracted from the body, discard the signature span. + // FIXME: This preserves existing behavior; consider getting rid of it. + if initial_spans.len() == 1 { + initial_spans.clear(); + } } - initial_spans.push(CoverageSpan::for_fn_sig(fn_sig_span)); - initial_spans.sort_by(|a, b| { // First sort by span start. Ord::cmp(&a.span.lo(), &b.span.lo()) From df0df5256b95023e1e5e6bc49ee3857d90d91555 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 28 Dec 2023 15:08:24 +1100 Subject: [PATCH 2/6] coverage: Overhaul how "visible macros" are determined --- .../rustc_mir_transform/src/coverage/spans.rs | 65 ++----------------- .../src/coverage/spans/from_mir.rs | 59 ++++++++++++++--- tests/coverage/inline-dead.cov-map | 4 +- tests/coverage/inline-dead.coverage | 2 +- 4 files changed, 61 insertions(+), 69 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index ed091752187ef..6c0c62a1b7649 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -1,9 +1,7 @@ -use std::cell::OnceCell; - use rustc_data_structures::graph::WithNumNodes; use rustc_index::IndexVec; use rustc_middle::mir; -use rustc_span::{BytePos, ExpnKind, MacroKind, Span, Symbol, DUMMY_SP}; +use rustc_span::{BytePos, Span, Symbol, DUMMY_SP}; use super::graph::{BasicCoverageBlock, CoverageGraph, START_BCB}; use crate::coverage::ExtractedHirInfo; @@ -71,8 +69,7 @@ impl CoverageSpans { #[derive(Debug, Clone)] struct CoverageSpan { pub span: Span, - pub expn_span: Span, - pub current_macro_or_none: OnceCell>, + pub visible_macro: Option, pub bcb: BasicCoverageBlock, /// 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. @@ -82,23 +79,16 @@ struct CoverageSpan { impl CoverageSpan { pub fn for_fn_sig(fn_sig_span: Span) -> Self { - Self::new(fn_sig_span, fn_sig_span, START_BCB, false) + Self::new(fn_sig_span, None, START_BCB, false) } pub(super) fn new( span: Span, - expn_span: Span, + visible_macro: Option, bcb: BasicCoverageBlock, is_closure: bool, ) -> Self { - Self { - span, - expn_span, - current_macro_or_none: Default::default(), - bcb, - merged_spans: vec![span], - is_closure, - } + Self { span, visible_macro, bcb, merged_spans: vec![span], is_closure } } pub fn merge_from(&mut self, other: &Self) { @@ -123,37 +113,6 @@ impl CoverageSpan { pub fn is_in_same_bcb(&self, other: &Self) -> bool { self.bcb == other.bcb } - - /// If the span is part of a macro, returns the macro name symbol. - pub fn current_macro(&self) -> Option { - self.current_macro_or_none - .get_or_init(|| { - if let ExpnKind::Macro(MacroKind::Bang, current_macro) = - self.expn_span.ctxt().outer_expn_data().kind - { - return Some(current_macro); - } - None - }) - .map(|symbol| symbol) - } - - /// If the span is part of a macro, and the macro is visible (expands directly to the given - /// body_span), returns the macro name symbol. - pub fn visible_macro(&self, body_span: Span) -> Option { - let current_macro = self.current_macro()?; - let parent_callsite = self.expn_span.parent_callsite()?; - - // In addition to matching the context of the body span, the parent callsite - // must also be the source callsite, i.e. the parent must have no parent. - let is_visible_macro = - parent_callsite.parent_callsite().is_none() && parent_callsite.eq_ctxt(body_span); - is_visible_macro.then_some(current_macro) - } - - pub fn is_macro_expansion(&self) -> bool { - self.current_macro().is_some() - } } /// Converts the initial set of `CoverageSpan`s (one per MIR `Statement` or `Terminator`) into a @@ -164,10 +123,6 @@ impl CoverageSpan { /// execution /// * Carve out (leave uncovered) any span that will be counted by another MIR (notably, closures) struct CoverageSpansGenerator<'a> { - /// A `Span` covering the function body of the MIR (typically from left curly brace to right - /// curly brace). - body_span: Span, - /// The BasicCoverageBlock Control Flow Graph (BCB CFG). basic_coverage_blocks: &'a CoverageGraph, @@ -244,7 +199,6 @@ impl<'a> CoverageSpansGenerator<'a> { ); let coverage_spans = Self { - body_span: hir_info.body_span, basic_coverage_blocks, sorted_spans_iter: sorted_spans.into_iter(), some_curr: None, @@ -303,7 +257,7 @@ impl<'a> CoverageSpansGenerator<'a> { // **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() { + if prev.visible_macro.is_some() && curr.visible_macro.is_some() { // Macros that expand to include branching (such as // `assert_eq!()`, `assert_ne!()`, `info!()`, `debug!()`, or // `trace!()`) typically generate callee spans with identical @@ -365,12 +319,7 @@ impl<'a> CoverageSpansGenerator<'a> { fn maybe_push_macro_name_span(&mut self) { let curr = self.curr(); - let Some(visible_macro) = curr.visible_macro(self.body_span) else { return }; - if let Some(prev) = &self.some_prev - && prev.expn_span.eq_ctxt(curr.expn_span) - { - return; - } + let Some(visible_macro) = curr.visible_macro else { return }; // The split point is relative to `curr_original_span`, // because `curr.span` may have been merged with preceding spans. 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 913819ef33d32..fde25ad994fdf 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -3,7 +3,7 @@ use rustc_middle::mir::{ self, AggregateKind, FakeReadCause, Rvalue, Statement, StatementKind, Terminator, TerminatorKind, }; -use rustc_span::Span; +use rustc_span::{ExpnKind, MacroKind, Span, Symbol}; use crate::coverage::graph::{BasicCoverageBlock, BasicCoverageBlockData, CoverageGraph}; use crate::coverage::spans::CoverageSpan; @@ -71,16 +71,18 @@ fn bcb_to_initial_coverage_spans<'a, 'tcx>( let statement_spans = data.statements.iter().filter_map(move |statement| { let expn_span = filtered_statement_span(statement)?; - let span = unexpand_into_body_span(expn_span, body_span)?; + let (span, visible_macro) = + unexpand_into_body_span_with_visible_macro(expn_span, body_span)?; - Some(CoverageSpan::new(span, expn_span, bcb, is_closure_or_coroutine(statement))) + Some(CoverageSpan::new(span, visible_macro, bcb, is_closure_or_coroutine(statement))) }); let terminator_span = Some(data.terminator()).into_iter().filter_map(move |terminator| { let expn_span = filtered_terminator_span(terminator)?; - let span = unexpand_into_body_span(expn_span, body_span)?; + let (span, visible_macro) = + unexpand_into_body_span_with_visible_macro(expn_span, body_span)?; - Some(CoverageSpan::new(span, expn_span, bcb, false)) + Some(CoverageSpan::new(span, visible_macro, bcb, false)) }); statement_spans.chain(terminator_span) @@ -201,7 +203,48 @@ fn filtered_terminator_span(terminator: &Terminator<'_>) -> Option { /// /// [^1]Expansions result from Rust syntax including macros, syntactic sugar, /// etc.). -#[inline] -fn unexpand_into_body_span(span: Span, body_span: Span) -> Option { - span.find_ancestor_inside_same_ctxt(body_span) +fn unexpand_into_body_span_with_visible_macro( + original_span: Span, + body_span: Span, +) -> Option<(Span, Option)> { + let (span, prev) = unexpand_into_body_span_with_prev(original_span, body_span)?; + + let visible_macro = prev + .map(|prev| match prev.ctxt().outer_expn_data().kind { + ExpnKind::Macro(MacroKind::Bang, name) => Some(name), + _ => None, + }) + .flatten(); + + Some((span, visible_macro)) +} + +/// Walks through the expansion ancestors of `original_span` to find a span that +/// is contained in `body_span` and has the same [`SyntaxContext`] as `body_span`. +/// The ancestor that was traversed just before the matching span (if any) is +/// also returned. +/// +/// For example, a return value of `Some((ancestor, Some(prev))` means that: +/// - `ancestor == original_span.find_ancestor_inside_same_ctxt(body_span)` +/// - `ancestor == prev.parent_callsite()` +/// +/// [`SyntaxContext`]: rustc_span::SyntaxContext +fn unexpand_into_body_span_with_prev( + original_span: Span, + body_span: Span, +) -> Option<(Span, Option)> { + let mut prev = None; + let mut curr = original_span; + + while !body_span.contains(curr) || !curr.eq_ctxt(body_span) { + prev = Some(curr); + curr = curr.parent_callsite()?; + } + + debug_assert_eq!(Some(curr), original_span.find_ancestor_in_same_ctxt(body_span)); + if let Some(prev) = prev { + debug_assert_eq!(Some(curr), prev.parent_callsite()); + } + + Some((curr, prev)) } diff --git a/tests/coverage/inline-dead.cov-map b/tests/coverage/inline-dead.cov-map index 958b423f24cf8..ab04e746b9165 100644 --- a/tests/coverage/inline-dead.cov-map +++ b/tests/coverage/inline-dead.cov-map @@ -31,14 +31,14 @@ Number of file 0 mappings: 2 - Code(Counter(0)) at (prev + 7, 6) to (start + 2, 2) Function name: inline_dead::main::{closure#0} -Raw bytes (23): 0x[01, 01, 02, 00, 06, 01, 00, 03, 01, 07, 17, 00, 18, 00, 02, 0d, 00, 0e, 03, 02, 05, 00, 06] +Raw bytes (23): 0x[01, 01, 02, 00, 06, 01, 00, 03, 01, 07, 17, 01, 16, 00, 02, 0d, 00, 0e, 03, 02, 05, 00, 06] Number of files: 1 - file 0 => global file 1 Number of expressions: 2 - expression 0 operands: lhs = Zero, rhs = Expression(1, Sub) - expression 1 operands: lhs = Counter(0), rhs = Zero Number of file 0 mappings: 3 -- Code(Counter(0)) at (prev + 7, 23) to (start + 0, 24) +- Code(Counter(0)) at (prev + 7, 23) to (start + 1, 22) - Code(Zero) at (prev + 2, 13) to (start + 0, 14) - Code(Expression(0, Add)) at (prev + 2, 5) to (start + 0, 6) = (Zero + (c0 - Zero)) diff --git a/tests/coverage/inline-dead.coverage b/tests/coverage/inline-dead.coverage index de96aa17acd62..7c201f482db31 100644 --- a/tests/coverage/inline-dead.coverage +++ b/tests/coverage/inline-dead.coverage @@ -5,7 +5,7 @@ LL| 1| println!("{}", live::()); LL| 1| LL| 1| let f = |x: bool| { - LL| | debug_assert!( + LL| 1| debug_assert!( LL| 0| x LL| | ); LL| 1| }; From cd3a9760e453e9660c9c44316076a3e63248e730 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 21 Dec 2023 23:03:45 +1100 Subject: [PATCH 3/6] coverage: Hoist the removal of unwanted macro expansion spans --- .../rustc_mir_transform/src/coverage/spans.rs | 29 ++----------------- .../src/coverage/spans/from_mir.rs | 24 +++++++++++++++ 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index 6c0c62a1b7649..576f098e6e4aa 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -251,32 +251,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.visible_macro.is_some() && curr.visible_macro.is_some() { - // 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(); 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 fde25ad994fdf..b28abcf71d57e 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/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, @@ -35,6 +36,9 @@ pub(super) fn mir_to_initial_sorted_coverage_spans( } } + 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()) @@ -55,6 +59,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) { + 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() { + return true; + } + + // Retain only the first macro-expanded covspan with this span. + seen_macro_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 From d4d2f1428cda381fe0c0d58700ead551fdf96f6b Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 22 Dec 2023 11:21:52 +1100 Subject: [PATCH 4/6] coverage: Hoist the splitting of visible macro invocations --- .../rustc_mir_transform/src/coverage/spans.rs | 34 --------------- .../src/coverage/spans/from_mir.rs | 36 ++++++++++++++++ tests/coverage/closure.cov-map | 42 ++++++++----------- 3 files changed, 54 insertions(+), 58 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index 576f098e6e4aa..f346aa1810c9d 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -220,7 +220,6 @@ impl<'a> CoverageSpansGenerator<'a> { // span-processing steps don't make sense yet. if self.some_prev.is_none() { debug!(" initial span"); - self.maybe_push_macro_name_span(); continue; } @@ -232,7 +231,6 @@ impl<'a> CoverageSpansGenerator<'a> { debug!(" same bcb (and neither is a closure), merge with prev={prev:?}"); let prev = self.take_prev(); self.curr_mut().merge_from(&prev); - self.maybe_push_macro_name_span(); // Note that curr.span may now differ from curr_original_span } else if prev.span.hi() <= curr.span.lo() { debug!( @@ -240,7 +238,6 @@ impl<'a> CoverageSpansGenerator<'a> { ); let prev = self.take_prev(); self.refined_spans.push(prev); - self.maybe_push_macro_name_span(); } else if prev.is_closure { // drop any equal or overlapping span (`curr`) and keep `prev` to test again in the // next iter @@ -256,7 +253,6 @@ impl<'a> CoverageSpansGenerator<'a> { self.update_pending_dups(); } else { self.cutoff_prev_at_overlapping_curr(); - self.maybe_push_macro_name_span(); } } @@ -291,36 +287,6 @@ impl<'a> CoverageSpansGenerator<'a> { self.refined_spans } - /// If `curr` is part of a new macro expansion, carve out and push a separate - /// span that ends just after the macro name and its subsequent `!`. - fn maybe_push_macro_name_span(&mut self) { - let curr = self.curr(); - - let Some(visible_macro) = curr.visible_macro else { return }; - - // The split point is relative to `curr_original_span`, - // because `curr.span` may have been merged with preceding spans. - let split_point_after_macro_bang = self.curr_original_span.lo() - + BytePos(visible_macro.as_str().len() as u32) - + BytePos(1); // add 1 for the `!` - debug_assert!(split_point_after_macro_bang <= curr.span.hi()); - if split_point_after_macro_bang > curr.span.hi() { - // Something is wrong with the macro name span; - // return now to avoid emitting malformed mappings (e.g. #117788). - return; - } - - let mut macro_name_cov = curr.clone(); - macro_name_cov.span = macro_name_cov.span.with_hi(split_point_after_macro_bang); - self.curr_mut().span = curr.span.with_lo(split_point_after_macro_bang); - - debug!( - " and curr starts a new macro expansion, so add a new span just for \ - the macro `{visible_macro}!`, new span={macro_name_cov:?}", - ); - self.refined_spans.push(macro_name_cov); - } - #[track_caller] fn curr(&self) -> &CoverageSpan { self.some_curr.as_ref().unwrap_or_else(|| bug!("some_curr is None (curr)")) 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 b28abcf71d57e..637424753d96c 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -38,6 +38,7 @@ pub(super) fn mir_to_initial_sorted_coverage_spans( initial_spans.sort_by(|a, b| basic_coverage_blocks.cmp_in_dominator_order(a.bcb, b.bcb)); remove_unwanted_macro_spans(&mut initial_spans); + split_visible_macro_spans(&mut initial_spans); initial_spans.sort_by(|a, b| { // First sort by span start. @@ -79,6 +80,41 @@ fn remove_unwanted_macro_spans(initial_spans: &mut Vec) { }); } +/// When a span corresponds to a macro invocation that is visible from the +/// function body, split it into two parts. The first part covers just the +/// macro name plus `!`, and the second part covers the rest of the macro +/// invocation. This seems to give better results for code that uses macros. +fn split_visible_macro_spans(initial_spans: &mut Vec) { + let mut extra_spans = vec![]; + + initial_spans.retain(|covspan| { + if covspan.is_closure { + return true; + } + + let Some(visible_macro) = covspan.visible_macro else { return true }; + + let split_len = visible_macro.as_str().len() as u32 + 1; + let (before, after) = covspan.span.split_at(split_len); + if !covspan.span.contains(before) || !covspan.span.contains(after) { + // Something is unexpectedly wrong with the split point. + // The debug assertion in `split_at` will have already caught this, + // but in release builds it's safer to do nothing and maybe get a + // bug report for unexpected coverage, rather than risk an ICE. + return true; + } + + assert!(!covspan.is_closure); + extra_spans.push(CoverageSpan::new(before, covspan.visible_macro, covspan.bcb, false)); + extra_spans.push(CoverageSpan::new(after, covspan.visible_macro, covspan.bcb, false)); + false // Discard the original covspan that we just split. + }); + + // The newly-split spans are added at the end, so any previous sorting + // is not preserved. + 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 // for each `Statement` and `Terminator`. (Note that subsequent stages of coverage analysis will diff --git a/tests/coverage/closure.cov-map b/tests/coverage/closure.cov-map index 522c1e73afe3d..c5ac17600cd2a 100644 --- a/tests/coverage/closure.cov-map +++ b/tests/coverage/closure.cov-map @@ -81,21 +81,18 @@ 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} @@ -118,21 +115,18 @@ Number of file 0 mappings: 6 = (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} From cd5084388a8e7cbecdea90f071ec0bb5be7c3ee8 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Fri, 22 Dec 2023 14:20:30 +1100 Subject: [PATCH 5/6] coverage: Split out `SpanFromMir` from `CoverageSpan` This draws a clear distinction between the fields/methods that are needed by initial span extraction and preprocessing, and those that are needed by the main "refinement" loop. --- .../rustc_mir_transform/src/coverage/spans.rs | 18 ++---- .../src/coverage/spans/from_mir.rs | 57 +++++++++++++++---- 2 files changed, 51 insertions(+), 24 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index f346aa1810c9d..6604e13398fa8 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -1,9 +1,9 @@ use rustc_data_structures::graph::WithNumNodes; use rustc_index::IndexVec; use rustc_middle::mir; -use rustc_span::{BytePos, Span, Symbol, DUMMY_SP}; +use rustc_span::{BytePos, Span, DUMMY_SP}; -use super::graph::{BasicCoverageBlock, CoverageGraph, START_BCB}; +use super::graph::{BasicCoverageBlock, CoverageGraph}; use crate::coverage::ExtractedHirInfo; mod from_mir; @@ -69,7 +69,6 @@ impl CoverageSpans { #[derive(Debug, Clone)] struct CoverageSpan { pub span: Span, - pub visible_macro: Option, pub bcb: BasicCoverageBlock, /// 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. @@ -78,17 +77,8 @@ struct CoverageSpan { } impl CoverageSpan { - pub fn for_fn_sig(fn_sig_span: Span) -> Self { - Self::new(fn_sig_span, None, START_BCB, false) - } - - pub(super) fn new( - span: Span, - visible_macro: Option, - bcb: BasicCoverageBlock, - is_closure: bool, - ) -> Self { - Self { span, visible_macro, bcb, merged_spans: vec![span], is_closure } + fn new(span: Span, bcb: BasicCoverageBlock, is_closure: bool) -> Self { + Self { span, bcb, merged_spans: vec![span], is_closure } } pub fn merge_from(&mut self, other: &Self) { 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 637424753d96c..1b6dfccd57495 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -6,7 +6,9 @@ use rustc_middle::mir::{ }; use rustc_span::{ExpnKind, MacroKind, Span, Symbol}; -use crate::coverage::graph::{BasicCoverageBlock, BasicCoverageBlockData, CoverageGraph}; +use crate::coverage::graph::{ + BasicCoverageBlock, BasicCoverageBlockData, CoverageGraph, START_BCB, +}; use crate::coverage::spans::CoverageSpan; use crate::coverage::ExtractedHirInfo; @@ -17,7 +19,7 @@ pub(super) fn mir_to_initial_sorted_coverage_spans( ) -> Vec { let &ExtractedHirInfo { is_async_fn, fn_sig_span, body_span, .. } = hir_info; - let mut initial_spans = vec![CoverageSpan::for_fn_sig(fn_sig_span)]; + let mut initial_spans = vec![SpanFromMir::for_fn_sig(fn_sig_span)]; if is_async_fn { // An async function desugars into a function that returns a future, @@ -57,7 +59,7 @@ pub(super) fn mir_to_initial_sorted_coverage_spans( .then_with(|| Ord::cmp(&a.is_closure, &b.is_closure).reverse()) }); - initial_spans + initial_spans.into_iter().map(SpanFromMir::into_coverage_span).collect::>() } /// Macros that expand into branches (e.g. `assert!`, `trace!`) tend to generate @@ -67,7 +69,7 @@ pub(super) fn mir_to_initial_sorted_coverage_spans( /// /// (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) { +fn remove_unwanted_macro_spans(initial_spans: &mut Vec) { let mut seen_macro_spans = FxHashSet::default(); initial_spans.retain(|covspan| { // Ignore (retain) closure spans and non-macro-expansion spans. @@ -84,7 +86,7 @@ fn remove_unwanted_macro_spans(initial_spans: &mut Vec) { /// function body, split it into two parts. The first part covers just the /// macro name plus `!`, and the second part covers the rest of the macro /// invocation. This seems to give better results for code that uses macros. -fn split_visible_macro_spans(initial_spans: &mut Vec) { +fn split_visible_macro_spans(initial_spans: &mut Vec) { let mut extra_spans = vec![]; initial_spans.retain(|covspan| { @@ -105,8 +107,8 @@ fn split_visible_macro_spans(initial_spans: &mut Vec) { } assert!(!covspan.is_closure); - extra_spans.push(CoverageSpan::new(before, covspan.visible_macro, covspan.bcb, false)); - extra_spans.push(CoverageSpan::new(after, covspan.visible_macro, covspan.bcb, false)); + 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. }); @@ -125,7 +127,7 @@ fn bcb_to_initial_coverage_spans<'a, 'tcx>( body_span: Span, bcb: BasicCoverageBlock, bcb_data: &'a BasicCoverageBlockData, -) -> impl Iterator + Captures<'a> + Captures<'tcx> { +) -> impl Iterator + Captures<'a> + Captures<'tcx> { bcb_data.basic_blocks.iter().flat_map(move |&bb| { let data = &mir_body[bb]; @@ -134,7 +136,7 @@ fn bcb_to_initial_coverage_spans<'a, 'tcx>( let (span, visible_macro) = unexpand_into_body_span_with_visible_macro(expn_span, body_span)?; - Some(CoverageSpan::new(span, visible_macro, bcb, is_closure_or_coroutine(statement))) + Some(SpanFromMir::new(span, visible_macro, bcb, is_closure_or_coroutine(statement))) }); let terminator_span = Some(data.terminator()).into_iter().filter_map(move |terminator| { @@ -142,7 +144,7 @@ fn bcb_to_initial_coverage_spans<'a, 'tcx>( let (span, visible_macro) = unexpand_into_body_span_with_visible_macro(expn_span, body_span)?; - Some(CoverageSpan::new(span, visible_macro, bcb, false)) + Some(SpanFromMir::new(span, visible_macro, bcb, false)) }); statement_spans.chain(terminator_span) @@ -308,3 +310,38 @@ fn unexpand_into_body_span_with_prev( Some((curr, prev)) } + +#[derive(Debug)] +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 + /// mappings. + /// + /// With the exception of `fn_sig_span`, this should always be contained + /// within `body_span`. + span: Span, + visible_macro: Option, + bcb: BasicCoverageBlock, + is_closure: bool, +} + +impl SpanFromMir { + fn for_fn_sig(fn_sig_span: Span) -> Self { + Self::new(fn_sig_span, None, START_BCB, false) + } + + fn new( + span: Span, + visible_macro: Option, + bcb: BasicCoverageBlock, + is_closure: bool, + ) -> 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 514e026853fcd426528cc23a7768d0e43f05ce7c Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 28 Dec 2023 14:49:16 +1100 Subject: [PATCH 6/6] coverage: Make the remaining fields of `CoverageSpan` non-public The struct itself is already non-public, so having public fields doesn't achieve anything. --- compiler/rustc_mir_transform/src/coverage/spans.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index 6604e13398fa8..5983189984d91 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -68,12 +68,12 @@ impl CoverageSpans { /// `dominates()` the `BasicBlock`s in this `CoverageSpan`. #[derive(Debug, Clone)] struct CoverageSpan { - pub span: Span, - pub bcb: BasicCoverageBlock, + span: Span, + bcb: BasicCoverageBlock, /// 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. - pub merged_spans: Vec, - pub is_closure: bool, + merged_spans: Vec, + is_closure: bool, } impl CoverageSpan {