From eee209d9e2dde700a3958f3e539eff02b63f50bd Mon Sep 17 00:00:00 2001 From: James Miller Date: Tue, 16 Dec 2014 12:21:08 +1300 Subject: [PATCH 1/5] Only count nested returns when the outer return is reachable This narrows the definition of nested returns such that only when the outer return has a chance of being executed (due to the inner return being conditional) do we mark the function as having nested returns. Fixes #19684 --- src/librustc/middle/cfg/mod.rs | 7 +++ src/librustc/middle/graph.rs | 37 ++++++++++++ src/librustc_driver/lib.rs | 2 +- src/librustc_trans/trans/base.rs | 94 ++++++++++++++++-------------- src/librustc_trans/trans/common.rs | 3 + 5 files changed, 99 insertions(+), 44 deletions(-) diff --git a/src/librustc/middle/cfg/mod.rs b/src/librustc/middle/cfg/mod.rs index bc512a73a4bd8..0e9bd42a23a13 100644 --- a/src/librustc/middle/cfg/mod.rs +++ b/src/librustc/middle/cfg/mod.rs @@ -49,4 +49,11 @@ impl CFG { blk: &ast::Block) -> CFG { construct::construct(tcx, blk) } + + pub fn node_is_reachable(&self, id: ast::NodeId) -> bool { + for node in self.graph.depth_traverse(self.entry) { + if node.id == id { return true } + } + return false; + } } diff --git a/src/librustc/middle/graph.rs b/src/librustc/middle/graph.rs index 4c03ed2a480ef..4397f03a64286 100644 --- a/src/librustc/middle/graph.rs +++ b/src/librustc/middle/graph.rs @@ -34,6 +34,7 @@ use std::fmt::{Formatter, Error, Show}; use std::uint; +use std::collections::BitvSet; pub struct Graph { nodes: Vec> , @@ -294,6 +295,42 @@ impl Graph { } } } + + pub fn depth_traverse<'a>(&'a self, start: NodeIndex) -> DepthFirstTraversal<'a, N, E> { + DepthFirstTraversal { + graph: self, + stack: vec![start], + visited: BitvSet::new() + } + } +} + +pub struct DepthFirstTraversal<'g, N:'g, E:'g> { + graph: &'g Graph, + stack: Vec, + visited: BitvSet +} + +impl<'g, N, E> Iterator<&'g N> for DepthFirstTraversal<'g, N, E> { + fn next(&mut self) -> Option<&'g N> { + while self.stack.len() > 0 { + let idx = self.stack.pop().unwrap(); + if self.visited.contains(&idx.node_id()) { + continue; + } + self.visited.insert(idx.node_id()); + self.graph.each_outgoing_edge(idx, |_, e| -> bool { + if !self.visited.contains(&e.target().node_id()) { + self.stack.push(e.target()); + } + true + }); + + return Some(self.graph.node_data(idx)); + } + + return None; + } } pub fn each_edge_index(max_edge_index: EdgeIndex, mut f: F) where diff --git a/src/librustc_driver/lib.rs b/src/librustc_driver/lib.rs index b0f8b3bdbe7df..82d3edfa5e045 100644 --- a/src/librustc_driver/lib.rs +++ b/src/librustc_driver/lib.rs @@ -472,7 +472,7 @@ pub fn list_metadata(sess: &Session, path: &Path, /// The diagnostic emitter yielded to the procedure should be used for reporting /// errors of the compiler. pub fn monitor(f: F) { - static STACK_SIZE: uint = 32000000; // 32MB + static STACK_SIZE: uint = 8 * 1024 * 1024; // 8MB let (tx, rx) = channel(); let w = io::ChanWriter::new(tx); diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index 83779ffbe161c..1a82a7131a774 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -38,6 +38,7 @@ use llvm::{BasicBlockRef, Linkage, ValueRef, Vector, get_param}; use llvm; use metadata::{csearch, encoder, loader}; use middle::astencode; +use middle::cfg; use middle::lang_items::{LangItem, ExchangeMallocFnLangItem, StartFnLangItem}; use middle::subst; use middle::weak_lang_items; @@ -1305,47 +1306,33 @@ pub fn make_return_slot_pointer<'a, 'tcx>(fcx: &FunctionContext<'a, 'tcx>, } } -struct CheckForNestedReturnsVisitor { +struct FindNestedReturn { found: bool, - in_return: bool } -impl CheckForNestedReturnsVisitor { - fn explicit() -> CheckForNestedReturnsVisitor { - CheckForNestedReturnsVisitor { found: false, in_return: false } - } - fn implicit() -> CheckForNestedReturnsVisitor { - CheckForNestedReturnsVisitor { found: false, in_return: true } +impl FindNestedReturn { + fn new() -> FindNestedReturn { + FindNestedReturn { found: false } } } -impl<'v> Visitor<'v> for CheckForNestedReturnsVisitor { +impl<'v> Visitor<'v> for FindNestedReturn { fn visit_expr(&mut self, e: &ast::Expr) { match e.node { ast::ExprRet(..) => { - if self.in_return { - self.found = true; - } else { - self.in_return = true; - visit::walk_expr(self, e); - self.in_return = false; - } + self.found = true; } _ => visit::walk_expr(self, e) } } } -fn has_nested_returns(tcx: &ty::ctxt, id: ast::NodeId) -> bool { - match tcx.map.find(id) { +fn build_cfg(tcx: &ty::ctxt, id: ast::NodeId) -> (ast::NodeId, Option) { + let blk = match tcx.map.find(id) { Some(ast_map::NodeItem(i)) => { match i.node { ast::ItemFn(_, _, _, _, ref blk) => { - let mut explicit = CheckForNestedReturnsVisitor::explicit(); - let mut implicit = CheckForNestedReturnsVisitor::implicit(); - visit::walk_item(&mut explicit, &*i); - visit::walk_expr_opt(&mut implicit, &blk.expr); - explicit.found || implicit.found + blk } _ => tcx.sess.bug("unexpected item variant in has_nested_returns") } @@ -1355,11 +1342,7 @@ fn has_nested_returns(tcx: &ty::ctxt, id: ast::NodeId) -> bool { ast::ProvidedMethod(ref m) => { match m.node { ast::MethDecl(_, _, _, _, _, _, ref blk, _) => { - let mut explicit = CheckForNestedReturnsVisitor::explicit(); - let mut implicit = CheckForNestedReturnsVisitor::implicit(); - visit::walk_method_helper(&mut explicit, &**m); - visit::walk_expr_opt(&mut implicit, &blk.expr); - explicit.found || implicit.found + blk } ast::MethMac(_) => tcx.sess.bug("unexpanded macro") } @@ -1379,11 +1362,7 @@ fn has_nested_returns(tcx: &ty::ctxt, id: ast::NodeId) -> bool { ast::MethodImplItem(ref m) => { match m.node { ast::MethDecl(_, _, _, _, _, _, ref blk, _) => { - let mut explicit = CheckForNestedReturnsVisitor::explicit(); - let mut implicit = CheckForNestedReturnsVisitor::implicit(); - visit::walk_method_helper(&mut explicit, &**m); - visit::walk_expr_opt(&mut implicit, &blk.expr); - explicit.found || implicit.found + blk } ast::MethMac(_) => tcx.sess.bug("unexpanded macro") } @@ -1397,24 +1376,47 @@ fn has_nested_returns(tcx: &ty::ctxt, id: ast::NodeId) -> bool { Some(ast_map::NodeExpr(e)) => { match e.node { ast::ExprClosure(_, _, _, ref blk) => { - let mut explicit = CheckForNestedReturnsVisitor::explicit(); - let mut implicit = CheckForNestedReturnsVisitor::implicit(); - visit::walk_expr(&mut explicit, e); - visit::walk_expr_opt(&mut implicit, &blk.expr); - explicit.found || implicit.found + blk } _ => tcx.sess.bug("unexpected expr variant in has_nested_returns") } } - - Some(ast_map::NodeVariant(..)) | Some(ast_map::NodeStructCtor(..)) => false, + Some(ast_map::NodeVariant(..)) | Some(ast_map::NodeStructCtor(..)) => return (ast::DUMMY_NODE_ID, None), // glue, shims, etc - None if id == ast::DUMMY_NODE_ID => false, + None if id == ast::DUMMY_NODE_ID => return (ast::DUMMY_NODE_ID, None), _ => tcx.sess.bug(format!("unexpected variant in has_nested_returns: {}", tcx.map.path_to_string(id)).as_slice()) + }; + + (blk.id, Some(cfg::CFG::new(tcx, &**blk))) +} + +fn has_nested_returns(tcx: &ty::ctxt, cfg: &cfg::CFG, blk_id: ast::NodeId) -> bool { + for n in cfg.graph.depth_traverse(cfg.entry) { + match tcx.map.find(n.id) { + Some(ast_map::NodeExpr(ex)) => { + if let ast::ExprRet(Some(ref ret_expr)) = ex.node { + let mut visitor = FindNestedReturn::new(); + visit::walk_expr(&mut visitor, &**ret_expr); + if visitor.found { + return true; + } + } + } + Some(ast_map::NodeBlock(blk)) if blk.id == blk_id => { + let mut visitor = FindNestedReturn::new(); + visit::walk_expr_opt(&mut visitor, &blk.expr); + if visitor.found { + return true; + } + } + _ => {} + } } + + return false; } // NB: must keep 4 fns in sync: @@ -1453,7 +1455,12 @@ pub fn new_fn_ctxt<'a, 'tcx>(ccx: &'a CrateContext<'a, 'tcx>, ty::FnDiverging => false }; let debug_context = debuginfo::create_function_debug_context(ccx, id, param_substs, llfndecl); - let nested_returns = has_nested_returns(ccx.tcx(), id); + let (blk_id, cfg) = build_cfg(ccx.tcx(), id); + let nested_returns = if let Some(ref cfg) = cfg { + has_nested_returns(ccx.tcx(), cfg, blk_id) + } else { + false + }; let mut fcx = FunctionContext { llfn: llfndecl, @@ -1472,7 +1479,8 @@ pub fn new_fn_ctxt<'a, 'tcx>(ccx: &'a CrateContext<'a, 'tcx>, block_arena: block_arena, ccx: ccx, debug_context: debug_context, - scopes: RefCell::new(Vec::new()) + scopes: RefCell::new(Vec::new()), + cfg: cfg }; if has_env { diff --git a/src/librustc_trans/trans/common.rs b/src/librustc_trans/trans/common.rs index 83938fa335708..c8a628a33e7a1 100644 --- a/src/librustc_trans/trans/common.rs +++ b/src/librustc_trans/trans/common.rs @@ -18,6 +18,7 @@ use session::Session; use llvm; use llvm::{ValueRef, BasicBlockRef, BuilderRef, ContextRef}; use llvm::{True, False, Bool}; +use middle::cfg; use middle::def; use middle::infer; use middle::lang_items::LangItem; @@ -264,6 +265,8 @@ pub struct FunctionContext<'a, 'tcx: 'a> { // Cleanup scopes. pub scopes: RefCell>>, + + pub cfg: Option, } impl<'a, 'tcx> FunctionContext<'a, 'tcx> { From fb3e871734957d83aab0cc55d45be7d75d6a7b15 Mon Sep 17 00:00:00 2001 From: James Miller Date: Tue, 16 Dec 2014 12:35:59 +1300 Subject: [PATCH 2/5] Add some documentation --- src/librustc_trans/trans/base.rs | 10 ++++++++++ src/librustc_trans/trans/controlflow.rs | 9 +++++++++ src/librustc_trans/trans/expr.rs | 20 +++++++++++++++++++- 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index 1a82a7131a774..87f9d4c447f9f 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -1393,6 +1393,16 @@ fn build_cfg(tcx: &ty::ctxt, id: ast::NodeId) -> (ast::NodeId, Option) (blk.id, Some(cfg::CFG::new(tcx, &**blk))) } +// Checks for the presence of "nested returns" in a function. +// Nested returns are when the inner expression of a return expression +// (the 'expr' in 'return expr') contains a return expression. Only cases +// where the outer return is actually reachable are considered. Implicit +// returns from the end of blocks are considered as well. +// +// This check is needed to handle the case where the inner expression is +// part of a larger expression that may have already partially-filled the +// return slot alloca. This can cause errors related to clean-up due to +// the clobbering of the existing value in the return slot. fn has_nested_returns(tcx: &ty::ctxt, cfg: &cfg::CFG, blk_id: ast::NodeId) -> bool { for n in cfg.graph.depth_traverse(cfg.entry) { match tcx.map.find(n.id) { diff --git a/src/librustc_trans/trans/controlflow.rs b/src/librustc_trans/trans/controlflow.rs index a1574aa2f0e43..e55da561c9409 100644 --- a/src/librustc_trans/trans/controlflow.rs +++ b/src/librustc_trans/trans/controlflow.rs @@ -112,8 +112,17 @@ pub fn trans_block<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, if dest != expr::Ignore { let block_ty = node_id_type(bcx, b.id); + if b.expr.is_none() || type_is_zero_size(bcx.ccx(), block_ty) { dest = expr::Ignore; + } else if b.expr.is_some() { + // If the block has an expression, but that expression isn't reachable, + // don't save into the destination given, ignore it. + if let Some(ref cfg) = bcx.fcx.cfg { + if !cfg.node_is_reachable(b.expr.as_ref().unwrap().id) { + dest = expr::Ignore; + } + } } } diff --git a/src/librustc_trans/trans/expr.rs b/src/librustc_trans/trans/expr.rs index 690e7cf81f5f5..0307412ce74ce 100644 --- a/src/librustc_trans/trans/expr.rs +++ b/src/librustc_trans/trans/expr.rs @@ -928,7 +928,25 @@ fn trans_rvalue_stmt_unadjusted<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, controlflow::trans_cont(bcx, expr.id, label_opt) } ast::ExprRet(ref ex) => { - controlflow::trans_ret(bcx, ex.as_ref().map(|e| &**e)) + // Check to see if the return expression itself is reachable. + // This can occur when the inner expression contains a return + let reachable = if let Some(ref cfg) = bcx.fcx.cfg { + cfg.node_is_reachable(expr.id) + } else { + true + }; + + if reachable { + controlflow::trans_ret(bcx, ex.as_ref().map(|e| &**e)) + } else { + // If it's not reachable, just translate the inner expression + // directly. This avoids having to manage a return slot when + // it won't actually be used anyway. + if let &Some(ref x) = ex { + bcx = trans_into(bcx, &**x, Ignore); + } + bcx + } } ast::ExprWhile(ref cond, ref body, _) => { controlflow::trans_while(bcx, expr.id, &**cond, &**body) From 9115b319c31d644f01387c9fe3eff9a498941090 Mon Sep 17 00:00:00 2001 From: James Miller Date: Tue, 16 Dec 2014 13:09:35 +1300 Subject: [PATCH 3/5] Fix formatting issues --- src/librustc_trans/trans/base.rs | 3 ++- src/librustc_trans/trans/controlflow.rs | 2 +- src/librustc_trans/trans/expr.rs | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index 87f9d4c447f9f..2cfcce2a961a9 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -1381,7 +1381,8 @@ fn build_cfg(tcx: &ty::ctxt, id: ast::NodeId) -> (ast::NodeId, Option) _ => tcx.sess.bug("unexpected expr variant in has_nested_returns") } } - Some(ast_map::NodeVariant(..)) | Some(ast_map::NodeStructCtor(..)) => return (ast::DUMMY_NODE_ID, None), + Some(ast_map::NodeVariant(..)) | + Some(ast_map::NodeStructCtor(..)) => return (ast::DUMMY_NODE_ID, None), // glue, shims, etc None if id == ast::DUMMY_NODE_ID => return (ast::DUMMY_NODE_ID, None), diff --git a/src/librustc_trans/trans/controlflow.rs b/src/librustc_trans/trans/controlflow.rs index e55da561c9409..550cf93b314e9 100644 --- a/src/librustc_trans/trans/controlflow.rs +++ b/src/librustc_trans/trans/controlflow.rs @@ -112,7 +112,7 @@ pub fn trans_block<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, if dest != expr::Ignore { let block_ty = node_id_type(bcx, b.id); - + if b.expr.is_none() || type_is_zero_size(bcx.ccx(), block_ty) { dest = expr::Ignore; } else if b.expr.is_some() { diff --git a/src/librustc_trans/trans/expr.rs b/src/librustc_trans/trans/expr.rs index 0307412ce74ce..6cefda5973766 100644 --- a/src/librustc_trans/trans/expr.rs +++ b/src/librustc_trans/trans/expr.rs @@ -944,7 +944,7 @@ fn trans_rvalue_stmt_unadjusted<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, // it won't actually be used anyway. if let &Some(ref x) = ex { bcx = trans_into(bcx, &**x, Ignore); - } + } bcx } } From b4f54f96df60337cb939f6f753a1cc6181e70f4b Mon Sep 17 00:00:00 2001 From: James Miller Date: Wed, 17 Dec 2014 13:24:35 +1300 Subject: [PATCH 4/5] Minor fixes --- src/librustc/middle/cfg/mod.rs | 5 +---- src/librustc/middle/graph.rs | 6 ++---- src/librustc_trans/trans/base.rs | 2 +- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/librustc/middle/cfg/mod.rs b/src/librustc/middle/cfg/mod.rs index 0e9bd42a23a13..04f86d0a9bad4 100644 --- a/src/librustc/middle/cfg/mod.rs +++ b/src/librustc/middle/cfg/mod.rs @@ -51,9 +51,6 @@ impl CFG { } pub fn node_is_reachable(&self, id: ast::NodeId) -> bool { - for node in self.graph.depth_traverse(self.entry) { - if node.id == id { return true } - } - return false; + self.graph.depth_traverse(self.entry).any(|node| node.id == id) } } diff --git a/src/librustc/middle/graph.rs b/src/librustc/middle/graph.rs index 4397f03a64286..3ba72801e2bff 100644 --- a/src/librustc/middle/graph.rs +++ b/src/librustc/middle/graph.rs @@ -313,12 +313,10 @@ pub struct DepthFirstTraversal<'g, N:'g, E:'g> { impl<'g, N, E> Iterator<&'g N> for DepthFirstTraversal<'g, N, E> { fn next(&mut self) -> Option<&'g N> { - while self.stack.len() > 0 { - let idx = self.stack.pop().unwrap(); - if self.visited.contains(&idx.node_id()) { + while let Some(idx) = self.stack.pop() { + if self.visited.insert(idx.node_id()) { continue; } - self.visited.insert(idx.node_id()); self.graph.each_outgoing_edge(idx, |_, e| -> bool { if !self.visited.contains(&e.target().node_id()) { self.stack.push(e.target()); diff --git a/src/librustc_trans/trans/base.rs b/src/librustc_trans/trans/base.rs index 2cfcce2a961a9..5b90dec8323b9 100644 --- a/src/librustc_trans/trans/base.rs +++ b/src/librustc_trans/trans/base.rs @@ -1468,7 +1468,7 @@ pub fn new_fn_ctxt<'a, 'tcx>(ccx: &'a CrateContext<'a, 'tcx>, let debug_context = debuginfo::create_function_debug_context(ccx, id, param_substs, llfndecl); let (blk_id, cfg) = build_cfg(ccx.tcx(), id); let nested_returns = if let Some(ref cfg) = cfg { - has_nested_returns(ccx.tcx(), cfg, blk_id) + has_nested_returns(ccx.tcx(), cfg, blk_id) } else { false }; From 5722410f72d0698f6ad9ba668e2282ff0bac5043 Mon Sep 17 00:00:00 2001 From: James Miller Date: Thu, 18 Dec 2014 17:43:50 +1300 Subject: [PATCH 5/5] Fix logic error and add unreachable after returns --- src/librustc/middle/graph.rs | 2 +- src/librustc_trans/trans/expr.rs | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/librustc/middle/graph.rs b/src/librustc/middle/graph.rs index 3ba72801e2bff..45bdf1c89cfc3 100644 --- a/src/librustc/middle/graph.rs +++ b/src/librustc/middle/graph.rs @@ -314,7 +314,7 @@ pub struct DepthFirstTraversal<'g, N:'g, E:'g> { impl<'g, N, E> Iterator<&'g N> for DepthFirstTraversal<'g, N, E> { fn next(&mut self) -> Option<&'g N> { while let Some(idx) = self.stack.pop() { - if self.visited.insert(idx.node_id()) { + if !self.visited.insert(idx.node_id()) { continue; } self.graph.each_outgoing_edge(idx, |_, e| -> bool { diff --git a/src/librustc_trans/trans/expr.rs b/src/librustc_trans/trans/expr.rs index 6cefda5973766..b185e8098e42b 100644 --- a/src/librustc_trans/trans/expr.rs +++ b/src/librustc_trans/trans/expr.rs @@ -945,6 +945,10 @@ fn trans_rvalue_stmt_unadjusted<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, if let &Some(ref x) = ex { bcx = trans_into(bcx, &**x, Ignore); } + // Mark the end of the block as unreachable. Once we get to + // a return expression, there's no more we should be doing + // after this. + Unreachable(bcx); bcx } }