Skip to content

Commit bc241fd

Browse files
fmeasefmease
authored
Rollup merge of rust-lang#120292 - Zalathar:dismantle, r=oli-obk
coverage: Dismantle `Instrumentor` and flatten span refinement This is a combination of two refactorings that are unrelated, but would otherwise have a merge conflict. No functional changes, other than a small tweak to debug logging as part of rearranging some functions. Ignoring whitespace is highly recommended, since most of the modified lines have just been reindented. --- The first change is to dismantle `Instrumentor` into ordinary functions. This is one of those cases where encapsulating several values into a struct ultimately hurts more than it helps. With everything stored as local variables in one main function, and passed explicitly into helper functions, it's easier to see what is used where, and make changes as necessary. --- The second change is to flatten the functions for extracting/refining coverage spans. Consolidating this code into flatter functions reduces the amount of pointer-chasing required to read and modify it.
2 parents 7c1523f + 572d7e9 commit bc241fd

File tree

3 files changed

+178
-212
lines changed

3 files changed

+178
-212
lines changed

‎compiler/rustc_mir_transform/src/coverage/mod.rs

+126-139
Original file line numberDiff line numberDiff line change
@@ -59,167 +59,154 @@ impl<'tcx> MirPass<'tcx> for InstrumentCoverage {
5959
_ => {}
6060
}
6161

62-
trace!("InstrumentCoverage starting for {def_id:?}");
63-
Instrumentor::new(tcx, mir_body).inject_counters();
64-
trace!("InstrumentCoverage done for {def_id:?}");
62+
instrument_function_for_coverage(tcx, mir_body);
6563
}
6664
}
6765

68-
struct Instrumentor<'a, 'tcx> {
69-
tcx: TyCtxt<'tcx>,
70-
mir_body: &'a mut mir::Body<'tcx>,
71-
hir_info: ExtractedHirInfo,
72-
basic_coverage_blocks: CoverageGraph,
73-
}
74-
75-
impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
76-
fn new(tcx: TyCtxt<'tcx>, mir_body: &'a mut mir::Body<'tcx>) -> Self {
77-
let hir_info = extract_hir_info(tcx, mir_body.source.def_id().expect_local());
66+
fn instrument_function_for_coverage<'tcx>(tcx: TyCtxt<'tcx>, mir_body: &mut mir::Body<'tcx>) {
67+
let def_id = mir_body.source.def_id();
68+
let _span = debug_span!("instrument_function_for_coverage", ?def_id).entered();
7869

79-
debug!(?hir_info, "instrumenting {:?}", mir_body.source.def_id());
70+
let hir_info = extract_hir_info(tcx, def_id.expect_local());
71+
let basic_coverage_blocks = CoverageGraph::from_mir(mir_body);
8072

81-
let basic_coverage_blocks = CoverageGraph::from_mir(mir_body);
73+
////////////////////////////////////////////////////
74+
// Compute coverage spans from the `CoverageGraph`.
75+
let Some(coverage_spans) =
76+
spans::generate_coverage_spans(mir_body, &hir_info, &basic_coverage_blocks)
77+
else {
78+
// No relevant spans were found in MIR, so skip instrumenting this function.
79+
return;
80+
};
8281

83-
Self { tcx, mir_body, hir_info, basic_coverage_blocks }
82+
////////////////////////////////////////////////////
83+
// Create an optimized mix of `Counter`s and `Expression`s for the `CoverageGraph`. Ensure
84+
// every coverage span has a `Counter` or `Expression` assigned to its `BasicCoverageBlock`
85+
// and all `Expression` dependencies (operands) are also generated, for any other
86+
// `BasicCoverageBlock`s not already associated with a coverage span.
87+
let bcb_has_coverage_spans = |bcb| coverage_spans.bcb_has_coverage_spans(bcb);
88+
let coverage_counters =
89+
CoverageCounters::make_bcb_counters(&basic_coverage_blocks, bcb_has_coverage_spans);
90+
91+
let mappings = create_mappings(tcx, &hir_info, &coverage_spans, &coverage_counters);
92+
if mappings.is_empty() {
93+
// No spans could be converted into valid mappings, so skip this function.
94+
debug!("no spans could be converted into valid mappings; skipping");
95+
return;
8496
}
8597

86-
fn inject_counters(&'a mut self) {
87-
////////////////////////////////////////////////////
88-
// Compute coverage spans from the `CoverageGraph`.
89-
let Some(coverage_spans) = CoverageSpans::generate_coverage_spans(
90-
self.mir_body,
91-
&self.hir_info,
92-
&self.basic_coverage_blocks,
93-
) else {
94-
// No relevant spans were found in MIR, so skip instrumenting this function.
95-
return;
96-
};
97-
98-
////////////////////////////////////////////////////
99-
// Create an optimized mix of `Counter`s and `Expression`s for the `CoverageGraph`. Ensure
100-
// every coverage span has a `Counter` or `Expression` assigned to its `BasicCoverageBlock`
101-
// and all `Expression` dependencies (operands) are also generated, for any other
102-
// `BasicCoverageBlock`s not already associated with a coverage span.
103-
let bcb_has_coverage_spans = |bcb| coverage_spans.bcb_has_coverage_spans(bcb);
104-
let coverage_counters = CoverageCounters::make_bcb_counters(
105-
&self.basic_coverage_blocks,
106-
bcb_has_coverage_spans,
107-
);
98+
inject_coverage_statements(
99+
mir_body,
100+
&basic_coverage_blocks,
101+
bcb_has_coverage_spans,
102+
&coverage_counters,
103+
);
108104

109-
let mappings = self.create_mappings(&coverage_spans, &coverage_counters);
110-
if mappings.is_empty() {
111-
// No spans could be converted into valid mappings, so skip this function.
112-
debug!("no spans could be converted into valid mappings; skipping");
113-
return;
114-
}
105+
mir_body.function_coverage_info = Some(Box::new(FunctionCoverageInfo {
106+
function_source_hash: hir_info.function_source_hash,
107+
num_counters: coverage_counters.num_counters(),
108+
expressions: coverage_counters.into_expressions(),
109+
mappings,
110+
}));
111+
}
115112

116-
self.inject_coverage_statements(bcb_has_coverage_spans, &coverage_counters);
113+
/// For each coverage span extracted from MIR, create a corresponding
114+
/// mapping.
115+
///
116+
/// Precondition: All BCBs corresponding to those spans have been given
117+
/// coverage counters.
118+
fn create_mappings<'tcx>(
119+
tcx: TyCtxt<'tcx>,
120+
hir_info: &ExtractedHirInfo,
121+
coverage_spans: &CoverageSpans,
122+
coverage_counters: &CoverageCounters,
123+
) -> Vec<Mapping> {
124+
let source_map = tcx.sess.source_map();
125+
let body_span = hir_info.body_span;
126+
127+
let source_file = source_map.lookup_source_file(body_span.lo());
128+
use rustc_session::RemapFileNameExt;
129+
let file_name = Symbol::intern(&source_file.name.for_codegen(tcx.sess).to_string_lossy());
130+
131+
let term_for_bcb = |bcb| {
132+
coverage_counters
133+
.bcb_counter(bcb)
134+
.expect("all BCBs with spans were given counters")
135+
.as_term()
136+
};
117137

118-
self.mir_body.function_coverage_info = Some(Box::new(FunctionCoverageInfo {
119-
function_source_hash: self.hir_info.function_source_hash,
120-
num_counters: coverage_counters.num_counters(),
121-
expressions: coverage_counters.into_expressions(),
122-
mappings,
123-
}));
124-
}
138+
coverage_spans
139+
.all_bcb_mappings()
140+
.filter_map(|&BcbMapping { kind: bcb_mapping_kind, span }| {
141+
let kind = match bcb_mapping_kind {
142+
BcbMappingKind::Code(bcb) => MappingKind::Code(term_for_bcb(bcb)),
143+
};
144+
let code_region = make_code_region(source_map, file_name, span, body_span)?;
145+
Some(Mapping { kind, code_region })
146+
})
147+
.collect::<Vec<_>>()
148+
}
125149

126-
/// For each coverage span extracted from MIR, create a corresponding
127-
/// mapping.
128-
///
129-
/// Precondition: All BCBs corresponding to those spans have been given
130-
/// coverage counters.
131-
fn create_mappings(
132-
&self,
133-
coverage_spans: &CoverageSpans,
134-
coverage_counters: &CoverageCounters,
135-
) -> Vec<Mapping> {
136-
let source_map = self.tcx.sess.source_map();
137-
let body_span = self.hir_info.body_span;
138-
139-
let source_file = source_map.lookup_source_file(body_span.lo());
140-
use rustc_session::RemapFileNameExt;
141-
let file_name =
142-
Symbol::intern(&source_file.name.for_codegen(self.tcx.sess).to_string_lossy());
143-
144-
let term_for_bcb = |bcb| {
145-
coverage_counters
146-
.bcb_counter(bcb)
147-
.expect("all BCBs with spans were given counters")
148-
.as_term()
150+
/// For each BCB node or BCB edge that has an associated coverage counter,
151+
/// inject any necessary coverage statements into MIR.
152+
fn inject_coverage_statements<'tcx>(
153+
mir_body: &mut mir::Body<'tcx>,
154+
basic_coverage_blocks: &CoverageGraph,
155+
bcb_has_coverage_spans: impl Fn(BasicCoverageBlock) -> bool,
156+
coverage_counters: &CoverageCounters,
157+
) {
158+
// Process the counters associated with BCB nodes.
159+
for (bcb, counter_kind) in coverage_counters.bcb_node_counters() {
160+
let do_inject = match counter_kind {
161+
// Counter-increment statements always need to be injected.
162+
BcbCounter::Counter { .. } => true,
163+
// The only purpose of expression-used statements is to detect
164+
// when a mapping is unreachable, so we only inject them for
165+
// expressions with one or more mappings.
166+
BcbCounter::Expression { .. } => bcb_has_coverage_spans(bcb),
149167
};
150-
151-
coverage_spans
152-
.all_bcb_mappings()
153-
.filter_map(|&BcbMapping { kind: bcb_mapping_kind, span }| {
154-
let kind = match bcb_mapping_kind {
155-
BcbMappingKind::Code(bcb) => MappingKind::Code(term_for_bcb(bcb)),
156-
};
157-
let code_region = make_code_region(source_map, file_name, span, body_span)?;
158-
Some(Mapping { kind, code_region })
159-
})
160-
.collect::<Vec<_>>()
168+
if do_inject {
169+
inject_statement(
170+
mir_body,
171+
make_mir_coverage_kind(counter_kind),
172+
basic_coverage_blocks[bcb].leader_bb(),
173+
);
174+
}
161175
}
162176

163-
/// For each BCB node or BCB edge that has an associated coverage counter,
164-
/// inject any necessary coverage statements into MIR.
165-
fn inject_coverage_statements(
166-
&mut self,
167-
bcb_has_coverage_spans: impl Fn(BasicCoverageBlock) -> bool,
168-
coverage_counters: &CoverageCounters,
169-
) {
170-
// Process the counters associated with BCB nodes.
171-
for (bcb, counter_kind) in coverage_counters.bcb_node_counters() {
172-
let do_inject = match counter_kind {
173-
// Counter-increment statements always need to be injected.
174-
BcbCounter::Counter { .. } => true,
175-
// The only purpose of expression-used statements is to detect
176-
// when a mapping is unreachable, so we only inject them for
177-
// expressions with one or more mappings.
178-
BcbCounter::Expression { .. } => bcb_has_coverage_spans(bcb),
179-
};
180-
if do_inject {
181-
inject_statement(
182-
self.mir_body,
183-
self.make_mir_coverage_kind(counter_kind),
184-
self.basic_coverage_blocks[bcb].leader_bb(),
185-
);
186-
}
177+
// Process the counters associated with BCB edges.
178+
for (from_bcb, to_bcb, counter_kind) in coverage_counters.bcb_edge_counters() {
179+
let do_inject = match counter_kind {
180+
// Counter-increment statements always need to be injected.
181+
BcbCounter::Counter { .. } => true,
182+
// BCB-edge expressions never have mappings, so they never need
183+
// a corresponding statement.
184+
BcbCounter::Expression { .. } => false,
185+
};
186+
if !do_inject {
187+
continue;
187188
}
188189

189-
// Process the counters associated with BCB edges.
190-
for (from_bcb, to_bcb, counter_kind) in coverage_counters.bcb_edge_counters() {
191-
let do_inject = match counter_kind {
192-
// Counter-increment statements always need to be injected.
193-
BcbCounter::Counter { .. } => true,
194-
// BCB-edge expressions never have mappings, so they never need
195-
// a corresponding statement.
196-
BcbCounter::Expression { .. } => false,
197-
};
198-
if !do_inject {
199-
continue;
200-
}
190+
// We need to inject a coverage statement into a new BB between the
191+
// last BB of `from_bcb` and the first BB of `to_bcb`.
192+
let from_bb = basic_coverage_blocks[from_bcb].last_bb();
193+
let to_bb = basic_coverage_blocks[to_bcb].leader_bb();
201194

202-
// We need to inject a coverage statement into a new BB between the
203-
// last BB of `from_bcb` and the first BB of `to_bcb`.
204-
let from_bb = self.basic_coverage_blocks[from_bcb].last_bb();
205-
let to_bb = self.basic_coverage_blocks[to_bcb].leader_bb();
206-
207-
let new_bb = inject_edge_counter_basic_block(self.mir_body, from_bb, to_bb);
208-
debug!(
209-
"Edge {from_bcb:?} (last {from_bb:?}) -> {to_bcb:?} (leader {to_bb:?}) \
195+
let new_bb = inject_edge_counter_basic_block(mir_body, from_bb, to_bb);
196+
debug!(
197+
"Edge {from_bcb:?} (last {from_bb:?}) -> {to_bcb:?} (leader {to_bb:?}) \
210198
requires a new MIR BasicBlock {new_bb:?} for edge counter {counter_kind:?}",
211-
);
199+
);
212200

213-
// Inject a counter into the newly-created BB.
214-
inject_statement(self.mir_body, self.make_mir_coverage_kind(counter_kind), new_bb);
215-
}
201+
// Inject a counter into the newly-created BB.
202+
inject_statement(mir_body, make_mir_coverage_kind(counter_kind), new_bb);
216203
}
204+
}
217205

218-
fn make_mir_coverage_kind(&self, counter_kind: &BcbCounter) -> CoverageKind {
219-
match *counter_kind {
220-
BcbCounter::Counter { id } => CoverageKind::CounterIncrement { id },
221-
BcbCounter::Expression { id } => CoverageKind::ExpressionUsed { id },
222-
}
206+
fn make_mir_coverage_kind(counter_kind: &BcbCounter) -> CoverageKind {
207+
match *counter_kind {
208+
BcbCounter::Counter { id } => CoverageKind::CounterIncrement { id },
209+
BcbCounter::Expression { id } => CoverageKind::ExpressionUsed { id },
223210
}
224211
}
225212

0 commit comments

Comments
 (0)