From 1702098e6f0717cd593cc88bf0463de8bc26ba9d Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Sat, 11 Apr 2015 18:42:33 +0200 Subject: [PATCH 1/2] Dataflow changes and associated borrowck fix. Revise rustc::middle::dataflow: one must select kill-kind when calling add_kill. The current kill-kinds are (1.) kills associated with ends-of-scopes and (2.) kills associated with the actual action of the expression/pattern. Then, use this to fix borrowck analysis so that it will not treat a break that pops through an assignment `x = { ... break; ... }` as a kill of the "moved-out" bit for `x`. Fix #24267. (incorporated review feedback.) --- src/librustc/middle/dataflow.rs | 80 ++++++++++++++++----- src/librustc_borrowck/borrowck/mod.rs | 3 +- src/librustc_borrowck/borrowck/move_data.rs | 19 +++-- 3 files changed, 79 insertions(+), 23 deletions(-) diff --git a/src/librustc/middle/dataflow.rs b/src/librustc/middle/dataflow.rs index b20e4c3f563..41b4495c5f0 100644 --- a/src/librustc/middle/dataflow.rs +++ b/src/librustc/middle/dataflow.rs @@ -64,8 +64,14 @@ pub struct DataFlowContext<'a, 'tcx: 'a, O> { /// bits generated as we exit the cfg node. Updated by `add_gen()`. gens: Vec, - /// bits killed as we exit the cfg node. Updated by `add_kill()`. - kills: Vec, + /// bits killed as we exit the cfg node, or non-locally jump over + /// it. Updated by `add_kill(KillFrom::ScopeEnd)`. + scope_kills: Vec, + + /// bits killed as we exit the cfg node directly; if it is jumped + /// over, e.g. via `break`, the kills are not reflected in the + /// jump's effects. Updated by `add_kill(KillFrom::Execution)`. + action_kills: Vec, /// bits that are valid on entry to the cfg node. Updated by /// `propagate()`. @@ -130,15 +136,23 @@ impl<'a, 'tcx, O:DataFlowOperator> pprust::PpAnn for DataFlowContext<'a, 'tcx, O "".to_string() }; - let kills = &self.kills[start .. end]; - let kills_str = if kills.iter().any(|&u| u != 0) { - format!(" kill: {}", bits_to_string(kills)) + let action_kills = &self.action_kills[start .. end]; + let action_kills_str = if action_kills.iter().any(|&u| u != 0) { + format!(" action_kill: {}", bits_to_string(action_kills)) } else { "".to_string() }; - try!(ps.synth_comment(format!("id {}: {}{}{}", id, entry_str, - gens_str, kills_str))); + let scope_kills = &self.scope_kills[start .. end]; + let scope_kills_str = if scope_kills.iter().any(|&u| u != 0) { + format!(" scope_kill: {}", bits_to_string(scope_kills)) + } else { + "".to_string() + }; + + try!(ps.synth_comment( + format!("id {}: {}{}{}{}", id, entry_str, + gens_str, action_kills_str, scope_kills_str))); try!(pp::space(&mut ps.s)); } Ok(()) @@ -187,6 +201,25 @@ fn build_nodeid_to_index(decl: Option<&ast::FnDecl>, } } +/// Flag used by `add_kill` to indicate whether the provided kill +/// takes effect only when control flows directly through the node in +/// question, or if the kill's effect is associated with any +/// control-flow directly through or indirectly over the node. +#[derive(Copy, Clone, PartialEq, Debug)] +pub enum KillFrom { + /// A `ScopeEnd` kill is one that takes effect when any control + /// flow goes over the node. A kill associated with the end of the + /// scope of a variable declaration `let x;` is an example of a + /// `ScopeEnd` kill. + ScopeEnd, + + /// An `Execution` kill is one that takes effect only when control + /// flow goes through the node to completion. A kill associated + /// with an assignment statement `x = expr;` is an example of an + /// `Execution` kill. + Execution, +} + impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> { pub fn new(tcx: &'a ty::ctxt<'tcx>, analysis_name: &'static str, @@ -206,8 +239,10 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> { let entry = if oper.initial_value() { usize::MAX } else {0}; - let gens: Vec<_> = repeat(0).take(num_nodes * words_per_id).collect(); - let kills: Vec<_> = repeat(0).take(num_nodes * words_per_id).collect(); + let zeroes: Vec<_> = repeat(0).take(num_nodes * words_per_id).collect(); + let gens: Vec<_> = zeroes.clone(); + let kills1: Vec<_> = zeroes.clone(); + let kills2: Vec<_> = zeroes; let on_entry: Vec<_> = repeat(entry).take(num_nodes * words_per_id).collect(); let nodeid_to_index = build_nodeid_to_index(decl, cfg); @@ -220,7 +255,8 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> { bits_per_id: bits_per_id, oper: oper, gens: gens, - kills: kills, + action_kills: kills1, + scope_kills: kills2, on_entry: on_entry } } @@ -240,7 +276,7 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> { } } - pub fn add_kill(&mut self, id: ast::NodeId, bit: usize) { + pub fn add_kill(&mut self, kind: KillFrom, id: ast::NodeId, bit: usize) { //! Indicates that `id` kills `bit` debug!("{} add_kill(id={}, bit={})", self.analysis_name, id, bit); @@ -250,7 +286,10 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> { let indices = get_cfg_indices(id, &self.nodeid_to_index); for &cfgidx in indices { let (start, end) = self.compute_id_range(cfgidx); - let kills = &mut self.kills[start.. end]; + let kills = match kind { + KillFrom::Execution => &mut self.action_kills[start.. end], + KillFrom::ScopeEnd => &mut self.scope_kills[start.. end], + }; set_bit(kills, bit); } } @@ -264,7 +303,9 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> { let (start, end) = self.compute_id_range(cfgidx); let gens = &self.gens[start.. end]; bitwise(bits, gens, &Union); - let kills = &self.kills[start.. end]; + let kills = &self.action_kills[start.. end]; + bitwise(bits, kills, &Subtract); + let kills = &self.scope_kills[start.. end]; bitwise(bits, kills, &Subtract); debug!("{} apply_gen_kill(cfgidx={:?}, bits={}) [after]", @@ -278,7 +319,8 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> { assert!(start < self.gens.len()); assert!(end <= self.gens.len()); - assert!(self.gens.len() == self.kills.len()); + assert!(self.gens.len() == self.action_kills.len()); + assert!(self.gens.len() == self.scope_kills.len()); assert!(self.gens.len() == self.on_entry.len()); (start, end) @@ -412,7 +454,7 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> { cfg.graph.each_edge(|_edge_index, edge| { let flow_exit = edge.source(); let (start, end) = self.compute_id_range(flow_exit); - let mut orig_kills = self.kills[start.. end].to_vec(); + let mut orig_kills = self.scope_kills[start.. end].to_vec(); let mut changed = false; for &node_id in &edge.data.exiting_scopes { @@ -421,8 +463,12 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> { Some(indices) => { for &cfg_idx in indices { let (start, end) = self.compute_id_range(cfg_idx); - let kills = &self.kills[start.. end]; + let kills = &self.scope_kills[start.. end]; if bitwise(&mut orig_kills, kills, &Union) { + debug!("scope exits: scope id={} \ + (node={:?} of {:?}) added killset: {}", + node_id, cfg_idx, indices, + bits_to_string(kills)); changed = true; } } @@ -436,7 +482,7 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> { } if changed { - let bits = &mut self.kills[start.. end]; + let bits = &mut self.scope_kills[start.. end]; debug!("{} add_kills_from_flow_exits flow_exit={:?} bits={} [before]", self.analysis_name, flow_exit, mut_bits_to_string(bits)); bits.clone_from_slice(&orig_kills[..]); diff --git a/src/librustc_borrowck/borrowck/mod.rs b/src/librustc_borrowck/borrowck/mod.rs index db947a27472..502321d0759 100644 --- a/src/librustc_borrowck/borrowck/mod.rs +++ b/src/librustc_borrowck/borrowck/mod.rs @@ -24,6 +24,7 @@ use rustc::middle::cfg; use rustc::middle::dataflow::DataFlowContext; use rustc::middle::dataflow::BitwiseOperator; use rustc::middle::dataflow::DataFlowOperator; +use rustc::middle::dataflow::KillFrom; use rustc::middle::expr_use_visitor as euv; use rustc::middle::mem_categorization as mc; use rustc::middle::region; @@ -167,7 +168,7 @@ fn build_borrowck_dataflow_data<'a, 'tcx>(this: &mut BorrowckCtxt<'a, 'tcx>, all_loans.len()); for (loan_idx, loan) in all_loans.iter().enumerate() { loan_dfcx.add_gen(loan.gen_scope.node_id(), loan_idx); - loan_dfcx.add_kill(loan.kill_scope.node_id(), loan_idx); + loan_dfcx.add_kill(KillFrom::ScopeEnd, loan.kill_scope.node_id(), loan_idx); } loan_dfcx.add_kills_from_flow_exits(cfg); loan_dfcx.propagate(cfg, body); diff --git a/src/librustc_borrowck/borrowck/move_data.rs b/src/librustc_borrowck/borrowck/move_data.rs index 2d1b57243d1..1180717140e 100644 --- a/src/librustc_borrowck/borrowck/move_data.rs +++ b/src/librustc_borrowck/borrowck/move_data.rs @@ -18,6 +18,7 @@ use rustc::middle::cfg; use rustc::middle::dataflow::DataFlowContext; use rustc::middle::dataflow::BitwiseOperator; use rustc::middle::dataflow::DataFlowOperator; +use rustc::middle::dataflow::KillFrom; use rustc::middle::expr_use_visitor as euv; use rustc::middle::ty; use rustc::util::nodemap::{FnvHashMap, NodeSet}; @@ -473,11 +474,13 @@ impl<'tcx> MoveData<'tcx> { for (i, assignment) in self.var_assignments.borrow().iter().enumerate() { dfcx_assign.add_gen(assignment.id, i); - self.kill_moves(assignment.path, assignment.id, dfcx_moves); + self.kill_moves(assignment.path, assignment.id, + KillFrom::Execution, dfcx_moves); } for assignment in &*self.path_assignments.borrow() { - self.kill_moves(assignment.path, assignment.id, dfcx_moves); + self.kill_moves(assignment.path, assignment.id, + KillFrom::Execution, dfcx_moves); } // Kill all moves related to a variable `x` when @@ -487,7 +490,8 @@ impl<'tcx> MoveData<'tcx> { LpVar(..) | LpUpvar(..) | LpDowncast(..) => { let kill_scope = path.loan_path.kill_scope(tcx); let path = *self.path_map.borrow().get(&path.loan_path).unwrap(); - self.kill_moves(path, kill_scope.node_id(), dfcx_moves); + self.kill_moves(path, kill_scope.node_id(), + KillFrom::ScopeEnd, dfcx_moves); } LpExtend(..) => {} } @@ -500,7 +504,9 @@ impl<'tcx> MoveData<'tcx> { match lp.kind { LpVar(..) | LpUpvar(..) | LpDowncast(..) => { let kill_scope = lp.kill_scope(tcx); - dfcx_assign.add_kill(kill_scope.node_id(), assignment_index); + dfcx_assign.add_kill(KillFrom::ScopeEnd, + kill_scope.node_id(), + assignment_index); } LpExtend(..) => { tcx.sess.bug("var assignment for non var path"); @@ -568,6 +574,7 @@ impl<'tcx> MoveData<'tcx> { fn kill_moves(&self, path: MovePathIndex, kill_id: ast::NodeId, + kill_kind: KillFrom, dfcx_moves: &mut MoveDataFlow) { // We can only perform kills for paths that refer to a unique location, // since otherwise we may kill a move from one location with an @@ -576,7 +583,9 @@ impl<'tcx> MoveData<'tcx> { let loan_path = self.path_loan_path(path); if loan_path_is_precise(&*loan_path) { self.each_applicable_move(path, |move_index| { - dfcx_moves.add_kill(kill_id, move_index.get()); + debug!("kill_moves add_kill {:?} kill_id={} move_index={}", + kill_kind, kill_id, move_index.get()); + dfcx_moves.add_kill(kill_kind, kill_id, move_index.get()); true }); } From 77bf827968d90594643ad0641161540ed1763730 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Sat, 11 Apr 2015 18:57:41 +0200 Subject: [PATCH 2/2] Regression test. --- .../compile-fail/issue-24267-flow-exit.rs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 src/test/compile-fail/issue-24267-flow-exit.rs diff --git a/src/test/compile-fail/issue-24267-flow-exit.rs b/src/test/compile-fail/issue-24267-flow-exit.rs new file mode 100644 index 00000000000..4aca6bf38e1 --- /dev/null +++ b/src/test/compile-fail/issue-24267-flow-exit.rs @@ -0,0 +1,29 @@ +// Copyright 2015 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. + +// Ensure that we reject code when a nonlocal exit (`break`, +// `continue`) causes us to pop over a needed assignment. + +pub fn main() { + foo1(); + foo2(); +} + +pub fn foo1() { + let x: i32; + loop { x = break; } + println!("{}", x); //~ ERROR use of possibly uninitialized variable: `x` +} + +pub fn foo2() { + let x: i32; + for _ in 0..10 { x = continue; } + println!("{}", x); //~ ERROR use of possibly uninitialized variable: `x` +}