rollup merge of rust-lang#19898: Aatch/issue-19684

rust-lang#16081 fixed an issue where a nested return statement would cause incorrect behaviour due to the inner return writing over the return stack slot that had already been written too. However, the check was very broad and picked many cases that wouldn't ever be affected by this issue.

As a result, the number of allocas increased dramatically and therefore stack-size increased. LLVM is not able to remove all of the extraneous allocas. Any code that had multiple return values in a compound expression at the end of a function (including loops) would be hit by the issue.

The check now uses a control-flow graph to only consider the case when the inner return is executed conditionally. By itself, this narrowed definition causes rust-lang#15763 to return, so the control-flow graph is also used to avoid passing the return slot as a destination when the result won't be used.

This change allows the stack-size of the main rustc task to be reduced to 8MB from 32MB.

Author: alexcrichton

src/librustc/middle/cfg/mod.rs

@@ -48,4 +48,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>> ,
@@ -288,6 +289,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

@@ -480,7 +480,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;
@@ -1306,47 +1307,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")
             }
@@ -1356,11 +1343,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")
                     }
@@ -1380,11 +1363,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")
                     }
@@ -1398,24 +1377,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:
@@ -1454,7 +1467,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,
@@ -1473,7 +1491,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;
@@ -262,6 +263,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

@@ -926,7 +926,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)