From bea112ba07ec3da0de8de041bc3d510fe445b157 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Tue, 11 May 2021 12:47:08 -0700 Subject: [PATCH] Revert "Auto merge of #84797 - richkadel:cover-unreachable-statements, r=tmandry" This reverts commit e5f83d24aee866a14753a7cedbb4e301dfe5bef5, reversing changes made to ac888e8675182c703c2cd097957878faf88dad94. --- .../rustc_mir/src/transform/const_goto.rs | 2 +- .../src/transform/deduplicate_blocks.rs | 2 +- .../src/transform/early_otherwise_branch.rs | 2 +- compiler/rustc_mir/src/transform/generator.rs | 4 +- compiler/rustc_mir/src/transform/inline.rs | 2 +- .../rustc_mir/src/transform/match_branches.rs | 2 +- .../transform/multiple_return_terminators.rs | 2 +- .../src/transform/remove_unneeded_drops.rs | 2 +- compiler/rustc_mir/src/transform/simplify.rs | 42 +++---------------- .../rustc_mir/src/transform/simplify_try.rs | 2 +- .../src/transform/unreachable_prop.rs | 2 +- .../expected_show_coverage.async2.txt | 1 - .../expected_show_coverage.conditions.txt | 8 +--- .../expected_show_coverage.doctest.txt | 4 +- .../expected_show_coverage.drop_trait.txt | 10 ++--- .../expected_show_coverage.generics.txt | 18 ++++---- .../expected_show_coverage.loops_branches.txt | 38 ++++++++--------- .../expected_show_coverage.tight_inf_loop.txt | 2 +- .../run-make-fulldeps/coverage/conditions.rs | 4 +- .../run-make-fulldeps/coverage/generics.rs | 10 ++--- .../coverage/loops_branches.rs | 8 ++-- 21 files changed, 65 insertions(+), 102 deletions(-) diff --git a/compiler/rustc_mir/src/transform/const_goto.rs b/compiler/rustc_mir/src/transform/const_goto.rs index ba10b54c5ae2e..b5c8b4bebc360 100644 --- a/compiler/rustc_mir/src/transform/const_goto.rs +++ b/compiler/rustc_mir/src/transform/const_goto.rs @@ -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(tcx, body); + simplify_cfg(body); simplify_locals(body, tcx); } } diff --git a/compiler/rustc_mir/src/transform/deduplicate_blocks.rs b/compiler/rustc_mir/src/transform/deduplicate_blocks.rs index 912505c65983e..c41e71e09a4ef 100644 --- a/compiler/rustc_mir/src/transform/deduplicate_blocks.rs +++ b/compiler/rustc_mir/src/transform/deduplicate_blocks.rs @@ -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(tcx, body); + simplify_cfg(body); } } } diff --git a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs index ac39206623308..f7ea9faec4728 100644 --- a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs +++ b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs @@ -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(tcx, body); + simplify_cfg(body); } } } diff --git a/compiler/rustc_mir/src/transform/generator.rs b/compiler/rustc_mir/src/transform/generator.rs index 3560b4b1e8645..003003a8abbea 100644 --- a/compiler/rustc_mir/src/transform/generator.rs +++ b/compiler/rustc_mir/src/transform/generator.rs @@ -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(tcx, &mut body); + simplify::remove_dead_blocks(&mut body); dump_mir(tcx, None, "generator_drop", &0, &body, |_, _| Ok(())); @@ -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(tcx, body); + simplify::remove_dead_blocks(body); dump_mir(tcx, None, "generator_resume", &0, body, |_, _| Ok(())); } diff --git a/compiler/rustc_mir/src/transform/inline.rs b/compiler/rustc_mir/src/transform/inline.rs index f1c95a84ade85..b6f80763bc8c4 100644 --- a/compiler/rustc_mir/src/transform/inline.rs +++ b/compiler/rustc_mir/src/transform/inline.rs @@ -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(tcx, body); + remove_dead_blocks(body); } } } diff --git a/compiler/rustc_mir/src/transform/match_branches.rs b/compiler/rustc_mir/src/transform/match_branches.rs index 21b208a08c2dc..f7a9835353e5c 100644 --- a/compiler/rustc_mir/src/transform/match_branches.rs +++ b/compiler/rustc_mir/src/transform/match_branches.rs @@ -167,7 +167,7 @@ impl<'tcx> MirPass<'tcx> for MatchBranchSimplification { } if should_cleanup { - simplify_cfg(tcx, body); + simplify_cfg(body); } } } diff --git a/compiler/rustc_mir/src/transform/multiple_return_terminators.rs b/compiler/rustc_mir/src/transform/multiple_return_terminators.rs index cd2db18055286..4aaa0baa9f46a 100644 --- a/compiler/rustc_mir/src/transform/multiple_return_terminators.rs +++ b/compiler/rustc_mir/src/transform/multiple_return_terminators.rs @@ -38,6 +38,6 @@ impl<'tcx> MirPass<'tcx> for MultipleReturnTerminators { } } - simplify::remove_dead_blocks(tcx, body) + simplify::remove_dead_blocks(body) } } diff --git a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs index 02e45021a0aaf..5144d48750de7 100644 --- a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs +++ b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs @@ -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(tcx, body); + simplify_cfg(body); } } } diff --git a/compiler/rustc_mir/src/transform/simplify.rs b/compiler/rustc_mir/src/transform/simplify.rs index 63373b0cffbb0..65e2d096b2094 100644 --- a/compiler/rustc_mir/src/transform/simplify.rs +++ b/compiler/rustc_mir/src/transform/simplify.rs @@ -29,7 +29,6 @@ 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; @@ -47,9 +46,9 @@ impl SimplifyCfg { } } -pub fn simplify_cfg(tcx: TyCtxt<'tcx>, body: &mut Body<'_>) { +pub fn simplify_cfg(body: &mut Body<'_>) { CfgSimplifier::new(body).simplify(); - remove_dead_blocks(tcx, body); + remove_dead_blocks(body); // FIXME: Should probably be moved into some kind of pass manager body.basic_blocks_mut().raw.shrink_to_fit(); @@ -60,9 +59,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(tcx, body); + simplify_cfg(body); } } @@ -287,7 +286,7 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> { } } -pub fn remove_dead_blocks(tcx: TyCtxt<'tcx>, body: &mut Body<'_>) { +pub fn remove_dead_blocks(body: &mut Body<'_>) { let reachable = traversal::reachable_as_bitset(body); let num_blocks = body.basic_blocks().len(); if num_blocks == reachable.count() { @@ -307,11 +306,6 @@ pub fn remove_dead_blocks(tcx: TyCtxt<'tcx>, 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 { @@ -321,32 +315,6 @@ pub fn remove_dead_blocks(tcx: TyCtxt<'tcx>, body: &mut Body<'_>) { } } -fn save_unreachable_coverage( - basic_blocks: &mut IndexVec>, - 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 { diff --git a/compiler/rustc_mir/src/transform/simplify_try.rs b/compiler/rustc_mir/src/transform/simplify_try.rs index c9c4492062720..b42543c04eb3d 100644 --- a/compiler/rustc_mir/src/transform/simplify_try.rs +++ b/compiler/rustc_mir/src/transform/simplify_try.rs @@ -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(tcx, body); + simplify::remove_dead_blocks(body); } } } diff --git a/compiler/rustc_mir/src/transform/unreachable_prop.rs b/compiler/rustc_mir/src/transform/unreachable_prop.rs index e7fb6b4f6b4ad..658c6b6e9db20 100644 --- a/compiler/rustc_mir/src/transform/unreachable_prop.rs +++ b/compiler/rustc_mir/src/transform/unreachable_prop.rs @@ -60,7 +60,7 @@ impl MirPass<'_> for UnreachablePropagation { } if replaced { - simplify::remove_dead_blocks(tcx, body); + simplify::remove_dead_blocks(body); } } } diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.async2.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.async2.txt index dc06a485a8fc1..322f5681b3fd9 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.async2.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.async2.txt @@ -12,7 +12,6 @@ 12| 1| if b { 13| 1| println!("non_async_func println in block"); 14| 1| } - ^0 15| 1|} 16| | 17| | diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.conditions.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.conditions.txt index 2d8a98a5d0c92..656a26597759d 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.conditions.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.conditions.txt @@ -5,7 +5,6 @@ 5| 1| if true { 6| 1| countdown = 10; 7| 1| } - ^0 8| | 9| | const B: u32 = 100; 10| 1| let x = if countdown > 7 { @@ -25,7 +24,6 @@ 24| 1| if true { 25| 1| countdown = 10; 26| 1| } - ^0 27| | 28| 1| if countdown > 7 { 29| 1| countdown -= 4; @@ -44,7 +42,6 @@ 41| 1| if true { 42| 1| countdown = 10; 43| 1| } - ^0 44| | 45| 1| if countdown > 7 { 46| 1| countdown -= 4; @@ -57,14 +54,13 @@ 53| | } else { 54| 0| return; 55| | } - 56| 0| } - 57| | + 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. 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 diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt index 7ae0e978808e7..1b6bb9ff8891d 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.doctest.txt @@ -9,7 +9,7 @@ 8| 1|//! assert_eq!(1, 1); 9| |//! } else { 10| |//! // this is not! - 11| 0|//! assert_eq!(1, 2); + 11| |//! assert_eq!(1, 2); 12| |//! } 13| 1|//! ``` 14| |//! @@ -84,7 +84,7 @@ 74| 1| if true { 75| 1| assert_eq!(1, 1); 76| | } else { - 77| 0| assert_eq!(1, 2); + 77| | assert_eq!(1, 2); 78| | } 79| 1|} 80| | diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.drop_trait.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.drop_trait.txt index fe6a9e93cbf71..fab5be41901c9 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.drop_trait.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.drop_trait.txt @@ -19,11 +19,11 @@ 19| 1| if true { 20| 1| println!("Exiting with error..."); 21| 1| return Err(1); - 22| 0| } - 23| 0| - 24| 0| let _ = Firework { strength: 1000 }; - 25| 0| - 26| 0| Ok(()) + 22| | } + 23| | + 24| | let _ = Firework { strength: 1000 }; + 25| | + 26| | Ok(()) 27| 1|} 28| | 29| |// Expected program output: diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.generics.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.generics.txt index 8e8bc0fd18943..7b38ffb87cba8 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.generics.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.generics.txt @@ -52,15 +52,15 @@ 30| 1| if true { 31| 1| println!("Exiting with error..."); 32| 1| return Err(1); - 33| 0| } - 34| 0| - 35| 0| - 36| 0| - 37| 0| - 38| 0| - 39| 0| let _ = Firework { strength: 1000 }; - 40| 0| - 41| 0| Ok(()) + 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(()) 42| 1|} 43| | 44| |// Expected program output: diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.loops_branches.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.loops_branches.txt index 5d572db7cc60d..81d5c7d90346d 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.loops_branches.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.loops_branches.txt @@ -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| 0| while true { - 13| 0| } + 12| | while true { + 13| | } 14| 1| } - 15| 1| write!(f, "cool")?; - ^0 - 16| 0| } else { - 17| 0| } + 15| 1| write!(f, "error")?; + ^0 + 16| | } else { + 17| | } 18| | 19| 10| for i in 0..10 { 20| 10| if true { 21| 10| if false { - 22| 0| while true {} + 22| | while true {} 23| 10| } - 24| 10| write!(f, "cool")?; - ^0 - 25| 0| } else { - 26| 0| } + 24| 10| write!(f, "error")?; + ^0 + 25| | } else { + 26| | } 27| | } 28| 1| Ok(()) 29| 1| } @@ -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| 0| } else { + 37| | } else { 38| 1| if false { - 39| 0| while true {} + 39| | while true {} 40| 1| } - 41| 1| write!(f, "cool")?; - ^0 + 41| 1| write!(f, "error")?; + ^0 42| | } 43| 10| for i in 0..10 { 44| 10| if false { - 45| 0| } else { + 45| | } else { 46| 10| if false { - 47| 0| while true {} + 47| | while true {} 48| 10| } - 49| 10| write!(f, "cool")?; - ^0 + 49| 10| write!(f, "error")?; + ^0 50| | } 51| | } 52| 1| Ok(()) diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.tight_inf_loop.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.tight_inf_loop.txt index 2d4c57f451a2d..5adeef7d0850b 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.tight_inf_loop.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.tight_inf_loop.txt @@ -1,6 +1,6 @@ 1| 1|fn main() { 2| 1| if false { - 3| 0| loop {} + 3| | loop {} 4| 1| } 5| 1|} diff --git a/src/test/run-make-fulldeps/coverage/conditions.rs b/src/test/run-make-fulldeps/coverage/conditions.rs index 057599d1b471a..8a2a0b53e5862 100644 --- a/src/test/run-make-fulldeps/coverage/conditions.rs +++ b/src/test/run-make-fulldeps/coverage/conditions.rs @@ -53,8 +53,8 @@ fn main() { } else { return; } - } - + } // Note: closing brace shows uncovered (vs. `0` for implicit else) because condition literal + // `true` was const-evaluated. The compiler knows the `if` block will be executed. let mut countdown = 0; if true { diff --git a/src/test/run-make-fulldeps/coverage/generics.rs b/src/test/run-make-fulldeps/coverage/generics.rs index 18b38868496d4..cbeda35d3b8cf 100644 --- a/src/test/run-make-fulldeps/coverage/generics.rs +++ b/src/test/run-make-fulldeps/coverage/generics.rs @@ -30,11 +30,11 @@ fn main() -> Result<(),u8> { if true { println!("Exiting with error..."); return Err(1); - } - - - - + } // The remaining lines below have no coverage because `if true` (with the constant literal + // `true`) is guaranteed to execute the `then` block, which is also guaranteed to `return`. + // Thankfully, in the normal case, conditions are not guaranteed ahead of time, and as shown + // in other tests, the lines below would have coverage (which would show they had `0` + // executions, assuming the condition still evaluated to `true`). let _ = Firework { strength: 1000 }; diff --git a/src/test/run-make-fulldeps/coverage/loops_branches.rs b/src/test/run-make-fulldeps/coverage/loops_branches.rs index 7116ce47f4b9d..4d9bbad3367f6 100644 --- a/src/test/run-make-fulldeps/coverage/loops_branches.rs +++ b/src/test/run-make-fulldeps/coverage/loops_branches.rs @@ -12,7 +12,7 @@ impl std::fmt::Debug for DebugTest { while true { } } - write!(f, "cool")?; + write!(f, "error")?; } else { } @@ -21,7 +21,7 @@ impl std::fmt::Debug for DebugTest { if false { while true {} } - write!(f, "cool")?; + write!(f, "error")?; } else { } } @@ -38,7 +38,7 @@ impl std::fmt::Display for DisplayTest { if false { while true {} } - write!(f, "cool")?; + write!(f, "error")?; } for i in 0..10 { if false { @@ -46,7 +46,7 @@ impl std::fmt::Display for DisplayTest { if false { while true {} } - write!(f, "cool")?; + write!(f, "error")?; } } Ok(())