diff options
Diffstat (limited to 'compiler/rustc_codegen_ssa/src')
| -rw-r--r-- | compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs | 22 | ||||
| -rw-r--r-- | compiler/rustc_codegen_ssa/src/coverageinfo/map.rs | 77 | ||||
| -rw-r--r-- | compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs | 2 |
3 files changed, 84 insertions, 17 deletions
diff --git a/compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs b/compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs index af6c476292b..962c01c2ee7 100644 --- a/compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs +++ b/compiler/rustc_codegen_ssa/src/coverageinfo/ffi.rs @@ -24,21 +24,39 @@ pub enum CounterKind { pub struct Counter { // Important: The layout (order and types of fields) must match its C++ counterpart. pub kind: CounterKind, - pub id: u32, + id: u32, } impl Counter { + /// Constructs a new `Counter` of kind `Zero`. For this `CounterKind`, the + /// `id` is not used. pub fn zero() -> Self { Self { kind: CounterKind::Zero, id: 0 } } + /// Constructs a new `Counter` of kind `CounterValueReference`, and converts + /// the given 1-based counter_id to the required 0-based equivalent for + /// the `Counter` encoding. pub fn counter_value_reference(counter_id: CounterValueReference) -> Self { - Self { kind: CounterKind::CounterValueReference, id: counter_id.into() } + Self { kind: CounterKind::CounterValueReference, id: counter_id.zero_based_index() } } + /// Constructs a new `Counter` of kind `Expression`. pub fn expression(mapped_expression_index: MappedExpressionIndex) -> Self { Self { kind: CounterKind::Expression, id: mapped_expression_index.into() } } + + /// Returns true if the `Counter` kind is `Zero`. + pub fn is_zero(&self) -> bool { + matches!(self.kind, CounterKind::Zero) + } + + /// An explicitly-named function to get the ID value, making it more obvious + /// that the stored value is now 0-based. + pub fn zero_based_id(&self) -> u32 { + debug_assert!(!self.is_zero(), "`id` is undefined for CounterKind::Zero"); + self.id + } } /// Aligns with [llvm::coverage::CounterExpression::ExprKind](https://github.com/rust-lang/llvm-project/blob/rustc/11.0-2020-10-12/llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h#L147) diff --git a/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs b/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs index 7a17bced1c0..4458fd68678 100644 --- a/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs +++ b/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs @@ -163,9 +163,7 @@ impl<'tcx> FunctionCoverage<'tcx> { self.counters.iter_enumerated().filter_map(|(index, entry)| { // Option::map() will return None to filter out missing counters. This may happen // if, for example, a MIR-instrumented counter is removed during an optimization. - entry.as_ref().map(|region| { - (Counter::counter_value_reference(index as CounterValueReference), region) - }) + entry.as_ref().map(|region| (Counter::counter_value_reference(index), region)) }) } @@ -206,9 +204,15 @@ impl<'tcx> FunctionCoverage<'tcx> { if id == ExpressionOperandId::ZERO { Some(Counter::zero()) } else if id.index() < self.counters.len() { + debug_assert!( + id.index() > 0, + "ExpressionOperandId indexes for counters are 1-based, but this id={}", + id.index() + ); // Note: Some codegen-injected Counters may be only referenced by `Expression`s, // and may not have their own `CodeRegion`s, let index = CounterValueReference::from(id.index()); + // Note, the conversion to LLVM `Counter` adjusts the index to be zero-based. Some(Counter::counter_value_reference(index)) } else { let index = self.expression_index(u32::from(id)); @@ -233,19 +237,60 @@ impl<'tcx> FunctionCoverage<'tcx> { let optional_region = &expression.region; let Expression { lhs, op, rhs, .. } = *expression; - if let Some(Some((lhs_counter, rhs_counter))) = - id_to_counter(&new_indexes, lhs).map(|lhs_counter| { + if let Some(Some((lhs_counter, mut rhs_counter))) = id_to_counter(&new_indexes, lhs) + .map(|lhs_counter| { id_to_counter(&new_indexes, rhs).map(|rhs_counter| (lhs_counter, rhs_counter)) }) { + if lhs_counter.is_zero() && op.is_subtract() { + // The left side of a subtraction was probably optimized out. As an example, + // a branch condition might be evaluated as a constant expression, and the + // branch could be removed, dropping unused counters in the process. + // + // Since counters are unsigned, we must assume the result of the expression + // can be no more and no less than zero. An expression known to evaluate to zero + // does not need to be added to the coverage map. + // + // Coverage test `loops_branches.rs` includes multiple variations of branches + // based on constant conditional (literal `true` or `false`), and demonstrates + // that the expected counts are still correct. + debug!( + "Expression subtracts from zero (assume unreachable): \ + original_index={:?}, lhs={:?}, op={:?}, rhs={:?}, region={:?}", + original_index, lhs, op, rhs, optional_region, + ); + rhs_counter = Counter::zero(); + } debug_assert!( - (lhs_counter.id as usize) - < usize::max(self.counters.len(), self.expressions.len()) + lhs_counter.is_zero() + // Note: with `as usize` the ID _could_ overflow/wrap if `usize = u16` + || ((lhs_counter.zero_based_id() as usize) + <= usize::max(self.counters.len(), self.expressions.len())), + "lhs id={} > both counters.len()={} and expressions.len()={} + ({:?} {:?} {:?})", + lhs_counter.zero_based_id(), + self.counters.len(), + self.expressions.len(), + lhs_counter, + op, + rhs_counter, ); + debug_assert!( - (rhs_counter.id as usize) - < usize::max(self.counters.len(), self.expressions.len()) + rhs_counter.is_zero() + // Note: with `as usize` the ID _could_ overflow/wrap if `usize = u16` + || ((rhs_counter.zero_based_id() as usize) + <= usize::max(self.counters.len(), self.expressions.len())), + "rhs id={} > both counters.len()={} and expressions.len()={} + ({:?} {:?} {:?})", + rhs_counter.zero_based_id(), + self.counters.len(), + self.expressions.len(), + lhs_counter, + op, + rhs_counter, ); + // Both operands exist. `Expression` operands exist in `self.expressions` and have // been assigned a `new_index`. let mapped_expression_index = @@ -268,11 +313,15 @@ impl<'tcx> FunctionCoverage<'tcx> { expression_regions.push((Counter::expression(mapped_expression_index), region)); } } else { - debug!( - "Ignoring expression with one or more missing operands: \ - original_index={:?}, lhs={:?}, op={:?}, rhs={:?}, region={:?}", - original_index, lhs, op, rhs, optional_region, - ) + bug!( + "expression has one or more missing operands \ + original_index={:?}, lhs={:?}, op={:?}, rhs={:?}, region={:?}", + original_index, + lhs, + op, + rhs, + optional_region, + ); } } (counter_expressions, expression_regions.into_iter()) diff --git a/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs b/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs index e2f75b2e337..621ec0519c9 100644 --- a/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs +++ b/compiler/rustc_codegen_ssa/src/mir/coverageinfo.rs @@ -36,7 +36,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let fn_name = bx.get_pgo_func_name_var(instance); let hash = bx.const_u64(function_source_hash); let num_counters = bx.const_u32(coverageinfo.num_counters); - let index = bx.const_u32(u32::from(id)); + let index = bx.const_u32(id.zero_based_index()); debug!( "codegen intrinsic instrprof.increment(fn_name={:?}, hash={:?}, num_counters={:?}, index={:?})", fn_name, hash, num_counters, index, |