Skip to content

coverage: Rename mir::coverage::BranchInfo to CoverageInfoHi #127352

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

Merged
merged 2 commits into from
Jul 5, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions compiler/rustc_middle/src/mir/coverage.rs
Original file line number Diff line number Diff line change
@@ -103,7 +103,7 @@ pub enum CoverageKind {
SpanMarker,

/// Marks its enclosing basic block with an ID that can be referred to by
/// side data in [`BranchInfo`].
/// side data in [`CoverageInfoHi`].
///
/// Should be erased before codegen (at some point after `InstrumentCoverage`).
BlockMarker { id: BlockMarkerId },
@@ -274,10 +274,15 @@ pub struct FunctionCoverageInfo {
pub mcdc_num_condition_bitmaps: usize,
}

/// Branch information recorded during THIR-to-MIR lowering, and stored in MIR.
/// Coverage information for a function, recorded during MIR building and
/// attached to the corresponding `mir::Body`. Used by the `InstrumentCoverage`
/// MIR pass.
///
/// ("Hi" indicates that this is "high-level" information collected at the
/// THIR/MIR boundary, before the MIR-based coverage instrumentation pass.)
#[derive(Clone, Debug)]
#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
pub struct BranchInfo {
pub struct CoverageInfoHi {
/// 1 more than the highest-numbered [`CoverageKind::BlockMarker`] that was
/// injected into the MIR body. This makes it possible to allocate per-ID
/// data structures without having to scan the entire body first.
13 changes: 7 additions & 6 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
@@ -430,11 +430,12 @@ pub struct Body<'tcx> {

pub tainted_by_errors: Option<ErrorGuaranteed>,

/// Branch coverage information collected during MIR building, to be used by
/// the `InstrumentCoverage` pass.
/// Coverage information collected from THIR/MIR during MIR building,
/// to be used by the `InstrumentCoverage` pass.
///
/// Only present if branch coverage is enabled and this function is eligible.
pub coverage_branch_info: Option<Box<coverage::BranchInfo>>,
/// Only present if coverage is enabled and this function is eligible.
/// Boxed to limit space overhead in non-coverage builds.
pub coverage_info_hi: Option<Box<coverage::CoverageInfoHi>>,

/// Per-function coverage information added by the `InstrumentCoverage`
/// pass, to be used in conjunction with the coverage statements injected
@@ -484,7 +485,7 @@ impl<'tcx> Body<'tcx> {
is_polymorphic: false,
injection_phase: None,
tainted_by_errors,
coverage_branch_info: None,
coverage_info_hi: None,
function_coverage_info: None,
};
body.is_polymorphic = body.has_non_region_param();
@@ -515,7 +516,7 @@ impl<'tcx> Body<'tcx> {
is_polymorphic: false,
injection_phase: None,
tainted_by_errors: None,
coverage_branch_info: None,
coverage_info_hi: None,
function_coverage_info: None,
};
body.is_polymorphic = body.has_non_region_param();
25 changes: 17 additions & 8 deletions compiler/rustc_middle/src/mir/pretty.rs
Original file line number Diff line number Diff line change
@@ -473,8 +473,8 @@ pub fn write_mir_intro<'tcx>(
// Add an empty line before the first block is printed.
writeln!(w)?;

if let Some(branch_info) = &body.coverage_branch_info {
write_coverage_branch_info(branch_info, w)?;
if let Some(coverage_info_hi) = &body.coverage_info_hi {
write_coverage_info_hi(coverage_info_hi, w)?;
}
if let Some(function_coverage_info) = &body.function_coverage_info {
write_function_coverage_info(function_coverage_info, w)?;
@@ -483,18 +483,26 @@ pub fn write_mir_intro<'tcx>(
Ok(())
}

fn write_coverage_branch_info(
branch_info: &coverage::BranchInfo,
fn write_coverage_info_hi(
coverage_info_hi: &coverage::CoverageInfoHi,
w: &mut dyn io::Write,
) -> io::Result<()> {
let coverage::BranchInfo { branch_spans, mcdc_branch_spans, mcdc_decision_spans, .. } =
branch_info;
let coverage::CoverageInfoHi {
num_block_markers: _,
branch_spans,
mcdc_branch_spans,
mcdc_decision_spans,
} = coverage_info_hi;

// Only add an extra trailing newline if we printed at least one thing.
let mut did_print = false;

for coverage::BranchSpan { span, true_marker, false_marker } in branch_spans {
writeln!(
w,
"{INDENT}coverage branch {{ true: {true_marker:?}, false: {false_marker:?} }} => {span:?}",
)?;
did_print = true;
}

for coverage::MCDCBranchSpan {
@@ -510,6 +518,7 @@ fn write_coverage_branch_info(
"{INDENT}coverage mcdc branch {{ condition_id: {:?}, true: {true_marker:?}, false: {false_marker:?}, depth: {decision_depth:?} }} => {span:?}",
condition_info.map(|info| info.condition_id)
)?;
did_print = true;
}

for coverage::MCDCDecisionSpan { span, num_conditions, end_markers, decision_depth } in
@@ -519,10 +528,10 @@ fn write_coverage_branch_info(
w,
"{INDENT}coverage mcdc decision {{ num_conditions: {num_conditions:?}, end: {end_markers:?}, depth: {decision_depth:?} }} => {span:?}"
)?;
did_print = true;
}

if !branch_spans.is_empty() || !mcdc_branch_spans.is_empty() || !mcdc_decision_spans.is_empty()
{
if did_print {
writeln!(w)?;
}

107 changes: 60 additions & 47 deletions compiler/rustc_mir_build/src/build/coverageinfo.rs
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@ use std::assert_matches::assert_matches;
use std::collections::hash_map::Entry;

use rustc_data_structures::fx::FxHashMap;
use rustc_middle::mir::coverage::{BlockMarkerId, BranchSpan, CoverageKind};
use rustc_middle::mir::coverage::{BlockMarkerId, BranchSpan, CoverageInfoHi, CoverageKind};
use rustc_middle::mir::{self, BasicBlock, SourceInfo, UnOp};
use rustc_middle::thir::{ExprId, ExprKind, Pat, Thir};
use rustc_middle::ty::TyCtxt;
@@ -13,16 +13,25 @@ use crate::build::{Builder, CFG};

mod mcdc;

pub(crate) struct BranchInfoBuilder {
/// Collects coverage-related information during MIR building, to eventually be
/// turned into a function's [`CoverageInfoHi`] when MIR building is complete.
pub(crate) struct CoverageInfoBuilder {
/// Maps condition expressions to their enclosing `!`, for better instrumentation.
nots: FxHashMap<ExprId, NotInfo>,

markers: BlockMarkerGen,
branch_spans: Vec<BranchSpan>,

/// Present if branch coverage is enabled.
branch_info: Option<BranchInfo>,
/// Present if MC/DC coverage is enabled.
mcdc_info: Option<MCDCInfoBuilder>,
}

#[derive(Default)]
struct BranchInfo {
branch_spans: Vec<BranchSpan>,
}

#[derive(Clone, Copy)]
struct NotInfo {
/// When visiting the associated expression as a branch condition, treat this
@@ -62,20 +71,20 @@ impl BlockMarkerGen {
}
}

impl BranchInfoBuilder {
/// Creates a new branch info builder, but only if branch coverage instrumentation
impl CoverageInfoBuilder {
/// Creates a new coverage info builder, but only if coverage instrumentation
/// is enabled and `def_id` represents a function that is eligible for coverage.
pub(crate) fn new_if_enabled(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Option<Self> {
if tcx.sess.instrument_coverage_branch() && tcx.is_eligible_for_coverage(def_id) {
Some(Self {
nots: FxHashMap::default(),
markers: BlockMarkerGen::default(),
branch_spans: vec![],
mcdc_info: tcx.sess.instrument_coverage_mcdc().then(MCDCInfoBuilder::new),
})
} else {
None
if !tcx.sess.instrument_coverage() || !tcx.is_eligible_for_coverage(def_id) {
return None;
}

Some(Self {
nots: FxHashMap::default(),
markers: BlockMarkerGen::default(),
branch_info: tcx.sess.instrument_coverage_branch().then(BranchInfo::default),
mcdc_info: tcx.sess.instrument_coverage_mcdc().then(MCDCInfoBuilder::new),
})
}

/// Unary `!` expressions inside an `if` condition are lowered by lowering
@@ -88,6 +97,12 @@ impl BranchInfoBuilder {
pub(crate) fn visit_unary_not(&mut self, thir: &Thir<'_>, unary_not: ExprId) {
assert_matches!(thir[unary_not].kind, ExprKind::Unary { op: UnOp::Not, .. });

// The information collected by this visitor is only needed when branch
// coverage or higher is enabled.
if self.branch_info.is_none() {
return;
}

self.visit_with_not_info(
thir,
unary_not,
@@ -137,40 +152,40 @@ impl BranchInfoBuilder {
false_block,
inject_block_marker,
);
} else {
let true_marker = self.markers.inject_block_marker(cfg, source_info, true_block);
let false_marker = self.markers.inject_block_marker(cfg, source_info, false_block);

self.branch_spans.push(BranchSpan {
span: source_info.span,
true_marker,
false_marker,
});
return;
}

// Bail out if branch coverage is not enabled.
let Some(branch_info) = self.branch_info.as_mut() else { return };

let true_marker = self.markers.inject_block_marker(cfg, source_info, true_block);
let false_marker = self.markers.inject_block_marker(cfg, source_info, false_block);

branch_info.branch_spans.push(BranchSpan {
span: source_info.span,
true_marker,
false_marker,
});
}

pub(crate) fn into_done(self) -> Option<Box<mir::coverage::BranchInfo>> {
let Self {
nots: _,
markers: BlockMarkerGen { num_block_markers },
branch_spans,
mcdc_info,
} = self;
pub(crate) fn into_done(self) -> Box<CoverageInfoHi> {
let Self { nots: _, markers: BlockMarkerGen { num_block_markers }, branch_info, mcdc_info } =
self;

if num_block_markers == 0 {
assert!(branch_spans.is_empty());
return None;
}
let branch_spans =
branch_info.map(|branch_info| branch_info.branch_spans).unwrap_or_default();

let (mcdc_decision_spans, mcdc_branch_spans) =
mcdc_info.map(MCDCInfoBuilder::into_done).unwrap_or_default();

Some(Box::new(mir::coverage::BranchInfo {
// For simplicity, always return an info struct (without Option), even
// if there's nothing interesting in it.
Box::new(CoverageInfoHi {
num_block_markers,
branch_spans,
mcdc_branch_spans,
mcdc_decision_spans,
}))
})
}
}

@@ -184,7 +199,7 @@ impl<'tcx> Builder<'_, 'tcx> {
block: &mut BasicBlock,
) {
// Bail out if condition coverage is not enabled for this function.
let Some(branch_info) = self.coverage_branch_info.as_mut() else { return };
let Some(coverage_info) = self.coverage_info.as_mut() else { return };
if !self.tcx.sess.instrument_coverage_condition() {
return;
};
@@ -224,7 +239,7 @@ impl<'tcx> Builder<'_, 'tcx> {
);

// Separate path for handling branches when MC/DC is enabled.
branch_info.register_two_way_branch(
coverage_info.register_two_way_branch(
self.tcx,
&mut self.cfg,
source_info,
@@ -247,12 +262,12 @@ impl<'tcx> Builder<'_, 'tcx> {
mut then_block: BasicBlock,
mut else_block: BasicBlock,
) {
// Bail out if branch coverage is not enabled for this function.
let Some(branch_info) = self.coverage_branch_info.as_mut() else { return };
// Bail out if coverage is not enabled for this function.
let Some(coverage_info) = self.coverage_info.as_mut() else { return };

// If this condition expression is nested within one or more `!` expressions,
// replace it with the enclosing `!` collected by `visit_unary_not`.
if let Some(&NotInfo { enclosing_not, is_flipped }) = branch_info.nots.get(&expr_id) {
if let Some(&NotInfo { enclosing_not, is_flipped }) = coverage_info.nots.get(&expr_id) {
expr_id = enclosing_not;
if is_flipped {
std::mem::swap(&mut then_block, &mut else_block);
@@ -261,7 +276,7 @@ impl<'tcx> Builder<'_, 'tcx> {

let source_info = SourceInfo { span: self.thir[expr_id].span, scope: self.source_scope };

branch_info.register_two_way_branch(
coverage_info.register_two_way_branch(
self.tcx,
&mut self.cfg,
source_info,
@@ -280,13 +295,11 @@ impl<'tcx> Builder<'_, 'tcx> {
true_block: BasicBlock,
false_block: BasicBlock,
) {
// Bail out if branch coverage is not enabled for this function.
let Some(branch_info) = self.coverage_branch_info.as_mut() else { return };

// FIXME(#124144) This may need special handling when MC/DC is enabled.
// Bail out if coverage is not enabled for this function.
let Some(coverage_info) = self.coverage_info.as_mut() else { return };

let source_info = SourceInfo { span: pattern.span, scope: self.source_scope };
branch_info.register_two_way_branch(
coverage_info.register_two_way_branch(
self.tcx,
&mut self.cfg,
source_info,
12 changes: 6 additions & 6 deletions compiler/rustc_mir_build/src/build/coverageinfo/mcdc.rs
Original file line number Diff line number Diff line change
@@ -250,24 +250,24 @@ impl MCDCInfoBuilder {

impl Builder<'_, '_> {
pub(crate) fn visit_coverage_branch_operation(&mut self, logical_op: LogicalOp, span: Span) {
if let Some(branch_info) = self.coverage_branch_info.as_mut()
&& let Some(mcdc_info) = branch_info.mcdc_info.as_mut()
if let Some(coverage_info) = self.coverage_info.as_mut()
&& let Some(mcdc_info) = coverage_info.mcdc_info.as_mut()
{
mcdc_info.state.record_conditions(logical_op, span);
}
}

pub(crate) fn mcdc_increment_depth_if_enabled(&mut self) {
if let Some(branch_info) = self.coverage_branch_info.as_mut()
&& let Some(mcdc_info) = branch_info.mcdc_info.as_mut()
if let Some(coverage_info) = self.coverage_info.as_mut()
&& let Some(mcdc_info) = coverage_info.mcdc_info.as_mut()
{
mcdc_info.state.decision_ctx_stack.push(MCDCDecisionCtx::default());
};
}

pub(crate) fn mcdc_decrement_depth_if_enabled(&mut self) {
if let Some(branch_info) = self.coverage_branch_info.as_mut()
&& let Some(mcdc_info) = branch_info.mcdc_info.as_mut()
if let Some(coverage_info) = self.coverage_info.as_mut()
&& let Some(mcdc_info) = coverage_info.mcdc_info.as_mut()
{
if mcdc_info.state.decision_ctx_stack.pop().is_none() {
bug!("Unexpected empty decision stack");
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/build/custom/mod.rs
Original file line number Diff line number Diff line change
@@ -62,7 +62,7 @@ pub(super) fn build_custom_mir<'tcx>(
tainted_by_errors: None,
injection_phase: None,
pass_count: 0,
coverage_branch_info: None,
coverage_info_hi: None,
function_coverage_info: None,
};

4 changes: 2 additions & 2 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
Original file line number Diff line number Diff line change
@@ -160,8 +160,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// Improve branch coverage instrumentation by noting conditions
// nested within one or more `!` expressions.
// (Skipped if branch coverage is not enabled.)
if let Some(branch_info) = this.coverage_branch_info.as_mut() {
branch_info.visit_unary_not(this.thir, expr_id);
if let Some(coverage_info) = this.coverage_info.as_mut() {
coverage_info.visit_unary_not(this.thir, expr_id);
}

let local_scope = this.local_scope();
Loading