From 448e52d97c9b76600fd1ec29c8159391329e48ab Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Sun, 18 Apr 2021 02:55:17 -0700 Subject: [PATCH 1/2] Add coverage to continue statements `continue` statements were missing coverage. This was particularly noticeable in a match pattern that contained only a `continue` statement, leaving the branch appear uncounted. This PR addresses the problem and adds tests to prove it. --- compiler/rustc_mir_build/src/build/scope.rs | 10 +++ ....main.SimplifyCfg-promote-consts.after.mir | 4 + .../expected_show_coverage.continue.txt | 75 +++++++++++++++++++ .../expected_show_coverage.uses_crate.txt | 4 +- .../run-make-fulldeps/coverage/continue.rs | 69 +++++++++++++++++ 5 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.continue.txt create mode 100644 src/test/run-make-fulldeps/coverage/continue.rs diff --git a/compiler/rustc_mir_build/src/build/scope.rs b/compiler/rustc_mir_build/src/build/scope.rs index b637b9b70bd..2727e70f278 100644 --- a/compiler/rustc_mir_build/src/build/scope.rs +++ b/compiler/rustc_mir_build/src/build/scope.rs @@ -618,6 +618,16 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } else { assert!(value.is_none(), "`return` and `break` should have a destination"); + // `continue` statements generate no MIR statement with the `continue` statement's Span, + // and the `InstrumentCoverage` statement will have no way to generate a coverage + // code region for the `continue` statement, unless we add a dummy `Assign` here: + let mut local_decl = LocalDecl::new(self.tcx.mk_unit(), span); + local_decl = local_decl.immutable(); + let temp = self.local_decls.push(local_decl); + let temp_place = Place::from(temp); + self.cfg.push(block, Statement { source_info, kind: StatementKind::StorageLive(temp) }); + self.cfg.push_assign_unit(block, source_info, temp_place, self.tcx); + self.cfg.push(block, Statement { source_info, kind: StatementKind::StorageDead(temp) }); } let region_scope = self.scopes.breakable_scopes[break_index].region_scope; diff --git a/src/test/mir-opt/loop_test.main.SimplifyCfg-promote-consts.after.mir b/src/test/mir-opt/loop_test.main.SimplifyCfg-promote-consts.after.mir index 99c7ac8d5b7..f6120447017 100644 --- a/src/test/mir-opt/loop_test.main.SimplifyCfg-promote-consts.after.mir +++ b/src/test/mir-opt/loop_test.main.SimplifyCfg-promote-consts.after.mir @@ -8,6 +8,7 @@ fn main() -> () { let mut _4: !; // in scope 0 at $DIR/loop_test.rs:13:5: 16:6 let mut _5: (); // in scope 0 at $DIR/loop_test.rs:6:1: 17:2 let _6: i32; // in scope 0 at $DIR/loop_test.rs:14:13: 14:14 + let _7: (); // in scope 0 at $DIR/loop_test.rs:15:9: 15:17 scope 1 { debug x => _6; // in scope 1 at $DIR/loop_test.rs:14:13: 14:14 } @@ -42,6 +43,9 @@ fn main() -> () { StorageLive(_6); // scope 0 at $DIR/loop_test.rs:14:13: 14:14 _6 = const 1_i32; // scope 0 at $DIR/loop_test.rs:14:17: 14:18 FakeRead(ForLet(None), _6); // scope 0 at $DIR/loop_test.rs:14:13: 14:14 + StorageLive(_7); // scope 1 at $DIR/loop_test.rs:15:9: 15:17 + _7 = const (); // scope 1 at $DIR/loop_test.rs:15:9: 15:17 + StorageDead(_7); // scope 1 at $DIR/loop_test.rs:15:9: 15:17 StorageDead(_6); // scope 0 at $DIR/loop_test.rs:16:5: 16:6 goto -> bb3; // scope 0 at $DIR/loop_test.rs:1:1: 1:1 } diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.continue.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.continue.txt new file mode 100644 index 00000000000..28e0f1953e0 --- /dev/null +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.continue.txt @@ -0,0 +1,75 @@ + 1| |#![allow(unused_assignments, unused_variables)] + 2| | + 3| 1|fn main() { + 4| 1| let is_true = std::env::args().len() == 1; + 5| 1| + 6| 1| let mut x = 0; + 7| 11| for _ in 0..10 { + ^10 + 8| 10| match is_true { + 9| | true => { + 10| 10| continue; + 11| | } + 12| 0| _ => { + 13| 0| x = 1; + 14| 0| } + 15| 0| } + 16| 0| x = 3; + 17| | } + 18| 11| for _ in 0..10 { + ^10 + 19| 10| match is_true { + 20| 0| false => { + 21| 0| x = 1; + 22| 0| } + 23| | _ => { + 24| 10| continue; + 25| | } + 26| | } + 27| 0| x = 3; + 28| | } + 29| 11| for _ in 0..10 { + ^10 + 30| 10| match is_true { + 31| 10| true => { + 32| 10| x = 1; + 33| 10| } + 34| | _ => { + 35| 0| continue; + 36| | } + 37| | } + 38| 10| x = 3; + 39| | } + 40| 11| for _ in 0..10 { + ^10 + 41| 10| if is_true { + 42| 10| continue; + 43| 0| } + 44| 0| x = 3; + 45| | } + 46| 11| for _ in 0..10 { + ^10 + 47| 10| match is_true { + 48| 0| false => { + 49| 0| x = 1; + 50| 0| } + 51| 10| _ => { + 52| 10| let _ = x; + 53| 10| } + 54| | } + 55| 10| x = 3; + 56| | } + 57| 1| for _ in 0..10 { + 58| 1| match is_true { + 59| 0| false => { + 60| 0| x = 1; + 61| 0| } + 62| | _ => { + 63| 1| break; + 64| | } + 65| | } + 66| 0| x = 3; + 67| | } + 68| | let _ = x; + 69| 1|} + diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt index cdcbd8fca94..f5beb9ef24a 100644 --- a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt +++ b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.uses_crate.txt @@ -19,12 +19,12 @@ 18| 2| println!("used_only_from_bin_crate_generic_function with {:?}", arg); 19| 2|} ------------------ - | used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec>: + | used_crate::used_only_from_bin_crate_generic_function::<&str>: | 17| 1|pub fn used_only_from_bin_crate_generic_function(arg: T) { | 18| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg); | 19| 1|} ------------------ - | used_crate::used_only_from_bin_crate_generic_function::<&str>: + | used_crate::used_only_from_bin_crate_generic_function::<&alloc::vec::Vec>: | 17| 1|pub fn used_only_from_bin_crate_generic_function(arg: T) { | 18| 1| println!("used_only_from_bin_crate_generic_function with {:?}", arg); | 19| 1|} diff --git a/src/test/run-make-fulldeps/coverage/continue.rs b/src/test/run-make-fulldeps/coverage/continue.rs new file mode 100644 index 00000000000..624aa98341b --- /dev/null +++ b/src/test/run-make-fulldeps/coverage/continue.rs @@ -0,0 +1,69 @@ +#![allow(unused_assignments, unused_variables)] + +fn main() { + let is_true = std::env::args().len() == 1; + + let mut x = 0; + for _ in 0..10 { + match is_true { + true => { + continue; + } + _ => { + x = 1; + } + } + x = 3; + } + for _ in 0..10 { + match is_true { + false => { + x = 1; + } + _ => { + continue; + } + } + x = 3; + } + for _ in 0..10 { + match is_true { + true => { + x = 1; + } + _ => { + continue; + } + } + x = 3; + } + for _ in 0..10 { + if is_true { + continue; + } + x = 3; + } + for _ in 0..10 { + match is_true { + false => { + x = 1; + } + _ => { + let _ = x; + } + } + x = 3; + } + for _ in 0..10 { + match is_true { + false => { + x = 1; + } + _ => { + break; + } + } + x = 3; + } + let _ = x; +} From d1d7fb1ae53a02f5073a7cf0ab1497aaa9617ebe Mon Sep 17 00:00:00 2001 From: Rich Kadel Date: Sun, 18 Apr 2021 11:51:42 -0700 Subject: [PATCH 2/2] Only generate dummy assign when instrumenting coverage And make the LocalDecl internal, to avoid needing to declare storage. (For multiple `continue` stateuemtns, it must also be mutable.) --- compiler/rustc_mir_build/src/build/scope.rs | 25 +++++++++++-------- ....main.SimplifyCfg-promote-consts.after.mir | 4 --- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/scope.rs b/compiler/rustc_mir_build/src/build/scope.rs index 2727e70f278..41fc925c049 100644 --- a/compiler/rustc_mir_build/src/build/scope.rs +++ b/compiler/rustc_mir_build/src/build/scope.rs @@ -618,16 +618,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } else { assert!(value.is_none(), "`return` and `break` should have a destination"); - // `continue` statements generate no MIR statement with the `continue` statement's Span, - // and the `InstrumentCoverage` statement will have no way to generate a coverage - // code region for the `continue` statement, unless we add a dummy `Assign` here: - let mut local_decl = LocalDecl::new(self.tcx.mk_unit(), span); - local_decl = local_decl.immutable(); - let temp = self.local_decls.push(local_decl); - let temp_place = Place::from(temp); - self.cfg.push(block, Statement { source_info, kind: StatementKind::StorageLive(temp) }); - self.cfg.push_assign_unit(block, source_info, temp_place, self.tcx); - self.cfg.push(block, Statement { source_info, kind: StatementKind::StorageDead(temp) }); + if self.tcx.sess.instrument_coverage() { + // Unlike `break` and `return`, which push an `Assign` statement to MIR, from which + // a Coverage code region can be generated, `continue` needs no `Assign`; but + // without one, the `InstrumentCoverage` MIR pass cannot generate a code region for + // `continue`. Coverage will be missing unless we add a dummy `Assign` to MIR. + self.add_dummy_assignment(&span, block, source_info); + } } let region_scope = self.scopes.breakable_scopes[break_index].region_scope; @@ -653,6 +650,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { self.cfg.start_new_block().unit() } + // Add a dummy `Assign` statement to the CFG, with the span for the source code's `continue` + // statement. + fn add_dummy_assignment(&mut self, span: &Span, block: BasicBlock, source_info: SourceInfo) { + let local_decl = LocalDecl::new(self.tcx.mk_unit(), *span).internal(); + let temp_place = Place::from(self.local_decls.push(local_decl)); + self.cfg.push_assign_unit(block, source_info, temp_place, self.tcx); + } + crate fn exit_top_scope( &mut self, mut block: BasicBlock, diff --git a/src/test/mir-opt/loop_test.main.SimplifyCfg-promote-consts.after.mir b/src/test/mir-opt/loop_test.main.SimplifyCfg-promote-consts.after.mir index f6120447017..99c7ac8d5b7 100644 --- a/src/test/mir-opt/loop_test.main.SimplifyCfg-promote-consts.after.mir +++ b/src/test/mir-opt/loop_test.main.SimplifyCfg-promote-consts.after.mir @@ -8,7 +8,6 @@ fn main() -> () { let mut _4: !; // in scope 0 at $DIR/loop_test.rs:13:5: 16:6 let mut _5: (); // in scope 0 at $DIR/loop_test.rs:6:1: 17:2 let _6: i32; // in scope 0 at $DIR/loop_test.rs:14:13: 14:14 - let _7: (); // in scope 0 at $DIR/loop_test.rs:15:9: 15:17 scope 1 { debug x => _6; // in scope 1 at $DIR/loop_test.rs:14:13: 14:14 } @@ -43,9 +42,6 @@ fn main() -> () { StorageLive(_6); // scope 0 at $DIR/loop_test.rs:14:13: 14:14 _6 = const 1_i32; // scope 0 at $DIR/loop_test.rs:14:17: 14:18 FakeRead(ForLet(None), _6); // scope 0 at $DIR/loop_test.rs:14:13: 14:14 - StorageLive(_7); // scope 1 at $DIR/loop_test.rs:15:9: 15:17 - _7 = const (); // scope 1 at $DIR/loop_test.rs:15:9: 15:17 - StorageDead(_7); // scope 1 at $DIR/loop_test.rs:15:9: 15:17 StorageDead(_6); // scope 0 at $DIR/loop_test.rs:16:5: 16:6 goto -> bb3; // scope 0 at $DIR/loop_test.rs:1:1: 1:1 }