Use control-flow information to improve handling of returns

src/librustc/middle/cfg/mod.rs

@@ -49,4 +49,8 @@ impl CFG {
                blk: &ast::Block) -> CFG {
         construct::construct(tcx, blk)
     }
+
+    pub fn node_is_reachable(&self, id: ast::NodeId) -> bool {
+        self.graph.depth_traverse(self.entry).any(|node| node.id == id)
+    }
 }

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<N,E> {
     nodes: Vec<Node<N>> ,
@@ -294,6 +295,40 @@ impl<N,E> Graph<N,E> {
             }
         }
     }
+
+    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<N, E>,
+    stack: Vec<NodeIndex>,
+    visited: BitvSet
+}
+
+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()) {
+                continue;
+            }
+            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<F>(max_edge_index: EdgeIndex, mut f: F) where

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:FnOnce()+Send>(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);

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<cfg::CFG>) {
+    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,58 @@ 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)))
+}
+
+// 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) {
+            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 +1466,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 +1490,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 {

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<Vec<cleanup::CleanupScope<'a, 'tcx>>>,
+
+    pub cfg: Option<cfg::CFG>,
 }
 
 impl<'a, 'tcx> FunctionContext<'a, 'tcx> {

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;
+                }
+            }
         }
     }
 

src/librustc_trans/trans/expr.rs

@@ -928,7 +928,29 @@ 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);
+                }
+                // 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
+            }
         }
         ast::ExprWhile(ref cond, ref body, _) => {
             controlflow::trans_while(bcx, expr.id, &**cond, &**body)