From c479bc7f3b8bb0441bef0634b0a60cfdd1ac0914 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 10 Sep 2023 16:35:37 +1000 Subject: [PATCH 01/10] coverage: Attach an optional `FunctionCoverageInfo` to `mir::Body` This allows coverage information to be attached to the function as a whole when appropriate, instead of being smuggled through coverage statements in the function's basic blocks. As an example, this patch moves the `function_source_hash` value out of individual `CoverageKind::Counter` statements and into the per-function info. When synthesizing unused functions for coverage purposes, the absence of this info is taken to indicate that a function was not eligible for coverage and should not be synthesized. --- .../src/coverageinfo/map_data.rs | 52 +++++++++---------- .../src/coverageinfo/mapgen.rs | 17 +++--- .../src/coverageinfo/mod.rs | 28 +++++----- compiler/rustc_middle/src/mir/coverage.rs | 12 ++++- compiler/rustc_middle/src/mir/mod.rs | 10 ++++ .../rustc_mir_build/src/build/custom/mod.rs | 1 + .../rustc_mir_transform/src/coverage/mod.rs | 8 +-- 7 files changed, 74 insertions(+), 54 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 55f43aa5341..f9d76108302 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -2,7 +2,9 @@ use crate::coverageinfo::ffi::{Counter, CounterExpression, ExprKind}; use rustc_data_structures::fx::FxIndexSet; use rustc_index::IndexVec; -use rustc_middle::mir::coverage::{CodeRegion, CounterId, ExpressionId, Op, Operand}; +use rustc_middle::mir::coverage::{ + CodeRegion, CounterId, ExpressionId, FunctionCoverageInfo, Op, Operand, +}; use rustc_middle::ty::Instance; use rustc_middle::ty::TyCtxt; @@ -27,8 +29,8 @@ pub struct Expression { /// line." #[derive(Debug)] pub struct FunctionCoverage<'tcx> { - instance: Instance<'tcx>, - source_hash: u64, + /// Coverage info that was attached to this function by the instrumentor. + function_coverage_info: &'tcx FunctionCoverageInfo, is_used: bool, counters: IndexVec>>, expressions: IndexVec>, @@ -37,24 +39,36 @@ pub struct FunctionCoverage<'tcx> { impl<'tcx> FunctionCoverage<'tcx> { /// Creates a new set of coverage data for a used (called) function. - pub fn new(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Self { - Self::create(tcx, instance, true) + pub fn new( + tcx: TyCtxt<'tcx>, + instance: Instance<'tcx>, + function_coverage_info: &'tcx FunctionCoverageInfo, + ) -> Self { + Self::create(tcx, instance, function_coverage_info, true) } /// Creates a new set of coverage data for an unused (never called) function. - pub fn unused(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Self { - Self::create(tcx, instance, false) + pub fn unused( + tcx: TyCtxt<'tcx>, + instance: Instance<'tcx>, + function_coverage_info: &'tcx FunctionCoverageInfo, + ) -> Self { + Self::create(tcx, instance, function_coverage_info, false) } - fn create(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>, is_used: bool) -> Self { + fn create( + tcx: TyCtxt<'tcx>, + instance: Instance<'tcx>, + function_coverage_info: &'tcx FunctionCoverageInfo, + is_used: bool, + ) -> Self { let coverageinfo = tcx.coverageinfo(instance.def); debug!( "FunctionCoverage::create(instance={:?}) has coverageinfo={:?}. is_used={}", instance, coverageinfo, is_used ); Self { - instance, - source_hash: 0, // will be set with the first `add_counter()` + function_coverage_info, is_used, counters: IndexVec::from_elem_n(None, coverageinfo.num_counters as usize), expressions: IndexVec::from_elem_n(None, coverageinfo.num_expressions as usize), @@ -67,16 +81,6 @@ impl<'tcx> FunctionCoverage<'tcx> { self.is_used } - /// Sets the function source hash value. If called multiple times for the same function, all - /// calls should have the same hash value. - pub fn set_function_source_hash(&mut self, source_hash: u64) { - if self.source_hash == 0 { - self.source_hash = source_hash; - } else { - debug_assert_eq!(source_hash, self.source_hash); - } - } - /// Adds code regions to be counted by an injected counter intrinsic. #[instrument(level = "debug", skip(self))] pub(crate) fn add_counter(&mut self, id: CounterId, code_regions: &[CodeRegion]) { @@ -195,7 +199,7 @@ impl<'tcx> FunctionCoverage<'tcx> { /// Return the source hash, generated from the HIR node structure, and used to indicate whether /// or not the source code structure changed between different compilations. pub fn source_hash(&self) -> u64 { - self.source_hash + if self.is_used { self.function_coverage_info.function_source_hash } else { 0 } } /// Generate an array of CounterExpressions, and an iterator over all `Counter`s and their @@ -204,12 +208,6 @@ impl<'tcx> FunctionCoverage<'tcx> { pub fn get_expressions_and_counter_regions( &self, ) -> (Vec, impl Iterator) { - assert!( - self.source_hash != 0 || !self.is_used, - "No counters provided the source_hash for used function: {:?}", - self.instance - ); - let counter_expressions = self.counter_expressions(); // Expression IDs are indices into `self.expressions`, and on the LLVM // side they will be treated as indices into `counter_expressions`, so diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index d4e77525698..a2bf53e605d 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -10,9 +10,8 @@ use rustc_hir::def::DefKind; use rustc_hir::def_id::DefId; use rustc_index::IndexVec; use rustc_middle::bug; -use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags; use rustc_middle::mir::coverage::CodeRegion; -use rustc_middle::ty::TyCtxt; +use rustc_middle::ty::{self, TyCtxt}; use rustc_span::Symbol; /// Generates and exports the Coverage Map. @@ -331,16 +330,14 @@ fn add_unused_functions(cx: &CodegenCx<'_, '_>) { for non_codegenned_def_id in eligible_def_ids.into_iter().filter(|id| !codegenned_def_ids.contains(id)) { - let codegen_fn_attrs = tcx.codegen_fn_attrs(non_codegenned_def_id); - - // If a function is marked `#[coverage(off)]`, then skip generating a - // dead code stub for it. - if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NO_COVERAGE) { - debug!("skipping unused fn marked #[coverage(off)]: {:?}", non_codegenned_def_id); + // Skip any function that didn't have coverage data added to it by the + // coverage instrumentor. + let body = tcx.instance_mir(ty::InstanceDef::Item(non_codegenned_def_id)); + let Some(function_coverage_info) = body.function_coverage_info.as_deref() else { continue; - } + }; debug!("generating unused fn: {:?}", non_codegenned_def_id); - cx.define_unused_fn(non_codegenned_def_id); + cx.define_unused_fn(non_codegenned_def_id, function_coverage_info); } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index dd2ce9b525b..0d9d3c7ad41 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -16,7 +16,7 @@ use rustc_hir as hir; use rustc_hir::def_id::DefId; use rustc_llvm::RustString; use rustc_middle::bug; -use rustc_middle::mir::coverage::{CounterId, CoverageKind}; +use rustc_middle::mir::coverage::{CounterId, CoverageKind, FunctionCoverageInfo}; use rustc_middle::mir::Coverage; use rustc_middle::ty; use rustc_middle::ty::layout::{FnAbiOf, HasTyCtxt}; @@ -91,31 +91,34 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> { /// codegenned, collect the coverage `CodeRegion`s from the MIR and add /// them. Since the function is never called, all of its `CodeRegion`s can be /// added as `unreachable_region`s. - fn define_unused_fn(&self, def_id: DefId) { + fn define_unused_fn(&self, def_id: DefId, function_coverage_info: &'tcx FunctionCoverageInfo) { let instance = declare_unused_fn(self, def_id); codegen_unused_fn_and_counter(self, instance); - add_unused_function_coverage(self, instance, def_id); + add_unused_function_coverage(self, instance, def_id, function_coverage_info); } } impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { + #[instrument(level = "debug", skip(self))] fn add_coverage(&mut self, instance: Instance<'tcx>, coverage: &Coverage) { let bx = self; + let Some(function_coverage_info) = + bx.tcx.instance_mir(instance.def).function_coverage_info.as_deref() + else { + debug!("function has a coverage statement but no coverage info"); + return; + }; + let Some(coverage_context) = bx.coverage_context() else { return }; let mut coverage_map = coverage_context.function_coverage_map.borrow_mut(); let func_coverage = coverage_map .entry(instance) - .or_insert_with(|| FunctionCoverage::new(bx.tcx(), instance)); + .or_insert_with(|| FunctionCoverage::new(bx.tcx(), instance, function_coverage_info)); let Coverage { kind, code_regions } = coverage; match *kind { - CoverageKind::Counter { function_source_hash, id } => { - debug!( - "ensuring function source hash is set for instance={:?}; function_source_hash={}", - instance, function_source_hash, - ); - func_coverage.set_function_source_hash(function_source_hash); + CoverageKind::Counter { id } => { func_coverage.add_counter(id, code_regions); // We need to explicitly drop the `RefMut` before calling into `instrprof_increment`, // as that needs an exclusive borrow. @@ -124,7 +127,7 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { let coverageinfo = bx.tcx().coverageinfo(instance.def); let fn_name = bx.get_pgo_func_name_var(instance); - let hash = bx.const_u64(function_source_hash); + let hash = bx.const_u64(function_coverage_info.function_source_hash); let num_counters = bx.const_u32(coverageinfo.num_counters); let index = bx.const_u32(id.as_u32()); debug!( @@ -201,10 +204,11 @@ fn add_unused_function_coverage<'tcx>( cx: &CodegenCx<'_, 'tcx>, instance: Instance<'tcx>, def_id: DefId, + function_coverage_info: &'tcx FunctionCoverageInfo, ) { let tcx = cx.tcx; - let mut function_coverage = FunctionCoverage::unused(tcx, instance); + let mut function_coverage = FunctionCoverage::unused(tcx, instance, function_coverage_info); for &code_region in tcx.covered_code_regions(def_id) { let code_region = std::slice::from_ref(code_region); function_coverage.add_unreachable_regions(code_region); diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index a6d6f6f5df4..e1434e71a47 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -60,7 +60,6 @@ impl Debug for Operand { #[derive(Clone, PartialEq, TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] pub enum CoverageKind { Counter { - function_source_hash: u64, /// ID of this counter within its enclosing function. /// Expressions in the same function can refer to it as an operand. id: CounterId, @@ -80,7 +79,7 @@ impl Debug for CoverageKind { fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result { use CoverageKind::*; match self { - Counter { id, .. } => write!(fmt, "Counter({:?})", id.index()), + Counter { id } => write!(fmt, "Counter({:?})", id.index()), Expression { id, lhs, op, rhs } => write!( fmt, "Expression({:?}) = {:?} {} {:?}", @@ -133,3 +132,12 @@ impl Op { matches!(self, Self::Subtract) } } + +/// Stores per-function coverage information attached to a `mir::Body`, +/// to be used in conjunction with the individual coverage statements injected +/// into the function's basic blocks. +#[derive(Clone, Debug)] +#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] +pub struct FunctionCoverageInfo { + pub function_source_hash: u64, +} diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index f979f736b15..3a5ff4dc91f 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -345,6 +345,14 @@ pub struct Body<'tcx> { pub injection_phase: Option, pub tainted_by_errors: Option, + + /// Per-function coverage information added by the `InstrumentCoverage` + /// pass, to be used in conjunction with the coverage statements injected + /// into this body's blocks. + /// + /// If `-Cinstrument-coverage` is not active, or if an individual function + /// is not eligible for coverage, then this should always be `None`. + pub function_coverage_info: Option>, } impl<'tcx> Body<'tcx> { @@ -392,6 +400,7 @@ impl<'tcx> Body<'tcx> { is_polymorphic: false, injection_phase: None, tainted_by_errors, + function_coverage_info: None, }; body.is_polymorphic = body.has_non_region_param(); body @@ -420,6 +429,7 @@ impl<'tcx> Body<'tcx> { is_polymorphic: false, injection_phase: None, tainted_by_errors: None, + function_coverage_info: None, }; body.is_polymorphic = body.has_non_region_param(); body diff --git a/compiler/rustc_mir_build/src/build/custom/mod.rs b/compiler/rustc_mir_build/src/build/custom/mod.rs index e5c2cc6c7bb..a81f70e3346 100644 --- a/compiler/rustc_mir_build/src/build/custom/mod.rs +++ b/compiler/rustc_mir_build/src/build/custom/mod.rs @@ -60,6 +60,7 @@ pub(super) fn build_custom_mir<'tcx>( tainted_by_errors: None, injection_phase: None, pass_count: 0, + function_coverage_info: None, }; body.local_decls.push(LocalDecl::new(return_ty, return_ty_span)); diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index abf13519e9e..1ce6ecc3bb5 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -211,6 +211,10 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { self.make_mir_coverage_kind(intermediate_expression), ); } + + self.mir_body.function_coverage_info = Some(Box::new(FunctionCoverageInfo { + function_source_hash: self.function_source_hash, + })); } /// Injects a single [`StatementKind::Coverage`] for each BCB that has one @@ -323,9 +327,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 { function_source_hash: self.function_source_hash, id } - } + BcbCounter::Counter { id } => CoverageKind::Counter { id }, BcbCounter::Expression { id, lhs, op, rhs } => { CoverageKind::Expression { id, lhs, op, rhs } } From a18c5f3b751dbfa6c192dc48a6c6f28250e9bc97 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 13 Sep 2023 12:15:40 +1000 Subject: [PATCH 02/10] coverage: Store the number of counters/expressions in function coverage info Coverage codegen can now allocate arrays based on the number of counters/expressions originally used by the instrumentor. The existing query that inspects coverage statements is still used for determining the number of counters passed to `llvm.instrprof.increment`. If some high-numbered counters were removed by MIR optimizations, the instrumented binary can potentially use less memory and disk space at runtime. --- .../src/coverageinfo/map_data.rs | 19 ++-- .../src/coverageinfo/mod.rs | 18 +++- compiler/rustc_middle/src/mir/coverage.rs | 2 + compiler/rustc_middle/src/mir/query.rs | 24 +++-- compiler/rustc_middle/src/query/mod.rs | 9 +- .../src/coverage/counters.rs | 8 ++ .../rustc_mir_transform/src/coverage/mod.rs | 2 + .../rustc_mir_transform/src/coverage/query.rs | 89 ++++--------------- 8 files changed, 69 insertions(+), 102 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index f9d76108302..4dbc0e403fd 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -6,7 +6,6 @@ use rustc_middle::mir::coverage::{ CodeRegion, CounterId, ExpressionId, FunctionCoverageInfo, Op, Operand, }; use rustc_middle::ty::Instance; -use rustc_middle::ty::TyCtxt; #[derive(Clone, Debug, PartialEq)] pub struct Expression { @@ -40,38 +39,36 @@ pub struct FunctionCoverage<'tcx> { impl<'tcx> FunctionCoverage<'tcx> { /// Creates a new set of coverage data for a used (called) function. pub fn new( - tcx: TyCtxt<'tcx>, instance: Instance<'tcx>, function_coverage_info: &'tcx FunctionCoverageInfo, ) -> Self { - Self::create(tcx, instance, function_coverage_info, true) + Self::create(instance, function_coverage_info, true) } /// Creates a new set of coverage data for an unused (never called) function. pub fn unused( - tcx: TyCtxt<'tcx>, instance: Instance<'tcx>, function_coverage_info: &'tcx FunctionCoverageInfo, ) -> Self { - Self::create(tcx, instance, function_coverage_info, false) + Self::create(instance, function_coverage_info, false) } fn create( - tcx: TyCtxt<'tcx>, instance: Instance<'tcx>, function_coverage_info: &'tcx FunctionCoverageInfo, is_used: bool, ) -> Self { - let coverageinfo = tcx.coverageinfo(instance.def); + let num_counters = function_coverage_info.num_counters; + let num_expressions = function_coverage_info.num_expressions; debug!( - "FunctionCoverage::create(instance={:?}) has coverageinfo={:?}. is_used={}", - instance, coverageinfo, is_used + "FunctionCoverage::create(instance={instance:?}) has \ + num_counters={num_counters}, num_expressions={num_expressions}, is_used={is_used}" ); Self { function_coverage_info, is_used, - counters: IndexVec::from_elem_n(None, coverageinfo.num_counters as usize), - expressions: IndexVec::from_elem_n(None, coverageinfo.num_expressions as usize), + counters: IndexVec::from_elem_n(None, num_counters), + expressions: IndexVec::from_elem_n(None, num_expressions), unreachable_regions: Vec::new(), } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 0d9d3c7ad41..c975d3432b9 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -114,7 +114,7 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { let mut coverage_map = coverage_context.function_coverage_map.borrow_mut(); let func_coverage = coverage_map .entry(instance) - .or_insert_with(|| FunctionCoverage::new(bx.tcx(), instance, function_coverage_info)); + .or_insert_with(|| FunctionCoverage::new(instance, function_coverage_info)); let Coverage { kind, code_regions } = coverage; match *kind { @@ -124,11 +124,21 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { // as that needs an exclusive borrow. drop(coverage_map); - let coverageinfo = bx.tcx().coverageinfo(instance.def); + // The number of counters passed to `llvm.instrprof.increment` might + // be smaller than the number originally inserted by the instrumentor, + // if some high-numbered counters were removed by MIR optimizations. + // If so, LLVM's profiler runtime will use fewer physical counters. + let num_counters = + bx.tcx().coverage_ids_info(instance.def).max_counter_id.as_u32() + 1; + assert!( + num_counters as usize <= function_coverage_info.num_counters, + "num_counters disagreement: query says {num_counters} but function info only has {}", + function_coverage_info.num_counters + ); let fn_name = bx.get_pgo_func_name_var(instance); let hash = bx.const_u64(function_coverage_info.function_source_hash); - let num_counters = bx.const_u32(coverageinfo.num_counters); + let num_counters = bx.const_u32(num_counters); let index = bx.const_u32(id.as_u32()); debug!( "codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?}, num_counters={:?}, index={:?})", @@ -208,7 +218,7 @@ fn add_unused_function_coverage<'tcx>( ) { let tcx = cx.tcx; - let mut function_coverage = FunctionCoverage::unused(tcx, instance, function_coverage_info); + let mut function_coverage = FunctionCoverage::unused(instance, function_coverage_info); for &code_region in tcx.covered_code_regions(def_id) { let code_region = std::slice::from_ref(code_region); function_coverage.add_unreachable_regions(code_region); diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index e1434e71a47..837331a2de8 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -140,4 +140,6 @@ impl Op { #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] pub struct FunctionCoverageInfo { pub function_source_hash: u64, + pub num_counters: usize, + pub num_expressions: usize, } diff --git a/compiler/rustc_middle/src/mir/query.rs b/compiler/rustc_middle/src/mir/query.rs index c74a9536b63..f407dc4d7ae 100644 --- a/compiler/rustc_middle/src/mir/query.rs +++ b/compiler/rustc_middle/src/mir/query.rs @@ -1,5 +1,6 @@ //! Values computed by queries that use MIR. +use crate::mir; use crate::ty::{self, OpaqueHiddenType, Ty, TyCtxt}; use rustc_data_structures::fx::FxIndexMap; use rustc_data_structures::unord::UnordSet; @@ -445,14 +446,19 @@ pub struct DestructuredConstant<'tcx> { pub fields: &'tcx [(ConstValue<'tcx>, Ty<'tcx>)], } -/// Coverage information summarized from a MIR if instrumented for source code coverage (see -/// compiler option `-Cinstrument-coverage`). This information is generated by the -/// `InstrumentCoverage` MIR pass and can be retrieved via the `coverageinfo` query. +/// Summarizes coverage IDs inserted by the `InstrumentCoverage` MIR pass +/// (for compiler option `-Cinstrument-coverage`), after MIR optimizations +/// have had a chance to potentially remove some of them. +/// +/// Used by the `coverage_ids_info` query. #[derive(Clone, TyEncodable, TyDecodable, Debug, HashStable)] -pub struct CoverageInfo { - /// The total number of coverage region counters added to the MIR `Body`. - pub num_counters: u32, - - /// The total number of coverage region counter expressions added to the MIR `Body`. - pub num_expressions: u32, +pub struct CoverageIdsInfo { + /// Coverage codegen needs to know the highest counter ID that is ever + /// incremented within a function, so that it can set the `num-counters` + /// argument of the `llvm.instrprof.increment` intrinsic. + /// + /// This may be less than the highest counter ID emitted by the + /// InstrumentCoverage MIR pass, if the highest-numbered counter increments + /// were removed by MIR optimizations. + pub max_counter_id: mir::coverage::CounterId, } diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs index 940ab190ffc..ed0f73fcf2f 100644 --- a/compiler/rustc_middle/src/query/mod.rs +++ b/compiler/rustc_middle/src/query/mod.rs @@ -573,10 +573,11 @@ rustc_queries! { separate_provide_extern } - /// Returns coverage summary info for a function, after executing the `InstrumentCoverage` - /// MIR pass (assuming the -Cinstrument-coverage option is enabled). - query coverageinfo(key: ty::InstanceDef<'tcx>) -> &'tcx mir::CoverageInfo { - desc { |tcx| "retrieving coverage info from MIR for `{}`", tcx.def_path_str(key.def_id()) } + /// Summarizes coverage IDs inserted by the `InstrumentCoverage` MIR pass + /// (for compiler option `-Cinstrument-coverage`), after MIR optimizations + /// have had a chance to potentially remove some of them. + query coverage_ids_info(key: ty::InstanceDef<'tcx>) -> &'tcx mir::CoverageIdsInfo { + desc { |tcx| "retrieving coverage IDs info from MIR for `{}`", tcx.def_path_str(key.def_id()) } arena_cache } diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index 77e3dee1fef..938c4b2bd3f 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -126,6 +126,14 @@ impl CoverageCounters { 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, diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index 1ce6ecc3bb5..e1b656bd981 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -214,6 +214,8 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { 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(), })); } diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs index 2c0164e765c..b1f46c8e05c 100644 --- a/compiler/rustc_mir_transform/src/coverage/query.rs +++ b/compiler/rustc_mir_transform/src/coverage/query.rs @@ -2,92 +2,33 @@ use super::*; use rustc_data_structures::captures::Captures; use rustc_middle::mir::coverage::*; -use rustc_middle::mir::{self, Body, Coverage, CoverageInfo}; +use rustc_middle::mir::{self, Body, Coverage, CoverageIdsInfo}; use rustc_middle::query::Providers; use rustc_middle::ty::{self, TyCtxt}; use rustc_span::def_id::DefId; /// A `query` provider for retrieving coverage information injected into MIR. pub(crate) fn provide(providers: &mut Providers) { - providers.coverageinfo = |tcx, def_id| coverageinfo(tcx, def_id); + providers.coverage_ids_info = |tcx, def_id| coverage_ids_info(tcx, def_id); providers.covered_code_regions = |tcx, def_id| covered_code_regions(tcx, def_id); } -/// Coverage codegen needs to know the total number of counter IDs and expression IDs that have -/// been used by a function's coverage mappings. These totals are used to create vectors to hold -/// the relevant counter and expression data, and the maximum counter ID (+ 1) is also needed by -/// the `llvm.instrprof.increment` intrinsic. -/// -/// MIR optimization may split and duplicate some BasicBlock sequences, or optimize out some code -/// including injected counters. (It is OK if some counters are optimized out, but those counters -/// are still included in the total `num_counters` or `num_expressions`.) Simply counting the -/// calls may not work; but computing the number of counters or expressions by adding `1` to the -/// highest ID (for a given instrumented function) is valid. -/// -/// It's possible for a coverage expression to remain in MIR while one or both of its operands -/// have been optimized away. To avoid problems in codegen, we include those operands' IDs when -/// determining the maximum counter/expression ID, even if the underlying counter/expression is -/// no longer present. -struct CoverageVisitor { - max_counter_id: CounterId, - max_expression_id: ExpressionId, -} - -impl CoverageVisitor { - /// Updates `max_counter_id` to the maximum encountered counter ID. - #[inline(always)] - fn update_max_counter_id(&mut self, counter_id: CounterId) { - self.max_counter_id = self.max_counter_id.max(counter_id); - } - - /// Updates `max_expression_id` to the maximum encountered expression ID. - #[inline(always)] - fn update_max_expression_id(&mut self, expression_id: ExpressionId) { - self.max_expression_id = self.max_expression_id.max(expression_id); - } - - fn update_from_expression_operand(&mut self, operand: Operand) { - match operand { - Operand::Counter(id) => self.update_max_counter_id(id), - Operand::Expression(id) => self.update_max_expression_id(id), - Operand::Zero => {} - } - } - - fn visit_body(&mut self, body: &Body<'_>) { - for coverage in all_coverage_in_mir_body(body) { - self.visit_coverage(coverage); - } - } - - fn visit_coverage(&mut self, coverage: &Coverage) { - match coverage.kind { - CoverageKind::Counter { id, .. } => self.update_max_counter_id(id), - CoverageKind::Expression { id, lhs, rhs, .. } => { - self.update_max_expression_id(id); - self.update_from_expression_operand(lhs); - self.update_from_expression_operand(rhs); - } - CoverageKind::Unreachable => {} - } - } -} - -fn coverageinfo<'tcx>(tcx: TyCtxt<'tcx>, instance_def: ty::InstanceDef<'tcx>) -> CoverageInfo { +/// Query implementation for `coverage_ids_info`. +fn coverage_ids_info<'tcx>( + tcx: TyCtxt<'tcx>, + instance_def: ty::InstanceDef<'tcx>, +) -> CoverageIdsInfo { let mir_body = tcx.instance_mir(instance_def); - let mut coverage_visitor = CoverageVisitor { - max_counter_id: CounterId::START, - max_expression_id: ExpressionId::START, - }; + let max_counter_id = all_coverage_in_mir_body(mir_body) + .filter_map(|coverage| match coverage.kind { + CoverageKind::Counter { id } => Some(id), + _ => None, + }) + .max() + .unwrap_or(CounterId::START); - coverage_visitor.visit_body(mir_body); - - // Add 1 to the highest IDs to get the total number of IDs. - CoverageInfo { - num_counters: (coverage_visitor.max_counter_id + 1).as_u32(), - num_expressions: (coverage_visitor.max_expression_id + 1).as_u32(), - } + CoverageIdsInfo { max_counter_id } } fn covered_code_regions(tcx: TyCtxt<'_>, def_id: DefId) -> Vec<&CodeRegion> { From 79f935b96c2447c979124628187125a9e381e9dc Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 31 Aug 2023 16:03:12 +1000 Subject: [PATCH 03/10] coverage: Rename `Operand` to `CovTerm` Later patches in this PR will use `CovTerm` to represent things that are not expression operands. --- .../src/coverageinfo/ffi.rs | 12 +++--- .../src/coverageinfo/map_data.rs | 32 +++++++-------- compiler/rustc_middle/src/mir/coverage.rs | 16 ++++---- .../src/coverage/counters.rs | 40 +++++++++---------- 4 files changed, 51 insertions(+), 49 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs index 763186a58bf..194ed7d661d 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs @@ -1,4 +1,4 @@ -use rustc_middle::mir::coverage::{CounterId, ExpressionId, Operand}; +use rustc_middle::mir::coverage::{CounterId, CovTerm, ExpressionId}; /// Must match the layout of `LLVMRustCounterKind`. #[derive(Copy, Clone, Debug)] @@ -43,11 +43,11 @@ impl Counter { Self { kind: CounterKind::Expression, id: expression_id.as_u32() } } - pub(crate) fn from_operand(operand: Operand) -> Self { - match operand { - Operand::Zero => Self::ZERO, - Operand::Counter(id) => Self::counter_value_reference(id), - Operand::Expression(id) => Self::expression(id), + pub(crate) fn from_term(term: CovTerm) -> Self { + match term { + CovTerm::Zero => Self::ZERO, + CovTerm::Counter(id) => Self::counter_value_reference(id), + CovTerm::Expression(id) => Self::expression(id), } } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 4dbc0e403fd..9798c18f21b 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -3,15 +3,15 @@ use crate::coverageinfo::ffi::{Counter, CounterExpression, ExprKind}; use rustc_data_structures::fx::FxIndexSet; use rustc_index::IndexVec; use rustc_middle::mir::coverage::{ - CodeRegion, CounterId, ExpressionId, FunctionCoverageInfo, Op, Operand, + CodeRegion, CounterId, CovTerm, ExpressionId, FunctionCoverageInfo, Op, }; use rustc_middle::ty::Instance; #[derive(Clone, Debug, PartialEq)] pub struct Expression { - lhs: Operand, + lhs: CovTerm, op: Op, - rhs: Operand, + rhs: CovTerm, code_regions: Vec, } @@ -101,15 +101,15 @@ impl<'tcx> FunctionCoverage<'tcx> { /// code regions mapped to that expression. /// /// Both counters and "counter expressions" (or simply, "expressions") can be operands in other - /// expressions. These are tracked as separate variants of `Operand`, so there is no ambiguity + /// expressions. These are tracked as separate variants of `CovTerm`, so there is no ambiguity /// between operands that are counter IDs and operands that are expression IDs. #[instrument(level = "debug", skip(self))] pub(crate) fn add_counter_expression( &mut self, expression_id: ExpressionId, - lhs: Operand, + lhs: CovTerm, op: Op, - rhs: Operand, + rhs: CovTerm, code_regions: &[CodeRegion], ) { debug_assert!( @@ -151,7 +151,7 @@ impl<'tcx> FunctionCoverage<'tcx> { // The set of expressions that either were optimized out entirely, or // have zero as both of their operands, and will therefore always have // a value of zero. Other expressions that refer to these as operands - // can have those operands replaced with `Operand::Zero`. + // can have those operands replaced with `CovTerm::Zero`. let mut zero_expressions = FxIndexSet::default(); // For each expression, perform simplifications based on lower-numbered @@ -168,10 +168,10 @@ impl<'tcx> FunctionCoverage<'tcx> { }; // If an operand refers to an expression that is always zero, then - // that operand can be replaced with `Operand::Zero`. - let maybe_set_operand_to_zero = |operand: &mut Operand| match &*operand { - Operand::Expression(id) if zero_expressions.contains(id) => { - *operand = Operand::Zero; + // that operand can be replaced with `CovTerm::Zero`. + let maybe_set_operand_to_zero = |operand: &mut CovTerm| match &*operand { + CovTerm::Expression(id) if zero_expressions.contains(id) => { + *operand = CovTerm::Zero; } _ => (), }; @@ -181,13 +181,13 @@ impl<'tcx> FunctionCoverage<'tcx> { // Coverage counter values cannot be negative, so if an expression // involves subtraction from zero, assume that its RHS must also be zero. // (Do this after simplifications that could set the LHS to zero.) - if let Expression { lhs: Operand::Zero, op: Op::Subtract, .. } = expression { - expression.rhs = Operand::Zero; + if let Expression { lhs: CovTerm::Zero, op: Op::Subtract, .. } = expression { + expression.rhs = CovTerm::Zero; } // After the above simplifications, if both operands are zero, then // we know that this expression is always zero too. - if let Expression { lhs: Operand::Zero, rhs: Operand::Zero, .. } = expression { + if let Expression { lhs: CovTerm::Zero, rhs: CovTerm::Zero, .. } = expression { zero_expressions.insert(id); } } @@ -254,12 +254,12 @@ impl<'tcx> FunctionCoverage<'tcx> { &Some(Expression { lhs, op, rhs, .. }) => { // Convert the operands and operator as normal. CounterExpression::new( - Counter::from_operand(lhs), + Counter::from_term(lhs), match op { Op::Add => ExprKind::Add, Op::Subtract => ExprKind::Subtract, }, - Counter::from_operand(rhs), + Counter::from_term(rhs), ) } }) diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 837331a2de8..fbb7f33a983 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -35,19 +35,21 @@ impl ExpressionId { pub const START: Self = Self::from_u32(0); } -/// Operand of a coverage-counter expression. +/// Enum that can hold a constant zero value, the ID of an physical coverage +/// counter, or the ID of a coverage-counter expression. /// -/// Operands can be a constant zero value, an actual coverage counter, or another -/// expression. Counter/expression operands are referred to by ID. +/// This was originally only used for expression operands (and named `Operand`), +/// but the zero/counter/expression distinction is also useful for representing +/// the value of code/gap mappings, and the true/false arms of branch mappings. #[derive(Copy, Clone, PartialEq, Eq)] #[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] -pub enum Operand { +pub enum CovTerm { Zero, Counter(CounterId), Expression(ExpressionId), } -impl Debug for Operand { +impl Debug for CovTerm { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { match self { Self::Zero => write!(f, "Zero"), @@ -68,9 +70,9 @@ 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: Operand, + lhs: CovTerm, op: Op, - rhs: Operand, + rhs: CovTerm, }, Unreachable, } diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index 938c4b2bd3f..4140d402c75 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: Operand, op: Op, rhs: Operand }, + Expression { id: ExpressionId, lhs: CovTerm, op: Op, rhs: CovTerm }, } impl BcbCounter { @@ -27,10 +27,10 @@ impl BcbCounter { matches!(self, Self::Expression { .. }) } - pub(super) fn as_operand(&self) -> Operand { + pub(super) fn as_term(&self) -> CovTerm { match *self { - BcbCounter::Counter { id, .. } => Operand::Counter(id), - BcbCounter::Expression { id, .. } => Operand::Expression(id), + BcbCounter::Counter { id, .. } => CovTerm::Counter(id), + BcbCounter::Expression { id, .. } => CovTerm::Expression(id), } } } @@ -106,7 +106,7 @@ impl CoverageCounters { BcbCounter::Counter { id } } - fn make_expression(&mut self, lhs: Operand, op: Op, rhs: Operand) -> BcbCounter { + fn make_expression(&mut self, lhs: CovTerm, op: Op, rhs: CovTerm) -> BcbCounter { let id = self.next_expression(); BcbCounter::Expression { id, lhs, op, rhs } } @@ -138,7 +138,7 @@ impl CoverageCounters { &mut self, bcb: BasicCoverageBlock, counter_kind: BcbCounter, - ) -> Result { + ) -> Result { debug_assert!( // If the BCB has an edge counter (to be injected into a new `BasicBlock`), it can also // have an expression (to be injected into an existing `BasicBlock` represented by this @@ -146,14 +146,14 @@ impl CoverageCounters { counter_kind.is_expression() || !self.bcb_has_incoming_edge_counters.contains(bcb), "attempt to add a `Counter` to a BCB target with existing incoming edge counters" ); - let operand = counter_kind.as_operand(); + let term = counter_kind.as_term(); if let Some(replaced) = self.bcb_counters[bcb].replace(counter_kind) { Error::from_string(format!( "attempt to set a BasicCoverageBlock coverage counter more than once; \ {bcb:?} already had counter {replaced:?}", )) } else { - Ok(operand) + Ok(term) } } @@ -162,7 +162,7 @@ impl CoverageCounters { from_bcb: BasicCoverageBlock, to_bcb: BasicCoverageBlock, counter_kind: BcbCounter, - ) -> Result { + ) -> Result { if level_enabled!(tracing::Level::DEBUG) { // If the BCB has an edge counter (to be injected into a new `BasicBlock`), it can also // have an expression (to be injected into an existing `BasicBlock` represented by this @@ -175,14 +175,14 @@ impl CoverageCounters { } } self.bcb_has_incoming_edge_counters.insert(to_bcb); - let operand = counter_kind.as_operand(); + let term = counter_kind.as_term(); if let Some(replaced) = self.bcb_edge_counters.insert((from_bcb, to_bcb), counter_kind) { Error::from_string(format!( "attempt to set an edge counter more than once; from_bcb: \ {from_bcb:?} already had counter {replaced:?}", )) } else { - Ok(operand) + Ok(term) } } @@ -284,7 +284,7 @@ impl<'a> MakeBcbCounters<'a> { &mut self, traversal: &TraverseCoverageGraphWithLoops<'_>, branching_bcb: BasicCoverageBlock, - branching_counter_operand: Operand, + branching_counter_operand: CovTerm, ) -> Result<(), Error> { let branches = self.bcb_branches(branching_bcb); debug!( @@ -332,7 +332,7 @@ impl<'a> MakeBcbCounters<'a> { sumup_counter_operand, ); debug!(" [new intermediate expression: {:?}]", intermediate_expression); - let intermediate_expression_operand = intermediate_expression.as_operand(); + let intermediate_expression_operand = intermediate_expression.as_term(); self.coverage_counters.intermediate_expressions.push(intermediate_expression); some_sumup_counter_operand.replace(intermediate_expression_operand); } @@ -364,7 +364,7 @@ impl<'a> MakeBcbCounters<'a> { Ok(()) } - fn get_or_make_counter_operand(&mut self, bcb: BasicCoverageBlock) -> Result { + fn get_or_make_counter_operand(&mut self, bcb: BasicCoverageBlock) -> Result { self.recursive_get_or_make_counter_operand(bcb, 1) } @@ -372,7 +372,7 @@ impl<'a> MakeBcbCounters<'a> { &mut self, bcb: BasicCoverageBlock, debug_indent_level: usize, - ) -> Result { + ) -> Result { // If the BCB already has a counter, return it. if let Some(counter_kind) = &self.coverage_counters.bcb_counters[bcb] { debug!( @@ -381,7 +381,7 @@ impl<'a> MakeBcbCounters<'a> { bcb, counter_kind, ); - return Ok(counter_kind.as_operand()); + return Ok(counter_kind.as_term()); } // A BCB with only one incoming edge gets a simple `Counter` (via `make_counter()`). @@ -445,7 +445,7 @@ impl<'a> MakeBcbCounters<'a> { NESTED_INDENT.repeat(debug_indent_level), intermediate_expression ); - let intermediate_expression_operand = intermediate_expression.as_operand(); + 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); } @@ -468,7 +468,7 @@ impl<'a> MakeBcbCounters<'a> { &mut self, from_bcb: BasicCoverageBlock, to_bcb: BasicCoverageBlock, - ) -> Result { + ) -> Result { self.recursive_get_or_make_edge_counter_operand(from_bcb, to_bcb, 1) } @@ -477,7 +477,7 @@ impl<'a> MakeBcbCounters<'a> { from_bcb: BasicCoverageBlock, to_bcb: BasicCoverageBlock, debug_indent_level: usize, - ) -> Result { + ) -> Result { // If the source BCB has only one successor (assumed to be the given target), an edge // counter is unnecessary. Just get or make a counter for the source BCB. let successors = self.bcb_successors(from_bcb).iter(); @@ -496,7 +496,7 @@ impl<'a> MakeBcbCounters<'a> { to_bcb, counter_kind ); - return Ok(counter_kind.as_operand()); + return Ok(counter_kind.as_term()); } // Make a new counter to count this edge. From 8efdd4cca61df9adf7f90c40fcc176a2842a9c28 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Mon, 4 Sep 2023 12:50:51 +1000 Subject: [PATCH 04/10] coverage: Collect a function's coverage mappings into a single list This is an intermediate step towards being able to store all of a function's mappings in function coverage info. --- .../src/coverageinfo/map_data.rs | 115 +++++++----------- .../src/coverageinfo/mapgen.rs | 11 +- compiler/rustc_middle/src/mir/coverage.rs | 14 +++ 3 files changed, 65 insertions(+), 75 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 9798c18f21b..407ac5662d3 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -1,9 +1,10 @@ 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, Op, + CodeRegion, CounterId, CovTerm, ExpressionId, FunctionCoverageInfo, Mapping, Op, }; use rustc_middle::ty::Instance; @@ -12,28 +13,21 @@ pub struct Expression { lhs: CovTerm, op: Op, rhs: CovTerm, - code_regions: Vec, } -/// Collects all of the coverage regions associated with (a) injected counters, (b) counter -/// expressions (additions or subtraction), and (c) unreachable regions (always counted as zero), -/// for a given Function. This struct also stores the `function_source_hash`, -/// computed during instrumentation, and forwarded with counters. -/// -/// Note, it may be important to understand LLVM's definitions of `unreachable` regions versus "gap -/// regions" (or "gap areas"). A gap region is a code region within a counted region (either counter -/// or expression), but the line or lines in the gap region are not executable (such as lines with -/// only whitespace or comments). According to LLVM Code Coverage Mapping documentation, "A count -/// for a gap area is only used as the line execution count if there are no other regions on a -/// line." +/// 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)] pub struct FunctionCoverage<'tcx> { /// Coverage info that was attached to this function by the instrumentor. function_coverage_info: &'tcx FunctionCoverageInfo, is_used: bool, - counters: IndexVec>>, + + /// Tracks which counters have been seen, to avoid duplicate mappings + /// that might be introduced by MIR inlining. + counters_seen: BitSet, expressions: IndexVec>, - unreachable_regions: Vec, + mappings: Vec, } impl<'tcx> FunctionCoverage<'tcx> { @@ -67,9 +61,9 @@ impl<'tcx> FunctionCoverage<'tcx> { Self { function_coverage_info, is_used, - counters: IndexVec::from_elem_n(None, num_counters), + counters_seen: BitSet::new_empty(num_counters), expressions: IndexVec::from_elem_n(None, num_expressions), - unreachable_regions: Vec::new(), + mappings: Vec::new(), } } @@ -81,19 +75,8 @@ impl<'tcx> FunctionCoverage<'tcx> { /// Adds code regions to be counted by an injected counter intrinsic. #[instrument(level = "debug", skip(self))] pub(crate) fn add_counter(&mut self, id: CounterId, code_regions: &[CodeRegion]) { - if code_regions.is_empty() { - return; - } - - let slot = &mut self.counters[id]; - match slot { - None => *slot = Some(code_regions.to_owned()), - // If this counter ID slot has already been filled, it should - // contain identical information. - Some(ref previous_regions) => assert_eq!( - previous_regions, code_regions, - "add_counter: code regions for id changed" - ), + if self.counters_seen.insert(id) { + self.add_mappings(CovTerm::Counter(id), code_regions); } } @@ -121,10 +104,13 @@ impl<'tcx> FunctionCoverage<'tcx> { self, ); - let expression = Expression { lhs, op, rhs, code_regions: code_regions.to_owned() }; + let expression = Expression { lhs, op, rhs }; let slot = &mut self.expressions[expression_id]; match slot { - None => *slot = Some(expression), + None => { + *slot = Some(expression); + self.add_mappings(CovTerm::Expression(expression_id), code_regions); + } // If this expression ID slot has already been filled, it should // contain identical information. Some(ref previous_expression) => assert_eq!( @@ -138,7 +124,25 @@ impl<'tcx> FunctionCoverage<'tcx> { #[instrument(level = "debug", skip(self))] pub(crate) fn add_unreachable_regions(&mut self, code_regions: &[CodeRegion]) { assert!(!code_regions.is_empty(), "unreachable regions always have code regions"); - self.unreachable_regions.extend_from_slice(code_regions); + self.add_mappings(CovTerm::Zero, code_regions); + } + + #[instrument(level = "debug", skip(self))] + fn add_mappings(&mut self, term: CovTerm, code_regions: &[CodeRegion]) { + self.mappings + .extend(code_regions.iter().cloned().map(|code_region| Mapping { term, code_region })); + } + + pub(crate) fn finalize(&mut self) { + self.simplify_expressions(); + + // Reorder the collected mappings so that counter mappings are first and + // zero mappings are last, matching the historical order. + self.mappings.sort_by_key(|mapping| match mapping.term { + CovTerm::Counter(_) => 0, + CovTerm::Expression(_) => 1, + CovTerm::Zero => u8::MAX, + }); } /// Perform some simplifications to make the final coverage mappings @@ -147,7 +151,7 @@ impl<'tcx> FunctionCoverage<'tcx> { /// This method mainly exists to preserve the simplifications that were /// already being performed by the Rust-side expression renumbering, so that /// the resulting coverage mappings don't get worse. - pub(crate) fn simplify_expressions(&mut self) { + fn simplify_expressions(&mut self) { // The set of expressions that either were optimized out entirely, or // have zero as both of their operands, and will therefore always have // a value of zero. Other expressions that refer to these as operands @@ -212,27 +216,10 @@ impl<'tcx> FunctionCoverage<'tcx> { assert_eq!(self.expressions.len(), counter_expressions.len()); let counter_regions = self.counter_regions(); - let expression_regions = self.expression_regions(); - let unreachable_regions = self.unreachable_regions(); - let counter_regions = - counter_regions.chain(expression_regions.into_iter().chain(unreachable_regions)); (counter_expressions, counter_regions) } - fn counter_regions(&self) -> impl Iterator { - self.counters - .iter_enumerated() - // Filter out counter IDs that we never saw during MIR traversal. - // This can happen if a counter was optimized out by MIR transforms - // (and replaced with `CoverageKind::Unreachable` instead). - .filter_map(|(id, maybe_code_regions)| Some((id, maybe_code_regions.as_ref()?))) - .flat_map(|(id, code_regions)| { - let counter = Counter::counter_value_reference(id); - code_regions.iter().map(move |region| (counter, region)) - }) - } - /// Convert this function's coverage expression data into a form that can be /// passed through FFI to LLVM. fn counter_expressions(&self) -> Vec { @@ -266,24 +253,12 @@ impl<'tcx> FunctionCoverage<'tcx> { .collect::>() } - fn expression_regions(&self) -> Vec<(Counter, &CodeRegion)> { - // Find all of the expression IDs that weren't optimized out AND have - // one or more attached code regions, and return the corresponding - // mappings as counter/region pairs. - self.expressions - .iter_enumerated() - .filter_map(|(id, maybe_expression)| { - let code_regions = &maybe_expression.as_ref()?.code_regions; - Some((id, code_regions)) - }) - .flat_map(|(id, code_regions)| { - let counter = Counter::expression(id); - code_regions.iter().map(move |code_region| (counter, code_region)) - }) - .collect::>() - } - - fn unreachable_regions(&self) -> impl Iterator { - self.unreachable_regions.iter().map(|region| (Counter::ZERO, region)) + /// Converts this function's coverage mappings into an intermediate form + /// that will be used by `mapgen` when preparing for FFI. + fn counter_regions(&self) -> impl Iterator { + self.mappings.iter().map(|&Mapping { term, ref code_region }| { + let counter = Counter::from_term(term); + (counter, code_region) + }) } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index a2bf53e605d..cde12b13307 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -61,7 +61,7 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { let mut function_data = Vec::new(); for (instance, mut function_coverage) in function_coverage_map { debug!("Generate function coverage for {}, {:?}", cx.codegen_unit.name(), instance); - function_coverage.simplify_expressions(); + function_coverage.finalize(); let function_coverage = function_coverage; let mangled_function_name = tcx.symbol_name(instance).name; @@ -169,10 +169,11 @@ fn encode_mappings_for_function( let mut virtual_file_mapping = IndexVec::::new(); let mut mapping_regions = Vec::with_capacity(counter_regions.len()); - // Sort the list of (counter, region) mapping pairs by region, so that they - // can be grouped by filename. Prepare file IDs for each filename, and - // prepare the mapping data so that we can pass it through FFI to LLVM. - counter_regions.sort_by_key(|(_counter, region)| *region); + // Sort and group the list of (counter, region) mapping pairs by filename. + // (Preserve any further ordering imposed by `FunctionCoverage`.) + // Prepare file IDs for each filename, and prepare the mapping data so that + // we can pass it through FFI to LLVM. + counter_regions.sort_by_key(|(_counter, region)| region.file_name); for counter_regions_for_file in counter_regions.group_by(|(_, a), (_, b)| a.file_name == b.file_name) { diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index fbb7f33a983..164a17ff77a 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -135,6 +135,20 @@ impl Op { } } +#[derive(Clone, Debug)] +#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] +pub struct Mapping { + pub code_region: CodeRegion, + + /// Indicates whether this mapping uses a counter value, expression value, + /// or zero value. + /// + /// FIXME: When we add support for mapping kinds other than `Code` + /// (e.g. branch regions, expansion regions), replace this with a dedicated + /// mapping-kind enum. + pub term: CovTerm, +} + /// Stores per-function coverage information attached to a `mir::Body`, /// to be used in conjunction with the individual coverage statements injected /// into the function's basic blocks. From 4099ab19979e2f22b7a949f83241f4f0adc00ca9 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 13 Sep 2023 19:22:17 +1000 Subject: [PATCH 05/10] coverage: Make expression simplification non-destructive Instead of modifying the accumulated expressions in-place, we now build a set of expressions that are known to be zero, and then consult that set on the fly when converting the expression data for FFI. This will be necessary when moving mappings and expression data into function coverage info, which can't be mutated during codegen. --- .../src/coverageinfo/map_data.rs | 80 ++++++++++++++----- 1 file changed, 58 insertions(+), 22 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 407ac5662d3..02831c92a08 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -134,8 +134,6 @@ impl<'tcx> FunctionCoverage<'tcx> { } pub(crate) fn finalize(&mut self) { - self.simplify_expressions(); - // Reorder the collected mappings so that counter mappings are first and // zero mappings are last, matching the historical order. self.mappings.sort_by_key(|mapping| match mapping.term { @@ -145,25 +143,25 @@ impl<'tcx> FunctionCoverage<'tcx> { }); } - /// Perform some simplifications to make the final coverage mappings - /// slightly smaller. + /// Identify expressions that will always have a value of zero, and note + /// their IDs in [`ZeroExpressions`]. Mappings that refer to a zero expression + /// can instead become mappings to a constant zero value. /// /// This method mainly exists to preserve the simplifications that were /// already being performed by the Rust-side expression renumbering, so that /// the resulting coverage mappings don't get worse. - fn simplify_expressions(&mut self) { + fn identify_zero_expressions(&self) -> ZeroExpressions { // The set of expressions that either were optimized out entirely, or // have zero as both of their operands, and will therefore always have // a value of zero. Other expressions that refer to these as operands // can have those operands replaced with `CovTerm::Zero`. let mut zero_expressions = FxIndexSet::default(); - // For each expression, perform simplifications based on lower-numbered - // expressions, and then update the set of always-zero expressions if - // necessary. + // Simplify a copy of each expression based on lower-numbered expressions, + // 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 simplification pass is sufficient.) - for (id, maybe_expression) in self.expressions.iter_enumerated_mut() { + // 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, // so any operand that refers to it can be replaced with zero. @@ -171,30 +169,50 @@ impl<'tcx> FunctionCoverage<'tcx> { continue; }; + // We don't need to simplify the actual expression data in the + // expressions list; we can just simplify a temporary copy and then + // use that to update the set of always-zero expressions. + let Expression { mut lhs, op, mut rhs } = *expression; + + // If an expression has an operand that is also an expression, the + // operand's ID must be strictly lower. This is what lets us find + // all zero expressions in one pass. + let assert_operand_expression_is_lower = |operand_id: ExpressionId| { + assert!( + operand_id < id, + "Operand {operand_id:?} should be less than {id:?} in {expression:?}", + ) + }; + // If an operand refers to an expression that is always zero, then // that operand can be replaced with `CovTerm::Zero`. - let maybe_set_operand_to_zero = |operand: &mut CovTerm| match &*operand { - CovTerm::Expression(id) if zero_expressions.contains(id) => { - *operand = CovTerm::Zero; + let maybe_set_operand_to_zero = |operand: &mut CovTerm| match *operand { + CovTerm::Expression(id) => { + assert_operand_expression_is_lower(id); + if zero_expressions.contains(&id) { + *operand = CovTerm::Zero; + } } _ => (), }; - maybe_set_operand_to_zero(&mut expression.lhs); - maybe_set_operand_to_zero(&mut expression.rhs); + maybe_set_operand_to_zero(&mut lhs); + maybe_set_operand_to_zero(&mut rhs); // Coverage counter values cannot be negative, so if an expression // involves subtraction from zero, assume that its RHS must also be zero. // (Do this after simplifications that could set the LHS to zero.) - if let Expression { lhs: CovTerm::Zero, op: Op::Subtract, .. } = expression { - expression.rhs = CovTerm::Zero; + if lhs == CovTerm::Zero && op == Op::Subtract { + rhs = CovTerm::Zero; } // After the above simplifications, if both operands are zero, then // we know that this expression is always zero too. - if let Expression { lhs: CovTerm::Zero, rhs: CovTerm::Zero, .. } = expression { + if lhs == CovTerm::Zero && rhs == CovTerm::Zero { zero_expressions.insert(id); } } + + ZeroExpressions(zero_expressions) } /// Return the source hash, generated from the HIR node structure, and used to indicate whether @@ -209,7 +227,9 @@ impl<'tcx> FunctionCoverage<'tcx> { pub fn get_expressions_and_counter_regions( &self, ) -> (Vec, impl Iterator) { - let counter_expressions = self.counter_expressions(); + let zero_expressions = self.identify_zero_expressions(); + + let counter_expressions = self.counter_expressions(&zero_expressions); // 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. @@ -222,12 +242,17 @@ impl<'tcx> FunctionCoverage<'tcx> { /// Convert this function's coverage expression data into a form that can be /// passed through FFI to LLVM. - fn counter_expressions(&self) -> Vec { + fn counter_expressions(&self, zero_expressions: &ZeroExpressions) -> Vec { // We know that LLVM will optimize out any unused expressions before // producing the final coverage map, so there's no need to do the same // thing on the Rust side unless we're confident we can do much better. // (See `CounterExpressionsMinimizer` in `CoverageMappingWriter.cpp`.) + let counter_from_operand = |operand: CovTerm| match operand { + CovTerm::Expression(id) if zero_expressions.contains(id) => Counter::ZERO, + _ => Counter::from_term(operand), + }; + self.expressions .iter() .map(|expression| match expression { @@ -241,12 +266,12 @@ impl<'tcx> FunctionCoverage<'tcx> { &Some(Expression { lhs, op, rhs, .. }) => { // Convert the operands and operator as normal. CounterExpression::new( - Counter::from_term(lhs), + counter_from_operand(lhs), match op { Op::Add => ExprKind::Add, Op::Subtract => ExprKind::Subtract, }, - Counter::from_term(rhs), + counter_from_operand(rhs), ) } }) @@ -262,3 +287,14 @@ impl<'tcx> FunctionCoverage<'tcx> { }) } } + +/// Set of expression IDs that are known to always evaluate to zero. +/// Any mapping or expression operand that refers to these expressions can have +/// that reference replaced with a constant zero value. +struct ZeroExpressions(FxIndexSet); + +impl ZeroExpressions { + fn contains(&self, id: ExpressionId) -> bool { + self.0.contains(&id) + } +} From 6da319f63583215d593919bcd994785141f7265d Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 13 Sep 2023 12:51:43 +1000 Subject: [PATCH 06/10] coverage: Store all of a function's mappings in function coverage info Previously, mappings were attached to individual coverage statements in MIR. That necessitated special handling in MIR optimizations to avoid deleting those statements, since otherwise codegen would be unable to reassemble the original list of mappings. With this change, a function's list of mappings is now attached to its MIR body, and survives intact even if individual statements are deleted by optimizations. --- .../src/coverageinfo/map_data.rs | 75 +++++-------- .../src/coverageinfo/mapgen.rs | 4 +- .../src/coverageinfo/mod.rs | 29 ++--- compiler/rustc_middle/src/mir/coverage.rs | 18 +-- compiler/rustc_middle/src/mir/pretty.rs | 26 +++-- compiler/rustc_middle/src/mir/syntax.rs | 3 +- compiler/rustc_middle/src/query/mod.rs | 11 -- .../rustc_mir_transform/src/coverage/mod.rs | 34 +++--- .../rustc_mir_transform/src/coverage/query.rs | 22 +--- compiler/rustc_mir_transform/src/simplify.rs | 103 +----------------- .../status-quo/issue-84561.cov-map | 5 +- .../status-quo/try_error_result.cov-map | 29 +---- ...ument_coverage.bar.InstrumentCoverage.diff | 4 +- ...ment_coverage.main.InstrumentCoverage.diff | 14 ++- 14 files changed, 107 insertions(+), 270 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 02831c92a08..e6b39c71453 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -23,11 +23,10 @@ pub struct FunctionCoverage<'tcx> { function_coverage_info: &'tcx FunctionCoverageInfo, is_used: bool, - /// Tracks which counters have been seen, to avoid duplicate mappings - /// that might be introduced by MIR inlining. + /// 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>, - mappings: Vec, } impl<'tcx> FunctionCoverage<'tcx> { @@ -63,7 +62,6 @@ impl<'tcx> FunctionCoverage<'tcx> { is_used, counters_seen: BitSet::new_empty(num_counters), expressions: IndexVec::from_elem_n(None, num_expressions), - mappings: Vec::new(), } } @@ -72,20 +70,13 @@ impl<'tcx> FunctionCoverage<'tcx> { self.is_used } - /// Adds code regions to be counted by an injected counter intrinsic. + /// Marks a counter ID as having been seen in a counter-increment statement. #[instrument(level = "debug", skip(self))] - pub(crate) fn add_counter(&mut self, id: CounterId, code_regions: &[CodeRegion]) { - if self.counters_seen.insert(id) { - self.add_mappings(CovTerm::Counter(id), code_regions); - } + pub(crate) fn mark_counter_id_seen(&mut self, id: CounterId) { + self.counters_seen.insert(id); } - /// Adds information about a coverage expression, along with zero or more - /// code regions mapped to that expression. - /// - /// Both counters and "counter expressions" (or simply, "expressions") can be operands in other - /// expressions. These are tracked as separate variants of `CovTerm`, so there is no ambiguity - /// between operands that are counter IDs and operands that are expression IDs. + /// Adds information about a coverage expression. #[instrument(level = "debug", skip(self))] pub(crate) fn add_counter_expression( &mut self, @@ -93,7 +84,6 @@ impl<'tcx> FunctionCoverage<'tcx> { lhs: CovTerm, op: Op, rhs: CovTerm, - code_regions: &[CodeRegion], ) { debug_assert!( expression_id.as_usize() < self.expressions.len(), @@ -107,10 +97,7 @@ impl<'tcx> FunctionCoverage<'tcx> { let expression = Expression { lhs, op, rhs }; let slot = &mut self.expressions[expression_id]; match slot { - None => { - *slot = Some(expression); - self.add_mappings(CovTerm::Expression(expression_id), code_regions); - } + None => *slot = Some(expression), // If this expression ID slot has already been filled, it should // contain identical information. Some(ref previous_expression) => assert_eq!( @@ -120,29 +107,6 @@ impl<'tcx> FunctionCoverage<'tcx> { } } - /// Adds regions that will be marked as "unreachable", with a constant "zero counter". - #[instrument(level = "debug", skip(self))] - pub(crate) fn add_unreachable_regions(&mut self, code_regions: &[CodeRegion]) { - assert!(!code_regions.is_empty(), "unreachable regions always have code regions"); - self.add_mappings(CovTerm::Zero, code_regions); - } - - #[instrument(level = "debug", skip(self))] - fn add_mappings(&mut self, term: CovTerm, code_regions: &[CodeRegion]) { - self.mappings - .extend(code_regions.iter().cloned().map(|code_region| Mapping { term, code_region })); - } - - pub(crate) fn finalize(&mut self) { - // Reorder the collected mappings so that counter mappings are first and - // zero mappings are last, matching the historical order. - self.mappings.sort_by_key(|mapping| match mapping.term { - CovTerm::Counter(_) => 0, - CovTerm::Expression(_) => 1, - CovTerm::Zero => u8::MAX, - }); - } - /// Identify expressions that will always have a value of zero, and note /// their IDs in [`ZeroExpressions`]. Mappings that refer to a zero expression /// can instead become mappings to a constant zero value. @@ -235,7 +199,7 @@ impl<'tcx> FunctionCoverage<'tcx> { // the two vectors should correspond 1:1. assert_eq!(self.expressions.len(), counter_expressions.len()); - let counter_regions = self.counter_regions(); + let counter_regions = self.counter_regions(zero_expressions); (counter_expressions, counter_regions) } @@ -280,9 +244,26 @@ impl<'tcx> FunctionCoverage<'tcx> { /// Converts this function's coverage mappings into an intermediate form /// that will be used by `mapgen` when preparing for FFI. - fn counter_regions(&self) -> impl Iterator { - self.mappings.iter().map(|&Mapping { term, ref code_region }| { - let counter = Counter::from_term(term); + fn counter_regions( + &self, + zero_expressions: ZeroExpressions, + ) -> impl Iterator { + // Historically, mappings were stored directly in counter/expression + // statements in MIR, and MIR optimizations would sometimes remove them. + // That's mostly no longer true, so now we detect cases where that would + // have happened, and zero out the corresponding mappings here instead. + let counter_for_term = move |term: CovTerm| { + let force_to_zero = match term { + CovTerm::Counter(id) => !self.counters_seen.contains(id), + CovTerm::Expression(id) => zero_expressions.contains(id), + CovTerm::Zero => false, + }; + if force_to_zero { Counter::ZERO } else { Counter::from_term(term) } + }; + + self.function_coverage_info.mappings.iter().map(move |mapping| { + let &Mapping { term, ref code_region } = mapping; + let counter = counter_for_term(term); (counter, code_region) }) } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs index cde12b13307..ef3647efd88 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs @@ -59,10 +59,8 @@ pub fn finalize(cx: &CodegenCx<'_, '_>) { // Encode coverage mappings and generate function records let mut function_data = Vec::new(); - for (instance, mut function_coverage) in function_coverage_map { + for (instance, function_coverage) in function_coverage_map { debug!("Generate function coverage for {}, {:?}", cx.codegen_unit.name(), instance); - function_coverage.finalize(); - let function_coverage = function_coverage; let mangled_function_name = tcx.symbol_name(instance).name; let source_hash = function_coverage.source_hash(); diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index c975d3432b9..14332927c8d 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -88,13 +88,12 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> { /// For used/called functions, the coverageinfo was already added to the /// `function_coverage_map` (keyed by function `Instance`) during codegen. /// But in this case, since the unused function was _not_ previously - /// codegenned, collect the coverage `CodeRegion`s from the MIR and add - /// them. Since the function is never called, all of its `CodeRegion`s can be - /// added as `unreachable_region`s. + /// codegenned, collect the function coverage info from MIR and add an + /// "unused" entry to the function coverage map. fn define_unused_fn(&self, def_id: DefId, function_coverage_info: &'tcx FunctionCoverageInfo) { let instance = declare_unused_fn(self, def_id); codegen_unused_fn_and_counter(self, instance); - add_unused_function_coverage(self, instance, def_id, function_coverage_info); + add_unused_function_coverage(self, instance, function_coverage_info); } } @@ -116,10 +115,10 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { .entry(instance) .or_insert_with(|| FunctionCoverage::new(instance, function_coverage_info)); - let Coverage { kind, code_regions } = coverage; + let Coverage { kind } = coverage; match *kind { - CoverageKind::Counter { id } => { - func_coverage.add_counter(id, code_regions); + CoverageKind::CounterIncrement { id } => { + func_coverage.mark_counter_id_seen(id); // We need to explicitly drop the `RefMut` before calling into `instrprof_increment`, // as that needs an exclusive borrow. drop(coverage_map); @@ -147,10 +146,7 @@ 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, code_regions); - } - CoverageKind::Unreachable => { - func_coverage.add_unreachable_regions(code_regions); + func_coverage.add_counter_expression(id, lhs, op, rhs); } } } @@ -213,16 +209,11 @@ fn codegen_unused_fn_and_counter<'tcx>(cx: &CodegenCx<'_, 'tcx>, instance: Insta fn add_unused_function_coverage<'tcx>( cx: &CodegenCx<'_, 'tcx>, instance: Instance<'tcx>, - def_id: DefId, function_coverage_info: &'tcx FunctionCoverageInfo, ) { - let tcx = cx.tcx; - - let mut function_coverage = FunctionCoverage::unused(instance, function_coverage_info); - for &code_region in tcx.covered_code_regions(def_id) { - let code_region = std::slice::from_ref(code_region); - function_coverage.add_unreachable_regions(code_region); - } + // An unused function's mappings will automatically be rewritten to map to + // zero, because none of its counters/expressions are marked as seen. + let function_coverage = FunctionCoverage::unused(instance, function_coverage_info); if let Some(coverage_context) = cx.coverage_context() { coverage_context.function_coverage_map.borrow_mut().insert(instance, function_coverage); diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 164a17ff77a..2587a6eae1a 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -61,11 +61,13 @@ impl Debug for CovTerm { #[derive(Clone, PartialEq, TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)] pub enum CoverageKind { - Counter { - /// ID of this counter within its enclosing function. - /// Expressions in the same function can refer to it as an operand. - id: CounterId, - }, + /// Marks the point in MIR control flow represented by a coverage counter. + /// + /// This is eventually lowered to `llvm.instrprof.increment` in LLVM IR. + /// + /// If this statement does not survive MIR optimizations, any mappings that + /// refer to this counter can have those references simplified to zero. + CounterIncrement { id: CounterId }, Expression { /// ID of this coverage-counter expression within its enclosing function. /// Other expressions in the same function can refer to it as an operand. @@ -74,14 +76,13 @@ pub enum CoverageKind { op: Op, rhs: CovTerm, }, - Unreachable, } impl Debug for CoverageKind { fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result { use CoverageKind::*; match self { - Counter { id } => write!(fmt, "Counter({:?})", id.index()), + CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()), Expression { id, lhs, op, rhs } => write!( fmt, "Expression({:?}) = {:?} {} {:?}", @@ -93,7 +94,6 @@ impl Debug for CoverageKind { }, rhs, ), - Unreachable => write!(fmt, "Unreachable"), } } } @@ -158,4 +158,6 @@ pub struct FunctionCoverageInfo { pub function_source_hash: u64, pub num_counters: usize, pub num_expressions: usize, + + pub mappings: Vec, } diff --git a/compiler/rustc_middle/src/mir/pretty.rs b/compiler/rustc_middle/src/mir/pretty.rs index c5e3ee575e1..11269e749f8 100644 --- a/compiler/rustc_middle/src/mir/pretty.rs +++ b/compiler/rustc_middle/src/mir/pretty.rs @@ -493,6 +493,24 @@ pub fn write_mir_intro<'tcx>( // Add an empty line before the first block is printed. writeln!(w)?; + if let Some(function_coverage_info) = &body.function_coverage_info { + write_function_coverage_info(function_coverage_info, w)?; + } + + Ok(()) +} + +fn write_function_coverage_info( + function_coverage_info: &coverage::FunctionCoverageInfo, + w: &mut dyn io::Write, +) -> io::Result<()> { + let coverage::FunctionCoverageInfo { mappings, .. } = function_coverage_info; + + for coverage::Mapping { term, code_region } in mappings { + writeln!(w, "{INDENT}coverage {term:?} => {code_region:?};")?; + } + writeln!(w)?; + Ok(()) } @@ -685,13 +703,7 @@ impl Debug for Statement<'_> { AscribeUserType(box (ref place, ref c_ty), ref variance) => { write!(fmt, "AscribeUserType({place:?}, {variance:?}, {c_ty:?})") } - Coverage(box mir::Coverage { ref kind, ref code_regions }) => { - if code_regions.is_empty() { - write!(fmt, "Coverage::{kind:?}") - } else { - write!(fmt, "Coverage::{kind:?} for {code_regions:?}") - } - } + Coverage(box mir::Coverage { ref kind }) => write!(fmt, "Coverage::{kind:?}"), Intrinsic(box ref intrinsic) => write!(fmt, "{intrinsic}"), ConstEvalCounter => write!(fmt, "ConstEvalCounter"), Nop => write!(fmt, "nop"), diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index c7d99648f1e..77b26e0a4cd 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -5,7 +5,7 @@ use super::{BasicBlock, Const, Local, UserTypeProjection}; -use crate::mir::coverage::{CodeRegion, CoverageKind}; +use crate::mir::coverage::CoverageKind; use crate::traits::Reveal; use crate::ty::adjustment::PointerCoercion; use crate::ty::GenericArgsRef; @@ -514,7 +514,6 @@ pub enum FakeReadCause { #[derive(TypeFoldable, TypeVisitable)] pub struct Coverage { pub kind: CoverageKind, - pub code_regions: Vec, } #[derive(Clone, Debug, PartialEq, TyEncodable, TyDecodable, Hash, HashStable)] diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs index ed0f73fcf2f..afe94d10752 100644 --- a/compiler/rustc_middle/src/query/mod.rs +++ b/compiler/rustc_middle/src/query/mod.rs @@ -581,17 +581,6 @@ rustc_queries! { arena_cache } - /// Returns the `CodeRegions` for a function that has instrumented coverage, in case the - /// function was optimized out before codegen, and before being added to the Coverage Map. - query covered_code_regions(key: DefId) -> &'tcx Vec<&'tcx mir::coverage::CodeRegion> { - desc { - |tcx| "retrieving the covered `CodeRegion`s, if instrumented, for `{}`", - tcx.def_path_str(key) - } - arena_cache - cache_on_disk_if { key.is_local() } - } - /// The `DefId` is the `DefId` of the containing MIR body. Promoteds do not have their own /// `DefId`. This function returns all promoteds in the specified body. The body references /// promoteds by the `DefId` and the `mir::Promoted` index. This is necessary, because diff --git a/compiler/rustc_mir_transform/src/coverage/mod.rs b/compiler/rustc_mir_transform/src/coverage/mod.rs index e1b656bd981..078123a7b7f 100644 --- a/compiler/rustc_mir_transform/src/coverage/mod.rs +++ b/compiler/rustc_mir_transform/src/coverage/mod.rs @@ -104,6 +104,7 @@ struct Instrumentor<'a, 'tcx> { function_source_hash: u64, basic_coverage_blocks: CoverageGraph, coverage_counters: CoverageCounters, + mappings: Vec, } impl<'a, 'tcx> Instrumentor<'a, 'tcx> { @@ -144,6 +145,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { function_source_hash, basic_coverage_blocks, coverage_counters, + mappings: Vec::new(), } } @@ -216,6 +218,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { function_source_hash: self.function_source_hash, num_counters: self.coverage_counters.num_counters(), num_expressions: self.coverage_counters.num_expressions(), + mappings: std::mem::take(&mut self.mappings), })); } @@ -232,18 +235,16 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { bug!("Every BasicCoverageBlock should have a Counter or Expression"); }); - // Convert the coverage spans into a vector of code regions to be - // associated with this BCB's coverage statement. - let code_regions = spans - .iter() - .map(|&span| make_code_region(source_map, file_name, span, body_span)) - .collect::>(); + let term = counter_kind.as_term(); + self.mappings.extend(spans.iter().map(|&span| { + let code_region = make_code_region(source_map, file_name, span, body_span); + Mapping { code_region, term } + })); inject_statement( self.mir_body, self.make_mir_coverage_kind(&counter_kind), self.bcb_leader_bb(bcb), - code_regions, ); } } @@ -301,7 +302,6 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { self.mir_body, self.make_mir_coverage_kind(&counter_kind), inject_to_bb, - Vec::new(), ); } BcbCounter::Expression { .. } => inject_intermediate_expression( @@ -329,7 +329,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::Counter { id } => CoverageKind::CounterIncrement { id }, BcbCounter::Expression { id, lhs, op, rhs } => { CoverageKind::Expression { id, lhs, op, rhs } } @@ -360,18 +360,13 @@ fn inject_edge_counter_basic_block( new_bb } -fn inject_statement( - mir_body: &mut mir::Body<'_>, - counter_kind: CoverageKind, - bb: BasicBlock, - code_regions: Vec, -) { - debug!(" injecting statement {counter_kind:?} for {bb:?} at code regions: {code_regions:?}"); +fn inject_statement(mir_body: &mut mir::Body<'_>, counter_kind: CoverageKind, bb: BasicBlock) { + debug!(" injecting statement {counter_kind:?} for {bb:?}"); let data = &mut mir_body[bb]; let source_info = data.terminator().source_info; let statement = Statement { source_info, - kind: StatementKind::Coverage(Box::new(Coverage { kind: counter_kind, code_regions })), + kind: StatementKind::Coverage(Box::new(Coverage { kind: counter_kind })), }; data.statements.insert(0, statement); } @@ -385,10 +380,7 @@ fn inject_intermediate_expression(mir_body: &mut mir::Body<'_>, expression: Cove let source_info = data.terminator().source_info; let statement = Statement { source_info, - kind: StatementKind::Coverage(Box::new(Coverage { - kind: expression, - code_regions: Vec::new(), - })), + kind: StatementKind::Coverage(Box::new(Coverage { kind: expression })), }; data.statements.push(statement); } diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs index b1f46c8e05c..809407f897d 100644 --- a/compiler/rustc_mir_transform/src/coverage/query.rs +++ b/compiler/rustc_mir_transform/src/coverage/query.rs @@ -2,15 +2,13 @@ use super::*; use rustc_data_structures::captures::Captures; use rustc_middle::mir::coverage::*; -use rustc_middle::mir::{self, Body, Coverage, CoverageIdsInfo}; +use rustc_middle::mir::{Body, Coverage, CoverageIdsInfo}; use rustc_middle::query::Providers; use rustc_middle::ty::{self, TyCtxt}; -use rustc_span::def_id::DefId; /// A `query` provider for retrieving coverage information injected into MIR. pub(crate) fn provide(providers: &mut Providers) { providers.coverage_ids_info = |tcx, def_id| coverage_ids_info(tcx, def_id); - providers.covered_code_regions = |tcx, def_id| covered_code_regions(tcx, def_id); } /// Query implementation for `coverage_ids_info`. @@ -22,7 +20,7 @@ fn coverage_ids_info<'tcx>( let max_counter_id = all_coverage_in_mir_body(mir_body) .filter_map(|coverage| match coverage.kind { - CoverageKind::Counter { id } => Some(id), + CoverageKind::CounterIncrement { id } => Some(id), _ => None, }) .max() @@ -31,14 +29,6 @@ fn coverage_ids_info<'tcx>( CoverageIdsInfo { max_counter_id } } -fn covered_code_regions(tcx: TyCtxt<'_>, def_id: DefId) -> Vec<&CodeRegion> { - let body = mir_body(tcx, def_id); - all_coverage_in_mir_body(body) - // Coverage statements have a list of code regions (possibly empty). - .flat_map(|coverage| coverage.code_regions.as_slice()) - .collect() -} - fn all_coverage_in_mir_body<'a, 'tcx>( body: &'a Body<'tcx>, ) -> impl Iterator + Captures<'tcx> { @@ -56,11 +46,3 @@ fn is_inlined(body: &Body<'_>, statement: &Statement<'_>) -> bool { let scope_data = &body.source_scopes[statement.source_info.scope]; scope_data.inlined.is_some() || scope_data.inlined_parent_scope.is_some() } - -/// This function ensures we obtain the correct MIR for the given item irrespective of -/// whether that means const mir or runtime mir. For `const fn` this opts for runtime -/// mir. -fn mir_body(tcx: TyCtxt<'_>, def_id: DefId) -> &mir::Body<'_> { - let def = ty::InstanceDef::Item(def_id); - tcx.instance_mir(def) -} diff --git a/compiler/rustc_mir_transform/src/simplify.rs b/compiler/rustc_mir_transform/src/simplify.rs index 73dae044355..3ecb7403387 100644 --- a/compiler/rustc_mir_transform/src/simplify.rs +++ b/compiler/rustc_mir_transform/src/simplify.rs @@ -28,10 +28,8 @@ //! return. use crate::MirPass; -use rustc_data_structures::fx::{FxHashSet, FxIndexSet}; -use rustc_index::bit_set::BitSet; +use rustc_data_structures::fx::FxIndexSet; use rustc_index::{Idx, IndexSlice, IndexVec}; -use rustc_middle::mir::coverage::*; use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor}; use rustc_middle::mir::*; use rustc_middle::ty::TyCtxt; @@ -337,7 +335,7 @@ pub fn remove_duplicate_unreachable_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut B } } -pub fn remove_dead_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { +pub fn remove_dead_blocks<'tcx>(_tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { let reachable = traversal::reachable_as_bitset(body); let num_blocks = body.basic_blocks.len(); if num_blocks == reachable.count() { @@ -345,10 +343,6 @@ pub fn remove_dead_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { } let basic_blocks = body.basic_blocks.as_mut(); - let source_scopes = &body.source_scopes; - if tcx.sess.instrument_coverage() { - save_unreachable_coverage(basic_blocks, source_scopes, &reachable); - } let mut replacements: Vec<_> = (0..num_blocks).map(BasicBlock::new).collect(); let mut orig_index = 0; @@ -370,99 +364,6 @@ pub fn remove_dead_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { } } -/// Some MIR transforms can determine at compile time that a sequences of -/// statements will never be executed, so they can be dropped from the MIR. -/// For example, an `if` or `else` block that is guaranteed to never be executed -/// because its condition can be evaluated at compile time, such as by const -/// evaluation: `if false { ... }`. -/// -/// Those statements are bypassed by redirecting paths in the CFG around the -/// `dead blocks`; but with `-C instrument-coverage`, the dead blocks usually -/// include `Coverage` statements representing the Rust source code regions to -/// be counted at runtime. Without these `Coverage` statements, the regions are -/// lost, and the Rust source code will show no coverage information. -/// -/// What we want to show in a coverage report is the dead code with coverage -/// counts of `0`. To do this, we need to save the code regions, by injecting -/// `Unreachable` coverage statements. These are non-executable statements whose -/// code regions are still recorded in the coverage map, representing regions -/// with `0` executions. -/// -/// If there are no live `Counter` `Coverage` statements remaining, we remove -/// `Coverage` statements along with the dead blocks. Since at least one -/// counter per function is required by LLVM (and necessary, to add the -/// `function_hash` to the counter's call to the LLVM intrinsic -/// `instrprof.increment()`). -/// -/// The `generator::StateTransform` MIR pass and MIR inlining can create -/// atypical conditions, where all live `Counter`s are dropped from the MIR. -/// -/// With MIR inlining we can have coverage counters belonging to different -/// instances in a single body, so the strategy described above is applied to -/// coverage counters from each instance individually. -fn save_unreachable_coverage( - basic_blocks: &mut IndexSlice>, - source_scopes: &IndexSlice>, - reachable: &BitSet, -) { - // Identify instances that still have some live coverage counters left. - let mut live = FxHashSet::default(); - for bb in reachable.iter() { - let basic_block = &basic_blocks[bb]; - for statement in &basic_block.statements { - let StatementKind::Coverage(coverage) = &statement.kind else { continue }; - let CoverageKind::Counter { .. } = coverage.kind else { continue }; - let instance = statement.source_info.scope.inlined_instance(source_scopes); - live.insert(instance); - } - } - - for bb in reachable.iter() { - let block = &mut basic_blocks[bb]; - for statement in &mut block.statements { - let StatementKind::Coverage(_) = &statement.kind else { continue }; - let instance = statement.source_info.scope.inlined_instance(source_scopes); - if !live.contains(&instance) { - statement.make_nop(); - } - } - } - - if live.is_empty() { - return; - } - - // Retain coverage for instances that still have some live counters left. - let mut retained_coverage = Vec::new(); - for dead_block in basic_blocks.indices() { - if reachable.contains(dead_block) { - continue; - } - let dead_block = &basic_blocks[dead_block]; - for statement in &dead_block.statements { - let StatementKind::Coverage(coverage) = &statement.kind else { continue }; - if coverage.code_regions.is_empty() { - continue; - }; - let instance = statement.source_info.scope.inlined_instance(source_scopes); - if live.contains(&instance) { - retained_coverage.push((statement.source_info, coverage.code_regions.clone())); - } - } - } - - let start_block = &mut basic_blocks[START_BLOCK]; - start_block.statements.extend(retained_coverage.into_iter().map( - |(source_info, code_regions)| Statement { - source_info, - kind: StatementKind::Coverage(Box::new(Coverage { - kind: CoverageKind::Unreachable, - code_regions, - })), - }, - )); -} - pub enum SimplifyLocals { BeforeConstProp, Final, diff --git a/tests/coverage-map/status-quo/issue-84561.cov-map b/tests/coverage-map/status-quo/issue-84561.cov-map index fe098fd396c..01fa7ec573c 100644 --- a/tests/coverage-map/status-quo/issue-84561.cov-map +++ b/tests/coverage-map/status-quo/issue-84561.cov-map @@ -1,11 +1,10 @@ Function name: ::eq -Raw bytes (14): 0x[01, 01, 00, 02, 01, 04, 0a, 00, 13, 00, 00, 0a, 00, 13] +Raw bytes (9): 0x[01, 01, 00, 01, 01, 04, 0a, 00, 13] Number of files: 1 - file 0 => global file 1 Number of expressions: 0 -Number of file 0 mappings: 2 +Number of file 0 mappings: 1 - Code(Counter(0)) at (prev + 4, 10) to (start + 0, 19) -- Code(Zero) at (prev + 0, 10) to (start + 0, 19) Function name: ::fmt Raw bytes (29): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 89, 01, 09, 00, 25, 05, 00, 25, 00, 26, 02, 01, 09, 00, 0f, 07, 01, 05, 00, 06] diff --git a/tests/coverage-map/status-quo/try_error_result.cov-map b/tests/coverage-map/status-quo/try_error_result.cov-map index d254ba7b3b6..8367103a21a 100644 --- a/tests/coverage-map/status-quo/try_error_result.cov-map +++ b/tests/coverage-map/status-quo/try_error_result.cov-map @@ -1,62 +1,45 @@ Function name: ::get_thing_2 -Raw bytes (63): 0x[01, 01, 02, 01, 05, 05, 02, 0b, 01, 28, 05, 01, 18, 05, 02, 0d, 00, 14, 00, 00, 0d, 00, 14, 00, 00, 0d, 00, 14, 00, 00, 0d, 00, 14, 00, 00, 0d, 00, 14, 00, 00, 0d, 00, 14, 02, 02, 0d, 00, 1a, 00, 00, 0d, 00, 1a, 00, 00, 0d, 00, 1a, 07, 02, 05, 00, 06] +Raw bytes (28): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 28, 05, 01, 18, 05, 02, 0d, 00, 14, 02, 02, 0d, 00, 1a, 07, 02, 05, 00, 06] Number of files: 1 - file 0 => global file 1 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: 11 +Number of file 0 mappings: 4 - Code(Counter(0)) at (prev + 40, 5) to (start + 1, 24) - Code(Counter(1)) at (prev + 2, 13) to (start + 0, 20) -- Code(Zero) at (prev + 0, 13) to (start + 0, 20) -- Code(Zero) at (prev + 0, 13) to (start + 0, 20) -- Code(Zero) at (prev + 0, 13) to (start + 0, 20) -- Code(Zero) at (prev + 0, 13) to (start + 0, 20) -- Code(Zero) at (prev + 0, 13) to (start + 0, 20) - Code(Expression(0, Sub)) at (prev + 2, 13) to (start + 0, 26) = (c0 - c1) -- Code(Zero) at (prev + 0, 13) to (start + 0, 26) -- Code(Zero) at (prev + 0, 13) to (start + 0, 26) - Code(Expression(1, Add)) at (prev + 2, 5) to (start + 0, 6) = (c1 + (c0 - c1)) Function name: ::call -Raw bytes (63): 0x[01, 01, 02, 01, 05, 05, 02, 0b, 01, 33, 05, 01, 18, 05, 02, 0d, 00, 14, 00, 00, 0d, 00, 14, 00, 00, 0d, 00, 14, 00, 00, 0d, 00, 14, 00, 00, 0d, 00, 14, 02, 02, 0d, 00, 13, 00, 00, 0d, 00, 13, 00, 00, 0d, 00, 13, 00, 00, 0d, 00, 13, 07, 02, 05, 00, 06] +Raw bytes (28): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 33, 05, 01, 18, 05, 02, 0d, 00, 14, 02, 02, 0d, 00, 13, 07, 02, 05, 00, 06] Number of files: 1 - file 0 => global file 1 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: 11 +Number of file 0 mappings: 4 - Code(Counter(0)) at (prev + 51, 5) to (start + 1, 24) - Code(Counter(1)) at (prev + 2, 13) to (start + 0, 20) -- Code(Zero) at (prev + 0, 13) to (start + 0, 20) -- Code(Zero) at (prev + 0, 13) to (start + 0, 20) -- Code(Zero) at (prev + 0, 13) to (start + 0, 20) -- Code(Zero) at (prev + 0, 13) to (start + 0, 20) - Code(Expression(0, Sub)) at (prev + 2, 13) to (start + 0, 19) = (c0 - c1) -- Code(Zero) at (prev + 0, 13) to (start + 0, 19) -- Code(Zero) at (prev + 0, 13) to (start + 0, 19) -- Code(Zero) at (prev + 0, 13) to (start + 0, 19) - Code(Expression(1, Add)) at (prev + 2, 5) to (start + 0, 6) = (c1 + (c0 - c1)) Function name: try_error_result::call -Raw bytes (43): 0x[01, 01, 02, 01, 05, 05, 02, 07, 01, 04, 01, 01, 14, 05, 02, 09, 00, 10, 00, 00, 09, 00, 10, 00, 00, 09, 00, 10, 02, 02, 09, 00, 0f, 00, 00, 09, 00, 0f, 07, 02, 01, 00, 02] +Raw bytes (28): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 04, 01, 01, 14, 05, 02, 09, 00, 10, 02, 02, 09, 00, 0f, 07, 02, 01, 00, 02] Number of files: 1 - file 0 => global file 1 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: 7 +Number of file 0 mappings: 4 - Code(Counter(0)) at (prev + 4, 1) to (start + 1, 20) - Code(Counter(1)) at (prev + 2, 9) to (start + 0, 16) -- Code(Zero) at (prev + 0, 9) to (start + 0, 16) -- Code(Zero) at (prev + 0, 9) to (start + 0, 16) - Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 0, 15) = (c0 - c1) -- Code(Zero) at (prev + 0, 9) to (start + 0, 15) - Code(Expression(1, Add)) at (prev + 2, 1) to (start + 0, 2) = (c1 + (c0 - c1)) diff --git a/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff index 28a7ffda371..13ff1e284d9 100644 --- a/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.bar.InstrumentCoverage.diff @@ -4,8 +4,10 @@ fn bar() -> bool { let mut _0: bool; ++ coverage Counter(0) => /the/src/instrument_coverage.rs:20:1 - 22:2; ++ bb0: { -+ Coverage::Counter(0) for [/the/src/instrument_coverage.rs:20:1 - 22:2]; ++ Coverage::CounterIncrement(0); _0 = const true; return; } diff --git a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff index 9a8caa26307..5459ecd98c5 100644 --- a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff @@ -7,13 +7,19 @@ let mut _2: bool; let mut _3: !; ++ coverage Counter(0) => /the/src/instrument_coverage.rs:11:1 - 11:11; ++ coverage Expression(0) => /the/src/instrument_coverage.rs:12:5 - 13:17; ++ coverage Expression(1) => /the/src/instrument_coverage.rs:14:13 - 14:18; ++ coverage Expression(1) => /the/src/instrument_coverage.rs:17:1 - 17:2; ++ coverage Counter(1) => /the/src/instrument_coverage.rs:15:10 - 15:11; ++ bb0: { -+ Coverage::Counter(0) for [/the/src/instrument_coverage.rs:11:1 - 11:11]; ++ Coverage::CounterIncrement(0); goto -> bb1; } bb1: { -+ Coverage::Expression(0) = Counter(0) + Counter(1) for [/the/src/instrument_coverage.rs:12:5 - 13:17]; ++ Coverage::Expression(0) = Counter(0) + Counter(1); falseUnwind -> [real: bb2, unwind: bb6]; } @@ -27,14 +33,14 @@ } bb4: { -+ Coverage::Expression(1) = Expression(0) - Counter(1) for [/the/src/instrument_coverage.rs:14:13 - 14:18, /the/src/instrument_coverage.rs:17:1 - 17:2]; ++ Coverage::Expression(1) = Expression(0) - Counter(1); _0 = const (); StorageDead(_2); return; } bb5: { -+ Coverage::Counter(1) for [/the/src/instrument_coverage.rs:15:10 - 15:11]; ++ Coverage::CounterIncrement(1); _1 = const (); StorageDead(_2); goto -> bb1; From 7d38f4a6114b461ce77e9cd1d7fd65ec79c05a64 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 7 Sep 2023 21:57:24 +1000 Subject: [PATCH 07/10] Remove unused `TyCtxt` from `remove_dead_blocks` This context was only needed by code for processing coverage statements, which has been removed. --- compiler/rustc_mir_transform/src/abort_unwinding_calls.rs | 2 +- compiler/rustc_mir_transform/src/dest_prop.rs | 2 +- compiler/rustc_mir_transform/src/generator.rs | 4 ++-- compiler/rustc_mir_transform/src/inline.rs | 2 +- .../rustc_mir_transform/src/multiple_return_terminators.rs | 2 +- compiler/rustc_mir_transform/src/simplify.rs | 4 ++-- compiler/rustc_mir_transform/src/unreachable_prop.rs | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_mir_transform/src/abort_unwinding_calls.rs b/compiler/rustc_mir_transform/src/abort_unwinding_calls.rs index 4500bb7ff0f..74243f1f8f2 100644 --- a/compiler/rustc_mir_transform/src/abort_unwinding_calls.rs +++ b/compiler/rustc_mir_transform/src/abort_unwinding_calls.rs @@ -113,6 +113,6 @@ impl<'tcx> MirPass<'tcx> for AbortUnwindingCalls { } // We may have invalidated some `cleanup` blocks so clean those up now. - super::simplify::remove_dead_blocks(tcx, body); + super::simplify::remove_dead_blocks(body); } } diff --git a/compiler/rustc_mir_transform/src/dest_prop.rs b/compiler/rustc_mir_transform/src/dest_prop.rs index d9a132e5cf1..99b070c018e 100644 --- a/compiler/rustc_mir_transform/src/dest_prop.rs +++ b/compiler/rustc_mir_transform/src/dest_prop.rs @@ -244,7 +244,7 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation { if round_count != 0 { // Merging can introduce overlap between moved arguments and/or call destination in an // unreachable code, which validator considers to be ill-formed. - remove_dead_blocks(tcx, body); + remove_dead_blocks(body); } trace!(round_count); diff --git a/compiler/rustc_mir_transform/src/generator.rs b/compiler/rustc_mir_transform/src/generator.rs index c16f07a453c..a6693519e54 100644 --- a/compiler/rustc_mir_transform/src/generator.rs +++ b/compiler/rustc_mir_transform/src/generator.rs @@ -1088,7 +1088,7 @@ fn create_generator_drop_shim<'tcx>( // Make sure we remove dead blocks to remove // unrelated code from the resume part of the function - simplify::remove_dead_blocks(tcx, &mut body); + simplify::remove_dead_blocks(&mut body); // Update the body's def to become the drop glue. // This needs to be updated before the AbortUnwindingCalls pass. @@ -1276,7 +1276,7 @@ fn create_generator_resume_function<'tcx>( // Make sure we remove dead blocks to remove // unrelated code from the drop part of the function - simplify::remove_dead_blocks(tcx, body); + simplify::remove_dead_blocks(body); pm::run_passes_no_validate(tcx, body, &[&abort_unwinding_calls::AbortUnwindingCalls], None); diff --git a/compiler/rustc_mir_transform/src/inline.rs b/compiler/rustc_mir_transform/src/inline.rs index a70371172f7..8f578b69694 100644 --- a/compiler/rustc_mir_transform/src/inline.rs +++ b/compiler/rustc_mir_transform/src/inline.rs @@ -63,7 +63,7 @@ impl<'tcx> MirPass<'tcx> for Inline { if inline(tcx, body) { debug!("running simplify cfg on {:?}", body.source); CfgSimplifier::new(body).simplify(); - remove_dead_blocks(tcx, body); + remove_dead_blocks(body); deref_finder(tcx, body); } } diff --git a/compiler/rustc_mir_transform/src/multiple_return_terminators.rs b/compiler/rustc_mir_transform/src/multiple_return_terminators.rs index c97d034544a..c9b42e75cb2 100644 --- a/compiler/rustc_mir_transform/src/multiple_return_terminators.rs +++ b/compiler/rustc_mir_transform/src/multiple_return_terminators.rs @@ -38,6 +38,6 @@ impl<'tcx> MirPass<'tcx> for MultipleReturnTerminators { } } - simplify::remove_dead_blocks(tcx, body) + simplify::remove_dead_blocks(body) } } diff --git a/compiler/rustc_mir_transform/src/simplify.rs b/compiler/rustc_mir_transform/src/simplify.rs index 3ecb7403387..88c89e106fd 100644 --- a/compiler/rustc_mir_transform/src/simplify.rs +++ b/compiler/rustc_mir_transform/src/simplify.rs @@ -66,7 +66,7 @@ impl SimplifyCfg { pub fn simplify_cfg<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { CfgSimplifier::new(body).simplify(); remove_duplicate_unreachable_blocks(tcx, body); - remove_dead_blocks(tcx, body); + remove_dead_blocks(body); // FIXME: Should probably be moved into some kind of pass manager body.basic_blocks_mut().raw.shrink_to_fit(); @@ -335,7 +335,7 @@ pub fn remove_duplicate_unreachable_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut B } } -pub fn remove_dead_blocks<'tcx>(_tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { +pub fn remove_dead_blocks(body: &mut Body<'_>) { let reachable = traversal::reachable_as_bitset(body); let num_blocks = body.basic_blocks.len(); if num_blocks == reachable.count() { diff --git a/compiler/rustc_mir_transform/src/unreachable_prop.rs b/compiler/rustc_mir_transform/src/unreachable_prop.rs index 0b9311a20ef..ea7aafd866b 100644 --- a/compiler/rustc_mir_transform/src/unreachable_prop.rs +++ b/compiler/rustc_mir_transform/src/unreachable_prop.rs @@ -65,7 +65,7 @@ impl MirPass<'_> for UnreachablePropagation { } if replaced { - simplify::remove_dead_blocks(tcx, body); + simplify::remove_dead_blocks(body); } } } From 13b2d604ec31f76b8e913b6d2b28e1d2dad2249f Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 13 Sep 2023 13:20:13 +1000 Subject: [PATCH 08/10] coverage: Store expression data in function coverage info Even though expression details are now stored in the info structure, we still need to inject `ExpressionUsed` 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/ffi.rs | 11 -- .../src/coverageinfo/map_data.rs | 105 +++++++----------- .../src/coverageinfo/mod.rs | 4 +- compiler/rustc_middle/src/mir/coverage.rs | 41 +++---- compiler/rustc_middle/src/mir/pretty.rs | 5 +- .../src/coverage/counters.rs | 47 +++----- .../rustc_mir_transform/src/coverage/mod.rs | 42 +------ .../rustc_mir_transform/src/coverage/tests.rs | 2 +- ...ment_coverage.main.InstrumentCoverage.diff | 6 +- 9 files changed, 90 insertions(+), 173 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs index 194ed7d661d..7ad2d03a5ed 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs @@ -73,17 +73,6 @@ pub struct CounterExpression { pub rhs: Counter, } -impl CounterExpression { - /// The dummy expression `(0 - 0)` has a representation of all zeroes, - /// making it marginally more efficient to initialize than `(0 + 0)`. - pub(crate) const DUMMY: Self = - Self { lhs: Counter::ZERO, kind: ExprKind::Subtract, rhs: Counter::ZERO }; - - pub fn new(lhs: Counter, kind: ExprKind, rhs: Counter) -> Self { - Self { kind, lhs, rhs } - } -} - /// Corresponds to enum `llvm::coverage::CounterMappingRegion::RegionKind`. /// /// Must match the layout of `LLVMRustCounterMappingRegionKind`. diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index e6b39c71453..84319b4ba2d 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,12 @@ 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>, + /// Contains all expression IDs that have been seen in an `ExpressionUsed` + /// coverage statement, plus all expression IDs that aren't directly used + /// by any mappings (and therefore do not have expression-used statements). + /// After MIR traversal is finished, we can conclude that any IDs missing + /// from this set must have had their statements deleted by MIR opts. + expressions_seen: BitSet, } impl<'tcx> FunctionCoverage<'tcx> { @@ -52,16 +49,30 @@ 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}" ); + + // Create a filled set of expression IDs, so that expressions not + // directly used by mappings will be treated as "seen". + // (If they end up being unused, LLVM will delete them for us.) + let mut expressions_seen = BitSet::new_filled(num_expressions); + // For each expression ID that is directly used by one or more mappings, + // mark it as not-yet-seen. This indicates that we expect to see a + // corresponding `ExpressionUsed` statement during MIR traversal. + for Mapping { term, .. } in &function_coverage_info.mappings { + if let &CovTerm::Expression(id) = term { + expressions_seen.remove(id); + } + } + Self { function_coverage_info, is_used, counters_seen: BitSet::new_empty(num_counters), - expressions: IndexVec::from_elem_n(None, num_expressions), + expressions_seen, } } @@ -76,35 +87,10 @@ impl<'tcx> FunctionCoverage<'tcx> { self.counters_seen.insert(id); } - /// Adds information about a coverage expression. + /// Marks an expression ID as having been seen in an expression-used statement. #[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 mark_expression_id_seen(&mut self, id: ExpressionId) { + self.expressions_seen.insert(id); } /// Identify expressions that will always have a value of zero, and note @@ -125,13 +111,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_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 @@ -197,7 +183,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); @@ -217,27 +203,16 @@ impl<'tcx> FunctionCoverage<'tcx> { _ => Counter::from_term(operand), }; - self.expressions + self.function_coverage_info + .expressions .iter() - .map(|expression| match expression { - None => { - // 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, .. }) => { - // Convert the operands and operator as normal. - CounterExpression::new( - counter_from_operand(lhs), - match op { - Op::Add => ExprKind::Add, - Op::Subtract => ExprKind::Subtract, - }, - counter_from_operand(rhs), - ) - } + .map(|&Expression { lhs, op, rhs }| CounterExpression { + lhs: counter_from_operand(lhs), + kind: match op { + Op::Add => ExprKind::Add, + Op::Subtract => ExprKind::Subtract, + }, + rhs: counter_from_operand(rhs), }) .collect::>() } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 14332927c8d..f6bc4783564 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -145,8 +145,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::ExpressionUsed { id } => { + func_coverage.mark_expression_id_seen(id); } } } diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 2587a6eae1a..db578854fdc 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; @@ -68,14 +69,16 @@ pub enum CoverageKind { /// If this statement does not survive MIR optimizations, any mappings that /// refer to this counter can have those references simplified to zero. CounterIncrement { id: CounterId }, - Expression { - /// 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, - }, + + /// Marks the point in MIR control-flow represented by a coverage expression. + /// + /// If this statement does not survive MIR optimizations, any mappings that + /// refer to this expression can have those references simplified to zero. + /// + /// (This is only inserted for expression IDs that are directly used by + /// mappings. Intermediate expressions with no direct mappings are + /// retained/zeroed based on whether they are transitively used.) + ExpressionUsed { id: ExpressionId }, } impl Debug for CoverageKind { @@ -83,17 +86,7 @@ impl Debug for CoverageKind { use CoverageKind::*; match self { CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()), - Expression { id, lhs, op, rhs } => write!( - fmt, - "Expression({:?}) = {:?} {} {:?}", - id.index(), - lhs, - match op { - Op::Add => "+", - Op::Subtract => "-", - }, - rhs, - ), + ExpressionUsed { id } => write!(fmt, "ExpressionUsed({:?})", id.index()), } } } @@ -135,6 +128,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 { @@ -157,7 +158,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_middle/src/mir/pretty.rs b/compiler/rustc_middle/src/mir/pretty.rs index 11269e749f8..3b3b61e4e21 100644 --- a/compiler/rustc_middle/src/mir/pretty.rs +++ b/compiler/rustc_middle/src/mir/pretty.rs @@ -504,8 +504,11 @@ fn write_function_coverage_info( function_coverage_info: &coverage::FunctionCoverageInfo, w: &mut dyn io::Write, ) -> io::Result<()> { - let coverage::FunctionCoverageInfo { mappings, .. } = function_coverage_info; + let coverage::FunctionCoverageInfo { expressions, mappings, .. } = function_coverage_info; + for (id, expression) in expressions.iter_enumerated() { + writeln!(w, "{INDENT}coverage {id:?} => {expression:?};")?; + } for coverage::Mapping { term, code_region } in mappings { writeln!(w, "{INDENT}coverage {term:?} => {code_region:?};")?; } diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index 4140d402c75..a83ccf8fc3c 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. + 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,20 +104,13 @@ 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() } + #[cfg(test)] pub(super) fn num_expressions(&self) -> usize { - self.next_expression_id.as_usize() + self.expressions.len() } fn set_bcb_counter( @@ -207,6 +186,10 @@ impl CoverageCounters { ) -> impl Iterator + '_ { self.bcb_edge_counters.drain() } + + pub(super) fn take_expressions(&mut self) -> IndexVec { + std::mem::take(&mut self.expressions) + } } /// Traverse the `CoverageGraph` and add either a `Counter` or `Expression` to every BCB, to be @@ -333,7 +316,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 +428,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 078123a7b7f..df4dccf0f0b 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: self.coverage_counters.take_expressions(), mappings: std::mem::take(&mut self.mappings), })); } @@ -304,10 +288,8 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { inject_to_bb, ); } - BcbCounter::Expression { .. } => inject_intermediate_expression( - self.mir_body, - self.make_mir_coverage_kind(&counter_kind), - ), + // Experessions with no associated spans don't need to inject a statement. + BcbCounter::Expression { .. } => {} } } } @@ -330,9 +312,7 @@ impl<'a, 'tcx> Instrumentor<'a, 'tcx> { fn make_mir_coverage_kind(&self, counter_kind: &BcbCounter) -> CoverageKind { match *counter_kind { BcbCounter::Counter { id } => CoverageKind::CounterIncrement { id }, - BcbCounter::Expression { id, lhs, op, rhs } => { - CoverageKind::Expression { id, lhs, op, rhs } - } + BcbCounter::Expression { id } => CoverageKind::ExpressionUsed { id }, } } } @@ -371,20 +351,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 487d2282364..795cbce963d 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_eq!(coverage_counters.num_expressions(), 0); 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 5459ecd98c5..f5c59c84693 100644 --- a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff @@ -7,6 +7,8 @@ let mut _2: bool; let mut _3: !; ++ coverage ExpressionId(0) => Expression { lhs: Counter(0), op: Add, rhs: Counter(1) }; ++ coverage ExpressionId(1) => Expression { lhs: Expression(0), op: Subtract, rhs: Counter(1) }; + coverage Counter(0) => /the/src/instrument_coverage.rs:11:1 - 11:11; + coverage Expression(0) => /the/src/instrument_coverage.rs:12:5 - 13:17; + coverage Expression(1) => /the/src/instrument_coverage.rs:14:13 - 14:18; @@ -19,7 +21,7 @@ } bb1: { -+ Coverage::Expression(0) = Counter(0) + Counter(1); ++ Coverage::ExpressionUsed(0); falseUnwind -> [real: bb2, unwind: bb6]; } @@ -33,7 +35,7 @@ } bb4: { -+ Coverage::Expression(1) = Expression(0) - Counter(1); ++ Coverage::ExpressionUsed(1); _0 = const (); StorageDead(_2); return; From 753caf292c10a172b45e749047221c31d35d76eb Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 23 Sep 2023 13:17:05 +1000 Subject: [PATCH 09/10] coverage: Update docs for `StatementKind::Coverage` This new description reflects the changes made in this PR, and should hopefully be more useful to non-coverage developers who need to care about coverage statements. --- compiler/rustc_middle/src/mir/syntax.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index 77b26e0a4cd..7a645fb5d62 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -361,11 +361,16 @@ pub enum StatementKind<'tcx> { /// Disallowed after drop elaboration. AscribeUserType(Box<(Place<'tcx>, UserTypeProjection)>, ty::Variance), - /// Marks the start of a "coverage region", injected with '-Cinstrument-coverage'. A - /// `Coverage` statement carries metadata about the coverage region, used to inject a coverage - /// map into the binary. If `Coverage::kind` is a `Counter`, the statement also generates - /// executable code, to increment a counter variable at runtime, each time the code region is - /// executed. + /// Carries control-flow-sensitive information injected by `-Cinstrument-coverage`, + /// such as where to generate physical coverage-counter-increments during codegen. + /// + /// Coverage statements are used in conjunction with the coverage mappings and other + /// information stored in the function's + /// [`mir::Body::function_coverage_info`](crate::mir::Body::function_coverage_info). + /// (For inlined MIR, take care to look up the *original function's* coverage info.) + /// + /// Interpreters and codegen backends that don't support coverage instrumentation + /// can usually treat this as a no-op. Coverage(Box), /// Denotes a call to an intrinsic that does not require an unwind path and always returns. From 33da0978ac2674d046d3b01a9db33dc7f19339c6 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 18 Oct 2023 12:44:47 +1100 Subject: [PATCH 10/10] coverage: Explicitly note that counter/expression IDs are function-local --- compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs | 7 +++++++ compiler/rustc_middle/src/mir/coverage.rs | 10 ++++++++++ 2 files changed, 17 insertions(+) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index f6bc4783564..204a73b788a 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -100,6 +100,13 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> { impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> { #[instrument(level = "debug", skip(self))] fn add_coverage(&mut self, instance: Instance<'tcx>, coverage: &Coverage) { + // Our caller should have already taken care of inlining subtleties, + // so we can assume that counter/expression IDs in this coverage + // statement are meaningful for the given instance. + // + // (Either the statement was not inlined and directly belongs to this + // instance, or it was inlined *from* this instance.) + let bx = self; let Some(function_coverage_info) = diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index db578854fdc..08d377a8695 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -9,6 +9,11 @@ use std::fmt::{self, Debug, Formatter}; rustc_index::newtype_index! { /// ID of a coverage counter. Values ascend from 0. /// + /// Before MIR inlining, counter IDs are local to their enclosing function. + /// After MIR inlining, coverage statements may have been inlined into + /// another function, so use the statement's source-scope to find which + /// function/instance its IDs are meaningful for. + /// /// Note that LLVM handles counter IDs as `uint32_t`, so there is no need /// to use a larger representation on the Rust side. #[derive(HashStable)] @@ -24,6 +29,11 @@ impl CounterId { rustc_index::newtype_index! { /// ID of a coverage-counter expression. Values ascend from 0. /// + /// Before MIR inlining, expression IDs are local to their enclosing function. + /// After MIR inlining, coverage statements may have been inlined into + /// another function, so use the statement's source-scope to find which + /// function/instance its IDs are meaningful for. + /// /// Note that LLVM handles expression IDs as `uint32_t`, so there is no need /// to use a larger representation on the Rust side. #[derive(HashStable)]