From b86e6755b982bafc36133d9016e7d5a4cd0de0e8 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sat, 15 Jun 2019 19:55:21 +0100 Subject: [PATCH] Add StorageDead statements for `while` conditions --- src/librustc_mir/build/expr/into.rs | 17 +--- src/librustc_mir/build/matches/mod.rs | 49 +++------- src/librustc_mir/build/scope.rs | 125 ++++++++++++++---------- src/test/mir-opt/match-arm-scopes.rs | 16 +-- src/test/mir-opt/match_false_edges.rs | 16 +-- src/test/mir-opt/match_test.rs | 2 +- src/test/mir-opt/remove_fake_borrows.rs | 16 +-- src/test/mir-opt/while-storage.rs | 59 +++++++++++ 8 files changed, 172 insertions(+), 128 deletions(-) create mode 100644 src/test/mir-opt/while-storage.rs diff --git a/src/librustc_mir/build/expr/into.rs b/src/librustc_mir/build/expr/into.rs index dc74466e63309..0a2ea78bfd7ab 100644 --- a/src/librustc_mir/build/expr/into.rs +++ b/src/librustc_mir/build/expr/into.rs @@ -179,19 +179,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // conduct the test, if necessary let body_block; if let Some(cond_expr) = opt_cond_expr { - let loop_block_end; - let cond = unpack!( - loop_block_end = this.as_local_operand(loop_block, cond_expr) - ); - body_block = this.cfg.start_new_block(); - let false_block = this.cfg.start_new_block(); - let term = TerminatorKind::if_( - this.hir.tcx(), - cond, - body_block, - false_block, - ); - this.cfg.terminate(loop_block_end, source_info, term); + let cond_expr = this.hir.mirror(cond_expr); + let (true_block, false_block) + = this.test_bool(loop_block, cond_expr, source_info); + body_block = true_block; // if the test is false, there's no `break` to assign `destination`, so // we have to do it diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index 5a0f5a01a04aa..cebfa681b5c47 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -1490,7 +1490,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { }; let source_info = self.source_info(guard.span); let guard_end = self.source_info(tcx.sess.source_map().end_point(guard.span)); - let cond = unpack!(block = self.as_local_operand(block, guard)); + let (post_guard_block, otherwise_post_guard_block) + = self.test_bool(block, guard, source_info); let guard_frame = self.guard_context.pop().unwrap(); debug!( "Exiting guard building context with locals: {:?}", @@ -1498,7 +1499,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ); for &(_, temp) in fake_borrows { - self.cfg.push(block, Statement { + self.cfg.push(post_guard_block, Statement { source_info: guard_end, kind: StatementKind::FakeRead( FakeReadCause::ForMatchGuard, @@ -1507,6 +1508,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { }); } + self.exit_scope( + source_info.span, + region_scope, + otherwise_post_guard_block, + candidate.otherwise_block.unwrap(), + ); + // We want to ensure that the matched candidates are bound // after we have confirmed this candidate *and* any // associated guard; Binding them on `block` is too soon, @@ -1533,41 +1541,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // ``` // // and that is clearly not correct. - let post_guard_block = self.cfg.start_new_block(); - let otherwise_post_guard_block = self.cfg.start_new_block(); - self.cfg.terminate( - block, - source_info, - TerminatorKind::if_( - self.hir.tcx(), - cond.clone(), - post_guard_block, - otherwise_post_guard_block, - ), - ); - - self.exit_scope( - source_info.span, - region_scope, - otherwise_post_guard_block, - candidate.otherwise_block.unwrap(), - ); - - if let Operand::Copy(cond_place) | Operand::Move(cond_place) = cond { - if let Place::Base(PlaceBase::Local(cond_temp)) = cond_place { - // We will call `clear_top_scope` if there's another guard. So - // we have to drop this variable now or it will be "storage - // leaked". - self.pop_variable( - post_guard_block, - region_scope.0, - cond_temp - ); - } else { - bug!("Expected as_local_operand to produce a temporary"); - } - } - let by_value_bindings = candidate.bindings.iter().filter(|binding| { if let BindingMode::ByValue = binding.binding_mode { true } else { false } }); @@ -1577,7 +1550,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let local_id = self.var_local_id(binding.var_id, RefWithinGuard); let place = Place::from(local_id); self.cfg.push( - block, + post_guard_block, Statement { source_info: guard_end, kind: StatementKind::FakeRead(FakeReadCause::ForGuardBinding, place), diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index 32b69082557b5..ec2e970906c79 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -83,7 +83,7 @@ should go to. */ use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder, CFG}; -use crate::hair::{ExprRef, LintLevel}; +use crate::hair::{Expr, ExprRef, LintLevel}; use rustc::middle::region; use rustc::ty::Ty; use rustc::hir; @@ -829,6 +829,78 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // Other // ===== + /// Branch based on a boolean condition. + /// + /// This is a special case because the temporary for the condition needs to + /// be dropped on both the true and the false arm. + pub fn test_bool( + &mut self, + mut block: BasicBlock, + condition: Expr<'tcx>, + source_info: SourceInfo, + ) -> (BasicBlock, BasicBlock) { + let cond = unpack!(block = self.as_local_operand(block, condition)); + let true_block = self.cfg.start_new_block(); + let false_block = self.cfg.start_new_block(); + let term = TerminatorKind::if_( + self.hir.tcx(), + cond.clone(), + true_block, + false_block, + ); + self.cfg.terminate(block, source_info, term); + + match cond { + // Don't try to drop a constant + Operand::Constant(_) => (), + // If constants and statics, we don't generate StorageLive for this + // temporary, so don't try to generate StorageDead for it either. + _ if self.local_scope().is_none() => (), + Operand::Copy(Place::Base(PlaceBase::Local(cond_temp))) + | Operand::Move(Place::Base(PlaceBase::Local(cond_temp))) => { + // Manually drop the condition on both branches. + let top_scope = self.scopes.scopes.last_mut().unwrap(); + let top_drop_data = top_scope.drops.pop().unwrap(); + + match top_drop_data.kind { + DropKind::Value { .. } => { + bug!("Drop scheduled on top of condition variable") + } + DropKind::Storage => { + // Drop the storage for both value and storage drops. + // Only temps and vars need their storage dead. + match top_drop_data.location { + Place::Base(PlaceBase::Local(index)) => { + let source_info = top_scope.source_info(top_drop_data.span); + assert_eq!(index, cond_temp, "Drop scheduled on top of condition"); + self.cfg.push( + true_block, + Statement { + source_info, + kind: StatementKind::StorageDead(index) + }, + ); + self.cfg.push( + false_block, + Statement { + source_info, + kind: StatementKind::StorageDead(index) + }, + ); + } + _ => unreachable!(), + } + } + } + + top_scope.invalidate_cache(true, self.is_generator, true); + } + _ => bug!("Expected as_local_operand to produce a temporary"), + } + + (true_block, false_block) + } + /// Creates a path that performs all required cleanup for unwinding. /// /// This path terminates in Resume. Returns the start of the path. @@ -942,57 +1014,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { top_scope.drops.clear(); top_scope.invalidate_cache(false, self.is_generator, true); } - - /// Drops the single variable provided - /// - /// * The scope must be the top scope. - /// * The variable must be in that scope. - /// * The variable must be at the top of that scope: it's the next thing - /// scheduled to drop. - /// * The drop must be of `DropKind::Storage`. - /// - /// This is used for the boolean holding the result of the match guard. We - /// do this because: - /// - /// * The boolean is different for each pattern - /// * There is only one exit for the arm scope - /// * The guard expression scope is too short, it ends just before the - /// boolean is tested. - pub(crate) fn pop_variable( - &mut self, - block: BasicBlock, - region_scope: region::Scope, - variable: Local, - ) { - let top_scope = self.scopes.scopes.last_mut().unwrap(); - - assert_eq!(top_scope.region_scope, region_scope); - - let top_drop_data = top_scope.drops.pop().unwrap(); - - match top_drop_data.kind { - DropKind::Value { .. } => { - bug!("Should not be calling pop_top_variable on non-copy type!") - } - DropKind::Storage => { - // Drop the storage for both value and storage drops. - // Only temps and vars need their storage dead. - match top_drop_data.location { - Place::Base(PlaceBase::Local(index)) => { - let source_info = top_scope.source_info(top_drop_data.span); - assert_eq!(index, variable); - self.cfg.push(block, Statement { - source_info, - kind: StatementKind::StorageDead(index) - }); - } - _ => unreachable!(), - } - } - } - - top_scope.invalidate_cache(true, self.is_generator, true); - } } /// Builds drops for pop_scope and exit_scope. diff --git a/src/test/mir-opt/match-arm-scopes.rs b/src/test/mir-opt/match-arm-scopes.rs index a2bc238c68ad9..18e0642eb3427 100644 --- a/src/test/mir-opt/match-arm-scopes.rs +++ b/src/test/mir-opt/match-arm-scopes.rs @@ -103,10 +103,6 @@ fn main() { // bb10: { // `else` block - first time // _9 = (*_6); // StorageDead(_10); -// FakeRead(ForMatchGuard, _3); -// FakeRead(ForMatchGuard, _4); -// FakeRead(ForGuardBinding, _6); -// FakeRead(ForGuardBinding, _8); // switchInt(move _9) -> [false: bb16, otherwise: bb15]; // } // bb11: { // `return 3` - first time @@ -128,6 +124,10 @@ fn main() { // } // bb15: { // StorageDead(_9); +// FakeRead(ForMatchGuard, _3); +// FakeRead(ForMatchGuard, _4); +// FakeRead(ForGuardBinding, _6); +// FakeRead(ForGuardBinding, _8); // StorageLive(_5); // _5 = (_2.1: bool); // StorageLive(_7); @@ -159,10 +159,6 @@ fn main() { // bb19: { // `else` block - second time // _12 = (*_6); // StorageDead(_13); -// FakeRead(ForMatchGuard, _3); -// FakeRead(ForMatchGuard, _4); -// FakeRead(ForGuardBinding, _6); -// FakeRead(ForGuardBinding, _8); // switchInt(move _12) -> [false: bb22, otherwise: bb21]; // } // bb20: { @@ -175,6 +171,10 @@ fn main() { // } // bb21: { // bindings for arm 1 // StorageDead(_12); +// FakeRead(ForMatchGuard, _3); +// FakeRead(ForMatchGuard, _4); +// FakeRead(ForGuardBinding, _6); +// FakeRead(ForGuardBinding, _8); // StorageLive(_5); // _5 = (_2.0: bool); // StorageLive(_7); diff --git a/src/test/mir-opt/match_false_edges.rs b/src/test/mir-opt/match_false_edges.rs index a62e1b21dd162..b275c06e05cd3 100644 --- a/src/test/mir-opt/match_false_edges.rs +++ b/src/test/mir-opt/match_false_edges.rs @@ -71,12 +71,12 @@ fn main() { // _7 = const guard() -> [return: bb7, unwind: bb1]; // } // bb7: { // end of guard -// FakeRead(ForMatchGuard, _4); -// FakeRead(ForGuardBinding, _6); // switchInt(move _7) -> [false: bb9, otherwise: bb8]; // } // bb8: { // arm1 // StorageDead(_7); +// FakeRead(ForMatchGuard, _4); +// FakeRead(ForGuardBinding, _6); // StorageLive(_5); // _5 = ((_2 as Some).0: i32); // StorageLive(_8); @@ -138,12 +138,12 @@ fn main() { // _7 = const guard() -> [return: bb6, unwind: bb1]; // } // bb6: { // end of guard -// FakeRead(ForMatchGuard, _4); -// FakeRead(ForGuardBinding, _6); // switchInt(move _7) -> [false: bb8, otherwise: bb7]; // } // bb7: { // StorageDead(_7); +// FakeRead(ForMatchGuard, _4); +// FakeRead(ForGuardBinding, _6); // StorageLive(_5); // _5 = ((_2 as Some).0: i32); // StorageLive(_8); @@ -209,12 +209,12 @@ fn main() { // _8 = const guard() -> [return: bb6, unwind: bb1]; // } // bb6: { //end of guard1 -// FakeRead(ForMatchGuard, _5); -// FakeRead(ForGuardBinding, _7); // switchInt(move _8) -> [false: bb8, otherwise: bb7]; // } // bb7: { // StorageDead(_8); +// FakeRead(ForMatchGuard, _5); +// FakeRead(ForGuardBinding, _7); // StorageLive(_6); // _6 = ((_2 as Some).0: i32); // _1 = const 1i32; @@ -245,12 +245,12 @@ fn main() { // } // bb11: { // end of guard2 // StorageDead(_13); -// FakeRead(ForMatchGuard, _5); -// FakeRead(ForGuardBinding, _11); // switchInt(move _12) -> [false: bb13, otherwise: bb12]; // } // bb12: { // binding4 & arm4 // StorageDead(_12); +// FakeRead(ForMatchGuard, _5); +// FakeRead(ForGuardBinding, _11); // StorageLive(_10); // _10 = ((_2 as Some).0: i32); // _1 = const 3i32; diff --git a/src/test/mir-opt/match_test.rs b/src/test/mir-opt/match_test.rs index aeb162772fac0..1ca75b100410b 100644 --- a/src/test/mir-opt/match_test.rs +++ b/src/test/mir-opt/match_test.rs @@ -54,11 +54,11 @@ fn main() { // _8 = &shallow _1; // StorageLive(_9); // _9 = _2; -// FakeRead(ForMatchGuard, _8); // switchInt(move _9) -> [false: bb11, otherwise: bb10]; // } // bb10: { // StorageDead(_9); +// FakeRead(ForMatchGuard, _8); // _3 = const 0i32; // goto -> bb14; // } diff --git a/src/test/mir-opt/remove_fake_borrows.rs b/src/test/mir-opt/remove_fake_borrows.rs index 0f9c6f62c2bd3..3245d38b2580b 100644 --- a/src/test/mir-opt/remove_fake_borrows.rs +++ b/src/test/mir-opt/remove_fake_borrows.rs @@ -38,14 +38,14 @@ fn main() { // _7 = &shallow (*(*((_1 as Some).0: &' &' i32))); // StorageLive(_8); // _8 = _2; -// FakeRead(ForMatchGuard, _4); -// FakeRead(ForMatchGuard, _5); -// FakeRead(ForMatchGuard, _6); -// FakeRead(ForMatchGuard, _7); // switchInt(move _8) -> [false: bb6, otherwise: bb5]; // } // bb5: { // StorageDead(_8); +// FakeRead(ForMatchGuard, _4); +// FakeRead(ForMatchGuard, _5); +// FakeRead(ForMatchGuard, _6); +// FakeRead(ForMatchGuard, _7); // _0 = const 0i32; // goto -> bb7; // } @@ -84,14 +84,14 @@ fn main() { // nop; // StorageLive(_8); // _8 = _2; -// nop; -// nop; -// nop; -// nop; // switchInt(move _8) -> [false: bb6, otherwise: bb5]; // } // bb5: { // StorageDead(_8); +// nop; +// nop; +// nop; +// nop; // _0 = const 0i32; // goto -> bb7; // } diff --git a/src/test/mir-opt/while-storage.rs b/src/test/mir-opt/while-storage.rs new file mode 100644 index 0000000000000..a486bd49a77d0 --- /dev/null +++ b/src/test/mir-opt/while-storage.rs @@ -0,0 +1,59 @@ +// Test that we correctly generate StorageDead statements for while loop +// conditions on all branches + +fn get_bool(c: bool) -> bool { + c +} + +fn while_loop(c: bool) { + while get_bool(c) { + if get_bool(c) { + break; + } + } +} + +fn main() { + while_loop(false); +} + +// END RUST SOURCE + +// START rustc.while_loop.PreCodegen.after.mir +// bb0: { +// StorageLive(_2); +// StorageLive(_3); +// _3 = _1; +// _2 = const get_bool(move _3) -> bb2; +// } +// bb1: { +// return; +// } +// bb2: { +// StorageDead(_3); +// switchInt(move _2) -> [false: bb4, otherwise: bb3]; +// } +// bb3: { +// StorageDead(_2); +// StorageLive(_4); +// StorageLive(_5); +// _5 = _1; +// _4 = const get_bool(move _5) -> bb5; +// } +// bb4: { +// StorageDead(_2); +// goto -> bb1; +// } +// bb5: { +// StorageDead(_5); +// switchInt(_4) -> [false: bb6, otherwise: bb7]; +// } +// bb6: { +// StorageDead(_4); +// goto -> bb0; +// } +// bb7: { +// StorageDead(_4); +// goto -> bb1; +// } +// END rustc.while_loop.PreCodegen.after.mir