Skip to content

Commit 54a5a49

Browse files
authored
Rollup merge of #122322 - Zalathar:branch, r=oli-obk
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.
2 parents 722514f + 060c7ce commit 54a5a49

31 files changed

+1250
-59
lines changed

compiler/rustc_codegen_llvm/src/coverageinfo/ffi.rs

+9-3
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,15 @@ impl CounterMappingRegion {
164164
end_line,
165165
end_col,
166166
),
167+
MappingKind::Branch { true_term, false_term } => Self::branch_region(
168+
Counter::from_term(true_term),
169+
Counter::from_term(false_term),
170+
local_file_id,
171+
start_line,
172+
start_col,
173+
end_line,
174+
end_col,
175+
),
167176
}
168177
}
169178

@@ -188,9 +197,6 @@ impl CounterMappingRegion {
188197
}
189198
}
190199

191-
// This function might be used in the future; the LLVM API is still evolving, as is coverage
192-
// support.
193-
#[allow(dead_code)]
194200
pub(crate) fn branch_region(
195201
counter: Counter,
196202
false_counter: Counter,

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
8888
match coverage.kind {
8989
// Marker statements have no effect during codegen,
9090
// so return early and don't create `func_coverage`.
91-
CoverageKind::SpanMarker => return,
91+
CoverageKind::SpanMarker | CoverageKind::BlockMarker { .. } => return,
9292
// Match exhaustively to ensure that newly-added kinds are classified correctly.
9393
CoverageKind::CounterIncrement { .. } | CoverageKind::ExpressionUsed { .. } => {}
9494
}
@@ -108,7 +108,7 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
108108

109109
let Coverage { kind } = coverage;
110110
match *kind {
111-
CoverageKind::SpanMarker => unreachable!(
111+
CoverageKind::SpanMarker | CoverageKind::BlockMarker { .. } => unreachable!(
112112
"unexpected marker statement {kind:?} should have caused an early return"
113113
),
114114
CoverageKind::CounterIncrement { id } => {

compiler/rustc_middle/src/hooks/mod.rs

+7
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use crate::mir;
77
use crate::query::TyCtxtAt;
88
use crate::ty::{Ty, TyCtxt};
9+
use rustc_span::def_id::LocalDefId;
910
use rustc_span::DUMMY_SP;
1011

1112
macro_rules! declare_hooks {
@@ -70,4 +71,10 @@ declare_hooks! {
7071

7172
/// Getting a &core::panic::Location referring to a span.
7273
hook const_caller_location(file: rustc_span::Symbol, line: u32, col: u32) -> mir::ConstValue<'tcx>;
74+
75+
/// Returns `true` if this def is a function-like thing that is eligible for
76+
/// coverage instrumentation under `-Cinstrument-coverage`.
77+
///
78+
/// (Eligible functions might nevertheless be skipped for other reasons.)
79+
hook is_eligible_for_coverage(key: LocalDefId) -> bool;
7380
}

compiler/rustc_middle/src/mir/coverage.rs

+44-2
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,19 @@
22
33
use rustc_index::IndexVec;
44
use rustc_macros::HashStable;
5-
use rustc_span::Symbol;
5+
use rustc_span::{Span, Symbol};
66

77
use std::fmt::{self, Debug, Formatter};
88

9+
rustc_index::newtype_index! {
10+
/// Used by [`CoverageKind::BlockMarker`] to mark blocks during THIR-to-MIR
11+
/// lowering, so that those blocks can be identified later.
12+
#[derive(HashStable)]
13+
#[encodable]
14+
#[debug_format = "BlockMarkerId({})"]
15+
pub struct BlockMarkerId {}
16+
}
17+
918
rustc_index::newtype_index! {
1019
/// ID of a coverage counter. Values ascend from 0.
1120
///
@@ -83,6 +92,12 @@ pub enum CoverageKind {
8392
/// codegen.
8493
SpanMarker,
8594

95+
/// Marks its enclosing basic block with an ID that can be referred to by
96+
/// side data in [`BranchInfo`].
97+
///
98+
/// Has no effect during codegen.
99+
BlockMarker { id: BlockMarkerId },
100+
86101
/// Marks the point in MIR control flow represented by a coverage counter.
87102
///
88103
/// This is eventually lowered to `llvm.instrprof.increment` in LLVM IR.
@@ -107,6 +122,7 @@ impl Debug for CoverageKind {
107122
use CoverageKind::*;
108123
match self {
109124
SpanMarker => write!(fmt, "SpanMarker"),
125+
BlockMarker { id } => write!(fmt, "BlockMarker({:?})", id.index()),
110126
CounterIncrement { id } => write!(fmt, "CounterIncrement({:?})", id.index()),
111127
ExpressionUsed { id } => write!(fmt, "ExpressionUsed({:?})", id.index()),
112128
}
@@ -163,14 +179,18 @@ pub struct Expression {
163179
pub enum MappingKind {
164180
/// Associates a normal region of code with a counter/expression/zero.
165181
Code(CovTerm),
182+
/// Associates a branch region with separate counters for true and false.
183+
Branch { true_term: CovTerm, false_term: CovTerm },
166184
}
167185

168186
impl MappingKind {
169187
/// Iterator over all coverage terms in this mapping kind.
170188
pub fn terms(&self) -> impl Iterator<Item = CovTerm> {
171-
let one = |a| std::iter::once(a);
189+
let one = |a| std::iter::once(a).chain(None);
190+
let two = |a, b| std::iter::once(a).chain(Some(b));
172191
match *self {
173192
Self::Code(term) => one(term),
193+
Self::Branch { true_term, false_term } => two(true_term, false_term),
174194
}
175195
}
176196

@@ -179,6 +199,9 @@ impl MappingKind {
179199
pub fn map_terms(&self, map_fn: impl Fn(CovTerm) -> CovTerm) -> Self {
180200
match *self {
181201
Self::Code(term) => Self::Code(map_fn(term)),
202+
Self::Branch { true_term, false_term } => {
203+
Self::Branch { true_term: map_fn(true_term), false_term: map_fn(false_term) }
204+
}
182205
}
183206
}
184207
}
@@ -202,3 +225,22 @@ pub struct FunctionCoverageInfo {
202225
pub expressions: IndexVec<ExpressionId, Expression>,
203226
pub mappings: Vec<Mapping>,
204227
}
228+
229+
/// Branch information recorded during THIR-to-MIR lowering, and stored in MIR.
230+
#[derive(Clone, Debug)]
231+
#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
232+
pub struct BranchInfo {
233+
/// 1 more than the highest-numbered [`CoverageKind::BlockMarker`] that was
234+
/// injected into the MIR body. This makes it possible to allocate per-ID
235+
/// data structures without having to scan the entire body first.
236+
pub num_block_markers: usize,
237+
pub branch_spans: Vec<BranchSpan>,
238+
}
239+
240+
#[derive(Clone, Debug)]
241+
#[derive(TyEncodable, TyDecodable, Hash, HashStable, TypeFoldable, TypeVisitable)]
242+
pub struct BranchSpan {
243+
pub span: Span,
244+
pub true_marker: BlockMarkerId,
245+
pub false_marker: BlockMarkerId,
246+
}

compiler/rustc_middle/src/mir/mod.rs

+8
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,12 @@ pub struct Body<'tcx> {
403403

404404
pub tainted_by_errors: Option<ErrorGuaranteed>,
405405

406+
/// Branch coverage information collected during MIR building, to be used by
407+
/// the `InstrumentCoverage` pass.
408+
///
409+
/// Only present if branch coverage is enabled and this function is eligible.
410+
pub coverage_branch_info: Option<Box<coverage::BranchInfo>>,
411+
406412
/// Per-function coverage information added by the `InstrumentCoverage`
407413
/// pass, to be used in conjunction with the coverage statements injected
408414
/// into this body's blocks.
@@ -450,6 +456,7 @@ impl<'tcx> Body<'tcx> {
450456
is_polymorphic: false,
451457
injection_phase: None,
452458
tainted_by_errors,
459+
coverage_branch_info: None,
453460
function_coverage_info: None,
454461
};
455462
body.is_polymorphic = body.has_non_region_param();
@@ -479,6 +486,7 @@ impl<'tcx> Body<'tcx> {
479486
is_polymorphic: false,
480487
injection_phase: None,
481488
tainted_by_errors: None,
489+
coverage_branch_info: None,
482490
function_coverage_info: None,
483491
};
484492
body.is_polymorphic = body.has_non_region_param();

compiler/rustc_middle/src/mir/pretty.rs

+22
Original file line numberDiff line numberDiff line change
@@ -461,13 +461,35 @@ pub fn write_mir_intro<'tcx>(
461461
// Add an empty line before the first block is printed.
462462
writeln!(w)?;
463463

464+
if let Some(branch_info) = &body.coverage_branch_info {
465+
write_coverage_branch_info(branch_info, w)?;
466+
}
464467
if let Some(function_coverage_info) = &body.function_coverage_info {
465468
write_function_coverage_info(function_coverage_info, w)?;
466469
}
467470

468471
Ok(())
469472
}
470473

474+
fn write_coverage_branch_info(
475+
branch_info: &coverage::BranchInfo,
476+
w: &mut dyn io::Write,
477+
) -> io::Result<()> {
478+
let coverage::BranchInfo { branch_spans, .. } = branch_info;
479+
480+
for coverage::BranchSpan { span, true_marker, false_marker } in branch_spans {
481+
writeln!(
482+
w,
483+
"{INDENT}coverage branch {{ true: {true_marker:?}, false: {false_marker:?} }} => {span:?}",
484+
)?;
485+
}
486+
if !branch_spans.is_empty() {
487+
writeln!(w)?;
488+
}
489+
490+
Ok(())
491+
}
492+
471493
fn write_function_coverage_info(
472494
function_coverage_info: &coverage::FunctionCoverageInfo,
473495
w: &mut dyn io::Write,

compiler/rustc_middle/src/ty/structural_impls.rs

+1
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,7 @@ TrivialTypeTraversalImpls! {
405405
::rustc_hir::HirId,
406406
::rustc_hir::MatchSource,
407407
::rustc_target::asm::InlineAsmRegOrRegClass,
408+
crate::mir::coverage::BlockMarkerId,
408409
crate::mir::coverage::CounterId,
409410
crate::mir::coverage::ExpressionId,
410411
crate::mir::Local,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
use std::assert_matches::assert_matches;
2+
use std::collections::hash_map::Entry;
3+
4+
use rustc_data_structures::fx::FxHashMap;
5+
use rustc_middle::mir::coverage::{BlockMarkerId, BranchSpan, CoverageKind};
6+
use rustc_middle::mir::{self, BasicBlock, UnOp};
7+
use rustc_middle::thir::{ExprId, ExprKind, Thir};
8+
use rustc_middle::ty::TyCtxt;
9+
use rustc_span::def_id::LocalDefId;
10+
11+
use crate::build::Builder;
12+
13+
pub(crate) struct BranchInfoBuilder {
14+
/// Maps condition expressions to their enclosing `!`, for better instrumentation.
15+
nots: FxHashMap<ExprId, NotInfo>,
16+
17+
num_block_markers: usize,
18+
branch_spans: Vec<BranchSpan>,
19+
}
20+
21+
#[derive(Clone, Copy)]
22+
struct NotInfo {
23+
/// When visiting the associated expression as a branch condition, treat this
24+
/// enclosing `!` as the branch condition instead.
25+
enclosing_not: ExprId,
26+
/// True if the associated expression is nested within an odd number of `!`
27+
/// expressions relative to `enclosing_not` (inclusive of `enclosing_not`).
28+
is_flipped: bool,
29+
}
30+
31+
impl BranchInfoBuilder {
32+
/// Creates a new branch info builder, but only if branch coverage instrumentation
33+
/// is enabled and `def_id` represents a function that is eligible for coverage.
34+
pub(crate) fn new_if_enabled(tcx: TyCtxt<'_>, def_id: LocalDefId) -> Option<Self> {
35+
if tcx.sess.instrument_coverage_branch() && tcx.is_eligible_for_coverage(def_id) {
36+
Some(Self { nots: FxHashMap::default(), num_block_markers: 0, branch_spans: vec![] })
37+
} else {
38+
None
39+
}
40+
}
41+
42+
/// Unary `!` expressions inside an `if` condition are lowered by lowering
43+
/// their argument instead, and then reversing the then/else arms of that `if`.
44+
///
45+
/// That's awkward for branch coverage instrumentation, so to work around that
46+
/// we pre-emptively visit any affected `!` expressions, and record extra
47+
/// information that [`Builder::visit_coverage_branch_condition`] can use to
48+
/// synthesize branch instrumentation for the enclosing `!`.
49+
pub(crate) fn visit_unary_not(&mut self, thir: &Thir<'_>, unary_not: ExprId) {
50+
assert_matches!(thir[unary_not].kind, ExprKind::Unary { op: UnOp::Not, .. });
51+
52+
self.visit_with_not_info(
53+
thir,
54+
unary_not,
55+
// Set `is_flipped: false` for the `!` itself, so that its enclosed
56+
// expression will have `is_flipped: true`.
57+
NotInfo { enclosing_not: unary_not, is_flipped: false },
58+
);
59+
}
60+
61+
fn visit_with_not_info(&mut self, thir: &Thir<'_>, expr_id: ExprId, not_info: NotInfo) {
62+
match self.nots.entry(expr_id) {
63+
// This expression has already been marked by an enclosing `!`.
64+
Entry::Occupied(_) => return,
65+
Entry::Vacant(entry) => entry.insert(not_info),
66+
};
67+
68+
match thir[expr_id].kind {
69+
ExprKind::Unary { op: UnOp::Not, arg } => {
70+
// Invert the `is_flipped` flag for the contents of this `!`.
71+
let not_info = NotInfo { is_flipped: !not_info.is_flipped, ..not_info };
72+
self.visit_with_not_info(thir, arg, not_info);
73+
}
74+
ExprKind::Scope { value, .. } => self.visit_with_not_info(thir, value, not_info),
75+
ExprKind::Use { source } => self.visit_with_not_info(thir, source, not_info),
76+
// All other expressions (including `&&` and `||`) don't need any
77+
// special handling of their contents, so stop visiting.
78+
_ => {}
79+
}
80+
}
81+
82+
fn next_block_marker_id(&mut self) -> BlockMarkerId {
83+
let id = BlockMarkerId::from_usize(self.num_block_markers);
84+
self.num_block_markers += 1;
85+
id
86+
}
87+
88+
pub(crate) fn into_done(self) -> Option<Box<mir::coverage::BranchInfo>> {
89+
let Self { nots: _, num_block_markers, branch_spans } = self;
90+
91+
if num_block_markers == 0 {
92+
assert!(branch_spans.is_empty());
93+
return None;
94+
}
95+
96+
Some(Box::new(mir::coverage::BranchInfo { num_block_markers, branch_spans }))
97+
}
98+
}
99+
100+
impl Builder<'_, '_> {
101+
/// If branch coverage is enabled, inject marker statements into `then_block`
102+
/// and `else_block`, and record their IDs in the table of branch spans.
103+
pub(crate) fn visit_coverage_branch_condition(
104+
&mut self,
105+
mut expr_id: ExprId,
106+
mut then_block: BasicBlock,
107+
mut else_block: BasicBlock,
108+
) {
109+
// Bail out if branch coverage is not enabled for this function.
110+
let Some(branch_info) = self.coverage_branch_info.as_ref() else { return };
111+
112+
// If this condition expression is nested within one or more `!` expressions,
113+
// replace it with the enclosing `!` collected by `visit_unary_not`.
114+
if let Some(&NotInfo { enclosing_not, is_flipped }) = branch_info.nots.get(&expr_id) {
115+
expr_id = enclosing_not;
116+
if is_flipped {
117+
std::mem::swap(&mut then_block, &mut else_block);
118+
}
119+
}
120+
let source_info = self.source_info(self.thir[expr_id].span);
121+
122+
// Now that we have `source_info`, we can upgrade to a &mut reference.
123+
let branch_info = self.coverage_branch_info.as_mut().expect("upgrading & to &mut");
124+
125+
let mut inject_branch_marker = |block: BasicBlock| {
126+
let id = branch_info.next_block_marker_id();
127+
128+
let marker_statement = mir::Statement {
129+
source_info,
130+
kind: mir::StatementKind::Coverage(Box::new(mir::Coverage {
131+
kind: CoverageKind::BlockMarker { id },
132+
})),
133+
};
134+
self.cfg.push(block, marker_statement);
135+
136+
id
137+
};
138+
139+
let true_marker = inject_branch_marker(then_block);
140+
let false_marker = inject_branch_marker(else_block);
141+
142+
branch_info.branch_spans.push(BranchSpan {
143+
span: source_info.span,
144+
true_marker,
145+
false_marker,
146+
});
147+
}
148+
}

compiler/rustc_mir_build/src/build/custom/mod.rs

+1
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ pub(super) fn build_custom_mir<'tcx>(
6060
tainted_by_errors: None,
6161
injection_phase: None,
6262
pass_count: 0,
63+
coverage_branch_info: None,
6364
function_coverage_info: None,
6465
};
6566

0 commit comments

Comments
 (0)