From f3eabbd98fa4c05ddb524d1644fb8b668f5895a4 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Sat, 1 May 2021 14:56:48 -0700 Subject: [PATCH 1/2] 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. --- .../rustc_codegen_ssa/src/coverageinfo/map.rs | 16 +++ .../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 | 97 ++++++++++++++++++- .../rustc_mir/src/transform/simplify_try.rs | 2 +- .../src/transform/unreachable_prop.rs | 6 +- .../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 +- 22 files changed, 177 insertions(+), 65 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs b/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs index 08442c588f879..85ea4cee03a82 100644 --- a/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs +++ b/compiler/rustc_codegen_ssa/src/coverageinfo/map.rs @@ -28,6 +28,7 @@ pub struct Expression { /// only whitespace or comments). According to LLVM Code Coverage Mapping documentation, "A count /// for a gap area is only used as the line execution count if there are no other regions on a /// line." +#[derive(Debug)] pub struct FunctionCoverage<'tcx> { instance: Instance<'tcx>, source_hash: u64, @@ -113,6 +114,14 @@ impl<'tcx> FunctionCoverage<'tcx> { expression_id, lhs, op, rhs, region ); let expression_index = self.expression_index(u32::from(expression_id)); + debug_assert!( + expression_index.as_usize() < self.expressions.len(), + "expression_index {} is out of range for expressions.len() = {} + for {:?}", + expression_index.as_usize(), + self.expressions.len(), + self, + ); if let Some(previous_expression) = self.expressions[expression_index].replace(Expression { lhs, op, @@ -150,6 +159,13 @@ impl<'tcx> FunctionCoverage<'tcx> { self.instance ); + if !self.counters.iter().any(|region| region.is_some()) { + // FIXME(richkadel): How is this possible and why is it OK? I verified that the first + // test program that triggers this still works, and `llvm-cov` does not fail, so I need + // to allow it. + debug!("no counters for {:?}", self); + } + let counter_regions = self.counter_regions(); let (counter_expressions, expression_regions) = self.expressions_with_regions(); let unreachable_regions = self.unreachable_regions(); diff --git a/compiler/rustc_mir/src/transform/const_goto.rs b/compiler/rustc_mir/src/transform/const_goto.rs index b5c8b4bebc360..ba10b54c5ae2e 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(body); + simplify_cfg(tcx, 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 c41e71e09a4ef..912505c65983e 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(body); + simplify_cfg(tcx, body); } } } diff --git a/compiler/rustc_mir/src/transform/early_otherwise_branch.rs b/compiler/rustc_mir/src/transform/early_otherwise_branch.rs index 7934d4ba8499c..07127042fa41e 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(body); + simplify_cfg(tcx, body); } } } diff --git a/compiler/rustc_mir/src/transform/generator.rs b/compiler/rustc_mir/src/transform/generator.rs index 003003a8abbea..642e726ccdf62 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(&mut body); + simplify::remove_dead_blocks_with_coverage(tcx, &mut body, simplify::DroppedCoverage::Allow); 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(body); + simplify::remove_dead_blocks_with_coverage(tcx, body, simplify::DroppedCoverage::Allow); 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 b6f80763bc8c4..f1c95a84ade85 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(body); + remove_dead_blocks(tcx, body); } } } diff --git a/compiler/rustc_mir/src/transform/match_branches.rs b/compiler/rustc_mir/src/transform/match_branches.rs index f7a9835353e5c..21b208a08c2dc 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(body); + simplify_cfg(tcx, body); } } } diff --git a/compiler/rustc_mir/src/transform/multiple_return_terminators.rs b/compiler/rustc_mir/src/transform/multiple_return_terminators.rs index 4aaa0baa9f46a..cd2db18055286 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(body) + simplify::remove_dead_blocks(tcx, body) } } diff --git a/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs b/compiler/rustc_mir/src/transform/remove_unneeded_drops.rs index 5144d48750de7..02e45021a0aaf 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(body); + simplify_cfg(tcx, body); } } } diff --git a/compiler/rustc_mir/src/transform/simplify.rs b/compiler/rustc_mir/src/transform/simplify.rs index 65e2d096b2094..56f252da22808 100644 --- a/compiler/rustc_mir/src/transform/simplify.rs +++ b/compiler/rustc_mir/src/transform/simplify.rs @@ -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; @@ -46,9 +47,17 @@ impl SimplifyCfg { } } -pub fn simplify_cfg(body: &mut Body<'_>) { +pub fn simplify_cfg(tcx: TyCtxt<'tcx>, body: &mut Body<'_>) { + simplify_cfg_with_coverage(tcx, body, DroppedCoverage::AssertNone); +} + +pub fn simplify_cfg_with_coverage( + tcx: TyCtxt<'tcx>, + body: &mut Body<'_>, + dropped_coverage: DroppedCoverage, +) { CfgSimplifier::new(body).simplify(); - remove_dead_blocks(body); + remove_dead_blocks_with_coverage(tcx, body, dropped_coverage); // FIXME: Should probably be moved into some kind of pass manager body.basic_blocks_mut().raw.shrink_to_fit(); @@ -59,12 +68,31 @@ 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_with_coverage(tcx, body, DroppedCoverage::SaveUnreachable); } } +/// How to handle `Coverage` statements in dropped `BasicBlocks`. +pub enum DroppedCoverage { + /// Allow the `Coverage` statement(s) in a dropped `BasicBlock` to be + /// dropped as well. (This may be because the `Coverage` statement was + /// already cloned.) + Allow, + + /// Assert that there are none. The current or most recent MIR pass(es) + /// should not have done anything that would have resulted in an + /// unreachable block with a `Coverage` statement. + AssertNone, + + /// Assume the `Coverage` statement(s) in a dropped `BacicBlock` are part + /// of a dead code region. Save them in the `START_BLOCK` but mark `Counter` + /// coverage as `is_dead` so they are not incremented. (Expressions from + /// dead blocks should already add up to zero.) + SaveUnreachable, +} + pub struct CfgSimplifier<'a, 'tcx> { basic_blocks: &'a mut IndexVec>, pred_count: IndexVec, @@ -286,7 +314,15 @@ 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<'_>) { + remove_dead_blocks_with_coverage(tcx, body, DroppedCoverage::AssertNone); +} + +pub fn remove_dead_blocks_with_coverage( + tcx: TyCtxt<'tcx>, + body: &mut Body<'_>, + dropped_coverage: DroppedCoverage, +) { let reachable = traversal::reachable_as_bitset(body); let num_blocks = body.basic_blocks().len(); if num_blocks == reachable.count() { @@ -306,6 +342,16 @@ pub fn remove_dead_blocks(body: &mut Body<'_>) { } used_blocks += 1; } + + if tcx.sess.instrument_coverage() { + use DroppedCoverage::*; + match dropped_coverage { + Allow => {} + AssertNone => assert_no_unreachable_coverage(basic_blocks, used_blocks), + SaveUnreachable => save_unreachable_coverage(basic_blocks, used_blocks), + } + } + basic_blocks.raw.truncate(used_blocks); for block in basic_blocks { @@ -315,6 +361,47 @@ pub fn remove_dead_blocks(body: &mut Body<'_>) { } } +fn assert_no_unreachable_coverage( + basic_blocks: &mut IndexVec>, + first_dead_block: usize, +) { + for dead_block in first_dead_block..basic_blocks.len() { + for statement in &basic_blocks[BasicBlock::new(dead_block)].statements { + assert!( + !matches!(statement.kind, StatementKind::Coverage(..)), + "Dead blocks should not have `Coverage` statements at this stage." + ); + } + } +} + +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 89fddc95c98f7..dd2ec39c066ab 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(body); + simplify::remove_dead_blocks(tcx, body); } } } diff --git a/compiler/rustc_mir/src/transform/unreachable_prop.rs b/compiler/rustc_mir/src/transform/unreachable_prop.rs index 658c6b6e9db20..16400ddaba9fa 100644 --- a/compiler/rustc_mir/src/transform/unreachable_prop.rs +++ b/compiler/rustc_mir/src/transform/unreachable_prop.rs @@ -60,7 +60,11 @@ impl MirPass<'_> for UnreachablePropagation { } if replaced { - simplify::remove_dead_blocks(body); + simplify::remove_dead_blocks_with_coverage( + tcx, + body, + simplify::DroppedCoverage::SaveUnreachable, + ); } } } 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 322f5681b3fd9..dc06a485a8fc1 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,6 +12,7 @@ 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 656a26597759d..2d8a98a5d0c92 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,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 { @@ -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; @@ -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; @@ -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 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 1b6bb9ff8891d..7ae0e978808e7 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| |//! assert_eq!(1, 2); + 11| 0|//! 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| | assert_eq!(1, 2); + 77| 0| 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 fab5be41901c9..fe6a9e93cbf71 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| | } - 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: 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 7b38ffb87cba8..8e8bc0fd18943 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| | } // 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: 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 81d5c7d90346d..5d572db7cc60d 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| | 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| } @@ -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(()) 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 5adeef7d0850b..2d4c57f451a2d 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| | loop {} + 3| 0| 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 8a2a0b53e5862..057599d1b471a 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 cbeda35d3b8cf..18b38868496d4 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 4d9bbad3367f6..7116ce47f4b9d 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, "error")?; + write!(f, "cool")?; } else { } @@ -21,7 +21,7 @@ impl std::fmt::Debug for DebugTest { if false { while true {} } - write!(f, "error")?; + write!(f, "cool")?; } else { } } @@ -38,7 +38,7 @@ impl std::fmt::Display for DisplayTest { if false { while true {} } - write!(f, "error")?; + write!(f, "cool")?; } for i in 0..10 { if false { @@ -46,7 +46,7 @@ impl std::fmt::Display for DisplayTest { if false { while true {} } - write!(f, "error")?; + write!(f, "cool")?; } } Ok(()) From 1d7b248cd08a3bcc372b395e56a8dc9813217556 Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Tue, 11 May 2021 01:31:49 -0700 Subject: [PATCH 2/2] fix coverage in attr macro spans Such as #[fuchsia_async::run_singlethreaded] Note the spanview output seems to show good coverage spans, but when running coverage reports, the fuchsia_async functions showed `0` for coverage of tests that ran once, and `llvm-cov` reports a Malformed Coverage error. --- .../rustc_mir/src/transform/coverage/mod.rs | 103 ++++++++++++++++-- .../rustc_mir/src/transform/coverage/spans.rs | 18 +++ 2 files changed, 112 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_mir/src/transform/coverage/mod.rs b/compiler/rustc_mir/src/transform/coverage/mod.rs index 71c244fdd4a3a..a44e28a8ab54c 100644 --- a/compiler/rustc_mir/src/transform/coverage/mod.rs +++ b/compiler/rustc_mir/src/transform/coverage/mod.rs @@ -32,7 +32,7 @@ use rustc_middle::mir::{ use rustc_middle::ty::TyCtxt; use rustc_span::def_id::DefId; use rustc_span::source_map::SourceMap; -use rustc_span::{CharPos, ExpnKind, Pos, SourceFile, Span, Symbol}; +use rustc_span::{CharPos, ExpnData, ExpnKind, Pos, SourceFile, Span, Symbol}; /// A simple error message wrapper for `coverage::Error`s. #[derive(Debug)] @@ -552,24 +552,109 @@ fn get_body_span<'tcx>( let mut body_span = hir_body.value.span; let def_id = mir_body.source.def_id(); - if tcx.is_closure(def_id) { + // If debug is enabled, log expansion span info, if any. + debug!("{}", { + let mut span = body_span; + while span.from_expansion() { + let ExpnData { kind, call_site, .. } = span.ctxt().outer_expn_data(); + debug!( + "Body expansion span: {:?}... + expanded from {:?} + at call_site={:?}", + span, kind, call_site + ); + span = call_site; + } + "-- end of debug message showing body_span expansion --" + }); + + if body_span.in_derive_expansion() { + // Derive macro expansions can be expanded, but they expand to structs + // and struct fields, and I think it's less helpful in coverage reports. + return body_span; + } + + let mut body_is_from_expansion = false; + if !tcx.is_closure(def_id) { + body_is_from_expansion = body_span.from_expansion(); + } else { // If the MIR function is a closure, and if the closure body span // starts from a macro, but it's content is not in that macro, try // to find a non-macro callsite, and instrument the spans there // instead. - loop { + while body_span.from_expansion() { + body_is_from_expansion = true; let expn_data = body_span.ctxt().outer_expn_data(); - if expn_data.is_root() { - break; + body_span = expn_data.call_site; + match expn_data.kind { + ExpnKind::Macro { .. } => {} + _ => break, + } + } + }; + + if !body_is_from_expansion { + // If the body_span (or recomputed body_span, for closures with macros) + // is not from an expansion, then there is no need to try to compute + // a span from the MIR statements and terminators. + return body_span; + } + + // If the body is from an expansion, then there's no certainty that the + // internal statements were also expanded into the same ctxt or elsewhere. + // So, search the MIR `Statement` and `Terminator` spans, and if their + // spans exist ouside the `body_span` compute a different `body_span` + // from the spans of the MIR's `Statement`s and `Terminator`s, so + // coverage can be applied to those spans. + + if let Ok(Some(max_span)) = spans::filtered_from_mir(mir_body).try_fold( + None, + |some_max_span: Option, mut span| { + if span.is_empty() || span == body_span { + // A span that matches the body_span doesn't add anything, and both spans that match + // the body_span, and empty spans can exist in the given body_span, even if the + // statements we want to cover are from a separate and distinct span, so we need + // to ignore them. If no other statements exist outside the body_span, then we will + // keep the given body_span. + return Ok(some_max_span); // continue iterating + } + + while span.ctxt() != body_span.ctxt() { + if !span.from_expansion() { + return Ok(some_max_span); // continue iterating + } + span = span.ctxt().outer_expn_data().call_site; + } + + if body_span.contains(span) { + debug!( + "using body_span={:?}; it contains a statement or terminator with span={:?}", + body_span, span + ); + return Err(()); // break from iterating } - if let ExpnKind::Macro { .. } = expn_data.kind { - body_span = expn_data.call_site; + + if let Some(max_span) = some_max_span { + Ok(Some(max_span.to(span))) // extend max_span } else { - break; + Ok(Some(span)) // initialize max_span to first useful span } + }, + ) { + debug!( + "max_span = {:?}; overlaps body_span? {}, body_span hi() only touches? {}", + max_span, + body_span.overlaps(max_span), + body_span.hi() == max_span.lo() + ); + if body_span.overlaps(max_span) { + // extend the body_span to include the computed max_span + body_span = body_span.to(max_span); + } else { + // use the computed max_span instead of the original body_span + body_span = max_span; } } - body_span } diff --git a/compiler/rustc_mir/src/transform/coverage/spans.rs b/compiler/rustc_mir/src/transform/coverage/spans.rs index f62171b3c535c..da896b72d37e3 100644 --- a/compiler/rustc_mir/src/transform/coverage/spans.rs +++ b/compiler/rustc_mir/src/transform/coverage/spans.rs @@ -1,5 +1,6 @@ use super::debug::term_type; use super::graph::{BasicCoverageBlock, BasicCoverageBlockData, CoverageGraph, START_BCB}; +use super::spans; use crate::util::spanview::source_range_no_file; @@ -889,6 +890,23 @@ pub(super) fn filtered_terminator_span(terminator: &'a Terminator<'tcx>) -> Opti } } +/// Returns an iterator over all filtered statement and terminator spans. +pub(super) fn filtered_from_mir<'a>( + mir_body: &'a mir::Body<'a>, +) -> impl Iterator + 'a { + mir_body.basic_blocks().iter().flat_map(|data| { + data.statements + .iter() + .enumerate() + .filter_map(move |(_, statement)| spans::filtered_statement_span(statement)) + .chain( + data.terminator + .iter() + .filter_map(move |term| spans::filtered_terminator_span(term)), + ) + }) +} + /// Returns an extrapolated span (pre-expansion[^1]) corresponding to a range /// within the function's body source. This span is guaranteed to be contained /// within, or equal to, the `body_span`. If the extrapolated span is not