Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coverage: Allow each coverage statement to have multiple code regions #115301

Merged
merged 7 commits into from
Oct 3, 2023
93 changes: 58 additions & 35 deletions compiler/rustc_codegen_llvm/src/coverageinfo/map_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub struct Expression {
lhs: Operand,
op: Op,
rhs: Operand,
region: Option<CodeRegion>,
code_regions: Vec<CodeRegion>,
}

/// Collects all of the coverage regions associated with (a) injected counters, (b) counter
Expand All @@ -30,7 +30,7 @@ pub struct FunctionCoverage<'tcx> {
instance: Instance<'tcx>,
source_hash: u64,
is_used: bool,
counters: IndexVec<CounterId, Option<CodeRegion>>,
counters: IndexVec<CounterId, Option<Vec<CodeRegion>>>,
expressions: IndexVec<ExpressionId, Option<Expression>>,
unreachable_regions: Vec<CodeRegion>,
}
Expand Down Expand Up @@ -77,28 +77,40 @@ impl<'tcx> FunctionCoverage<'tcx> {
}
}

/// Adds a code region to be counted by an injected counter intrinsic.
pub fn add_counter(&mut self, id: CounterId, region: CodeRegion) {
if let Some(previous_region) = self.counters[id].replace(region.clone()) {
assert_eq!(previous_region, region, "add_counter: code region for id changed");
/// 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"
),
}
}

/// 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 `Operand`, so there is no ambiguity
/// between operands that are counter IDs and operands that are expression IDs.
pub fn add_counter_expression(
#[instrument(level = "debug", skip(self))]
pub(crate) fn add_counter_expression(
&mut self,
expression_id: ExpressionId,
lhs: Operand,
op: Op,
rhs: Operand,
region: Option<CodeRegion>,
code_regions: &[CodeRegion],
) {
debug!(
"add_counter_expression({:?}, lhs={:?}, op={:?}, rhs={:?} at {:?}",
expression_id, lhs, op, rhs, region
);
debug_assert!(
expression_id.as_usize() < self.expressions.len(),
"expression_id {} is out of range for expressions.len() = {}
Expand All @@ -107,23 +119,25 @@ impl<'tcx> FunctionCoverage<'tcx> {
self.expressions.len(),
self,
);
if let Some(previous_expression) = self.expressions[expression_id].replace(Expression {
lhs,
op,
rhs,
region: region.clone(),
}) {
assert_eq!(
previous_expression,
Expression { lhs, op, rhs, region },

let expression = Expression { lhs, op, rhs, code_regions: code_regions.to_owned() };
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"
);
),
}
}

/// Add a region that will be marked as "unreachable", with a constant "zero counter".
pub fn add_unreachable_region(&mut self, region: CodeRegion) {
self.unreachable_regions.push(region)
/// 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.unreachable_regions.extend_from_slice(code_regions);
}

/// Perform some simplifications to make the final coverage mappings
Expand Down Expand Up @@ -212,11 +226,16 @@ impl<'tcx> FunctionCoverage<'tcx> {
}

fn counter_regions(&self) -> impl Iterator<Item = (Counter, &CodeRegion)> {
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), region))
})
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
Expand Down Expand Up @@ -254,13 +273,17 @@ impl<'tcx> FunctionCoverage<'tcx> {

fn expression_regions(&self) -> Vec<(Counter, &CodeRegion)> {
// Find all of the expression IDs that weren't optimized out AND have
// an attached code region, and return the corresponding mapping as a
// counter/region pair.
// one or more attached code regions, and return the corresponding
// mappings as counter/region pairs.
self.expressions
.iter_enumerated()
.filter_map(|(id, expression)| {
let code_region = expression.as_ref()?.region.as_ref()?;
Some((Counter::expression(id), code_region))
.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::<Vec<_>>()
}
Expand Down
45 changes: 9 additions & 36 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,7 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
/// `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. The first `CodeRegion` is used to add a single counter, with the
/// same counter ID used in the injected `instrprof.increment` intrinsic
/// call. Since the function is never called, all other `CodeRegion`s can be
/// 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) {
let instance = declare_unused_fn(self, def_id);
Expand All @@ -110,25 +108,15 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
.entry(instance)
.or_insert_with(|| FunctionCoverage::new(bx.tcx(), instance));

let Coverage { kind, code_region } = coverage.clone();
match kind {
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);

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.
debug!(
"adding counter to coverage_map: instance={:?}, id={:?}, region={:?}",
instance, id, code_region,
);
Comment on lines -126 to -129
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These debug! invocations have been replaced with #[instrument(...)] tracing on the methods being called.

func_coverage.add_counter(id, code_region);
}
func_coverage.add_counter(id, code_regions);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now call add_counter unconditionally here, and leave the callee to decide how to handle counters with zero code regions.

// We need to explicitly drop the `RefMut` before calling into `instrprof_increment`,
// as that needs an exclusive borrow.
drop(coverage_map);
Expand All @@ -146,20 +134,10 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
bx.instrprof_increment(fn_name, hash, num_counters, index);
}
CoverageKind::Expression { id, lhs, op, rhs } => {
debug!(
"adding counter expression to coverage_map: instance={:?}, id={:?}, {:?} {:?} {:?}; region: {:?}",
instance, id, lhs, op, rhs, code_region,
);
func_coverage.add_counter_expression(id, lhs, op, rhs, code_region);
func_coverage.add_counter_expression(id, lhs, op, rhs, code_regions);
}
CoverageKind::Unreachable => {
let code_region =
code_region.expect("unreachable regions always have code regions");
debug!(
"adding unreachable code to coverage_map: instance={:?}, at {:?}",
instance, code_region,
);
func_coverage.add_unreachable_region(code_region);
func_coverage.add_unreachable_regions(code_regions);
}
}
}
Expand Down Expand Up @@ -227,14 +205,9 @@ fn add_unused_function_coverage<'tcx>(
let tcx = cx.tcx;

let mut function_coverage = FunctionCoverage::unused(tcx, instance);
for (index, &code_region) in tcx.covered_code_regions(def_id).iter().enumerate() {
if index == 0 {
// Insert at least one real counter so the LLVM CoverageMappingReader will find expected
// definitions.
function_coverage.add_counter(UNUSED_FUNCTION_COUNTER_ID, code_region.clone());
} else {
function_coverage.add_unreachable_region(code_region.clone());
}
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);
}

if let Some(coverage_context) = cx.coverage_context() {
Expand Down
10 changes: 0 additions & 10 deletions compiler/rustc_middle/src/mir/coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,6 @@ rustc_index::newtype_index! {

impl CounterId {
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! {
Expand All @@ -38,11 +33,6 @@ rustc_index::newtype_index! {

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)
}
}

/// Operand of a coverage-counter expression.
Expand Down
11 changes: 7 additions & 4 deletions compiler/rustc_middle/src/mir/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use rustc_middle::mir::interpret::{
Pointer, Provenance,
};
use rustc_middle::mir::visit::Visitor;
use rustc_middle::mir::*;
use rustc_middle::mir::{self, *};
use rustc_middle::ty::{self, TyCtxt};
use rustc_target::abi::Size;

Expand Down Expand Up @@ -685,10 +685,13 @@ impl Debug for Statement<'_> {
AscribeUserType(box (ref place, ref c_ty), ref variance) => {
write!(fmt, "AscribeUserType({place:?}, {variance:?}, {c_ty:?})")
}
Coverage(box self::Coverage { ref kind, code_region: Some(ref rgn) }) => {
write!(fmt, "Coverage::{kind:?} for {rgn:?}")
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 ref coverage) => write!(fmt, "Coverage::{:?}", coverage.kind),
Intrinsic(box ref intrinsic) => write!(fmt, "{intrinsic}"),
ConstEvalCounter => write!(fmt, "ConstEvalCounter"),
Nop => write!(fmt, "nop"),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/mir/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ pub enum FakeReadCause {
#[derive(TypeFoldable, TypeVisitable)]
pub struct Coverage {
pub kind: CoverageKind,
pub code_region: Option<CodeRegion>,
pub code_regions: Vec<CodeRegion>,
}

#[derive(Clone, Debug, PartialEq, TyEncodable, TyDecodable, Hash, HashStable)]
Expand Down
37 changes: 14 additions & 23 deletions compiler/rustc_mir_transform/src/coverage/counters.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
use super::Error;

use super::graph;
use super::spans;

use graph::{BasicCoverageBlock, BcbBranch, CoverageGraph, TraverseCoverageGraphWithLoops};
use spans::CoverageSpan;

use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::graph::WithNumNodes;
Expand Down Expand Up @@ -93,14 +91,14 @@ impl CoverageCounters {
}

/// Makes [`BcbCounter`] `Counter`s and `Expressions` for the `BasicCoverageBlock`s directly or
/// indirectly associated with `CoverageSpans`, and accumulates additional `Expression`s
/// indirectly associated with coverage spans, and accumulates additional `Expression`s
/// representing intermediate values.
pub fn make_bcb_counters(
&mut self,
basic_coverage_blocks: &CoverageGraph,
coverage_spans: &[CoverageSpan],
bcb_has_coverage_spans: impl Fn(BasicCoverageBlock) -> bool,
) -> Result<(), Error> {
MakeBcbCounters::new(self, basic_coverage_blocks).make_bcb_counters(coverage_spans)
MakeBcbCounters::new(self, basic_coverage_blocks).make_bcb_counters(bcb_has_coverage_spans)
}

fn make_counter(&mut self) -> BcbCounter {
Expand All @@ -113,22 +111,18 @@ impl CoverageCounters {
BcbCounter::Expression { id, lhs, op, rhs }
}

pub fn make_identity_counter(&mut self, counter_operand: Operand) -> BcbCounter {
self.make_expression(counter_operand, Op::Add, Operand::Zero)
}

/// Counter IDs start from one and go up.
fn next_counter(&mut self) -> CounterId {
let next = self.next_counter_id;
self.next_counter_id = next.next_id();
self.next_counter_id = self.next_counter_id + 1;
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();
self.next_expression_id = self.next_expression_id + 1;
next
}

Expand Down Expand Up @@ -208,7 +202,7 @@ impl CoverageCounters {
}

/// Traverse the `CoverageGraph` and add either a `Counter` or `Expression` to every BCB, to be
/// injected with `CoverageSpan`s. `Expressions` have no runtime overhead, so if a viable expression
/// injected with coverage spans. `Expressions` have no runtime overhead, so if a viable expression
/// (adding or subtracting two other counters or expressions) can compute the same result as an
/// embedded counter, an `Expression` should be used.
struct MakeBcbCounters<'a> {
Expand All @@ -234,17 +228,14 @@ impl<'a> MakeBcbCounters<'a> {
/// Returns any non-code-span expressions created to represent intermediate values (such as to
/// add two counters so the result can be subtracted from another counter), or an Error with
/// message for subsequent debugging.
fn make_bcb_counters(&mut self, coverage_spans: &[CoverageSpan]) -> Result<(), Error> {
fn make_bcb_counters(
&mut self,
bcb_has_coverage_spans: impl Fn(BasicCoverageBlock) -> bool,
) -> Result<(), Error> {
debug!("make_bcb_counters(): adding a counter or expression to each BasicCoverageBlock");
let num_bcbs = self.basic_coverage_blocks.num_nodes();

let mut bcbs_with_coverage = BitSet::new_empty(num_bcbs);
for covspan in coverage_spans {
bcbs_with_coverage.insert(covspan.bcb);
}

// Walk the `CoverageGraph`. For each `BasicCoverageBlock` node with an associated
// `CoverageSpan`, add a counter. If the `BasicCoverageBlock` branches, add a counter or
// coverage span, add a counter. If the `BasicCoverageBlock` branches, add a counter or
// expression to each branch `BasicCoverageBlock` (if the branch BCB has only one incoming
// edge) or edge from the branching BCB to the branch BCB (if the branch BCB has multiple
// incoming edges).
Expand All @@ -255,16 +246,16 @@ impl<'a> MakeBcbCounters<'a> {
// the current BCB is in one or more nested loops or not.
let mut traversal = TraverseCoverageGraphWithLoops::new(&self.basic_coverage_blocks);
while let Some(bcb) = traversal.next(self.basic_coverage_blocks) {
if bcbs_with_coverage.contains(bcb) {
debug!("{:?} has at least one `CoverageSpan`. Get or make its counter", bcb);
if bcb_has_coverage_spans(bcb) {
debug!("{:?} has at least one coverage span. Get or make its counter", bcb);
let branching_counter_operand = self.get_or_make_counter_operand(bcb)?;

if self.bcb_needs_branch_counters(bcb) {
self.make_branch_counters(&mut traversal, bcb, branching_counter_operand)?;
}
} else {
debug!(
"{:?} does not have any `CoverageSpan`s. A counter will only be added if \
"{:?} does not have any coverage spans. A counter will only be added if \
and when a covered BCB has an expression dependency.",
bcb,
);
Expand Down
Loading
Loading