From adce72b2fa14fadb30ad89b1b3297d1a032f8ece Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 13 Sep 2023 13:20:13 +1000 Subject: [PATCH] coverage: Store expression data in function coverage info Even though expression details are now stored in the info structure, we still need to inject `Expression` statements into MIR, because if one is missing during codegen then we know that it was optimized out and we can remap all of its associated code regions to zero. --- .../src/coverageinfo/map_data.rs | 82 +++++++------------ .../src/coverageinfo/mod.rs | 4 +- compiler/rustc_middle/src/mir/coverage.rs | 26 +++--- .../src/coverage/counters.rs | 44 ++-------- .../rustc_mir_transform/src/coverage/mod.rs | 41 +--------- .../rustc_mir_transform/src/coverage/tests.rs | 2 +- ...ment_coverage.main.InstrumentCoverage.diff | 4 +- 7 files changed, 58 insertions(+), 145 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 2d0b2568bd6c4..1be2047f3f605 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -2,19 +2,11 @@ use crate::coverageinfo::ffi::{Counter, CounterExpression, ExprKind}; use rustc_data_structures::fx::FxIndexSet; use rustc_index::bit_set::BitSet; -use rustc_index::IndexVec; use rustc_middle::mir::coverage::{ - CodeRegion, CounterId, CovTerm, ExpressionId, FunctionCoverageInfo, Mapping, Op, + CodeRegion, CounterId, CovTerm, Expression, ExpressionId, FunctionCoverageInfo, Mapping, Op, }; use rustc_middle::ty::Instance; -#[derive(Clone, Debug, PartialEq)] -pub struct Expression { - lhs: CovTerm, - op: Op, - rhs: CovTerm, -} - /// Holds all of the coverage mapping data associated with a function instance, /// collected during traversal of `Coverage` statements in the function's MIR. #[derive(Debug)] @@ -26,7 +18,9 @@ pub struct FunctionCoverage<'tcx> { /// Tracks which counters have been seen, so that we can identify mappings /// to counters that were optimized out, and set them to zero. counters_seen: BitSet, - expressions: IndexVec>, + /// Tracks which expressions have one or more associated mappings, but have + /// not yet been seen. This lets us detect that they were optimized out. + expressions_not_seen: BitSet, } impl<'tcx> FunctionCoverage<'tcx> { @@ -52,16 +46,27 @@ impl<'tcx> FunctionCoverage<'tcx> { is_used: bool, ) -> Self { let num_counters = function_coverage_info.num_counters; - let num_expressions = function_coverage_info.num_expressions; + let num_expressions = function_coverage_info.expressions.len(); debug!( "FunctionCoverage::create(instance={instance:?}) has \ num_counters={num_counters}, num_expressions={num_expressions}, is_used={is_used}" ); + + // Every expression that has an associated mapping should have a + // corresponding statement in MIR. If we don't see that statement, we + // know that it was removed by MIR optimizations. + let mut expressions_not_seen = BitSet::new_empty(num_expressions); + for Mapping { term, .. } in &function_coverage_info.mappings { + if let &CovTerm::Expression(id) = term { + expressions_not_seen.insert(id); + } + } + Self { function_coverage_info, is_used, counters_seen: BitSet::new_empty(num_counters), - expressions: IndexVec::from_elem_n(None, num_expressions), + expressions_not_seen, } } @@ -76,35 +81,10 @@ impl<'tcx> FunctionCoverage<'tcx> { self.counters_seen.insert(id); } - /// Adds information about a coverage expression. + /// Marks an expression ID as having been seen. #[instrument(level = "debug", skip(self))] - pub(crate) fn add_counter_expression( - &mut self, - expression_id: ExpressionId, - lhs: CovTerm, - op: Op, - rhs: CovTerm, - ) { - debug_assert!( - expression_id.as_usize() < self.expressions.len(), - "expression_id {} is out of range for expressions.len() = {} - for {:?}", - expression_id.as_usize(), - self.expressions.len(), - self, - ); - - let expression = Expression { lhs, op, rhs }; - let slot = &mut self.expressions[expression_id]; - match slot { - None => *slot = Some(expression), - // If this expression ID slot has already been filled, it should - // contain identical information. - Some(ref previous_expression) => assert_eq!( - previous_expression, &expression, - "add_counter_expression: expression for id changed" - ), - } + pub(crate) fn add_counter_expression(&mut self, expression_id: ExpressionId) { + self.expressions_not_seen.remove(expression_id); } /// Identify expressions that will always have a value of zero, and note @@ -125,13 +105,13 @@ impl<'tcx> FunctionCoverage<'tcx> { // and then update the set of always-zero expressions if necessary. // (By construction, expressions can only refer to other expressions // that have lower IDs, so one pass is sufficient.) - for (id, maybe_expression) in self.expressions.iter_enumerated() { - let Some(expression) = maybe_expression else { - // If an expression is missing, it must have been optimized away, + for (id, expression) in self.function_coverage_info.expressions.iter_enumerated() { + if self.expressions_not_seen.contains(id) { + // If an expression was not seen, it must have been optimized away, // so any operand that refers to it can be replaced with zero. zero_expressions.insert(id); continue; - }; + } // We don't need to simplify the actual expression data in the // expressions list; we can just simplify a temporary copy and then @@ -184,7 +164,7 @@ impl<'tcx> FunctionCoverage<'tcx> { // Expression IDs are indices into `self.expressions`, and on the LLVM // side they will be treated as indices into `counter_expressions`, so // the two vectors should correspond 1:1. - assert_eq!(self.expressions.len(), counter_expressions.len()); + assert_eq!(self.function_coverage_info.expressions.len(), counter_expressions.len()); let counter_regions = self.counter_regions(zero_expressions); @@ -204,17 +184,17 @@ impl<'tcx> FunctionCoverage<'tcx> { _ => Counter::from_term(operand), }; - self.expressions - .iter() - .map(|expression| match expression { - None => { + self.function_coverage_info + .expressions + .iter_enumerated() + .map(|(id, &Expression { lhs, op, rhs })| { + if self.expressions_not_seen.contains(id) { // This expression ID was allocated, but we never saw the // actual expression, so it must have been optimized out. // Replace it with a dummy expression, and let LLVM take // care of omitting it from the expression list. CounterExpression::DUMMY - } - &Some(Expression { lhs, op, rhs, .. }) => { + } else { // Convert the operands and operator as normal. CounterExpression::new( counter_from_operand(lhs), diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 8c3817bc0240b..a1972a71b2e79 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -147,8 +147,8 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { ); bx.instrprof_increment(fn_name, hash, num_counters, index); } - CoverageKind::Expression { id, lhs, op, rhs } => { - func_coverage.add_counter_expression(id, lhs, op, rhs); + CoverageKind::Expression { id } => { + func_coverage.add_counter_expression(id); } } } diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 4a4b6b918a509..08be33379e2b0 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -1,5 +1,6 @@ //! Metadata from source code coverage analysis and instrumentation. +use rustc_index::IndexVec; use rustc_macros::HashStable; use rustc_span::Symbol; @@ -70,9 +71,6 @@ pub enum CoverageKind { /// ID of this coverage-counter expression within its enclosing function. /// Other expressions in the same function can refer to it as an operand. id: ExpressionId, - lhs: CovTerm, - op: Op, - rhs: CovTerm, }, } @@ -81,17 +79,7 @@ impl Debug for CoverageKind { use CoverageKind::*; match self { Counter { id } => write!(fmt, "Counter({:?})", id.index()), - Expression { id, lhs, op, rhs } => write!( - fmt, - "Expression({:?}) = {:?} {} {:?}", - id.index(), - lhs, - match op { - Op::Add => "+", - Op::Subtract => "-", - }, - rhs, - ), + Expression { id } => write!(fmt, "Expression({:?})", id.index()), } } } @@ -133,6 +121,14 @@ impl Op { } } +#[derive(Clone, Debug)] +#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] +pub struct Expression { + pub lhs: CovTerm, + pub op: Op, + pub rhs: CovTerm, +} + #[derive(Clone, Debug)] #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] pub struct Mapping { @@ -155,7 +151,7 @@ pub struct Mapping { pub struct FunctionCoverageInfo { pub function_source_hash: u64, pub num_counters: usize, - pub num_expressions: usize, + pub expressions: IndexVec, pub mappings: Vec, } diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index eda2f6ff51c96..118a78aebaf9d 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -19,7 +19,7 @@ const NESTED_INDENT: &str = " "; #[derive(Clone)] pub(super) enum BcbCounter { Counter { id: CounterId }, - Expression { id: ExpressionId, lhs: CovTerm, op: Op, rhs: CovTerm }, + Expression { id: ExpressionId }, } impl BcbCounter { @@ -39,17 +39,7 @@ impl Debug for BcbCounter { fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::Counter { id, .. } => write!(fmt, "Counter({:?})", id.index()), - Self::Expression { id, lhs, op, rhs } => write!( - fmt, - "Expression({:?}) = {:?} {} {:?}", - id.index(), - lhs, - match op { - Op::Add => "+", - Op::Subtract => "-", - }, - rhs, - ), + Self::Expression { id } => write!(fmt, "Expression({:?})", id.index()), } } } @@ -58,7 +48,6 @@ impl Debug for BcbCounter { /// associated with nodes/edges in the BCB graph. pub(super) struct CoverageCounters { next_counter_id: CounterId, - next_expression_id: ExpressionId, /// Coverage counters/expressions that are associated with individual BCBs. bcb_counters: IndexVec>, @@ -69,10 +58,9 @@ pub(super) struct CoverageCounters { /// Only used by debug assertions, to verify that BCBs with incoming edge /// counters do not have their own physical counters (expressions are allowed). bcb_has_incoming_edge_counters: BitSet, - /// Expression nodes that are not directly associated with any particular - /// BCB/edge, but are needed as operands to more complex expressions. - /// These are always [`BcbCounter::Expression`]. - pub(super) intermediate_expressions: Vec, + /// Table of expression data, associating each expression ID with its + /// corresponding operator (+ or -) and its LHS/RHS operands. + pub(super) expressions: IndexVec, } impl CoverageCounters { @@ -81,12 +69,10 @@ impl CoverageCounters { Self { next_counter_id: CounterId::START, - next_expression_id: ExpressionId::START, - bcb_counters: IndexVec::from_elem_n(None, num_bcbs), bcb_edge_counters: FxHashMap::default(), bcb_has_incoming_edge_counters: BitSet::new_empty(num_bcbs), - intermediate_expressions: Vec::new(), + expressions: IndexVec::new(), } } @@ -107,8 +93,8 @@ impl CoverageCounters { } fn make_expression(&mut self, lhs: CovTerm, op: Op, rhs: CovTerm) -> BcbCounter { - let id = self.next_expression(); - BcbCounter::Expression { id, lhs, op, rhs } + let id = self.expressions.push(Expression { lhs, op, rhs }); + BcbCounter::Expression { id } } /// Counter IDs start from one and go up. @@ -118,22 +104,10 @@ impl CoverageCounters { next } - /// Expression IDs start from 0 and go up. - /// (Counter IDs and Expression IDs are distinguished by the `Operand` enum.) - fn next_expression(&mut self) -> ExpressionId { - let next = self.next_expression_id; - self.next_expression_id = self.next_expression_id + 1; - next - } - pub(super) fn num_counters(&self) -> usize { self.next_counter_id.as_usize() } - pub(super) fn num_expressions(&self) -> usize { - self.next_expression_id.as_usize() - } - fn set_bcb_counter( &mut self, bcb: BasicCoverageBlock, @@ -333,7 +307,6 @@ impl<'a> MakeBcbCounters<'a> { ); debug!(" [new intermediate expression: {:?}]", intermediate_expression); let intermediate_expression_operand = intermediate_expression.as_term(); - self.coverage_counters.intermediate_expressions.push(intermediate_expression); some_sumup_counter_operand.replace(intermediate_expression_operand); } } @@ -446,7 +419,6 @@ impl<'a> MakeBcbCounters<'a> { intermediate_expression ); let intermediate_expression_operand = intermediate_expression.as_term(); - self.coverage_counters.intermediate_expressions.push(intermediate_expression); some_sumup_edge_counter_operand.replace(intermediate_expression_operand); } } diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 8fb00a9931b5c..5f9526b714e3c 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -167,9 +167,6 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { // every coverage span has a `Counter` or `Expression` assigned to its `BasicCoverageBlock` // and all `Expression` dependencies (operands) are also generated, for any other // `BasicCoverageBlock`s not already associated with a coverage span. - // - // Intermediate expressions (used to compute other `Expression` values), which have no - // direct association with any `BasicCoverageBlock`, are accumulated inside `coverage_counters`. let bcb_has_coverage_spans = |bcb| coverage_spans.bcb_has_coverage_spans(bcb); let result = self .coverage_counters @@ -195,29 +192,16 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { // are in fact counted, even though they don't directly contribute to counting // their own independent code region's coverage. self.inject_indirect_counters(); - - // Intermediate expressions will be injected as the final step, after generating - // debug output, if any. - //////////////////////////////////////////////////// }; if let Err(e) = result { bug!("Error processing: {:?}: {:?}", self.mir_body.source.def_id(), e.message) }; - //////////////////////////////////////////////////// - // Finally, inject the intermediate expressions collected along the way. - for intermediate_expression in &self.coverage_counters.intermediate_expressions { - inject_intermediate_expression( - self.mir_body, - self.make_mir_coverage_kind(intermediate_expression), - ); - } - self.mir_body.function_coverage_info = Some(Box::new(FunctionCoverageInfo { function_source_hash: self.function_source_hash, num_counters: self.coverage_counters.num_counters(), - num_expressions: self.coverage_counters.num_expressions(), + expressions: std::mem::take(&mut self.coverage_counters.expressions), mappings: std::mem::take(&mut self.mappings), })); } @@ -304,10 +288,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { inject_to_bb, ); } - BcbCounter::Expression { .. } => inject_intermediate_expression( - self.mir_body, - self.make_mir_coverage_kind(&counter_kind), - ), + BcbCounter::Expression { .. } => {} } } } @@ -330,9 +311,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { fn make_mir_coverage_kind(&self, counter_kind: &BcbCounter) -> CoverageKind { match *counter_kind { BcbCounter::Counter { id } => CoverageKind::Counter { id }, - BcbCounter::Expression { id, lhs, op, rhs } => { - CoverageKind::Expression { id, lhs, op, rhs } - } + BcbCounter::Expression { id } => CoverageKind::Expression { id }, } } } @@ -371,20 +350,6 @@ fn inject_statement(mir_body: &mut mir::Body<'_>, counter_kind: CoverageKind, bb data.statements.insert(0, statement); } -// Non-code expressions are injected into the coverage map, without generating executable code. -fn inject_intermediate_expression(mir_body: &mut mir::Body<'_>, expression: CoverageKind) { - debug_assert!(matches!(expression, CoverageKind::Expression { .. })); - debug!(" injecting non-code expression {:?}", expression); - let inject_in_bb = mir::START_BLOCK; - let data = &mut mir_body[inject_in_bb]; - let source_info = data.terminator().source_info; - let statement = Statement { - source_info, - kind: StatementKind::Coverage(Box::new(Coverage { kind: expression })), - }; - data.statements.push(statement); -} - /// Convert the Span into its file name, start line and column, and end line and column fn make_code_region( source_map: &SourceMap, diff --git a/compiler/rustc_mir_transform/src/coverage/tests.rs b/compiler/rustc_mir_transform/src/coverage/tests.rs index 7476d3ce9272a..02544ee4eb03d 100644 --- a/compiler/rustc_mir_transform/src/coverage/tests.rs +++ b/compiler/rustc_mir_transform/src/coverage/tests.rs @@ -656,7 +656,7 @@ fn test_make_bcb_counters() { coverage_counters .make_bcb_counters(&mut basic_coverage_blocks, bcb_has_coverage_spans) .expect("should be Ok"); - assert_eq!(coverage_counters.intermediate_expressions.len(), 0); + assert!(coverage_counters.expressions.is_empty()); let_bcb!(1); assert_eq!( diff --git a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff index 57c221cb26381..e10ea330c8796 100644 --- a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff @@ -13,7 +13,7 @@ } bb1: { -+ Coverage::Expression(0) = Counter(0) + Counter(1); ++ Coverage::Expression(0); falseUnwind -> [real: bb2, unwind: bb6]; } @@ -27,7 +27,7 @@ } bb4: { -+ Coverage::Expression(1) = Expression(0) - Counter(1); ++ Coverage::Expression(1); _0 = const (); StorageDead(_2); return;