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: Rename mir::coverage::BranchInfo to CoverageInfoHi #127352

Merged
merged 2 commits into from
Jul 5, 2024
Merged
Show file tree
Hide file tree
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
Expand Up @@ -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 },
Expand Down Expand Up @@ -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.
Expand Down
13 changes: 7 additions & 6 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
25 changes: 17 additions & 8 deletions compiler/rustc_middle/src/mir/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -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)?;
}

Expand Down
107 changes: 60 additions & 47 deletions compiler/rustc_mir_build/src/build/coverageinfo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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,
}))
})
}
}

Expand All @@ -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;
};
Expand Down Expand Up @@ -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,
Expand All @@ -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);
Expand All @@ -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,
Expand All @@ -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,
Expand Down
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
Expand Up @@ -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");
Expand Down
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
Expand Up @@ -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,
};

Expand Down
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
Expand Up @@ -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();
Expand Down
Loading
Loading