Skip to content

Commit

Permalink
Rollup merge of rust-lang#84797 - richkadel:cover-unreachable-stateme…
Browse files Browse the repository at this point in the history
…nts, r=tmandry

Report coverage `0` of dead blocks

Fixes: rust-lang#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.

If `instrument-coverage` is enabled, `simplify::remove_dead_blocks()`
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.

I also addressed an outstanding issue/request to move coverage tests from run-make-fulldeps to run-make (in commit 2).

Fixes: rust-lang#83830

r? ````@tmandry````
cc: ````@wesleywiser````
  • Loading branch information
Dylan-DPC authored May 4, 2021
2 parents 12699ab + 3fca198 commit 5d5ba5a
Show file tree
Hide file tree
Showing 103 changed files with 112 additions and 75 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/transform/const_goto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ impl<'tcx> MirPass<'tcx> for ConstGoto {
// if we applied optimizations, we potentially have some cfg to cleanup to
// make it easier for further passes
if should_simplify {
simplify_cfg(body);
simplify_cfg(tcx, body);
simplify_locals(body, tcx);
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/transform/deduplicate_blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ impl<'tcx> MirPass<'tcx> for DeduplicateBlocks {
if has_opts_to_apply {
let mut opt_applier = OptApplier { tcx, duplicates };
opt_applier.visit_body(body);
simplify_cfg(body);
simplify_cfg(tcx, body);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/transform/early_otherwise_branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch {
// Since this optimization adds new basic blocks and invalidates others,
// clean up the cfg to make it nicer for other passes
if should_cleanup {
simplify_cfg(body);
simplify_cfg(tcx, body);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir/src/transform/generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -964,7 +964,7 @@ fn create_generator_drop_shim<'tcx>(

// Make sure we remove dead blocks to remove
// unrelated code from the resume part of the function
simplify::remove_dead_blocks(&mut body);
simplify::remove_dead_blocks(tcx, &mut body);

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

Expand Down Expand Up @@ -1137,7 +1137,7 @@ fn create_generator_resume_function<'tcx>(

// Make sure we remove dead blocks to remove
// unrelated code from the drop part of the function
simplify::remove_dead_blocks(body);
simplify::remove_dead_blocks(tcx, body);

dump_mir(tcx, None, "generator_resume", &0, body, |_, _| Ok(()));
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/transform/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ impl<'tcx> MirPass<'tcx> for Inline {
if inline(tcx, body) {
debug!("running simplify cfg on {:?}", body.source);
CfgSimplifier::new(body).simplify();
remove_dead_blocks(body);
remove_dead_blocks(tcx, body);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/transform/match_branches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ impl<'tcx> MirPass<'tcx> for MatchBranchSimplification {
}

if should_cleanup {
simplify_cfg(body);
simplify_cfg(tcx, body);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,6 @@ impl<'tcx> MirPass<'tcx> for MultipleReturnTerminators {
}
}

simplify::remove_dead_blocks(body)
simplify::remove_dead_blocks(tcx, body)
}
}
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/transform/remove_unneeded_drops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ impl<'tcx> MirPass<'tcx> for RemoveUnneededDrops {
// if we applied optimizations, we potentially have some cfg to cleanup to
// make it easier for further passes
if should_simplify {
simplify_cfg(body);
simplify_cfg(tcx, body);
}
}
}
42 changes: 37 additions & 5 deletions compiler/rustc_mir/src/transform/simplify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

use crate::transform::MirPass;
use rustc_index::vec::{Idx, IndexVec};
use rustc_middle::mir::coverage::*;
use rustc_middle::mir::visit::{MutVisitor, MutatingUseContext, PlaceContext, Visitor};
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;
Expand All @@ -46,9 +47,9 @@ impl SimplifyCfg {
}
}

pub fn simplify_cfg(body: &mut Body<'_>) {
pub fn simplify_cfg(tcx: TyCtxt<'tcx>, body: &mut Body<'_>) {
CfgSimplifier::new(body).simplify();
remove_dead_blocks(body);
remove_dead_blocks(tcx, body);

// FIXME: Should probably be moved into some kind of pass manager
body.basic_blocks_mut().raw.shrink_to_fit();
Expand All @@ -59,9 +60,9 @@ impl<'tcx> MirPass<'tcx> for SimplifyCfg {
Cow::Borrowed(&self.label)
}

fn run_pass(&self, _tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
debug!("SimplifyCfg({:?}) - simplifying {:?}", self.label, body.source);
simplify_cfg(body);
simplify_cfg(tcx, body);
}
}

Expand Down Expand Up @@ -286,7 +287,7 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> {
}
}

pub fn remove_dead_blocks(body: &mut Body<'_>) {
pub fn remove_dead_blocks(tcx: TyCtxt<'tcx>, body: &mut Body<'_>) {
let reachable = traversal::reachable_as_bitset(body);
let num_blocks = body.basic_blocks().len();
if num_blocks == reachable.count() {
Expand All @@ -306,6 +307,11 @@ pub fn remove_dead_blocks(body: &mut Body<'_>) {
}
used_blocks += 1;
}

if tcx.sess.instrument_coverage() {
save_unreachable_coverage(basic_blocks, used_blocks);
}

basic_blocks.raw.truncate(used_blocks);

for block in basic_blocks {
Expand All @@ -315,6 +321,32 @@ pub fn remove_dead_blocks(body: &mut Body<'_>) {
}
}

fn save_unreachable_coverage(
basic_blocks: &mut IndexVec<BasicBlock, BasicBlockData<'_>>,
first_dead_block: usize,
) {
// retain coverage info for dead blocks, so coverage reports will still
// report `0` executions for the uncovered code regions.
let mut dropped_coverage = Vec::new();
for dead_block in first_dead_block..basic_blocks.len() {
for statement in basic_blocks[BasicBlock::new(dead_block)].statements.iter() {
if let StatementKind::Coverage(coverage) = &statement.kind {
if let Some(code_region) = &coverage.code_region {
dropped_coverage.push((statement.source_info, code_region.clone()));
}
}
}
}
for (source_info, code_region) in dropped_coverage {
basic_blocks[START_BLOCK].statements.push(Statement {
source_info,
kind: StatementKind::Coverage(box Coverage {
kind: CoverageKind::Unreachable,
code_region: Some(code_region),
}),
})
}
}
pub struct SimplifyLocals;

impl<'tcx> MirPass<'tcx> for SimplifyLocals {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/transform/simplify_try.rs
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ impl<'tcx> MirPass<'tcx> for SimplifyBranchSame {

if did_remove_blocks {
// We have dead blocks now, so remove those.
simplify::remove_dead_blocks(body);
simplify::remove_dead_blocks(tcx, body);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/transform/unreachable_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ impl MirPass<'_> for UnreachablePropagation {
}

if replaced {
simplify::remove_dead_blocks(body);
simplify::remove_dead_blocks(tcx, body);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/bootstrap/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1197,7 +1197,7 @@ note: if you're sure you want to do this, please open an issue as to why. In the
.arg(builder.ensure(tool::JsonDocCk { compiler: json_compiler, target }));
}

if mode == "run-make" && suite.ends_with("fulldeps") {
if mode == "run-make" && !suite.ends_with("fulldeps") {
let rust_demangler = builder
.ensure(tool::RustDemangler { compiler, target, extra_features: Vec::new() })
.expect("in-tree tool");
Expand Down
3 changes: 0 additions & 3 deletions src/test/run-make-fulldeps/coverage/compiletest-ignore-dir

This file was deleted.

6 changes: 0 additions & 6 deletions src/test/run-make-fulldeps/coverage/coverage_tools.mk

This file was deleted.

File renamed without changes.
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
12| 1| if b {
13| 1| println!("non_async_func println in block");
14| 1| }
^0
15| 1|}
16| |
17| |// FIXME(#83985): The auto-generated closure in an async function is failing to include
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
5| 1| if true {
6| 1| countdown = 10;
7| 1| }
^0
8| |
9| | const B: u32 = 100;
10| 1| let x = if countdown > 7 {
Expand All @@ -24,6 +25,7 @@
24| 1| if true {
25| 1| countdown = 10;
26| 1| }
^0
27| |
28| 1| if countdown > 7 {
29| 1| countdown -= 4;
Expand All @@ -42,6 +44,7 @@
41| 1| if true {
42| 1| countdown = 10;
43| 1| }
^0
44| |
45| 1| if countdown > 7 {
46| 1| countdown -= 4;
Expand All @@ -54,13 +57,14 @@
53| | } else {
54| 0| return;
55| | }
56| | } // Note: closing brace shows uncovered (vs. `0` for implicit else) because condition literal
57| | // `true` was const-evaluated. The compiler knows the `if` block will be executed.
56| 0| }
57| |
58| |
59| 1| let mut countdown = 0;
60| 1| if true {
61| 1| countdown = 1;
62| 1| }
^0
63| |
64| 1| let z = if countdown > 7 {
^0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
8| 1|//! assert_eq!(1, 1);
9| |//! } else {
10| |//! // this is not!
11| |//! assert_eq!(1, 2);
11| 0|//! assert_eq!(1, 2);
12| |//! }
13| 1|//! ```
14| |//!
Expand Down Expand Up @@ -84,7 +84,7 @@
74| 1| if true {
75| 1| assert_eq!(1, 1);
76| | } else {
77| | assert_eq!(1, 2);
77| 0| assert_eq!(1, 2);
78| | }
79| 1|}
80| |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@
19| 1| if true {
20| 1| println!("Exiting with error...");
21| 1| return Err(1);
22| | }
23| |
24| | let _ = Firework { strength: 1000 };
25| |
26| | Ok(())
22| 0| }
23| 0|
24| 0| let _ = Firework { strength: 1000 };
25| 0|
26| 0| Ok(())
27| 1|}
28| |
29| |// Expected program output:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,15 @@
30| 1| if true {
31| 1| println!("Exiting with error...");
32| 1| return Err(1);
33| | } // The remaining lines below have no coverage because `if true` (with the constant literal
34| | // `true`) is guaranteed to execute the `then` block, which is also guaranteed to `return`.
35| | // Thankfully, in the normal case, conditions are not guaranteed ahead of time, and as shown
36| | // in other tests, the lines below would have coverage (which would show they had `0`
37| | // executions, assuming the condition still evaluated to `true`).
38| |
39| | let _ = Firework { strength: 1000 };
40| |
41| | Ok(())
33| 0| }
34| 0|
35| 0|
36| 0|
37| 0|
38| 0|
39| 0| let _ = Firework { strength: 1000 };
40| 0|
41| 0| Ok(())
42| 1|}
43| |
44| |// Expected program output:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,23 @@
9| 1| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
10| 1| if true {
11| 1| if false {
12| | while true {
13| | }
12| 0| while true {
13| 0| }
14| 1| }
15| 1| write!(f, "error")?;
^0
16| | } else {
17| | }
15| 1| write!(f, "cool")?;
^0
16| 0| } else {
17| 0| }
18| |
19| 10| for i in 0..10 {
20| 10| if true {
21| 10| if false {
22| | while true {}
22| 0| while true {}
23| 10| }
24| 10| write!(f, "error")?;
^0
25| | } else {
26| | }
24| 10| write!(f, "cool")?;
^0
25| 0| } else {
26| 0| }
27| | }
28| 1| Ok(())
29| 1| }
Expand All @@ -36,21 +36,21 @@
34| |impl std::fmt::Display for DisplayTest {
35| 1| fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
36| 1| if false {
37| | } else {
37| 0| } else {
38| 1| if false {
39| | while true {}
39| 0| while true {}
40| 1| }
41| 1| write!(f, "error")?;
^0
41| 1| write!(f, "cool")?;
^0
42| | }
43| 10| for i in 0..10 {
44| 10| if false {
45| | } else {
45| 0| } else {
46| 10| if false {
47| | while true {}
47| 0| while true {}
48| 10| }
49| 10| write!(f, "error")?;
^0
49| 10| write!(f, "cool")?;
^0
50| | }
51| | }
52| 1| Ok(())
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
1| 1|fn main() {
2| 1| if false {
3| | loop {}
3| 0| loop {}
4| 1| }
5| 1|}

File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
3 changes: 3 additions & 0 deletions src/test/run-make/coverage/compiletest-ignore-dir
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Directory "coverage" supports the tests at prefix ../coverage-*

# Use ./x.py [options] test src/test/run-make/coverage to run all related tests.
Loading

0 comments on commit 5d5ba5a

Please sign in to comment.