From 6755fb8ba2300a121cb14bd79327c3eb730bc55d Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sun, 26 Feb 2017 16:21:26 +0200 Subject: [PATCH 1/4] schedule drops on bindings only after initializing them This reduces the number of dynamic drops in libstd from 1141 to 899. However, without this change, the next patch would have created much more dynamic drops. A basic merge unswitching hack reduced the number of dynamic drops to 644, with no effect on stack usage. I should be writing a more dedicated drop unswitching pass. No performance measurements. --- .../borrowck/mir/elaborate_drops.rs | 1 + src/librustc_mir/build/block.rs | 5 +- src/librustc_mir/build/matches/mod.rs | 120 ++++++++---------- src/test/mir-opt/storage_ranges.rs | 2 +- src/test/run-pass/mir_drop_order.rs | 42 ++++++ 5 files changed, 98 insertions(+), 72 deletions(-) create mode 100644 src/test/run-pass/mir_drop_order.rs diff --git a/src/librustc_borrowck/borrowck/mir/elaborate_drops.rs b/src/librustc_borrowck/borrowck/mir/elaborate_drops.rs index 13f898219bc..a71d23e7e1e 100644 --- a/src/librustc_borrowck/borrowck/mir/elaborate_drops.rs +++ b/src/librustc_borrowck/borrowck/mir/elaborate_drops.rs @@ -161,6 +161,7 @@ impl<'b, 'tcx> ElaborateDropsCtxt<'b, 'tcx> { fn create_drop_flag(&mut self, index: MovePathIndex) { let tcx = self.tcx; let patch = &mut self.patch; + debug!("create_drop_flag({:?})", self.mir.span); self.drop_flags.entry(index).or_insert_with(|| { patch.new_temp(tcx.types.bool) }); diff --git a/src/librustc_mir/build/block.rs b/src/librustc_mir/build/block.rs index 121d592da03..3305cfc0dfe 100644 --- a/src/librustc_mir/build/block.rs +++ b/src/librustc_mir/build/block.rs @@ -67,7 +67,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { this.expr_into_pattern(block, pattern, init) })); } else { - this.storage_live_for_bindings(block, &pattern); + this.visit_bindings(&pattern, &mut |this, _, _, node, span, _| { + this.storage_live_binding(block, node, span); + this.schedule_drop_for_binding(node, span); + }) } // Enter the visibility scope, after evaluating the initializer. diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index 6b6acb054b1..fd39f511e6d 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -123,16 +123,16 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { PatternKind::Binding { mode: BindingMode::ByValue, var, subpattern: None, .. } => { - self.storage_live_for_bindings(block, &irrefutable_pat); - let lvalue = Lvalue::Local(self.var_indices[&var]); - return self.into(&lvalue, block, initializer); + let lvalue = self.storage_live_binding(block, var, irrefutable_pat.span); + unpack!(block = self.into(&lvalue, block, initializer)); + self.schedule_drop_for_binding(var, irrefutable_pat.span); + block.unit() + } + _ => { + let lvalue = unpack!(block = self.as_lvalue(block, initializer)); + self.lvalue_into_pattern(block, irrefutable_pat, &lvalue) } - _ => {} } - let lvalue = unpack!(block = self.as_lvalue(block, initializer)); - self.lvalue_into_pattern(block, - irrefutable_pat, - &lvalue) } pub fn lvalue_into_pattern(&mut self, @@ -174,79 +174,70 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { scope_span: Span, pattern: &Pattern<'tcx>) -> Option { - match *pattern.kind { - PatternKind::Binding { mutability, name, mode: _, var, ty, ref subpattern } => { - if var_scope.is_none() { - var_scope = Some(self.new_visibility_scope(scope_span)); - } - let source_info = SourceInfo { - span: pattern.span, - scope: var_scope.unwrap() - }; - self.declare_binding(source_info, mutability, name, var, ty); - if let Some(subpattern) = subpattern.as_ref() { - var_scope = self.declare_bindings(var_scope, scope_span, subpattern); - } + self.visit_bindings(pattern, &mut |this, mutability, name, var, span, ty| { + if var_scope.is_none() { + var_scope = Some(this.new_visibility_scope(scope_span)); } - PatternKind::Array { ref prefix, ref slice, ref suffix } | - PatternKind::Slice { ref prefix, ref slice, ref suffix } => { - for subpattern in prefix.iter().chain(slice).chain(suffix) { - var_scope = self.declare_bindings(var_scope, scope_span, subpattern); - } - } - PatternKind::Constant { .. } | PatternKind::Range { .. } | PatternKind::Wild => { - } - PatternKind::Deref { ref subpattern } => { - var_scope = self.declare_bindings(var_scope, scope_span, subpattern); - } - PatternKind::Leaf { ref subpatterns } | - PatternKind::Variant { ref subpatterns, .. } => { - for subpattern in subpatterns { - var_scope = self.declare_bindings(var_scope, scope_span, &subpattern.pattern); - } - } - } + let source_info = SourceInfo { + span: span, + scope: var_scope.unwrap() + }; + this.declare_binding(source_info, mutability, name, var, ty); + }); var_scope } - /// Emit `StorageLive` for every binding in the pattern. - pub fn storage_live_for_bindings(&mut self, - block: BasicBlock, - pattern: &Pattern<'tcx>) { - match *pattern.kind { - PatternKind::Binding { var, ref subpattern, .. } => { - let lvalue = Lvalue::Local(self.var_indices[&var]); - let source_info = self.source_info(pattern.span); - self.cfg.push(block, Statement { - source_info: source_info, - kind: StatementKind::StorageLive(lvalue) - }); + pub fn storage_live_binding(&mut self, block: BasicBlock, var: NodeId, span: Span) + -> Lvalue<'tcx> + { + let local_id = self.var_indices[&var]; + let source_info = self.source_info(span); + self.cfg.push(block, Statement { + source_info: source_info, + kind: StatementKind::StorageLive(Lvalue::Local(local_id)) + }); + Lvalue::Local(local_id) + } + pub fn schedule_drop_for_binding(&mut self, var: NodeId, span: Span) { + let local_id = self.var_indices[&var]; + let var_ty = self.local_decls[local_id].ty; + let extent = self.hir.tcx().region_maps.var_scope(var); + self.schedule_drop(span, extent, &Lvalue::Local(local_id), var_ty); + } + + pub fn visit_bindings(&mut self, pattern: &Pattern<'tcx>, mut f: &mut F) + where F: FnMut(&mut Self, Mutability, Name, NodeId, Span, Ty<'tcx>) + { + match *pattern.kind { + PatternKind::Binding { mutability, name, var, ty, ref subpattern, .. } => { + f(self, mutability, name, var, pattern.span, ty); if let Some(subpattern) = subpattern.as_ref() { - self.storage_live_for_bindings(block, subpattern); + self.visit_bindings(subpattern, f); } } PatternKind::Array { ref prefix, ref slice, ref suffix } | PatternKind::Slice { ref prefix, ref slice, ref suffix } => { for subpattern in prefix.iter().chain(slice).chain(suffix) { - self.storage_live_for_bindings(block, subpattern); + self.visit_bindings(subpattern, f); } } PatternKind::Constant { .. } | PatternKind::Range { .. } | PatternKind::Wild => { } PatternKind::Deref { ref subpattern } => { - self.storage_live_for_bindings(block, subpattern); + self.visit_bindings(subpattern, f); } PatternKind::Leaf { ref subpatterns } | PatternKind::Variant { ref subpatterns, .. } => { for subpattern in subpatterns { - self.storage_live_for_bindings(block, &subpattern.pattern); + self.visit_bindings(&subpattern.pattern, f); } } } } } + /// List of blocks for each arm (and potentially other metadata in the /// future). struct ArmBlocks { @@ -691,25 +682,16 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // Assign each of the bindings. This may trigger moves out of the candidate. for binding in bindings { - // Find the variable for the `var_id` being bound. It - // should have been created by a previous call to - // `declare_bindings`. - let var_index = self.var_indices[&binding.var_id]; - + let source_info = self.source_info(binding.span); + let local = self.storage_live_binding(block, binding.var_id, binding.span); + self.schedule_drop_for_binding(binding.var_id, binding.span); let rvalue = match binding.binding_mode { BindingMode::ByValue => Rvalue::Use(Operand::Consume(binding.source)), BindingMode::ByRef(region, borrow_kind) => Rvalue::Ref(region, borrow_kind, binding.source), }; - - let source_info = self.source_info(binding.span); - self.cfg.push(block, Statement { - source_info: source_info, - kind: StatementKind::StorageLive(Lvalue::Local(var_index)) - }); - self.cfg.push_assign(block, source_info, - &Lvalue::Local(var_index), rvalue); + self.cfg.push_assign(block, source_info, &local, rvalue); } } @@ -730,8 +712,6 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { name: Some(name), source_info: Some(source_info), }); - let extent = self.hir.tcx().region_maps.var_scope(var_id); - self.schedule_drop(source_info.span, extent, &Lvalue::Local(var), var_ty); self.var_indices.insert(var_id, var); debug!("declare_binding: var={:?}", var); diff --git a/src/test/mir-opt/storage_ranges.rs b/src/test/mir-opt/storage_ranges.rs index 933bfa8df2e..c63db741e5a 100644 --- a/src/test/mir-opt/storage_ranges.rs +++ b/src/test/mir-opt/storage_ranges.rs @@ -31,8 +31,8 @@ fn main() { // _3 = &_4; // StorageDead(_5); // _2 = (); -// StorageDead(_4); // StorageDead(_3); +// StorageDead(_4); // StorageLive(_6); // _6 = const 1i32; // _0 = (); diff --git a/src/test/run-pass/mir_drop_order.rs b/src/test/run-pass/mir_drop_order.rs new file mode 100644 index 00000000000..55ac5ca067b --- /dev/null +++ b/src/test/run-pass/mir_drop_order.rs @@ -0,0 +1,42 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use std::cell::RefCell; + +pub struct DropLogger<'a> { + id: usize, + log: &'a RefCell> +} + +impl<'a> Drop for DropLogger<'a> { + fn drop(&mut self) { + self.log.borrow_mut().push(self.id); + } +} + +fn main() { + let log = RefCell::new(vec![]); + let d = |id| DropLogger { id: id, log: &log }; + let get = || -> Vec<_> { + let mut m = log.borrow_mut(); + let n = m.drain(..); + n.collect() + }; + + { + let _x = (d(0), &d(1), d(2), &d(3)); + // all borrows are extended - nothing has been dropped yet + assert_eq!(get(), vec![]); + } + // in a let-statement, extended lvalues are dropped + // *after* the let result (tho they have the same scope + // as far as scope-based borrowck goes). + assert_eq!(get(), vec![0, 2, 3, 1]); +} From 906c06a2f6add77531c08dfd4014e7c65c1743b2 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Sun, 26 Feb 2017 16:21:08 +0200 Subject: [PATCH 2/4] make operands live to the end of their containing expression In MIR construction, operands need to live exactly until they are used, which is during the (sub)expression that made the call to `as_operand`. Before this PR, operands lived until the end of the temporary scope, which was sometimes unnecessarily longer and sometimes too short. Fixes #38669. --- src/librustc_mir/build/expr/as_lvalue.rs | 8 ++- src/librustc_mir/build/expr/as_operand.rs | 32 +++++++-- src/librustc_mir/build/expr/as_rvalue.rs | 53 ++++++++------ src/librustc_mir/build/expr/as_temp.rs | 21 ++++-- src/librustc_mir/build/expr/into.rs | 16 ++--- src/librustc_mir/build/expr/stmt.rs | 8 +-- src/librustc_mir/build/matches/mod.rs | 2 +- src/librustc_mir/build/scope.rs | 10 ++- src/test/mir-opt/basic_assignment.rs | 85 +++++++++++++++++++++++ src/test/mir-opt/issue-38669.rs | 52 ++++++++++++++ src/test/mir-opt/storage_ranges.rs | 5 +- src/test/run-pass/mir_drop_order.rs | 21 ++++-- 12 files changed, 257 insertions(+), 56 deletions(-) create mode 100644 src/test/mir-opt/basic_assignment.rs create mode 100644 src/test/mir-opt/issue-38669.rs diff --git a/src/librustc_mir/build/expr/as_lvalue.rs b/src/librustc_mir/build/expr/as_lvalue.rs index 5abfe084f22..8886a310429 100644 --- a/src/librustc_mir/build/expr/as_lvalue.rs +++ b/src/librustc_mir/build/expr/as_lvalue.rs @@ -56,8 +56,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { let (usize_ty, bool_ty) = (this.hir.usize_ty(), this.hir.bool_ty()); let slice = unpack!(block = this.as_lvalue(block, lhs)); - - let idx = unpack!(block = this.as_operand(block, index)); + // extent=None so lvalue indexes live forever. They are scalars so they + // do not need storage annotations, and they are often copied between + // places. + let idx = unpack!(block = this.as_operand(block, None, index)); // bounds check: let (len, lt) = (this.temp(usize_ty.clone()), this.temp(bool_ty)); @@ -121,7 +123,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { Some(Category::Lvalue) => false, _ => true, }); - this.as_temp(block, expr) + this.as_temp(block, expr.temp_lifetime, expr) } } } diff --git a/src/librustc_mir/build/expr/as_operand.rs b/src/librustc_mir/build/expr/as_operand.rs index 09cdcc74ef6..8d79e755685 100644 --- a/src/librustc_mir/build/expr/as_operand.rs +++ b/src/librustc_mir/build/expr/as_operand.rs @@ -13,29 +13,52 @@ use build::{BlockAnd, BlockAndExtension, Builder}; use build::expr::category::Category; use hair::*; +use rustc::middle::region::CodeExtent; use rustc::mir::*; impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { + /// Returns an operand suitable for use until the end of the current + /// scope expression. + /// + /// The operand returned from this function will *not be valid* after + /// an ExprKind::Scope is passed, so please do *not* return it from + /// functions to avoid bad miscompiles. + pub fn as_local_operand(&mut self, block: BasicBlock, expr: M) + -> BlockAnd> + where M: Mirror<'tcx, Output = Expr<'tcx>> + { + let topmost_scope = self.topmost_scope(); // FIXME(#6393) + self.as_operand(block, Some(topmost_scope), expr) + } + /// Compile `expr` into a value that can be used as an operand. /// If `expr` is an lvalue like `x`, this will introduce a /// temporary `tmp = x`, so that we capture the value of `x` at /// this time. - pub fn as_operand(&mut self, block: BasicBlock, expr: M) -> BlockAnd> + /// + /// The operand is known to be live until the end of `scope`. + pub fn as_operand(&mut self, + block: BasicBlock, + scope: Option, + expr: M) -> BlockAnd> where M: Mirror<'tcx, Output = Expr<'tcx>> { let expr = self.hir.mirror(expr); - self.expr_as_operand(block, expr) + self.expr_as_operand(block, scope, expr) } fn expr_as_operand(&mut self, mut block: BasicBlock, + scope: Option, expr: Expr<'tcx>) -> BlockAnd> { debug!("expr_as_operand(block={:?}, expr={:?})", block, expr); let this = self; if let ExprKind::Scope { extent, value } = expr.kind { - return this.in_scope(extent, block, |this| this.as_operand(block, value)); + return this.in_scope(extent, block, |this| { + this.as_operand(block, scope, value) + }); } let category = Category::of(&expr.kind).unwrap(); @@ -47,7 +70,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } Category::Lvalue | Category::Rvalue(..) => { - let operand = unpack!(block = this.as_temp(block, expr)); + let operand = + unpack!(block = this.as_temp(block, scope, expr)); block.and(Operand::Consume(operand)) } } diff --git a/src/librustc_mir/build/expr/as_rvalue.rs b/src/librustc_mir/build/expr/as_rvalue.rs index 7f5d9c36ece..961713bcfe6 100644 --- a/src/librustc_mir/build/expr/as_rvalue.rs +++ b/src/librustc_mir/build/expr/as_rvalue.rs @@ -21,22 +21,34 @@ use build::expr::category::{Category, RvalueFunc}; use hair::*; use rustc_const_math::{ConstInt, ConstIsize}; use rustc::middle::const_val::ConstVal; +use rustc::middle::region::CodeExtent; use rustc::ty; use rustc::mir::*; use syntax::ast; use syntax_pos::Span; impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { + /// See comment on `as_local_operand` + pub fn as_local_rvalue(&mut self, block: BasicBlock, expr: M) + -> BlockAnd> + where M: Mirror<'tcx, Output = Expr<'tcx>> + { + let topmost_scope = self.topmost_scope(); // FIXME(#6393) + self.as_rvalue(block, Some(topmost_scope), expr) + } + /// Compile `expr`, yielding an rvalue. - pub fn as_rvalue(&mut self, block: BasicBlock, expr: M) -> BlockAnd> + pub fn as_rvalue(&mut self, block: BasicBlock, scope: Option, expr: M) + -> BlockAnd> where M: Mirror<'tcx, Output = Expr<'tcx>> { let expr = self.hir.mirror(expr); - self.expr_as_rvalue(block, expr) + self.expr_as_rvalue(block, scope, expr) } fn expr_as_rvalue(&mut self, mut block: BasicBlock, + scope: Option, expr: Expr<'tcx>) -> BlockAnd> { debug!("expr_as_rvalue(block={:?}, expr={:?})", block, expr); @@ -47,10 +59,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { match expr.kind { ExprKind::Scope { extent, value } => { - this.in_scope(extent, block, |this| this.as_rvalue(block, value)) + this.in_scope(extent, block, |this| this.as_rvalue(block, scope, value)) } ExprKind::Repeat { value, count } => { - let value_operand = unpack!(block = this.as_operand(block, value)); + let value_operand = unpack!(block = this.as_operand(block, scope, value)); block.and(Rvalue::Repeat(value_operand, count)) } ExprKind::Borrow { region, borrow_kind, arg } => { @@ -58,13 +70,13 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { block.and(Rvalue::Ref(region, borrow_kind, arg_lvalue)) } ExprKind::Binary { op, lhs, rhs } => { - let lhs = unpack!(block = this.as_operand(block, lhs)); - let rhs = unpack!(block = this.as_operand(block, rhs)); + let lhs = unpack!(block = this.as_operand(block, scope, lhs)); + let rhs = unpack!(block = this.as_operand(block, scope, rhs)); this.build_binary_op(block, op, expr_span, expr.ty, lhs, rhs) } ExprKind::Unary { op, arg } => { - let arg = unpack!(block = this.as_operand(block, arg)); + let arg = unpack!(block = this.as_operand(block, scope, arg)); // Check for -MIN on signed integers if this.hir.check_overflow() && op == UnOp::Neg && expr.ty.is_signed() { let bool_ty = this.hir.bool_ty(); @@ -97,27 +109,27 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { ExprKind::Cast { source } => { let source = this.hir.mirror(source); - let source = unpack!(block = this.as_operand(block, source)); + let source = unpack!(block = this.as_operand(block, scope, source)); block.and(Rvalue::Cast(CastKind::Misc, source, expr.ty)) } ExprKind::Use { source } => { - let source = unpack!(block = this.as_operand(block, source)); + let source = unpack!(block = this.as_operand(block, scope, source)); block.and(Rvalue::Use(source)) } ExprKind::ReifyFnPointer { source } => { - let source = unpack!(block = this.as_operand(block, source)); + let source = unpack!(block = this.as_operand(block, scope, source)); block.and(Rvalue::Cast(CastKind::ReifyFnPointer, source, expr.ty)) } ExprKind::UnsafeFnPointer { source } => { - let source = unpack!(block = this.as_operand(block, source)); + let source = unpack!(block = this.as_operand(block, scope, source)); block.and(Rvalue::Cast(CastKind::UnsafeFnPointer, source, expr.ty)) } ExprKind::ClosureFnPointer { source } => { - let source = unpack!(block = this.as_operand(block, source)); + let source = unpack!(block = this.as_operand(block, scope, source)); block.and(Rvalue::Cast(CastKind::ClosureFnPointer, source, expr.ty)) } ExprKind::Unsize { source } => { - let source = unpack!(block = this.as_operand(block, source)); + let source = unpack!(block = this.as_operand(block, scope, source)); block.and(Rvalue::Cast(CastKind::Unsize, source, expr.ty)) } ExprKind::Array { fields } => { @@ -150,7 +162,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // first process the set of fields let fields: Vec<_> = fields.into_iter() - .map(|f| unpack!(block = this.as_operand(block, f))) + .map(|f| unpack!(block = this.as_operand(block, scope, f))) .collect(); block.and(Rvalue::Aggregate(AggregateKind::Array, fields)) @@ -159,7 +171,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // first process the set of fields let fields: Vec<_> = fields.into_iter() - .map(|f| unpack!(block = this.as_operand(block, f))) + .map(|f| unpack!(block = this.as_operand(block, scope, f))) .collect(); block.and(Rvalue::Aggregate(AggregateKind::Tuple, fields)) @@ -167,7 +179,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { ExprKind::Closure { closure_id, substs, upvars } => { // see (*) above let upvars = upvars.into_iter() - .map(|upvar| unpack!(block = this.as_operand(block, upvar))) + .map(|upvar| unpack!(block = this.as_operand(block, scope, upvar))) .collect(); block.and(Rvalue::Aggregate(AggregateKind::Closure(closure_id, substs), upvars)) } @@ -179,10 +191,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // first process the set of fields that were provided // (evaluating them in order given by user) - let fields_map: FxHashMap<_, _> = - fields.into_iter() - .map(|f| (f.name, unpack!(block = this.as_operand(block, f.expr)))) - .collect(); + let fields_map: FxHashMap<_, _> = fields.into_iter() + .map(|f| (f.name, unpack!(block = this.as_operand(block, scope, f.expr)))) + .collect(); let field_names = this.hir.all_fields(adt_def, variant_index); @@ -235,7 +246,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { Some(Category::Rvalue(RvalueFunc::AsRvalue)) => false, _ => true, }); - let operand = unpack!(block = this.as_operand(block, expr)); + let operand = unpack!(block = this.as_operand(block, scope, expr)); block.and(Rvalue::Use(operand)) } } diff --git a/src/librustc_mir/build/expr/as_temp.rs b/src/librustc_mir/build/expr/as_temp.rs index 0ae4bcc4205..69b95702009 100644 --- a/src/librustc_mir/build/expr/as_temp.rs +++ b/src/librustc_mir/build/expr/as_temp.rs @@ -13,29 +13,38 @@ use build::{BlockAnd, BlockAndExtension, Builder}; use build::expr::category::Category; use hair::*; +use rustc::middle::region::CodeExtent; use rustc::mir::*; impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { /// Compile `expr` into a fresh temporary. This is used when building /// up rvalues so as to freeze the value that will be consumed. - pub fn as_temp(&mut self, block: BasicBlock, expr: M) -> BlockAnd> + pub fn as_temp(&mut self, + block: BasicBlock, + temp_lifetime: Option, + expr: M) + -> BlockAnd> where M: Mirror<'tcx, Output = Expr<'tcx>> { let expr = self.hir.mirror(expr); - self.expr_as_temp(block, expr) + self.expr_as_temp(block, temp_lifetime, expr) } - fn expr_as_temp(&mut self, mut block: BasicBlock, expr: Expr<'tcx>) -> BlockAnd> { + fn expr_as_temp(&mut self, + mut block: BasicBlock, + temp_lifetime: Option, + expr: Expr<'tcx>) + -> BlockAnd> { debug!("expr_as_temp(block={:?}, expr={:?})", block, expr); let this = self; - if let ExprKind::Scope { extent, value } = expr.kind { - return this.in_scope(extent, block, |this| this.as_temp(block, value)); + if let ExprKind::Scope { .. } = expr.kind { + span_bug!(expr.span, "unexpected scope expression in as_temp: {:?}", + expr); } let expr_ty = expr.ty.clone(); let temp = this.temp(expr_ty.clone()); - let temp_lifetime = expr.temp_lifetime; let expr_span = expr.span; let source_info = this.source_info(expr_span); diff --git a/src/librustc_mir/build/expr/into.rs b/src/librustc_mir/build/expr/into.rs index ae51951b519..dab85106873 100644 --- a/src/librustc_mir/build/expr/into.rs +++ b/src/librustc_mir/build/expr/into.rs @@ -52,7 +52,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { _ => false, }; - unpack!(block = this.as_rvalue(block, source)); + unpack!(block = this.as_local_rvalue(block, source)); // This is an optimization. If the expression was a call then we already have an // unreachable block. Don't bother to terminate it and create a new one. @@ -65,7 +65,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } } ExprKind::If { condition: cond_expr, then: then_expr, otherwise: else_expr } => { - let operand = unpack!(block = this.as_operand(block, cond_expr)); + let operand = unpack!(block = this.as_local_operand(block, cond_expr)); let mut then_block = this.cfg.start_new_block(); let mut else_block = this.cfg.start_new_block(); @@ -107,7 +107,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { (this.cfg.start_new_block(), this.cfg.start_new_block(), this.cfg.start_new_block(), this.cfg.start_new_block()); - let lhs = unpack!(block = this.as_operand(block, lhs)); + let lhs = unpack!(block = this.as_local_operand(block, lhs)); let blocks = match op { LogicalOp::And => (else_block, false_block), LogicalOp::Or => (true_block, else_block), @@ -115,7 +115,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { let term = TerminatorKind::if_(this.hir.tcx(), lhs, blocks.0, blocks.1); this.cfg.terminate(block, source_info, term); - let rhs = unpack!(else_block = this.as_operand(else_block, rhs)); + let rhs = unpack!(else_block = this.as_local_operand(else_block, rhs)); let term = TerminatorKind::if_(this.hir.tcx(), rhs, true_block, false_block); this.cfg.terminate(else_block, source_info, term); @@ -173,7 +173,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { if let Some(cond_expr) = opt_cond_expr { let loop_block_end; let cond = unpack!( - loop_block_end = this.as_operand(loop_block, cond_expr)); + loop_block_end = this.as_local_operand(loop_block, cond_expr)); body_block = this.cfg.start_new_block(); let term = TerminatorKind::if_(this.hir.tcx(), cond, body_block, exit_block); @@ -206,10 +206,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } _ => false }; - let fun = unpack!(block = this.as_operand(block, fun)); + let fun = unpack!(block = this.as_local_operand(block, fun)); let args: Vec<_> = args.into_iter() - .map(|arg| unpack!(block = this.as_operand(block, arg))) + .map(|arg| unpack!(block = this.as_local_operand(block, arg))) .collect(); let success = this.cfg.start_new_block(); @@ -265,7 +265,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { _ => true, }); - let rvalue = unpack!(block = this.as_rvalue(block, expr)); + let rvalue = unpack!(block = this.as_local_rvalue(block, expr)); this.cfg.push_assign(block, source_info, destination, rvalue); block.unit() } diff --git a/src/librustc_mir/build/expr/stmt.rs b/src/librustc_mir/build/expr/stmt.rs index c577aab40db..be39dcbf6d0 100644 --- a/src/librustc_mir/build/expr/stmt.rs +++ b/src/librustc_mir/build/expr/stmt.rs @@ -38,14 +38,14 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // Generate better code for things that don't need to be // dropped. if this.hir.needs_drop(lhs.ty) { - let rhs = unpack!(block = this.as_operand(block, rhs)); + let rhs = unpack!(block = this.as_local_operand(block, rhs)); let lhs = unpack!(block = this.as_lvalue(block, lhs)); unpack!(block = this.build_drop_and_replace( block, lhs_span, lhs, rhs )); block.unit() } else { - let rhs = unpack!(block = this.as_rvalue(block, rhs)); + let rhs = unpack!(block = this.as_local_rvalue(block, rhs)); let lhs = unpack!(block = this.as_lvalue(block, lhs)); this.cfg.push_assign(block, source_info, &lhs, rhs); block.unit() @@ -64,7 +64,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { let lhs_ty = lhs.ty; // As above, RTL. - let rhs = unpack!(block = this.as_operand(block, rhs)); + let rhs = unpack!(block = this.as_local_operand(block, rhs)); let lhs = unpack!(block = this.as_lvalue(block, lhs)); // we don't have to drop prior contents or anything @@ -122,7 +122,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { unpack!(block = this.as_lvalue(block, output)) }).collect(); let inputs = inputs.into_iter().map(|input| { - unpack!(block = this.as_operand(block, input)) + unpack!(block = this.as_local_operand(block, input)) }).collect(); this.cfg.push(block, Statement { source_info: source_info, diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index fd39f511e6d..705eb1f5660 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -661,7 +661,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // guard, this block is simply unreachable let guard = self.hir.mirror(guard); let source_info = self.source_info(guard.span); - let cond = unpack!(block = self.as_operand(block, guard)); + let cond = unpack!(block = self.as_local_operand(block, guard)); let otherwise = self.cfg.start_new_block(); self.cfg.terminate(block, source_info, TerminatorKind::if_(self.hir.tcx(), cond, arm_block, otherwise)); diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index 282361fc13e..3dab1717f6b 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -253,9 +253,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { f: F) where F: FnOnce(&mut Builder<'a, 'gcx, 'tcx>) { - let extent = self.scopes.last().map(|scope| scope.extent).unwrap(); + let extent = self.topmost_scope(); let loop_scope = LoopScope { - extent: extent.clone(), + extent: extent, continue_block: loop_block, break_block: break_block, break_destination: break_destination, @@ -416,6 +416,12 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { self.scopes[1].extent } + /// Returns the topmost active scope, which is known to be alive until + /// the next scope expression. + pub fn topmost_scope(&self) -> CodeExtent { + self.scopes.last().expect("topmost_scope: no scopes present").extent + } + // Scheduling drops // ================ /// Indicates that `lvalue` should be dropped on exit from diff --git a/src/test/mir-opt/basic_assignment.rs b/src/test/mir-opt/basic_assignment.rs new file mode 100644 index 00000000000..9c924a23903 --- /dev/null +++ b/src/test/mir-opt/basic_assignment.rs @@ -0,0 +1,85 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// this tests move up progration, which is not yet implemented +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// check that codegen of assignment expressions is sane. Assignments +// tend to be absent in simple code, so subtle breakage in them can +// leave a quite hard-to-find trail of destruction. + +fn main() { + let nodrop_x = false; + let nodrop_y; + + nodrop_y = nodrop_x; + + let drop_x : Option> = None; + let drop_y; + + drop_y = drop_x; +} + +// END RUST SOURCE +// START rustc.node4.SimplifyCfg.initial-after.mir +// bb0: { +// StorageLive(_1); +// _1 = const false; +// StorageLive(_2); +// StorageLive(_3); +// _3 = _1; +// _2 = _3; +// StorageDead(_3); +// StorageLive(_4); +// _4 = std::option::Option>::None; +// StorageLive(_6); +// StorageLive(_7); +// _7 = _4; +// replace(_6 <- _7) -> [return: bb5, unwind: bb4]; +// } +// bb1: { +// resume; +// } +// bb2: { +// drop(_4) -> bb1; +// } +// bb3: { +// drop(_6) -> bb2; +// } +// bb4: { +// drop(_7) -> bb3; +// } +// bb5: { +// drop(_7) -> [return: bb6, unwind: bb3]; +// } +// bb6: { +// StorageDead(_7); +// _0 = (); +// drop(_6) -> [return: bb7, unwind: bb2]; +// } +// bb7: { +// StorageDead(_6); +// drop(_4) -> bb8; +// } +// bb8: { +// StorageDead(_4); +// StorageDead(_2); +// StorageDead(_1); +// return; +// } +// END rustc.node4.SimplifyCfg.initial-after.mir diff --git a/src/test/mir-opt/issue-38669.rs b/src/test/mir-opt/issue-38669.rs new file mode 100644 index 00000000000..1d452907cf5 --- /dev/null +++ b/src/test/mir-opt/issue-38669.rs @@ -0,0 +1,52 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// check that we don't StorageDead booleans before they are used + +fn main() { + let mut should_break = false; + loop { + if should_break { + break; + } + should_break = true; + } +} + +// END RUST SOURCE +// START rustc.node4.SimplifyCfg.initial-after.mir +// bb0: { +// StorageLive(_1); +// _1 = const false; +// goto -> bb1; +// } +// +// bb1: { +// StorageLive(_4); +// _4 = _1; +// switchInt(_4) -> [0u8: bb3, otherwise: bb2]; +// } +// +// bb2: { +// StorageLive(_6); +// _0 = (); +// StorageDead(_4); +// StorageDead(_1); +// return; +// } +// +// bb3: { +// _3 = (); +// StorageDead(_4); +// _1 = const true; +// _2 = (); +// goto -> bb1; +// } +// END rustc.node4.SimplifyCfg.initial-after.mir diff --git a/src/test/mir-opt/storage_ranges.rs b/src/test/mir-opt/storage_ranges.rs index c63db741e5a..3fbd1a36f2f 100644 --- a/src/test/mir-opt/storage_ranges.rs +++ b/src/test/mir-opt/storage_ranges.rs @@ -28,8 +28,8 @@ fn main() { // StorageLive(_5); // _5 = _1; // _4 = std::option::Option::Some(_5,); -// _3 = &_4; // StorageDead(_5); +// _3 = &_4; // _2 = (); // StorageDead(_3); // StorageDead(_4); @@ -38,6 +38,5 @@ fn main() { // _0 = (); // StorageDead(_6); // StorageDead(_1); -// return; -// } +// } // END rustc.node4.TypeckMir.before.mir diff --git a/src/test/run-pass/mir_drop_order.rs b/src/test/run-pass/mir_drop_order.rs index 55ac5ca067b..e7da43597f1 100644 --- a/src/test/run-pass/mir_drop_order.rs +++ b/src/test/run-pass/mir_drop_order.rs @@ -9,23 +9,27 @@ // except according to those terms. use std::cell::RefCell; +use std::panic; pub struct DropLogger<'a> { id: usize, - log: &'a RefCell> + log: &'a panic::AssertUnwindSafe>> } impl<'a> Drop for DropLogger<'a> { fn drop(&mut self) { - self.log.borrow_mut().push(self.id); + self.log.0.borrow_mut().push(self.id); } } +struct InjectedFailure; + +#[allow(unreachable_code)] fn main() { - let log = RefCell::new(vec![]); + let log = panic::AssertUnwindSafe(RefCell::new(vec![])); let d = |id| DropLogger { id: id, log: &log }; let get = || -> Vec<_> { - let mut m = log.borrow_mut(); + let mut m = log.0.borrow_mut(); let n = m.drain(..); n.collect() }; @@ -39,4 +43,13 @@ fn main() { // *after* the let result (tho they have the same scope // as far as scope-based borrowck goes). assert_eq!(get(), vec![0, 2, 3, 1]); + + let _ = std::panic::catch_unwind(|| { + (d(4), &d(5), d(6), &d(7), panic!(InjectedFailure)); + }); + + // here, the temporaries (5/7) live until the end of the + // containing statement, which is destroyed after the operands + // (4/6) on a panic. + assert_eq!(get(), vec![6, 4, 7, 5]); } From 3ffa971212aba1e36cd8e7f3cd6a0ca9ae2858f5 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 28 Feb 2017 13:15:15 +0200 Subject: [PATCH 3/4] fix codegen test --- src/test/codegen/lifetime_start_end.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/test/codegen/lifetime_start_end.rs b/src/test/codegen/lifetime_start_end.rs index e3b35cf3552..5c1f1f8f2bb 100644 --- a/src/test/codegen/lifetime_start_end.rs +++ b/src/test/codegen/lifetime_start_end.rs @@ -11,11 +11,9 @@ // compile-flags: -O -C no-prepopulate-passes #![crate_type = "lib"] -#![feature(rustc_attrs)] // CHECK-LABEL: @test #[no_mangle] -#[rustc_mir] // FIXME #27840 MIR has different codegen. pub fn test() { let a = 0; &a; // keep variable in an alloca @@ -33,11 +31,11 @@ pub fn test() { // CHECK: [[S__5:%[0-9]+]] = bitcast %"core::option::Option"* %_5 to i8* // CHECK: call void @llvm.lifetime.start(i{{[0-9 ]+}}, i8* [[S__5]]) -// CHECK: [[E__5:%[0-9]+]] = bitcast %"core::option::Option"* %_5 to i8* -// CHECK: call void @llvm.lifetime.end(i{{[0-9 ]+}}, i8* [[E__5]]) - // CHECK: [[E_b:%[0-9]+]] = bitcast %"core::option::Option"** %b to i8* // CHECK: call void @llvm.lifetime.end(i{{[0-9 ]+}}, i8* [[E_b]]) + +// CHECK: [[E__5:%[0-9]+]] = bitcast %"core::option::Option"* %_5 to i8* +// CHECK: call void @llvm.lifetime.end(i{{[0-9 ]+}}, i8* [[E__5]]) } let c = 1; From f99f1f8975ea00605e809de5a78d1876a67fc6ee Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Fri, 3 Mar 2017 02:32:32 +0200 Subject: [PATCH 4/4] work around LLVM PR#32123 --- src/test/debuginfo/drop-locations.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/debuginfo/drop-locations.rs b/src/test/debuginfo/drop-locations.rs index 3a7c534c139..b4f6b33be26 100644 --- a/src/test/debuginfo/drop-locations.rs +++ b/src/test/debuginfo/drop-locations.rs @@ -14,7 +14,8 @@ #![allow(unused)] -// compile-flags:-g +// compile-flags:-g -O -C no-prepopulate-passes +// -O -C no-prepopulate-passes added to work around https://bugs.llvm.org/show_bug.cgi?id=32123 // This test checks that drop glue code gets attributed to scope's closing brace, // and function epilogues - to function's closing brace.