squiid/rust
1
0
Fork 0
Code Issues Pull requests Projects Activity

Ensure dataflow of a proc never looks at blocks from closed-over context.

Browse source 
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`.)
This commit is contained in:
Felix S. Klock II 2014-06-13 17:19:29 +02:00
parent 4d82456f69
commit 263a433f19
7 changed files with 82 additions and 42 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
src/librustc/middle/borrowck/check_loans.rs
View file

@ -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(_)) => {

5
src/librustc/middle/borrowck/gather_loans/mod.rs
View file

@ -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 {

17
src/librustc/middle/borrowck/gather_loans/restrictions.rs
View file

@ -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
}

42
src/librustc/middle/borrowck/mod.rs
View file

@ -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<LoanPath>, 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<Rc<LoanPath>> {
}
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()
}

13
src/librustc/middle/borrowck/move_data.rs
View file

@ -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");
}

38
src/librustc/middle/dataflow.rs
View file

@ -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]) {

5
src/librustc/middle/mem_categorization.rs
View file

@ -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
}))