Skip to content

Commit f3eabbd

Browse files
committed
Reland - Report coverage 0 of dead blocks
Fixes: #84018 With `-Z instrument-coverage`, coverage reporting of dead blocks (for example, blocks dropped because a conditional branch is dropped, based on const evaluation) is now supported. Note, this PR relands an earlier, reverted PR that failed when compiling generators. Generators clone blocks, so CFG simplification is used to remove the original `BasicBlocks`; and since they were cloned, the original blocks should not be saved. (Saving them resulted in duplicate coverage counters, and `llvm-cov` failed with a "Malformed coverage data" error. If `instrument-coverage` is enabled, `simplify::remove_dead_blocks_with_coverage()`, with `DroppedCoverage::SaveUnreachable`, finds all dropped coverage `Statement`s and adds their `code_region`s as `Unreachable` coverage `Statement`s to the `START_BLOCK`, so they are still included in the coverage map. Check out the resulting changes in the test coverage reports in this PR.
1 parent 2a245f4 commit f3eabbd

22 files changed

+177
-65
lines changed

compiler/rustc_codegen_ssa/src/coverageinfo/map.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ pub struct Expression {
2828
/// only whitespace or comments). According to LLVM Code Coverage Mapping documentation, "A count
2929
/// for a gap area is only used as the line execution count if there are no other regions on a
3030
/// line."
31+
#[derive(Debug)]
3132
pub struct FunctionCoverage<'tcx> {
3233
instance: Instance<'tcx>,
3334
source_hash: u64,
@@ -113,6 +114,14 @@ impl<'tcx> FunctionCoverage<'tcx> {
113114
expression_id, lhs, op, rhs, region
114115
);
115116
let expression_index = self.expression_index(u32::from(expression_id));
117+
debug_assert!(
118+
expression_index.as_usize() < self.expressions.len(),
119+
"expression_index {} is out of range for expressions.len() = {}
120+
for {:?}",
121+
expression_index.as_usize(),
122+
self.expressions.len(),
123+
self,
124+
);
116125
if let Some(previous_expression) = self.expressions[expression_index].replace(Expression {
117126
lhs,
118127
op,
@@ -150,6 +159,13 @@ impl<'tcx> FunctionCoverage<'tcx> {
150159
self.instance
151160
);
152161

162+
if !self.counters.iter().any(|region| region.is_some()) {
163+
// FIXME(richkadel): How is this possible and why is it OK? I verified that the first
164+
// test program that triggers this still works, and `llvm-cov` does not fail, so I need
165+
// to allow it.
166+
debug!("no counters for {:?}", self);
167+
}
168+
153169
let counter_regions = self.counter_regions();
154170
let (counter_expressions, expression_regions) = self.expressions_with_regions();
155171
let unreachable_regions = self.unreachable_regions();

compiler/rustc_mir/src/transform/const_goto.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ impl<'tcx> MirPass<'tcx> for ConstGoto {
4747
// if we applied optimizations, we potentially have some cfg to cleanup to
4848
// make it easier for further passes
4949
if should_simplify {
50-
simplify_cfg(body);
50+
simplify_cfg(tcx, body);
5151
simplify_locals(body, tcx);
5252
}
5353
}

compiler/rustc_mir/src/transform/deduplicate_blocks.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ impl<'tcx> MirPass<'tcx> for DeduplicateBlocks {
2626
if has_opts_to_apply {
2727
let mut opt_applier = OptApplier { tcx, duplicates };
2828
opt_applier.visit_body(body);
29-
simplify_cfg(body);
29+
simplify_cfg(tcx, body);
3030
}
3131
}
3232
}

compiler/rustc_mir/src/transform/early_otherwise_branch.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch {
164164
// Since this optimization adds new basic blocks and invalidates others,
165165
// clean up the cfg to make it nicer for other passes
166166
if should_cleanup {
167-
simplify_cfg(body);
167+
simplify_cfg(tcx, body);
168168
}
169169
}
170170
}

compiler/rustc_mir/src/transform/generator.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -964,7 +964,7 @@ fn create_generator_drop_shim<'tcx>(
964964

965965
// Make sure we remove dead blocks to remove
966966
// unrelated code from the resume part of the function
967-
simplify::remove_dead_blocks(&mut body);
967+
simplify::remove_dead_blocks_with_coverage(tcx, &mut body, simplify::DroppedCoverage::Allow);
968968

969969
dump_mir(tcx, None, "generator_drop", &0, &body, |_, _| Ok(()));
970970

@@ -1137,7 +1137,7 @@ fn create_generator_resume_function<'tcx>(
11371137

11381138
// Make sure we remove dead blocks to remove
11391139
// unrelated code from the drop part of the function
1140-
simplify::remove_dead_blocks(body);
1140+
simplify::remove_dead_blocks_with_coverage(tcx, body, simplify::DroppedCoverage::Allow);
11411141

11421142
dump_mir(tcx, None, "generator_resume", &0, body, |_, _| Ok(()));
11431143
}

compiler/rustc_mir/src/transform/inline.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ impl<'tcx> MirPass<'tcx> for Inline {
5757
if inline(tcx, body) {
5858
debug!("running simplify cfg on {:?}", body.source);
5959
CfgSimplifier::new(body).simplify();
60-
remove_dead_blocks(body);
60+
remove_dead_blocks(tcx, body);
6161
}
6262
}
6363
}

compiler/rustc_mir/src/transform/match_branches.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ impl<'tcx> MirPass<'tcx> for MatchBranchSimplification {
167167
}
168168

169169
if should_cleanup {
170-
simplify_cfg(body);
170+
simplify_cfg(tcx, body);
171171
}
172172
}
173173
}

compiler/rustc_mir/src/transform/multiple_return_terminators.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,6 @@ impl<'tcx> MirPass<'tcx> for MultipleReturnTerminators {
3838
}
3939
}
4040

41-
simplify::remove_dead_blocks(body)
41+
simplify::remove_dead_blocks(tcx, body)
4242
}
4343
}

compiler/rustc_mir/src/transform/remove_unneeded_drops.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ impl<'tcx> MirPass<'tcx> for RemoveUnneededDrops {
3636
// if we applied optimizations, we potentially have some cfg to cleanup to
3737
// make it easier for further passes
3838
if should_simplify {
39-
simplify_cfg(body);
39+
simplify_cfg(tcx, body);
4040
}
4141
}
4242
}

compiler/rustc_mir/src/transform/simplify.rs

Lines changed: 92 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
3030
use crate::transform::MirPass;
3131
use rustc_index::vec::{Idx, IndexVec};
32+
use rustc_middle::mir::coverage::*;
3233
use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor};
3334
use rustc_middle::mir::*;
3435
use rustc_middle::ty::TyCtxt;
@@ -46,9 +47,17 @@ impl SimplifyCfg {
4647
}
4748
}
4849

49-
pub fn simplify_cfg(body: &mut Body<'_>) {
50+
pub fn simplify_cfg(tcx: TyCtxt<'tcx>, body: &mut Body<'_>) {
51+
simplify_cfg_with_coverage(tcx, body, DroppedCoverage::AssertNone);
52+
}
53+
54+
pub fn simplify_cfg_with_coverage(
55+
tcx: TyCtxt<'tcx>,
56+
body: &mut Body<'_>,
57+
dropped_coverage: DroppedCoverage,
58+
) {
5059
CfgSimplifier::new(body).simplify();
51-
remove_dead_blocks(body);
60+
remove_dead_blocks_with_coverage(tcx, body, dropped_coverage);
5261

5362
// FIXME: Should probably be moved into some kind of pass manager
5463
body.basic_blocks_mut().raw.shrink_to_fit();
@@ -59,12 +68,31 @@ impl<'tcx> MirPass<'tcx> for SimplifyCfg {
5968
Cow::Borrowed(&self.label)
6069
}
6170

62-
fn run_pass(&self, _tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
71+
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
6372
debug!("SimplifyCfg({:?}) - simplifying {:?}", self.label, body.source);
64-
simplify_cfg(body);
73+
simplify_cfg_with_coverage(tcx, body, DroppedCoverage::SaveUnreachable);
6574
}
6675
}
6776

77+
/// How to handle `Coverage` statements in dropped `BasicBlocks`.
78+
pub enum DroppedCoverage {
79+
/// Allow the `Coverage` statement(s) in a dropped `BasicBlock` to be
80+
/// dropped as well. (This may be because the `Coverage` statement was
81+
/// already cloned.)
82+
Allow,
83+
84+
/// Assert that there are none. The current or most recent MIR pass(es)
85+
/// should not have done anything that would have resulted in an
86+
/// unreachable block with a `Coverage` statement.
87+
AssertNone,
88+
89+
/// Assume the `Coverage` statement(s) in a dropped `BacicBlock` are part
90+
/// of a dead code region. Save them in the `START_BLOCK` but mark `Counter`
91+
/// coverage as `is_dead` so they are not incremented. (Expressions from
92+
/// dead blocks should already add up to zero.)
93+
SaveUnreachable,
94+
}
95+
6896
pub struct CfgSimplifier<'a, 'tcx> {
6997
basic_blocks: &'a mut IndexVec<BasicBlock, BasicBlockData<'tcx>>,
7098
pred_count: IndexVec<BasicBlock, u32>,
@@ -286,7 +314,15 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
286314
}
287315
}
288316

289-
pub fn remove_dead_blocks(body: &mut Body<'_>) {
317+
pub fn remove_dead_blocks(tcx: TyCtxt<'tcx>, body: &mut Body<'_>) {
318+
remove_dead_blocks_with_coverage(tcx, body, DroppedCoverage::AssertNone);
319+
}
320+
321+
pub fn remove_dead_blocks_with_coverage(
322+
tcx: TyCtxt<'tcx>,
323+
body: &mut Body<'_>,
324+
dropped_coverage: DroppedCoverage,
325+
) {
290326
let reachable = traversal::reachable_as_bitset(body);
291327
let num_blocks = body.basic_blocks().len();
292328
if num_blocks == reachable.count() {
@@ -306,6 +342,16 @@ pub fn remove_dead_blocks(body: &mut Body<'_>) {
306342
}
307343
used_blocks += 1;
308344
}
345+
346+
if tcx.sess.instrument_coverage() {
347+
use DroppedCoverage::*;
348+
match dropped_coverage {
349+
Allow => {}
350+
AssertNone => assert_no_unreachable_coverage(basic_blocks, used_blocks),
351+
SaveUnreachable => save_unreachable_coverage(basic_blocks, used_blocks),
352+
}
353+
}
354+
309355
basic_blocks.raw.truncate(used_blocks);
310356

311357
for block in basic_blocks {
@@ -315,6 +361,47 @@ pub fn remove_dead_blocks(body: &mut Body<'_>) {
315361
}
316362
}
317363

364+
fn assert_no_unreachable_coverage(
365+
basic_blocks: &mut IndexVec<BasicBlock, BasicBlockData<'_>>,
366+
first_dead_block: usize,
367+
) {
368+
for dead_block in first_dead_block..basic_blocks.len() {
369+
for statement in &basic_blocks[BasicBlock::new(dead_block)].statements {
370+
assert!(
371+
!matches!(statement.kind, StatementKind::Coverage(..)),
372+
"Dead blocks should not have `Coverage` statements at this stage."
373+
);
374+
}
375+
}
376+
}
377+
378+
fn save_unreachable_coverage(
379+
basic_blocks: &mut IndexVec<BasicBlock, BasicBlockData<'_>>,
380+
first_dead_block: usize,
381+
) {
382+
// retain coverage info for dead blocks, so coverage reports will still
383+
// report `0` executions for the uncovered code regions.
384+
let mut dropped_coverage = Vec::new();
385+
for dead_block in first_dead_block..basic_blocks.len() {
386+
for statement in basic_blocks[BasicBlock::new(dead_block)].statements.iter() {
387+
if let StatementKind::Coverage(coverage) = &statement.kind {
388+
if let Some(code_region) = &coverage.code_region {
389+
dropped_coverage.push((statement.source_info, code_region.clone()));
390+
}
391+
}
392+
}
393+
}
394+
for (source_info, code_region) in dropped_coverage {
395+
basic_blocks[START_BLOCK].statements.push(Statement {
396+
source_info,
397+
kind: StatementKind::Coverage(box Coverage {
398+
kind: CoverageKind::Unreachable,
399+
code_region: Some(code_region),
400+
}),
401+
})
402+
}
403+
}
404+
318405
pub struct SimplifyLocals;
319406

320407
impl<'tcx> MirPass<'tcx> for SimplifyLocals {

0 commit comments

Comments
 (0)