From 263a433f1901592f91e6433ccc79539e619cd95f Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 13 Jun 2014 17:19:29 +0200 Subject: [PATCH] Ensure dataflow of a proc never looks at blocks from closed-over context. Details: in a program like: ``` type T = proc(int) -> int; /* 4 */ pub fn outer(captured /* pat 16 */: T) -> T { (proc(x /* pat 23 */) { ((captured /* 29 */).foo((x /* 30 */)) /* 28 */) } /* block 27 */ /* 20 */) } /* block 19 */ /* 12 */ ``` the `captured` arg is moved from the outer fn into the inner proc (id=20). The old dataflow analysis for flowed_move_data_moves, when looking at the inner proc, would attempt to add a kill bit for `captured` at the end of its scope; the problem is that it thought the end of the `captured` arg's scope was the outer fn (id=12), even though at that point in the analysis, the `captured` arg's scope should now be restricted to the proc itself (id=20). This patch fixes handling of upvars so that dataflow of a fn/proc should never attempts to add a gen or kill bit to any NodeId outside of the current fn/proc. It accomplishes this by adding an `LpUpvar` variant to `borrowck::LoanPath`, so for cases like `captured` above will carry both their original `var_id`, as before, as well as the `NodeId` for the closure that is capturing them. As a drive-by fix to another occurrence of a similar bug that nikomatsakis pointed out to me earlier, this also fixes `gather_loans::compute_kill_scope` so that it computes the kill scope of the `captured` arg to be block 27; that is, the block for the proc itself (id=20). (This is an updated version that generalizes the new loan path variant to cover all upvars, and thus renamed the variant from `LpCopiedUpvar` to just `LpUpvar`.) --- src/librustc/middle/borrowck/check_loans.rs | 4 +- .../middle/borrowck/gather_loans/mod.rs | 5 ++- .../borrowck/gather_loans/restrictions.rs | 17 ++++++-- src/librustc/middle/borrowck/mod.rs | 42 +++++++++++++++---- src/librustc/middle/borrowck/move_data.rs | 13 +++++- src/librustc/middle/dataflow.rs | 38 +++++++---------- src/librustc/middle/mem_categorization.rs | 5 ++- 7 files changed, 82 insertions(+), 42 deletions(-) diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs index 166d069880f..df208b9cdc1 100644 --- a/src/librustc/middle/borrowck/check_loans.rs +++ b/src/librustc/middle/borrowck/check_loans.rs @@ -242,7 +242,7 @@ impl<'a> CheckLoanCtxt<'a> { let mut loan_path = loan_path; loop { match *loan_path { - LpVar(_) => { + LpVar(_) | LpUpvar(_) => { break; } LpExtend(ref lp_base, _, _) => { @@ -632,7 +632,7 @@ impl<'a> CheckLoanCtxt<'a> { */ match **lp { - LpVar(_) => { + LpVar(_) | LpUpvar(_) => { // assigning to `x` does not require that `x` is initialized } LpExtend(ref lp_base, _, LpInterior(_)) => { diff --git a/src/librustc/middle/borrowck/gather_loans/mod.rs b/src/librustc/middle/borrowck/gather_loans/mod.rs index 89f304513ff..454c3dcd5d3 100644 --- a/src/librustc/middle/borrowck/gather_loans/mod.rs +++ b/src/librustc/middle/borrowck/gather_loans/mod.rs @@ -395,7 +395,8 @@ impl<'a> GatherLoanCtxt<'a> { //! from a local variable, mark the mutability decl as necessary. match *loan_path { - LpVar(local_id) => { + LpVar(local_id) | + LpUpvar(ty::UpvarId{ var_id: local_id, closure_expr_id: _ }) => { self.tcx().used_mut_nodes.borrow_mut().insert(local_id); } LpExtend(ref base, mc::McInherited, _) => { @@ -445,8 +446,8 @@ impl<'a> GatherLoanCtxt<'a> { //! with immutable `&` pointers, because borrows of such pointers //! do not require restrictions and hence do not cause a loan. + let lexical_scope = lp.kill_scope(self.bccx.tcx); let rm = &self.bccx.tcx.region_maps; - let lexical_scope = rm.var_scope(lp.node_id()); if rm.is_subscope_of(lexical_scope, loan_scope) { lexical_scope } else { diff --git a/src/librustc/middle/borrowck/gather_loans/restrictions.rs b/src/librustc/middle/borrowck/gather_loans/restrictions.rs index 5b3e1ac0b2c..d131b6f7eda 100644 --- a/src/librustc/middle/borrowck/gather_loans/restrictions.rs +++ b/src/librustc/middle/borrowck/gather_loans/restrictions.rs @@ -67,13 +67,23 @@ impl<'a> RestrictionsContext<'a> { } mc::cat_local(local_id) | - mc::cat_arg(local_id) | - mc::cat_upvar(ty::UpvarId {var_id: local_id, ..}, _) => { - // R-Variable + mc::cat_arg(local_id) => { + // R-Variable, locally declared let lp = Rc::new(LpVar(local_id)); SafeIf(lp.clone(), vec!(lp)) } + mc::cat_upvar(upvar_id, _) => { + // R-Variable, captured into closure + let lp = Rc::new(LpUpvar(upvar_id)); + SafeIf(lp.clone(), vec!(lp)) + } + + mc::cat_copied_upvar(..) => { + // FIXME(#2152) allow mutation of upvars + Safe + } + mc::cat_downcast(cmt_base) => { // When we borrow the interior of an enum, we have to // ensure the enum itself is not mutated, because that @@ -107,7 +117,6 @@ impl<'a> RestrictionsContext<'a> { self.extend(result, cmt.mutbl, LpDeref(pk)) } - mc::cat_copied_upvar(..) | // FIXME(#2152) allow mutation of upvars mc::cat_static_item(..) => { Safe } diff --git a/src/librustc/middle/borrowck/mod.rs b/src/librustc/middle/borrowck/mod.rs index 98edc6365cf..0c77e637790 100644 --- a/src/librustc/middle/borrowck/mod.rs +++ b/src/librustc/middle/borrowck/mod.rs @@ -201,6 +201,7 @@ pub struct Loan { #[deriving(PartialEq, Eq, Hash)] pub enum LoanPath { LpVar(ast::NodeId), // `x` in doc.rs + LpUpvar(ty::UpvarId), // `x` captured by-value into closure LpExtend(Rc, mc::MutabilityCategory, LoanPathElem) } @@ -210,11 +211,25 @@ pub enum LoanPathElem { LpInterior(mc::InteriorKind) // `LV.f` in doc.rs } +pub fn closure_to_block(closure_id: ast::NodeId, + tcx: &ty::ctxt) -> ast::NodeId { + match tcx.map.get(closure_id) { + ast_map::NodeExpr(expr) => match expr.node { + ast::ExprProc(_decl, block) | + ast::ExprFnBlock(_decl, block) => { block.id } + _ => fail!("encountered non-closure id: {}", closure_id) + }, + _ => fail!("encountered non-expr id: {}", closure_id) + } +} + impl LoanPath { - pub fn node_id(&self) -> ast::NodeId { + pub fn kill_scope(&self, tcx: &ty::ctxt) -> ast::NodeId { match *self { - LpVar(local_id) => local_id, - LpExtend(ref base, _, _) => base.node_id() + LpVar(local_id) => tcx.region_maps.var_scope(local_id), + LpUpvar(upvar_id) => + closure_to_block(upvar_id.closure_expr_id, tcx), + LpExtend(ref base, _, _) => base.kill_scope(tcx), } } } @@ -234,12 +249,18 @@ pub fn opt_loan_path(cmt: &mc::cmt) -> Option> { } mc::cat_local(id) | - mc::cat_arg(id) | - mc::cat_copied_upvar(mc::CopiedUpvar { upvar_id: id, .. }) | - mc::cat_upvar(ty::UpvarId {var_id: id, ..}, _) => { + mc::cat_arg(id) => { Some(Rc::new(LpVar(id))) } + mc::cat_upvar(ty::UpvarId {var_id: id, closure_expr_id: proc_id}, _) | + mc::cat_copied_upvar(mc::CopiedUpvar { upvar_id: id, + onceness: _, + capturing_proc: proc_id }) => { + let upvar_id = ty::UpvarId{ var_id: id, closure_expr_id: proc_id }; + Some(Rc::new(LpUpvar(upvar_id))) + } + mc::cat_deref(ref cmt_base, _, pk) => { opt_loan_path(cmt_base).map(|lp| { Rc::new(LpExtend(lp, cmt.mutbl, LpDeref(pk))) @@ -693,6 +714,7 @@ impl<'a> BorrowckCtxt<'a> { loan_path: &LoanPath, out: &mut String) { match *loan_path { + LpUpvar(ty::UpvarId{ var_id: id, closure_expr_id: _ }) | LpVar(id) => { out.push_str(ty::local_var_name_str(self.tcx, id).get()); } @@ -734,7 +756,7 @@ impl<'a> BorrowckCtxt<'a> { self.append_autoderefd_loan_path_to_str(&**lp_base, out) } - LpVar(..) | LpExtend(_, _, LpInterior(..)) => { + LpVar(..) | LpUpvar(..) | LpExtend(_, _, LpInterior(..)) => { self.append_loan_path_to_str(loan_path, out) } } @@ -796,6 +818,12 @@ impl Repr for LoanPath { (format!("$({})", tcx.map.node_to_str(id))).to_string() } + &LpUpvar(ty::UpvarId{ var_id, closure_expr_id }) => { + let s = tcx.map.node_to_str(var_id); + let s = format!("$({} captured by id={})", s, closure_expr_id); + s.to_string() + } + &LpExtend(ref lp, _, LpDeref(_)) => { (format!("{}.*", lp.repr(tcx))).to_string() } diff --git a/src/librustc/middle/borrowck/move_data.rs b/src/librustc/middle/borrowck/move_data.rs index 3e2f763a84b..bb92043b1ea 100644 --- a/src/librustc/middle/borrowck/move_data.rs +++ b/src/librustc/middle/borrowck/move_data.rs @@ -231,7 +231,7 @@ impl MoveData { } let index = match *lp { - LpVar(..) => { + LpVar(..) | LpUpvar(..) => { let index = MovePathIndex(self.paths.borrow().len()); self.paths.borrow_mut().push(MovePath { @@ -302,7 +302,7 @@ impl MoveData { } None => { match **lp { - LpVar(..) => { } + LpVar(..) | LpUpvar(..) => { } LpExtend(ref b, _, _) => { self.add_existing_base_paths(b, result); } @@ -418,6 +418,11 @@ impl MoveData { let path = *self.path_map.borrow().get(&path.loan_path); self.kill_moves(path, kill_id, dfcx_moves); } + LpUpvar(ty::UpvarId { var_id: _, closure_expr_id }) => { + let kill_id = closure_to_block(closure_expr_id, tcx); + let path = *self.path_map.borrow().get(&path.loan_path); + self.kill_moves(path, kill_id, dfcx_moves); + } LpExtend(..) => {} } } @@ -430,6 +435,10 @@ impl MoveData { let kill_id = tcx.region_maps.var_scope(id); dfcx_assign.add_kill(kill_id, assignment_index); } + LpUpvar(ty::UpvarId { var_id: _, closure_expr_id }) => { + let kill_id = closure_to_block(closure_expr_id, tcx); + dfcx_assign.add_kill(kill_id, assignment_index); + } LpExtend(..) => { tcx.sess.bug("var assignment for non var path"); } diff --git a/src/librustc/middle/dataflow.rs b/src/librustc/middle/dataflow.rs index 872a0878e37..7a26d210482 100644 --- a/src/librustc/middle/dataflow.rs +++ b/src/librustc/middle/dataflow.rs @@ -266,34 +266,24 @@ impl<'a, O:DataFlowOperator> DataFlowContext<'a, O> { pub fn add_gen(&mut self, id: ast::NodeId, bit: uint) { //! Indicates that `id` generates `bit` - if self.nodeid_to_index.contains_key(&id) { - debug!("add_gen(id={:?}, bit={:?})", id, bit); - let cfgidx = to_cfgidx_or_die(id, &self.nodeid_to_index); - let (start, end) = self.compute_id_range(cfgidx); - { - let gens = self.gens.mut_slice(start, end); - set_bit(gens, bit); - } - } else { - debug!("{:s} add_gen skip (id={:?}, bit={:?}); id not in current fn", - self.analysis_name, id, bit); - } + debug!("{:s} add_gen(id={:?}, bit={:?})", + self.analysis_name, id, bit); + assert!(self.nodeid_to_index.contains_key(&id)); + let cfgidx = to_cfgidx_or_die(id, &self.nodeid_to_index); + let (start, end) = self.compute_id_range(cfgidx); + let gens = self.gens.mut_slice(start, end); + set_bit(gens, bit); } pub fn add_kill(&mut self, id: ast::NodeId, bit: uint) { //! Indicates that `id` kills `bit` - if self.nodeid_to_index.contains_key(&id) { - debug!("add_kill(id={:?}, bit={:?})", id, bit); - let cfgidx = to_cfgidx_or_die(id, &self.nodeid_to_index); - let (start, end) = self.compute_id_range(cfgidx); - { - let kills = self.kills.mut_slice(start, end); - set_bit(kills, bit); - } - } else { - debug!("{:s} add_kill skip (id={:?}, bit={:?}); id not in current fn", - self.analysis_name, id, bit); - } + debug!("{:s} add_kill(id={:?}, bit={:?})", + self.analysis_name, id, bit); + assert!(self.nodeid_to_index.contains_key(&id)); + let cfgidx = to_cfgidx_or_die(id, &self.nodeid_to_index); + let (start, end) = self.compute_id_range(cfgidx); + let kills = self.kills.mut_slice(start, end); + set_bit(kills, bit); } fn apply_gen_kill(&mut self, cfgidx: CFGIndex, bits: &mut [uint]) { diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs index 8224557f860..03c9c56c787 100644 --- a/src/librustc/middle/mem_categorization.rs +++ b/src/librustc/middle/mem_categorization.rs @@ -97,6 +97,7 @@ pub enum categorization { pub struct CopiedUpvar { pub upvar_id: ast::NodeId, pub onceness: ast::Onceness, + pub capturing_proc: ast::NodeId, } // different kinds of pointers: @@ -559,7 +560,9 @@ impl<'t,TYPER:Typer> MemCategorizationContext<'t,TYPER> { span:span, cat:cat_copied_upvar(CopiedUpvar { upvar_id: var_id, - onceness: closure_ty.onceness}), + onceness: closure_ty.onceness, + capturing_proc: fn_node_id, + }), mutbl:McImmutable, ty:expr_ty }))