bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Rollup merge of #122322 - Zalathar:branch, r=oli-obk

Browse source 
coverage: Initial support for branch coverage instrumentation

(This is a review-ready version of the changes that were drafted in #118305.)

This PR adds support for branch coverage instrumentation, gated behind the unstable flag value `-Zcoverage-options=branch`. (Coverage instrumentation must also be enabled with `-Cinstrument-coverage`.)

During THIR-to-MIR lowering (MIR building), if branch coverage is enabled, we collect additional information about branch conditions and their corresponding then/else blocks. We inject special marker statements into those blocks, so that the `InstrumentCoverage` MIR pass can reliably identify them even after the initially-built MIR has been simplified and renumbered.

The rest of the changes are mostly just plumbing needed to gather up the information that was collected during MIR building, and include it in the coverage metadata that we embed in the final binary.

Note that `llvm-cov show` doesn't print branch coverage information in its source views by default; that needs to be explicitly enabled with `--show-branches=count` or similar.

---

The current implementation doesn't have any support for instrumenting `if let` or let-chains. I think it's still useful without that, and adding it would be non-trivial, so I'm happy to leave that for future work.
This commit is contained in:
Matthias Krüger 2024-03-14 20:00:19 +01:00 • committed by GitHub
commit 54a5a49af0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
31 changed files with 1250 additions and 59 deletions
Download patch file Download diff file Expand all files Collapse all files

148
compiler/rustc_mir_build/src/build/coverageinfo.rs Normal file
View file

@ -0,0 +1,148 @@
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::{self, BasicBlock, UnOp};
use rustc_middle::thir::{ExprId, ExprKind, Thir};
use rustc_middle::ty::TyCtxt;
use rustc_span::def_id::LocalDefId;
use crate::build::Builder;
pub(crate) struct BranchInfoBuilder {
/// Maps condition expressions to their enclosing `!`, for better instrumentation.
nots: FxHashMap<ExprId, NotInfo>,
num_block_markers: usize,
branch_spans: Vec<BranchSpan>,
}
#[derive(Clone, Copy)]
struct NotInfo {
/// When visiting the associated expression as a branch condition, treat this
/// enclosing `!` as the branch condition instead.
enclosing_not: ExprId,
/// True if the associated expression is nested within an odd number of `!`
/// expressions relative to `enclosing_not` (inclusive of `enclosing_not`).
is_flipped: bool,
}
impl BranchInfoBuilder {
/// Creates a new branch info builder, but only if branch 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(), num_block_markers: 0, branch_spans: vec![] })
} else {
None
}
}
/// Unary `!` expressions inside an `if` condition are lowered by lowering
/// their argument instead, and then reversing the then/else arms of that `if`.
///
/// That's awkward for branch coverage instrumentation, so to work around that
/// we pre-emptively visit any affected `!` expressions, and record extra
/// information that [`Builder::visit_coverage_branch_condition`] can use to
/// synthesize branch instrumentation for the enclosing `!`.
pub(crate) fn visit_unary_not(&mut self, thir: &Thir<'_>, unary_not: ExprId) {
assert_matches!(thir[unary_not].kind, ExprKind::Unary { op: UnOp::Not, .. });
self.visit_with_not_info(
thir,
unary_not,
// Set `is_flipped: false` for the `!` itself, so that its enclosed
// expression will have `is_flipped: true`.
NotInfo { enclosing_not: unary_not, is_flipped: false },
);
}
fn visit_with_not_info(&mut self, thir: &Thir<'_>, expr_id: ExprId, not_info: NotInfo) {
match self.nots.entry(expr_id) {
// This expression has already been marked by an enclosing `!`.
Entry::Occupied(_) => return,
Entry::Vacant(entry) => entry.insert(not_info),
};
match thir[expr_id].kind {
ExprKind::Unary { op: UnOp::Not, arg } => {
// Invert the `is_flipped` flag for the contents of this `!`.
let not_info = NotInfo { is_flipped: !not_info.is_flipped, ..not_info };
self.visit_with_not_info(thir, arg, not_info);
}
ExprKind::Scope { value, .. } => self.visit_with_not_info(thir, value, not_info),
ExprKind::Use { source } => self.visit_with_not_info(thir, source, not_info),
// All other expressions (including `&&` and `||`) don't need any
// special handling of their contents, so stop visiting.
_ => {}
}
}
fn next_block_marker_id(&mut self) -> BlockMarkerId {
let id = BlockMarkerId::from_usize(self.num_block_markers);
self.num_block_markers += 1;
id
}
pub(crate) fn into_done(self) -> Option<Box<mir::coverage::BranchInfo>> {
let Self { nots: _, num_block_markers, branch_spans } = self;
if num_block_markers == 0 {
assert!(branch_spans.is_empty());
return None;
}
Some(Box::new(mir::coverage::BranchInfo { num_block_markers, branch_spans }))
}
}
impl Builder<'_, '_> {
/// If branch coverage is enabled, inject marker statements into `then_block`
/// and `else_block`, and record their IDs in the table of branch spans.
pub(crate) fn visit_coverage_branch_condition(
&mut self,
mut expr_id: ExprId,
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_ref() 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) {
expr_id = enclosing_not;
if is_flipped {
std::mem::swap(&mut then_block, &mut else_block);
}
}
let source_info = self.source_info(self.thir[expr_id].span);
// Now that we have `source_info`, we can upgrade to a &mut reference.
let branch_info = self.coverage_branch_info.as_mut().expect("upgrading & to &mut");
let mut inject_branch_marker = |block: BasicBlock| {
let id = branch_info.next_block_marker_id();
let marker_statement = mir::Statement {
source_info,
kind: mir::StatementKind::Coverage(Box::new(mir::Coverage {
kind: CoverageKind::BlockMarker { id },
})),
};
self.cfg.push(block, marker_statement);
id
};
let true_marker = inject_branch_marker(then_block);
let false_marker = inject_branch_marker(else_block);
branch_info.branch_spans.push(BranchSpan {
span: source_info.span,
true_marker,
false_marker,
});
}
}

1
compiler/rustc_mir_build/src/build/custom/mod.rs
View file

@ -60,6 +60,7 @@ pub(super) fn build_custom_mir<'tcx>(
tainted_by_errors: None,
injection_phase: None,
pass_count: 0,
coverage_branch_info: None,
function_coverage_info: None,
};

11
compiler/rustc_mir_build/src/build/matches/mod.rs
View file

@ -105,6 +105,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
success_block.unit()
}
ExprKind::Unary { op: UnOp::Not, arg } => {
// 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);
}
let local_scope = this.local_scope();
let (success_block, failure_block) =
this.in_if_then_scope(local_scope, expr_span, |this| {
@ -149,6 +156,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let else_block = this.cfg.start_new_block();
let term = TerminatorKind::if_(operand, then_block, else_block);
// Record branch coverage info for this condition.
// (Does nothing if branch coverage is not enabled.)
this.visit_coverage_branch_condition(expr_id, then_block, else_block);
let source_info = this.source_info(expr_span);
this.cfg.terminate(block, source_info, term);
this.break_for_else(else_block, source_info);

12
compiler/rustc_mir_build/src/build/mod.rs
View file

@ -234,6 +234,10 @@ struct Builder<'a, 'tcx> {
// the root (most of them do) and saves us from retracing many sub-paths
// many times, and rechecking many nodes.
lint_level_roots_cache: GrowableBitSet<hir::ItemLocalId>,
/// Collects additional coverage information during MIR building.
/// Only present if branch coverage is enabled and this function is eligible.
coverage_branch_info: Option<coverageinfo::BranchInfoBuilder>,
}
type CaptureMap<'tcx> = SortedIndexMultiMap<usize, hir::HirId, Capture<'tcx>>;
@ -807,6 +811,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
unit_temp: None,
var_debug_info: vec![],
lint_level_roots_cache: GrowableBitSet::new_empty(),
coverage_branch_info: coverageinfo::BranchInfoBuilder::new_if_enabled(tcx, def),
};
assert_eq!(builder.cfg.start_new_block(), START_BLOCK);
@ -826,7 +831,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
}
Body::new(
let mut body = Body::new(
MirSource::item(self.def_id.to_def_id()),
self.cfg.basic_blocks,
self.source_scopes,
@ -837,7 +842,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
self.fn_span,
self.coroutine,
None,
)
);
body.coverage_branch_info = self.coverage_branch_info.and_then(|b| b.into_done());
body
}
fn insert_upvar_arg(&mut self) {
@ -1111,6 +1118,7 @@ pub(crate) fn parse_float_into_scalar(
mod block;
mod cfg;
mod coverageinfo;
mod custom;
mod expr;
mod matches;