From f103db894fdcf94822d57cf28e30bc498c042631 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 29 Jun 2023 12:14:04 +1000 Subject: [PATCH] Make coverage expression IDs count up from 0, not down from `u32::MAX` Operand types are now tracked explicitly, so there is no need for expression IDs to avoid counter IDs by descending from `u32::MAX`. Instead they can just count up from 0, and can be used directly as indices when necessary. --- .../src/coverageinfo/map_data.rs | 49 ++++++------------- .../src/coverageinfo/mod.rs | 4 +- compiler/rustc_middle/src/mir/coverage.rs | 26 +++++----- .../rustc_middle/src/ty/structural_impls.rs | 3 +- .../src/coverage/counters.rs | 19 +++---- .../rustc_mir_transform/src/coverage/debug.rs | 2 +- .../rustc_mir_transform/src/coverage/query.rs | 9 ++-- ...age_graphviz.main.InstrumentCoverage.0.dot | 2 +- ...ment_coverage.main.InstrumentCoverage.diff | 6 +-- 9 files changed, 49 insertions(+), 71 deletions(-) diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs index 0d4d8ca61e93..4fada3293670 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs @@ -3,8 +3,7 @@ pub use super::ffi::*; use rustc_index::{IndexSlice, IndexVec}; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, InjectedExpressionId, InjectedExpressionIndex, - MappedExpressionIndex, Op, Operand, + CodeRegion, CounterValueReference, ExpressionId, MappedExpressionIndex, Op, Operand, }; use rustc_middle::ty::Instance; use rustc_middle::ty::TyCtxt; @@ -19,8 +18,7 @@ pub struct Expression { /// 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. Counters and counter expressions have non-overlapping `id`s because they -/// can both be operands in an expression. This struct also stores the `function_source_hash`, +/// 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 @@ -35,7 +33,7 @@ pub struct FunctionCoverage<'tcx> { source_hash: u64, is_used: bool, counters: IndexVec>, - expressions: IndexVec>, + expressions: IndexVec>, unreachable_regions: Vec, } @@ -89,22 +87,11 @@ impl<'tcx> FunctionCoverage<'tcx> { } /// Both counters and "counter expressions" (or simply, "expressions") can be operands in other - /// expressions. Expression IDs start from `u32::MAX` and go down, so the range of expression - /// IDs will not overlap with the range of counter IDs. Counters and expressions can be added in - /// any order, and expressions can still be assigned contiguous (though descending) IDs, without - /// knowing what the last counter ID will be. - /// - /// When storing the expression data in the `expressions` vector in the `FunctionCoverage` - /// struct, its vector index is computed, from the given expression ID, by subtracting from - /// `u32::MAX`. - /// - /// Since the expression operands (`lhs` and `rhs`) can reference either counters or - /// expressions, an operand that references an expression also uses its original ID, descending - /// from `u32::MAX`. Theses operands are translated only during code generation, after all - /// counters and expressions have been added. + /// expressions. These are tracked as separate variants of `Operand`, so there is no ambiguity + /// between operands that are counter IDs and operands that are expression IDs. pub fn add_counter_expression( &mut self, - expression_id: InjectedExpressionId, + expression_id: ExpressionId, lhs: Operand, op: Op, rhs: Operand, @@ -114,16 +101,15 @@ impl<'tcx> FunctionCoverage<'tcx> { "add_counter_expression({:?}, lhs={:?}, op={:?}, rhs={:?} at {:?}", expression_id, lhs, op, rhs, region ); - let expression_index = self.expression_index(expression_id); debug_assert!( - expression_index.as_usize() < self.expressions.len(), - "expression_index {} is out of range for expressions.len() = {} + expression_id.as_usize() < self.expressions.len(), + "expression_id {} is out of range for expressions.len() = {} for {:?}", - expression_index.as_usize(), + expression_id.as_usize(), self.expressions.len(), self, ); - if let Some(previous_expression) = self.expressions[expression_index].replace(Expression { + if let Some(previous_expression) = self.expressions[expression_id].replace(Expression { lhs, op, rhs, @@ -190,7 +176,7 @@ impl<'tcx> FunctionCoverage<'tcx> { // // Expressions will be returned from this function in a sequential vector (array) of // `CounterExpression`, so the expression IDs must be mapped from their original, - // potentially sparse set of indexes, originally in reverse order from `u32::MAX`. + // potentially sparse set of indexes. // // An `Expression` as an operand will have already been encountered as an `Expression` with // operands, so its new_index will already have been generated (as a 1-up index value). @@ -203,7 +189,7 @@ impl<'tcx> FunctionCoverage<'tcx> { // `expression_index`s lower than the referencing `Expression`. Therefore, it is // reasonable to look up the new index of an expression operand while the `new_indexes` // vector is only complete up to the current `ExpressionIndex`. - type NewIndexes = IndexSlice>; + type NewIndexes = IndexSlice>; let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand { Operand::Zero => Some(Counter::zero()), Operand::Counter(id) => { @@ -219,15 +205,14 @@ impl<'tcx> FunctionCoverage<'tcx> { Some(Counter::counter_value_reference(index)) } Operand::Expression(id) => { - let index = self.expression_index(id); self.expressions - .get(index) + .get(id) .expect("expression id is out of range") .as_ref() // If an expression was optimized out, assume it would have produced a count // of zero. This ensures that expressions dependent on optimized-out // expressions are still valid. - .map_or(Some(Counter::zero()), |_| new_indexes[index].map(Counter::expression)) + .map_or(Some(Counter::zero()), |_| new_indexes[id].map(Counter::expression)) } }; @@ -334,10 +319,4 @@ impl<'tcx> FunctionCoverage<'tcx> { fn unreachable_regions(&self) -> impl Iterator { self.unreachable_regions.iter().map(|region| (Counter::zero(), region)) } - - fn expression_index(&self, id: InjectedExpressionId) -> InjectedExpressionIndex { - debug_assert!(id.as_usize() >= self.counters.len()); - let id_descending_from_max = id.as_u32(); - InjectedExpressionIndex::from(u32::MAX - id_descending_from_max) - } } diff --git a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs index d6a32497f7ff..5e0c5df2bcb9 100644 --- a/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs +++ b/compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs @@ -17,7 +17,7 @@ use rustc_hir::def_id::DefId; use rustc_llvm::RustString; use rustc_middle::bug; use rustc_middle::mir::coverage::{ - CodeRegion, CounterValueReference, CoverageKind, InjectedExpressionId, Op, Operand, + CodeRegion, CounterValueReference, CoverageKind, ExpressionId, Op, Operand, }; use rustc_middle::mir::Coverage; use rustc_middle::ty; @@ -202,7 +202,7 @@ impl<'tcx> Builder<'_, '_, 'tcx> { fn add_coverage_counter_expression( &mut self, instance: Instance<'tcx>, - id: InjectedExpressionId, + id: ExpressionId, lhs: Operand, op: Op, rhs: Operand, diff --git a/compiler/rustc_middle/src/mir/coverage.rs b/compiler/rustc_middle/src/mir/coverage.rs index 7894e564b02f..bcec46d9149a 100644 --- a/compiler/rustc_middle/src/mir/coverage.rs +++ b/compiler/rustc_middle/src/mir/coverage.rs @@ -26,19 +26,23 @@ impl CounterValueReference { } rustc_index::newtype_index! { - /// Values descend from u32::MAX. + /// ID of a coverage-counter expression. Values ascend from 0. + /// + /// 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)] #[max = 0xFFFF_FFFF] - #[debug_format = "InjectedExpressionId({})"] - pub struct InjectedExpressionId {} + #[debug_format = "ExpressionId({})"] + pub struct ExpressionId {} } -rustc_index::newtype_index! { - /// Values ascend from 0. - #[derive(HashStable)] - #[max = 0xFFFF_FFFF] - #[debug_format = "InjectedExpressionIndex({})"] - pub struct InjectedExpressionIndex {} +impl ExpressionId { + pub const START: Self = Self::from_u32(0); + + #[inline(always)] + pub fn next_id(self) -> Self { + Self::from_u32(self.as_u32() + 1) + } } rustc_index::newtype_index! { @@ -60,7 +64,7 @@ rustc_index::newtype_index! { pub enum Operand { Zero, Counter(CounterValueReference), - Expression(InjectedExpressionId), + Expression(ExpressionId), } impl Debug for Operand { @@ -84,7 +88,7 @@ pub enum CoverageKind { 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: InjectedExpressionId, + id: ExpressionId, lhs: Operand, op: Op, rhs: Operand, diff --git a/compiler/rustc_middle/src/ty/structural_impls.rs b/compiler/rustc_middle/src/ty/structural_impls.rs index 10cff0e2f500..15cf4c467888 100644 --- a/compiler/rustc_middle/src/ty/structural_impls.rs +++ b/compiler/rustc_middle/src/ty/structural_impls.rs @@ -471,8 +471,7 @@ TrivialTypeTraversalAndLiftImpls! { ::rustc_target::asm::InlineAsmRegOrRegClass, ::rustc_target::spec::abi::Abi, crate::mir::coverage::CounterValueReference, - crate::mir::coverage::InjectedExpressionId, - crate::mir::coverage::InjectedExpressionIndex, + crate::mir::coverage::ExpressionId, crate::mir::coverage::MappedExpressionIndex, crate::mir::Local, crate::mir::Promoted, diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index 9d168404264a..cd2539a5e663 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -17,7 +17,7 @@ use rustc_middle::mir::coverage::*; pub(super) struct CoverageCounters { function_source_hash: u64, next_counter_id: u32, - num_expressions: u32, + next_expression_id: ExpressionId, pub debug_counters: DebugCounters, } @@ -26,7 +26,7 @@ impl CoverageCounters { Self { function_source_hash, next_counter_id: CounterValueReference::START.as_u32(), - num_expressions: 0, + next_expression_id: ExpressionId::START, debug_counters: DebugCounters::new(), } } @@ -94,20 +94,17 @@ impl CoverageCounters { /// Counter IDs start from one and go up. fn next_counter(&mut self) -> CounterValueReference { - assert!(self.next_counter_id < u32::MAX - self.num_expressions); let next = self.next_counter_id; self.next_counter_id += 1; CounterValueReference::from(next) } - /// Expression IDs start from u32::MAX and go down because an Expression can reference - /// (add or subtract counts) of both Counter regions and Expression regions. The counter - /// expression operand IDs must be unique across both types. - fn next_expression(&mut self) -> InjectedExpressionId { - assert!(self.next_counter_id < u32::MAX - self.num_expressions); - let next = u32::MAX - self.num_expressions; - self.num_expressions += 1; - InjectedExpressionId::from(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 = next.next_id(); + next } } diff --git a/compiler/rustc_mir_transform/src/coverage/debug.rs b/compiler/rustc_mir_transform/src/coverage/debug.rs index 0e1065a40e70..26f9cfd0b86c 100644 --- a/compiler/rustc_mir_transform/src/coverage/debug.rs +++ b/compiler/rustc_mir_transform/src/coverage/debug.rs @@ -485,7 +485,7 @@ impl GraphvizData { /// _not_ used are retained in the `unused_expressions` Vec, to be included in debug output (logs /// and/or a `CoverageGraph` graphviz output). pub(super) struct UsedExpressions { - some_used_expression_operands: Option>>, + some_used_expression_operands: Option>>, some_unused_expressions: Option, BasicCoverageBlock)>>, } diff --git a/compiler/rustc_mir_transform/src/coverage/query.rs b/compiler/rustc_mir_transform/src/coverage/query.rs index 1261cf2765ca..c1a896dbd349 100644 --- a/compiler/rustc_mir_transform/src/coverage/query.rs +++ b/compiler/rustc_mir_transform/src/coverage/query.rs @@ -52,12 +52,11 @@ impl CoverageVisitor { self.info.num_counters = std::cmp::max(self.info.num_counters, counter_id + 1); } - /// Computes an expression index for each expression ID, and updates `num_expressions` to the - /// maximum encountered index plus 1. + /// Updates `num_expressions` to the maximum encountered expression ID plus 1. #[inline(always)] - fn update_num_expressions(&mut self, expression_id: InjectedExpressionId) { - let expression_index = u32::MAX - expression_id.as_u32(); - self.info.num_expressions = std::cmp::max(self.info.num_expressions, expression_index + 1); + fn update_num_expressions(&mut self, expression_id: ExpressionId) { + let expression_id = expression_id.as_u32(); + self.info.num_expressions = std::cmp::max(self.info.num_expressions, expression_id + 1); } fn update_from_expression_operand(&mut self, operand: Operand) { diff --git a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot index 5d19b863114e..affd0fb18d57 100644 --- a/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot +++ b/tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot @@ -3,7 +3,7 @@ digraph Cov_0_3 { node [fontname="Courier, monospace"]; edge [fontname="Courier, monospace"]; bcb3__Cov_0_3 [shape="none", label=<
bcb3
Counter(bcb3) at 13:10-13:10
13:10-13:10: @5[0]: Coverage::Counter(2) for $DIR/coverage_graphviz.rs:13:10 - 13:11
bb5: Goto
>]; - bcb2__Cov_0_3 [shape="none", label=<
bcb2
Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18
12:13-12:18: @4[0]: Coverage::Expression(4294967293) = Expression(4294967294) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2
Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2
15:2-15:2: @4.Return: return
bb4: Return
>]; + bcb2__Cov_0_3 [shape="none", label=<
bcb2
Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18
12:13-12:18: @4[0]: Coverage::Expression(2) = Expression(1) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2
Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2
15:2-15:2: @4.Return: return
bb4: Return
>]; bcb1__Cov_0_3 [shape="none", label=<
bcb1
Expression(bcb0 + bcb3) at 10:5-11:17
11:12-11:17: @2.Call: _2 = bar() -> [return: bb3, unwind: bb6]
bb1: FalseUnwind
bb2: Call
bb3: SwitchInt
>]; bcb0__Cov_0_3 [shape="none", label=<
bcb0
Counter(bcb0) at 9:1-9:11
bb0: Goto
>]; bcb3__Cov_0_3 -> bcb1__Cov_0_3 [label=<>]; diff --git a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff index 5bd271456c6b..e87ed5268e06 100644 --- a/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff +++ b/tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff @@ -13,7 +13,7 @@ } bb1: { -+ Coverage::Expression(4294967295) = Counter(1) + Counter(2) for /the/src/instrument_coverage.rs:12:5 - 13:17; ++ Coverage::Expression(0) = Counter(1) + Counter(2) for /the/src/instrument_coverage.rs:12:5 - 13:17; falseUnwind -> [real: bb2, unwind: bb6]; } @@ -27,8 +27,8 @@ } bb4: { -+ Coverage::Expression(4294967293) = Expression(4294967294) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2; -+ Coverage::Expression(4294967294) = Expression(4294967295) - Counter(2) for /the/src/instrument_coverage.rs:14:13 - 14:18; ++ Coverage::Expression(2) = Expression(1) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2; ++ Coverage::Expression(1) = Expression(0) - Counter(2) for /the/src/instrument_coverage.rs:14:13 - 14:18; _0 = const (); StorageDead(_2); return;