From e1a2babc0628a4d003ed04a7a59603b388cdbf5c Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 28 Dec 2023 13:23:01 +1100 Subject: [PATCH 1/2] coverage: Prepare mappings separately from injecting statements These two tasks historically needed to be interleaved, but after various recent changes (including #116046 and #116917) they can now be fully separated. --- .../rustc_mir_transform/src/coverage/mod.rs | 64 +++++++++++-------- .../rustc_mir_transform/src/coverage/spans.rs | 9 ++- 2 files changed, 44 insertions(+), 29 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 078612aa59ca0..95562344ec813 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -8,7 +8,7 @@ mod spans; mod tests; use self::counters::{BcbCounter, CoverageCounters}; -use self::graph::CoverageGraph; +use self::graph::{BasicCoverageBlock, CoverageGraph}; use self::spans::CoverageSpans; use crate::MirPass; @@ -106,7 +106,8 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { self.coverage_counters .make_bcb_counters(&self.basic_coverage_blocks, bcb_has_coverage_spans); - let mappings = self.create_mappings_and_inject_coverage_statements(&coverage_spans); + let mappings = self.create_mappings(&coverage_spans); + self.inject_coverage_statements(bcb_has_coverage_spans); self.mir_body.function_coverage_info = Some(Box::new(FunctionCoverageInfo { function_source_hash: self.hir_info.function_source_hash, @@ -116,13 +117,12 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { })); } - /// For each [`BcbCounter`] associated with a BCB node or BCB edge, create - /// any corresponding mappings (for BCB nodes only), and inject any necessary - /// coverage statements into MIR. - fn create_mappings_and_inject_coverage_statements( - &mut self, - coverage_spans: &CoverageSpans, - ) -> Vec { + /// For each coverage span extracted from MIR, create a corresponding + /// mapping. + /// + /// Precondition: All BCBs corresponding to those spans have been given + /// coverage counters. + fn create_mappings(&self, coverage_spans: &CoverageSpans) -> Vec { let source_map = self.tcx.sess.source_map(); let body_span = self.hir_info.body_span; @@ -131,30 +131,42 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { let file_name = Symbol::intern(&source_file.name.for_codegen(self.tcx.sess).to_string_lossy()); - let mut mappings = Vec::new(); + coverage_spans + .bcbs_with_coverage_spans() + // For each BCB with spans, get a coverage term for its counter. + .map(|(bcb, spans)| { + let term = self + .coverage_counters + .bcb_counter(bcb) + .expect("all BCBs with spans were given counters") + .as_term(); + (term, spans) + }) + // Flatten the spans into individual term/span pairs. + .flat_map(|(term, spans)| spans.iter().map(move |&span| (term, span))) + // Convert each span to a code region, and create the final mapping. + .map(|(term, span)| { + let code_region = make_code_region(source_map, file_name, span, body_span); + Mapping { term, code_region } + }) + .collect::>() + } - // Process the counters and spans associated with BCB nodes. + /// For each BCB node or BCB edge that has an associated coverage counter, + /// inject any necessary coverage statements into MIR. + fn inject_coverage_statements( + &mut self, + bcb_has_coverage_spans: impl Fn(BasicCoverageBlock) -> bool, + ) { + // Process the counters associated with BCB nodes. for (bcb, counter_kind) in self.coverage_counters.bcb_node_counters() { - let spans = coverage_spans.spans_for_bcb(bcb); - let has_mappings = !spans.is_empty(); - - // If this BCB has any coverage spans, add corresponding mappings to - // the mappings table. - if has_mappings { - let term = counter_kind.as_term(); - mappings.extend(spans.iter().map(|&span| { - let code_region = make_code_region(source_map, file_name, span, body_span); - Mapping { code_region, term } - })); - } - let do_inject = match counter_kind { // Counter-increment statements always need to be injected. BcbCounter::Counter { .. } => true, // The only purpose of expression-used statements is to detect // when a mapping is unreachable, so we only inject them for // expressions with one or more mappings. - BcbCounter::Expression { .. } => has_mappings, + BcbCounter::Expression { .. } => bcb_has_coverage_spans(bcb), }; if do_inject { inject_statement( @@ -192,8 +204,6 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { // Inject a counter into the newly-created BB. inject_statement(self.mir_body, self.make_mir_coverage_kind(counter_kind), new_bb); } - - mappings } fn make_mir_coverage_kind(&self, counter_kind: &BcbCounter) -> CoverageKind { diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index ae43a18ad4e4f..ed091752187ef 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -48,8 +48,13 @@ impl CoverageSpans { !self.bcb_to_spans[bcb].is_empty() } - pub(super) fn spans_for_bcb(&self, bcb: BasicCoverageBlock) -> &[Span] { - &self.bcb_to_spans[bcb] + pub(super) fn bcbs_with_coverage_spans( + &self, + ) -> impl Iterator { + self.bcb_to_spans.iter_enumerated().filter_map(|(bcb, spans)| { + // Only yield BCBs that have at least one coverage span. + (!spans.is_empty()).then_some((bcb, spans.as_slice())) + }) } } From 3f67118ae7dcd7c35ca18f23cac787254004bc62 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 30 Dec 2023 22:36:11 +1100 Subject: [PATCH 2/2] coverage: Make `coverage_counters` a local variable This avoids the awkwardness of having to create it in the pass's constructor, and then mutate it later to actually create the counters. --- .../src/coverage/counters.rs | 30 ++++++++--------- .../rustc_mir_transform/src/coverage/mod.rs | 32 +++++++++++-------- .../rustc_mir_transform/src/coverage/tests.rs | 6 ++-- 3 files changed, 37 insertions(+), 31 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index d995d562521da..8c11dea5d4ee6 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -61,27 +61,27 @@ pub(super) struct CoverageCounters { } impl CoverageCounters { - pub(super) fn new(basic_coverage_blocks: &CoverageGraph) -> Self { + /// Makes [`BcbCounter`] `Counter`s and `Expressions` for the `BasicCoverageBlock`s directly or + /// indirectly associated with coverage spans, and accumulates additional `Expression`s + /// representing intermediate values. + pub(super) fn make_bcb_counters( + basic_coverage_blocks: &CoverageGraph, + bcb_has_coverage_spans: impl Fn(BasicCoverageBlock) -> bool, + ) -> Self { let num_bcbs = basic_coverage_blocks.num_nodes(); - Self { + let mut this = Self { next_counter_id: CounterId::START, bcb_counters: IndexVec::from_elem_n(None, num_bcbs), bcb_edge_counters: FxIndexMap::default(), bcb_has_incoming_edge_counters: BitSet::new_empty(num_bcbs), expressions: IndexVec::new(), - } - } + }; - /// Makes [`BcbCounter`] `Counter`s and `Expressions` for the `BasicCoverageBlock`s directly or - /// indirectly associated with coverage spans, and accumulates additional `Expression`s - /// representing intermediate values. - pub fn make_bcb_counters( - &mut self, - basic_coverage_blocks: &CoverageGraph, - bcb_has_coverage_spans: impl Fn(BasicCoverageBlock) -> bool, - ) { - MakeBcbCounters::new(self, basic_coverage_blocks).make_bcb_counters(bcb_has_coverage_spans) + MakeBcbCounters::new(&mut this, basic_coverage_blocks) + .make_bcb_counters(bcb_has_coverage_spans); + + this } fn make_counter(&mut self) -> BcbCounter { @@ -189,8 +189,8 @@ impl CoverageCounters { .map(|(&(from_bcb, to_bcb), counter_kind)| (from_bcb, to_bcb, counter_kind)) } - pub(super) fn take_expressions(&mut self) -> IndexVec { - std::mem::take(&mut self.expressions) + pub(super) fn into_expressions(self) -> IndexVec { + self.expressions } } diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 95562344ec813..d2ec5d7d7cca0 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -70,7 +70,6 @@ struct Instrumentor<'a, 'tcx> { mir_body: &'a mut mir::Body<'tcx>, hir_info: ExtractedHirInfo, basic_coverage_blocks: CoverageGraph, - coverage_counters: CoverageCounters, } impl<'a, 'tcx> Instrumentor<'a, 'tcx> { @@ -80,9 +79,8 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { debug!(?hir_info, "instrumenting {:?}", mir_body.source.def_id()); let basic_coverage_blocks = CoverageGraph::from_mir(mir_body); - let coverage_counters = CoverageCounters::new(&basic_coverage_blocks); - Self { tcx, mir_body, hir_info, basic_coverage_blocks, coverage_counters } + Self { tcx, mir_body, hir_info, basic_coverage_blocks } } fn inject_counters(&'a mut self) { @@ -103,16 +101,18 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { // and all `Expression` dependencies (operands) are also generated, for any other // `BasicCoverageBlock`s not already associated with a coverage span. let bcb_has_coverage_spans = |bcb| coverage_spans.bcb_has_coverage_spans(bcb); - self.coverage_counters - .make_bcb_counters(&self.basic_coverage_blocks, bcb_has_coverage_spans); + let coverage_counters = CoverageCounters::make_bcb_counters( + &self.basic_coverage_blocks, + bcb_has_coverage_spans, + ); - let mappings = self.create_mappings(&coverage_spans); - self.inject_coverage_statements(bcb_has_coverage_spans); + let mappings = self.create_mappings(&coverage_spans, &coverage_counters); + self.inject_coverage_statements(bcb_has_coverage_spans, &coverage_counters); self.mir_body.function_coverage_info = Some(Box::new(FunctionCoverageInfo { function_source_hash: self.hir_info.function_source_hash, - num_counters: self.coverage_counters.num_counters(), - expressions: self.coverage_counters.take_expressions(), + num_counters: coverage_counters.num_counters(), + expressions: coverage_counters.into_expressions(), mappings, })); } @@ -122,7 +122,11 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { /// /// Precondition: All BCBs corresponding to those spans have been given /// coverage counters. - fn create_mappings(&self, coverage_spans: &CoverageSpans) -> Vec { + fn create_mappings( + &self, + coverage_spans: &CoverageSpans, + coverage_counters: &CoverageCounters, + ) -> Vec { let source_map = self.tcx.sess.source_map(); let body_span = self.hir_info.body_span; @@ -135,8 +139,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { .bcbs_with_coverage_spans() // For each BCB with spans, get a coverage term for its counter. .map(|(bcb, spans)| { - let term = self - .coverage_counters + let term = coverage_counters .bcb_counter(bcb) .expect("all BCBs with spans were given counters") .as_term(); @@ -157,9 +160,10 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { fn inject_coverage_statements( &mut self, bcb_has_coverage_spans: impl Fn(BasicCoverageBlock) -> bool, + coverage_counters: &CoverageCounters, ) { // Process the counters associated with BCB nodes. - for (bcb, counter_kind) in self.coverage_counters.bcb_node_counters() { + for (bcb, counter_kind) in coverage_counters.bcb_node_counters() { let do_inject = match counter_kind { // Counter-increment statements always need to be injected. BcbCounter::Counter { .. } => true, @@ -178,7 +182,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { } // Process the counters associated with BCB edges. - for (from_bcb, to_bcb, counter_kind) in self.coverage_counters.bcb_edge_counters() { + for (from_bcb, to_bcb, counter_kind) in coverage_counters.bcb_edge_counters() { let do_inject = match counter_kind { // Counter-increment statements always need to be injected. BcbCounter::Counter { .. } => true, diff --git a/compiler/rustc_mir_transform/src/coverage/tests.rs b/compiler/rustc_mir_transform/src/coverage/tests.rs index 931bc8e58ffc1..d9a3c0cb162f3 100644 --- a/compiler/rustc_mir_transform/src/coverage/tests.rs +++ b/compiler/rustc_mir_transform/src/coverage/tests.rs @@ -630,8 +630,10 @@ fn test_make_bcb_counters() { // coverage spans for BCBs 1 and 2. Now we skip that step and just tell // BCB counter construction that those BCBs have spans. let bcb_has_coverage_spans = |bcb: BasicCoverageBlock| (1..=2).contains(&bcb.as_usize()); - let mut coverage_counters = counters::CoverageCounters::new(&basic_coverage_blocks); - coverage_counters.make_bcb_counters(&basic_coverage_blocks, bcb_has_coverage_spans); + let coverage_counters = counters::CoverageCounters::make_bcb_counters( + &basic_coverage_blocks, + bcb_has_coverage_spans, + ); assert_eq!(coverage_counters.num_expressions(), 0); assert_eq!(