bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Make coverage expression IDs count up from 0, not down from u32::MAX

Browse source 
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.
This commit is contained in:
Zalathar 2023-06-29 12:14:04 +10:00
parent 1a014d42f4
commit f103db894f
9 changed files with 49 additions and 71 deletions
Download patch file Download diff file Expand all files Collapse all files

49
compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs
View file

@ -3,8 +3,7 @@ pub use super::ffi::*;
use rustc_index::{IndexSlice, IndexVec}; use rustc_index::{IndexSlice, IndexVec};
use rustc_middle::bug; use rustc_middle::bug;
use rustc_middle::mir::coverage::{ use rustc_middle::mir::coverage::{
CodeRegion, CounterValueReference, InjectedExpressionId, InjectedExpressionIndex, CodeRegion, CounterValueReference, ExpressionId, MappedExpressionIndex, Op, Operand,
MappedExpressionIndex, Op, Operand,
}; };
use rustc_middle::ty::Instance; use rustc_middle::ty::Instance;
use rustc_middle::ty::TyCtxt; 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 /// 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), /// 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 /// for a given Function. This struct also stores the `function_source_hash`,
/// can both be operands in an expression. This struct also stores the `function_source_hash`,
/// computed during instrumentation, and forwarded with counters. /// computed during instrumentation, and forwarded with counters.
/// ///
/// Note, it may be important to understand LLVM's definitions of `unreachable` regions versus "gap /// 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, source_hash: u64,
is_used: bool, is_used: bool,
counters: IndexVec<CounterValueReference, Option<CodeRegion>>, counters: IndexVec<CounterValueReference, Option<CodeRegion>>,
expressions: IndexVec<InjectedExpressionIndex, Option<Expression>>, expressions: IndexVec<ExpressionId, Option<Expression>>,
unreachable_regions: Vec<CodeRegion>, unreachable_regions: Vec<CodeRegion>,
} }
@ -89,22 +87,11 @@ impl<'tcx> FunctionCoverage<'tcx> {
} }
/// Both counters and "counter expressions" (or simply, "expressions") can be operands in other /// 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 /// expressions. These are tracked as separate variants of `Operand`, so there is no ambiguity
/// IDs will not overlap with the range of counter IDs. Counters and expressions can be added in /// between operands that are counter IDs and operands that are expression IDs.
/// 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.
pub fn add_counter_expression( pub fn add_counter_expression(
&mut self, &mut self,
expression_id: InjectedExpressionId, expression_id: ExpressionId,
lhs: Operand, lhs: Operand,
op: Op, op: Op,
rhs: Operand, rhs: Operand,
@ -114,16 +101,15 @@ impl<'tcx> FunctionCoverage<'tcx> {
"add_counter_expression({:?}, lhs={:?}, op={:?}, rhs={:?} at {:?}", "add_counter_expression({:?}, lhs={:?}, op={:?}, rhs={:?} at {:?}",
expression_id, lhs, op, rhs, region expression_id, lhs, op, rhs, region
); );
let expression_index = self.expression_index(expression_id);
debug_assert!( debug_assert!(
expression_index.as_usize() < self.expressions.len(), expression_id.as_usize() < self.expressions.len(),
"expression_index {} is out of range for expressions.len() = {} "expression_id {} is out of range for expressions.len() = {}
for {:?}", for {:?}",
expression_index.as_usize(), expression_id.as_usize(),
self.expressions.len(), self.expressions.len(),
self, self,
); );
if let Some(previous_expression) = self.expressions[expression_index].replace(Expression { if let Some(previous_expression) = self.expressions[expression_id].replace(Expression {
lhs, lhs,
op, op,
rhs, rhs,
@ -190,7 +176,7 @@ impl<'tcx> FunctionCoverage<'tcx> {
// //
// Expressions will be returned from this function in a sequential vector (array) of // Expressions will be returned from this function in a sequential vector (array) of
// `CounterExpression`, so the expression IDs must be mapped from their original, // `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 // 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). // 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 // `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` // reasonable to look up the new index of an expression operand while the `new_indexes`
// vector is only complete up to the current `ExpressionIndex`. // vector is only complete up to the current `ExpressionIndex`.
type NewIndexes = IndexSlice<InjectedExpressionIndex, Option<MappedExpressionIndex>>; type NewIndexes = IndexSlice<ExpressionId, Option<MappedExpressionIndex>>;
let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand { let id_to_counter = |new_indexes: &NewIndexes, operand: Operand| match operand {
Operand::Zero => Some(Counter::zero()), Operand::Zero => Some(Counter::zero()),
Operand::Counter(id) => { Operand::Counter(id) => {
@ -219,15 +205,14 @@ impl<'tcx> FunctionCoverage<'tcx> {
Some(Counter::counter_value_reference(index)) Some(Counter::counter_value_reference(index))
} }
Operand::Expression(id) => { Operand::Expression(id) => {
let index = self.expression_index(id);
self.expressions self.expressions
.get(index) .get(id)
.expect("expression id is out of range") .expect("expression id is out of range")
.as_ref() .as_ref()
// If an expression was optimized out, assume it would have produced a count // If an expression was optimized out, assume it would have produced a count
// of zero. This ensures that expressions dependent on optimized-out // of zero. This ensures that expressions dependent on optimized-out
// expressions are still valid. // 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<Item = (Counter, &CodeRegion)> { fn unreachable_regions(&self) -> impl Iterator<Item = (Counter, &CodeRegion)> {
self.unreachable_regions.iter().map(|region| (Counter::zero(), region)) 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)
}
} }

4
compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
View file

@ -17,7 +17,7 @@ use rustc_hir::def_id::DefId;
use rustc_llvm::RustString; use rustc_llvm::RustString;
use rustc_middle::bug; use rustc_middle::bug;
use rustc_middle::mir::coverage::{ 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::mir::Coverage;
use rustc_middle::ty; use rustc_middle::ty;
@ -202,7 +202,7 @@ impl<'tcx> Builder<'_, '_, 'tcx> {
fn add_coverage_counter_expression( fn add_coverage_counter_expression(
&mut self, &mut self,
instance: Instance<'tcx>, instance: Instance<'tcx>,
id: InjectedExpressionId, id: ExpressionId,
lhs: Operand, lhs: Operand,
op: Op, op: Op,
rhs: Operand, rhs: Operand,

26
compiler/rustc_middle/src/mir/coverage.rs
View file

@ -26,19 +26,23 @@ impl CounterValueReference {
} }
rustc_index::newtype_index! { 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)] #[derive(HashStable)]
#[max = 0xFFFF_FFFF] #[max = 0xFFFF_FFFF]
#[debug_format = "InjectedExpressionId({})"] #[debug_format = "ExpressionId({})"]
pub struct InjectedExpressionId {} pub struct ExpressionId {}
} }
rustc_index::newtype_index! { impl ExpressionId {
/// Values ascend from 0. pub const START: Self = Self::from_u32(0);
#[derive(HashStable)]
#[max = 0xFFFF_FFFF] #[inline(always)]
#[debug_format = "InjectedExpressionIndex({})"] pub fn next_id(self) -> Self {
pub struct InjectedExpressionIndex {} Self::from_u32(self.as_u32() + 1)
}
} }
rustc_index::newtype_index! { rustc_index::newtype_index! {
@ -60,7 +64,7 @@ rustc_index::newtype_index! {
pub enum Operand { pub enum Operand {
Zero, Zero,
Counter(CounterValueReference), Counter(CounterValueReference),
Expression(InjectedExpressionId), Expression(ExpressionId),
} }
impl Debug for Operand { impl Debug for Operand {
@ -84,7 +88,7 @@ pub enum CoverageKind {
Expression { Expression {
/// ID of this coverage-counter expression within its enclosing function. /// ID of this coverage-counter expression within its enclosing function.
/// Other expressions in the same function can refer to it as an operand. /// Other expressions in the same function can refer to it as an operand.
id: InjectedExpressionId, id: ExpressionId,
lhs: Operand, lhs: Operand,
op: Op, op: Op,
rhs: Operand, rhs: Operand,

3
compiler/rustc_middle/src/ty/structural_impls.rs
View file

@ -471,8 +471,7 @@ TrivialTypeTraversalAndLiftImpls! {
::rustc_target::asm::InlineAsmRegOrRegClass, ::rustc_target::asm::InlineAsmRegOrRegClass,
::rustc_target::spec::abi::Abi, ::rustc_target::spec::abi::Abi,
crate::mir::coverage::CounterValueReference, crate::mir::coverage::CounterValueReference,
crate::mir::coverage::InjectedExpressionId, crate::mir::coverage::ExpressionId,
crate::mir::coverage::InjectedExpressionIndex,
crate::mir::coverage::MappedExpressionIndex, crate::mir::coverage::MappedExpressionIndex,
crate::mir::Local, crate::mir::Local,
crate::mir::Promoted, crate::mir::Promoted,

19
compiler/rustc_mir_transform/src/coverage/counters.rs
View file

@ -17,7 +17,7 @@ use rustc_middle::mir::coverage::*;
pub(super) struct CoverageCounters { pub(super) struct CoverageCounters {
function_source_hash: u64, function_source_hash: u64,
next_counter_id: u32, next_counter_id: u32,
num_expressions: u32, next_expression_id: ExpressionId,
pub debug_counters: DebugCounters, pub debug_counters: DebugCounters,
} }
@ -26,7 +26,7 @@ impl CoverageCounters {
Self { Self {
function_source_hash, function_source_hash,
next_counter_id: CounterValueReference::START.as_u32(), next_counter_id: CounterValueReference::START.as_u32(),
num_expressions: 0, next_expression_id: ExpressionId::START,
debug_counters: DebugCounters::new(), debug_counters: DebugCounters::new(),
} }
} }
@ -94,20 +94,17 @@ impl CoverageCounters {
/// Counter IDs start from one and go up. /// Counter IDs start from one and go up.
fn next_counter(&mut self) -> CounterValueReference { fn next_counter(&mut self) -> CounterValueReference {
assert!(self.next_counter_id < u32::MAX - self.num_expressions);
let next = self.next_counter_id; let next = self.next_counter_id;
self.next_counter_id += 1; self.next_counter_id += 1;
CounterValueReference::from(next) CounterValueReference::from(next)
} }
/// Expression IDs start from u32::MAX and go down because an Expression can reference /// Expression IDs start from 0 and go up.
/// (add or subtract counts) of both Counter regions and Expression regions. The counter /// (Counter IDs and Expression IDs are distinguished by the `Operand` enum.)
/// expression operand IDs must be unique across both types. fn next_expression(&mut self) -> ExpressionId {
fn next_expression(&mut self) -> InjectedExpressionId { let next = self.next_expression_id;
assert!(self.next_counter_id < u32::MAX - self.num_expressions); self.next_expression_id = next.next_id();
let next = u32::MAX - self.num_expressions; next
self.num_expressions += 1;
InjectedExpressionId::from(next)
} }
} }

2
compiler/rustc_mir_transform/src/coverage/debug.rs
View file

@ -485,7 +485,7 @@ impl GraphvizData {
/// _not_ used are retained in the `unused_expressions` Vec, to be included in debug output (logs /// _not_ used are retained in the `unused_expressions` Vec, to be included in debug output (logs
/// and/or a `CoverageGraph` graphviz output). /// and/or a `CoverageGraph` graphviz output).
pub(super) struct UsedExpressions { pub(super) struct UsedExpressions {
some_used_expression_operands: Option<FxHashMap<Operand, Vec<InjectedExpressionId>>>, some_used_expression_operands: Option<FxHashMap<Operand, Vec<ExpressionId>>>,
some_unused_expressions: some_unused_expressions:
Option<Vec<(CoverageKind, Option<BasicCoverageBlock>, BasicCoverageBlock)>>, Option<Vec<(CoverageKind, Option<BasicCoverageBlock>, BasicCoverageBlock)>>,
} }

9
compiler/rustc_mir_transform/src/coverage/query.rs
View file

@ -52,12 +52,11 @@ impl CoverageVisitor {
self.info.num_counters = std::cmp::max(self.info.num_counters, counter_id + 1); 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 /// Updates `num_expressions` to the maximum encountered expression ID plus 1.
/// maximum encountered index plus 1.
#[inline(always)] #[inline(always)]
fn update_num_expressions(&mut self, expression_id: InjectedExpressionId) { fn update_num_expressions(&mut self, expression_id: ExpressionId) {
let expression_index = u32::MAX - expression_id.as_u32(); let expression_id = expression_id.as_u32();
self.info.num_expressions = std::cmp::max(self.info.num_expressions, expression_index + 1); self.info.num_expressions = std::cmp::max(self.info.num_expressions, expression_id + 1);
} }
fn update_from_expression_operand(&mut self, operand: Operand) { fn update_from_expression_operand(&mut self, operand: Operand) {

2
tests/mir-opt/coverage_graphviz.main.InstrumentCoverage.0.dot
View file

@ -3,7 +3,7 @@ digraph Cov_0_3 {
node [fontname="Courier, monospace"]; node [fontname="Courier, monospace"];
edge [fontname="Courier, monospace"]; edge [fontname="Courier, monospace"];
bcb3__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb3</td></tr><tr><td align="left" balign="left">Counter(bcb3) at 13:10-13:10<br align="left"/> 13:10-13:10: @5[0]: Coverage::Counter(2) for $DIR/coverage_graphviz.rs:13:10 - 13:11</td></tr><tr><td align="left" balign="left">bb5: Goto</td></tr></table>>]; bcb3__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb3</td></tr><tr><td align="left" balign="left">Counter(bcb3) at 13:10-13:10<br align="left"/> 13:10-13:10: @5[0]: Coverage::Counter(2) for $DIR/coverage_graphviz.rs:13:10 - 13:11</td></tr><tr><td align="left" balign="left">bb5: Goto</td></tr></table>>];
bcb2__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb2</td></tr><tr><td align="left" balign="left">Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18<br align="left"/> 12:13-12:18: @4[0]: Coverage::Expression(4294967293) = Expression(4294967294) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2<br align="left"/>Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2<br align="left"/> 15:2-15:2: @4.Return: return</td></tr><tr><td align="left" balign="left">bb4: Return</td></tr></table>>]; bcb2__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb2</td></tr><tr><td align="left" balign="left">Expression(bcb1:(bcb0 + bcb3) - bcb3) at 12:13-12:18<br align="left"/> 12:13-12:18: @4[0]: Coverage::Expression(2) = Expression(1) + Zero for $DIR/coverage_graphviz.rs:15:1 - 15:2<br align="left"/>Expression(bcb2:(bcb1:(bcb0 + bcb3) - bcb3) + 0) at 15:2-15:2<br align="left"/> 15:2-15:2: @4.Return: return</td></tr><tr><td align="left" balign="left">bb4: Return</td></tr></table>>];
bcb1__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb1</td></tr><tr><td align="left" balign="left">Expression(bcb0 + bcb3) at 10:5-11:17<br align="left"/> 11:12-11:17: @2.Call: _2 = bar() -&gt; [return: bb3, unwind: bb6]</td></tr><tr><td align="left" balign="left">bb1: FalseUnwind<br align="left"/>bb2: Call</td></tr><tr><td align="left" balign="left">bb3: SwitchInt</td></tr></table>>]; bcb1__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb1</td></tr><tr><td align="left" balign="left">Expression(bcb0 + bcb3) at 10:5-11:17<br align="left"/> 11:12-11:17: @2.Call: _2 = bar() -&gt; [return: bb3, unwind: bb6]</td></tr><tr><td align="left" balign="left">bb1: FalseUnwind<br align="left"/>bb2: Call</td></tr><tr><td align="left" balign="left">bb3: SwitchInt</td></tr></table>>];
bcb0__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb0</td></tr><tr><td align="left" balign="left"></td></tr><tr><td align="left" balign="left">Counter(bcb0) at 9:1-9:11<br align="left"/> </td></tr><tr><td align="left" balign="left">bb0: Goto</td></tr></table>>]; bcb0__Cov_0_3 [shape="none", label=<<table border="0" cellborder="1" cellspacing="0"><tr><td bgcolor="gray" align="center" colspan="1">bcb0</td></tr><tr><td align="left" balign="left"></td></tr><tr><td align="left" balign="left">Counter(bcb0) at 9:1-9:11<br align="left"/> </td></tr><tr><td align="left" balign="left">bb0: Goto</td></tr></table>>];
bcb3__Cov_0_3 -> bcb1__Cov_0_3 [label=<>]; bcb3__Cov_0_3 -> bcb1__Cov_0_3 [label=<>];

6
tests/mir-opt/instrument_coverage.main.InstrumentCoverage.diff
View file

@ -13,7 +13,7 @@
} }
bb1: { 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]; falseUnwind -> [real: bb2, unwind: bb6];
} }
@ -27,8 +27,8 @@
} }
bb4: { bb4: {
+ Coverage::Expression(4294967293) = Expression(4294967294) + Zero for /the/src/instrument_coverage.rs:17:1 - 17:2; + Coverage::Expression(2) = Expression(1) + 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(1) = Expression(0) - Counter(2) for /the/src/instrument_coverage.rs:14:13 - 14:18;
_0 = const (); _0 = const ();
StorageDead(_2); StorageDead(_2);
return; return;