From 1973f84ebbb3b2bb4b9a1488b6553ac46b2db8d4 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Sun, 25 Oct 2020 11:13:16 -0700 Subject: [PATCH] Addressed all feedback to date --- .../src/coverageinfo/mod.rs | 8 +++----- .../rustc_codegen_ssa/src/coverageinfo/map.rs | 16 +++------------ .../rustc_codegen_ssa/src/mir/coverageinfo.rs | 17 ++++++++-------- .../src/traits/coverageinfo.rs | 6 +++--- compiler/rustc_middle/src/mir/coverage.rs | 14 ------------- .../src/transform/coverage/counters.rs | 11 ++++++++++ .../rustc_mir/src/transform/coverage/query.rs | 20 +++++++++---------- .../rustc_mir/src/transform/coverage/spans.rs | 6 ++---- 8 files changed, 41 insertions(+), 57 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index 75e8abaf2a9..e21e03822eb 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -82,21 +82,19 @@ fn set_function_source_hash( fn add_coverage_counter( &mut self, instance: Instance<'tcx>, - function_source_hash: u64, id: CounterValueReference, region: CodeRegion, ) -> bool { if let Some(coverage_context) = self.coverage_context() { debug!( - "adding counter to coverage_map: instance={:?}, function_source_hash={}, id={:?}, \ - at {:?}", - instance, function_source_hash, id, region, + "adding counter to coverage_map: instance={:?}, id={:?}, region={:?}", + instance, id, region, ); let mut coverage_map = coverage_context.function_coverage_map.borrow_mut(); coverage_map .entry(instance) .or_insert_with(|| FunctionCoverage::new(self.tcx, instance)) - .add_counter(function_source_hash, id, region); + .add_counter(id, region); true } else { false diff --git a/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs b/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs index 24fb107b567..b0d7953f511 100644 --- a/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs +++ b/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs @@ -52,11 +52,8 @@ pub fn new(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Self { } } - /// Although every function should have at least one `Counter`, the `Counter` isn't required to - /// have a `CodeRegion`. (The `CodeRegion` may be associated only with `Expressions`.) This - /// method supports the ability to ensure the `function_source_hash` is set from `Counters` that - /// do not trigger the call to `add_counter()` because they don't have an associated - /// `CodeRegion` to add. + /// 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; @@ -66,14 +63,7 @@ pub fn set_function_source_hash(&mut self, source_hash: u64) { } /// Adds a code region to be counted by an injected counter intrinsic. - /// The source_hash (computed during coverage instrumentation) should also be provided, and - /// should be the same for all counters in a given function. - pub fn add_counter(&mut self, source_hash: u64, id: CounterValueReference, region: CodeRegion) { - if self.source_hash == 0 { - self.source_hash = source_hash; - } else { - debug_assert_eq!(source_hash, self.source_hash); - } + pub fn add_counter(&mut self, id: CounterValueReference, region: CodeRegion) { self.counters[id].replace(region).expect_none("add_counter called with duplicate `id`"); } diff --git a/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs b/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs index 339e0d95fdf..a115d358666 100644 --- a/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs +++ b/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs @@ -10,15 +10,16 @@ pub fn codegen_coverage(&self, bx: &mut Bx, coverage: Coverage) { let Coverage { kind, code_region } = coverage; match kind { CoverageKind::Counter { function_source_hash, id } => { - let covmap_updated = if let Some(code_region) = code_region { - // Note: Some counters do not have code regions, but may still be referenced from - // expressions. - bx.add_coverage_counter(self.instance, function_source_hash, id, code_region) - } else { - bx.set_function_source_hash(self.instance, function_source_hash) - }; + if bx.set_function_source_hash(self.instance, function_source_hash) { + // If `set_function_source_hash()` returned true, the coverage map is enabled, + // so continue adding the counter. + if let Some(code_region) = code_region { + // Note: Some counters do not have code regions, but may still be referenced + // from expressions. In that case, don't add the counter to the coverage map, + // but do inject the counter intrinsic. + bx.add_coverage_counter(self.instance, id, code_region); + } - if covmap_updated { let coverageinfo = bx.tcx().coverageinfo(self.instance.def_id()); let fn_name = bx.create_pgo_func_name_var(self.instance); diff --git a/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs b/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs index 7da38880d60..95bddfb4b41 100644 --- a/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs +++ b/compiler/rustc_codegen_ssa/src/traits/coverageinfo.rs @@ -9,8 +9,9 @@ pub trait CoverageInfoMethods: BackendTypes { pub trait CoverageInfoBuilderMethods<'tcx>: BackendTypes { fn create_pgo_func_name_var(&self, instance: Instance<'tcx>) -> Self::Value; - /// Returns true if the function source hash was added to the coverage map; false if - /// `-Z instrument-coverage` is not enabled (a coverage map is not being generated). + /// Returns true if the function source hash was added to the coverage map (even if it had + /// already been added, for this instance). Returns false *only* if `-Z instrument-coverage` is + /// not enabled (a coverage map is not being generated). fn set_function_source_hash( &mut self, instance: Instance<'tcx>, @@ -22,7 +23,6 @@ fn set_function_source_hash( fn add_coverage_counter( &mut self, instance: Instance<'tcx>, - function_source_hash: u64, index: CounterValueReference, region: CodeRegion, ) -> bool; diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index ad0ba292d02..3a97e05a39f 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -85,13 +85,6 @@ fn from(v: CounterValueReference) -> ExpressionOperandId { } } -impl From<&mut CounterValueReference> for ExpressionOperandId { - #[inline] - fn from(v: &mut CounterValueReference) -> ExpressionOperandId { - ExpressionOperandId::from(v.as_u32()) - } -} - impl From for ExpressionOperandId { #[inline] fn from(v: InjectedExpressionId) -> ExpressionOperandId { @@ -99,13 +92,6 @@ fn from(v: InjectedExpressionId) -> ExpressionOperandId { } } -impl From<&mut InjectedExpressionId> for ExpressionOperandId { - #[inline] - fn from(v: &mut InjectedExpressionId) -> ExpressionOperandId { - ExpressionOperandId::from(v.as_u32()) - } -} - #[derive(Clone, PartialEq, TyEncodable, TyDecodable, HashStable, TypeFoldable)] pub enum CoverageKind { Counter { diff --git a/compiler/rustc_mir/src/transform/coverage/counters.rs b/compiler/rustc_mir/src/transform/coverage/counters.rs index a3ae3021524..7454ec2f438 100644 --- a/compiler/rustc_mir/src/transform/coverage/counters.rs +++ b/compiler/rustc_mir/src/transform/coverage/counters.rs @@ -12,6 +12,16 @@ use rustc_index::bit_set::BitSet; use rustc_middle::mir::coverage::*; +// When evaluating an expression operand to determine if it references a `Counter` or an +// `Expression`, the range of counter or expression IDs must be known in order to answer the +// question: "Does this ID fall inside the range of counters," for example. If "yes," the ID refers +// to a counter, otherwise the ID refers to an expression. +// +// But in situations where the range is not currently known, the only fallback is to assume a +// specific range limit. `MAX_COUNTER_GUARD` enforces a limit on the number of counters, and +// therefore a limit on the range of counter IDs. +pub(crate) const MAX_COUNTER_GUARD: u32 = (u32::MAX / 2) + 1; + /// Manages the counter and expression indexes/IDs to generate `CoverageKind` components for MIR /// `Coverage` statements. pub(crate) struct CoverageCounters { @@ -95,6 +105,7 @@ pub fn make_identity_counter(&mut self, counter_operand: ExpressionOperandId) -> /// Counter IDs start from one and go up. fn next_counter(&mut self) -> CounterValueReference { assert!(self.next_counter_id < u32::MAX - self.num_expressions); + assert!(self.next_counter_id <= MAX_COUNTER_GUARD); let next = self.next_counter_id; self.next_counter_id += 1; CounterValueReference::from(next) diff --git a/compiler/rustc_mir/src/transform/coverage/query.rs b/compiler/rustc_mir/src/transform/coverage/query.rs index c17221b78dd..2fbbc9b675a 100644 --- a/compiler/rustc_mir/src/transform/coverage/query.rs +++ b/compiler/rustc_mir/src/transform/coverage/query.rs @@ -1,3 +1,5 @@ +use super::counters; + use rustc_middle::mir::coverage::*; use rustc_middle::mir::visit::Visitor; use rustc_middle::mir::{Coverage, CoverageInfo, Location}; @@ -32,21 +34,16 @@ pub(crate) fn provide(providers: &mut Providers) { /// safeguard, with `add_missing_operands` set to `true`, to find any other counter or expression /// IDs referenced by expression operands, if not already seen. /// -/// Ideally, every expression operand in the MIR will have a corresponding Counter or Expression, -/// but since current or future MIR optimizations can theoretically optimize out segments of a -/// MIR, it may not be possible to guarantee this, so the second pass ensures the `CoverageInfo` -/// counts include all referenced IDs. +/// Ideally, each operand ID in a MIR `CoverageKind::Expression` will have a separate MIR `Coverage` +/// statement for the `Counter` or `Expression` with the referenced ID. but since current or future +/// MIR optimizations can theoretically optimize out segments of a MIR, it may not be possible to +/// guarantee this, so the second pass ensures the `CoverageInfo` counts include all referenced IDs. struct CoverageVisitor { info: CoverageInfo, add_missing_operands: bool, } impl CoverageVisitor { - // If an expression operand is encountered with an ID outside the range of known counters and - // expressions, the only way to determine if the ID is a counter ID or an expression ID is to - // assume a maximum possible counter ID value. - const MAX_COUNTER_GUARD: u32 = (u32::MAX / 2) + 1; - #[inline(always)] fn update_num_counters(&mut self, counter_id: u32) { self.info.num_counters = std::cmp::max(self.info.num_counters, counter_id + 1); @@ -62,7 +59,10 @@ fn update_from_expression_operand(&mut self, operand_id: u32) { if operand_id >= self.info.num_counters { let operand_as_expression_index = u32::MAX - operand_id; if operand_as_expression_index >= self.info.num_expressions { - if operand_id <= Self::MAX_COUNTER_GUARD { + if operand_id <= counters::MAX_COUNTER_GUARD { + // Since the complete range of counter and expression IDs is not known here, the + // only way to determine if the ID is a counter ID or an expression ID is to + // assume a maximum possible counter ID value. self.update_num_counters(operand_id) } else { self.update_num_expressions(operand_id) diff --git a/compiler/rustc_mir/src/transform/coverage/spans.rs b/compiler/rustc_mir/src/transform/coverage/spans.rs index 61b5b148053..8549ac0e4af 100644 --- a/compiler/rustc_mir/src/transform/coverage/spans.rs +++ b/compiler/rustc_mir/src/transform/coverage/spans.rs @@ -388,8 +388,7 @@ fn bcb_to_initial_coverage_spans( bcb_data .basic_blocks .iter() - .map(|bbref| { - let bb = *bbref; + .flat_map(|&bb| { let data = &self.mir_body[bb]; data.statements .iter() @@ -404,7 +403,6 @@ fn bcb_to_initial_coverage_spans( .map(|span| CoverageSpan::for_terminator(span, bcb, bb)), ) }) - .flatten() .collect() } @@ -733,7 +731,7 @@ fn filtered_terminator_span(terminator: &'a Terminator<'tcx>, body_span: Span) - // However, in other cases, a visible `CoverageSpan` is not wanted, but the `Goto` // block must still be counted (for example, to contribute its count to an `Expression` // that reports the execution count for some other block). In these cases, the code region - // is set to `None`. + // is set to `None`. (See `Instrumentor::is_code_region_redundant()`.) TerminatorKind::Goto { .. } => { Some(function_source_span(terminator.source_info.span.shrink_to_hi(), body_span)) }