Auto merge of rust-lang#1908 - alexcrichton:moar-stack, r=huonw

There have been a number of reports of Cargo triggering a stack overflow in the
algorithm implemented in `cargo::core::resolve`, and although many attempts have
been made to reduce the stack space of the two relevant recursive functions it
seems likely that this will not always be enough. For now, before moving the
recursion to the heap manually, spawn the main thread with a large stack (e.g.
mirror what the compiler does) to ensure that the same scenarios happen across
platforms at least.

Currently on my machine I get a 2MB stack on Linux and a 512K stack on OSX, so
bumping this up to 8MB should be more than enough for the recursion in this
algorithm. I also hope that with nonzeroing drop a few of the recursive calls
will be able to become tail recursive, which should also help with stack space!

Closes rust-lang#1897

src/bin/cargo.rs

@@ -11,6 +11,7 @@ use std::fs;
 use std::io;
 use std::path::{PathBuf, Path};
 use std::process::Command;
+use std::thread::Builder;
 
 use cargo::{execute_main_without_stdin, handle_error, shell};
 use cargo::core::MultiShell;
@@ -57,7 +58,18 @@ See 'cargo help <command>' for more information on a specific command.
 
 fn main() {
     env_logger::init().unwrap();
-    execute_main_without_stdin(execute, true, USAGE)
+
+    // Right now the algorithm in cargo::core::resolve is pretty recursive and
+    // runs the risk of blowing the stack. Platforms tend to have different
+    // stack limits by default (I just witnessed 512K on OSX and 2MB on Linux)
+    // so to get a consistent experience just spawn ourselves with a large stack
+    // size.
+    let stack_size = env::var("CARGO_STACK_SIZE").ok()
+                         .and_then(|s| s.parse().ok())
+                         .unwrap_or(8 * 1024 * 1024); // 8MB
+    Builder::new().stack_size(stack_size).spawn(|| {
+        execute_main_without_stdin(execute, true, USAGE)
+    }).unwrap().join().unwrap();
 }
 
 macro_rules! each_subcommand{ ($mac:ident) => ({

src/cargo/core/resolver/mod.rs

@@ -243,7 +243,7 @@ struct Context {
 }
 
 /// Builds the list of all packages required to build the first argument.
-pub fn resolve(summary: &Summary, method: Method,
+pub fn resolve(summary: &Summary, method: &Method,
                registry: &mut Registry) -> CargoResult<Resolve> {
     trace!("resolve; summary={}", summary.package_id());
     let summary = Rc::new(summary.clone());
@@ -272,7 +272,7 @@ pub fn resolve(summary: &Summary, method: Method,
 fn activate(mut cx: Box<Context>,
             registry: &mut Registry,
             parent: &Rc<Summary>,
-            method: Method,
+            method: &Method,
             finished: &mut FnMut(Box<Context>, &mut Registry) -> ResolveResult)
             -> ResolveResult {
     // Dependency graphs are required to be a DAG, so we keep a set of
@@ -284,7 +284,7 @@ fn activate(mut cx: Box<Context>,
     }
 
     // If we're already activated, then that was easy!
-    if cx.flag_activated(parent, &method) {
+    if cx.flag_activated(parent, method) {
         cx.visited.remove(id);
         return finished(cx, registry)
     }
@@ -293,7 +293,7 @@ fn activate(mut cx: Box<Context>,
     let deps = try!(cx.build_deps(registry, parent, method));
 
     // Extracting the platform request.
-    let platform = match method {
+    let platform = match *method {
         Method::Required { target_platform, .. } => target_platform,
         Method::Everything => None,
     };
@@ -382,7 +382,7 @@ fn activate_deps<'a>(cx: Box<Context>,
         if !dep.is_transitive() {
             my_cx.visited.clear();
         }
-        let my_cx = try!(activate(my_cx, registry, candidate, method,
+        let my_cx = try!(activate(my_cx, registry, candidate, &method,
                                   &mut |cx, registry| {
             activate_deps(cx, registry, parent, platform, deps.clone(), cur + 1,
                           finished)
@@ -515,12 +515,12 @@ fn compatible(a: &semver::Version, b: &semver::Version) -> bool {
 // The all used features set is the set of features which this local package had
 // enabled, which is later used when compiling to instruct the code what
 // features were enabled.
-fn build_features(s: &Summary, method: Method)
+fn build_features(s: &Summary, method: &Method)
                   -> CargoResult<(HashMap<String, Vec<String>>, HashSet<String>)> {
     let mut deps = HashMap::new();
     let mut used = HashSet::new();
     let mut visited = HashSet::new();
-    match method {
+    match *method {
         Method::Everything => {
             for key in s.features().keys() {
                 try!(add_feature(s, key, &mut deps, &mut used, &mut visited));
@@ -536,7 +536,7 @@ fn build_features(s: &Summary, method: Method)
             }
         }
     }
-    match method {
+    match *method {
         Method::Everything |
         Method::Required { uses_default_features: true, .. } => {
             if s.features().get("default").is_some() {
@@ -632,7 +632,7 @@ impl Context {
     #[inline(never)] // see notes at the top of the module
     fn build_deps<'a>(&mut self, registry: &mut Registry,
                       parent: &'a Summary,
-                      method: Method) -> CargoResult<Vec<DepInfo<'a>>> {
+                      method: &Method) -> CargoResult<Vec<DepInfo<'a>>> {
         // First, figure out our set of dependencies based on the requsted set
         // of features. This also calculates what features we're going to enable
         // for our own dependencies.
@@ -669,9 +669,9 @@ impl Context {
     }
 
     #[allow(deprecated)] // connect => join in 1.3
-    fn resolve_features<'a>(&mut self, parent: &'a Summary, method: Method)
+    fn resolve_features<'a>(&mut self, parent: &'a Summary, method: &Method)
             -> CargoResult<Vec<(&'a Dependency, Vec<String>)>> {
-        let dev_deps = match method {
+        let dev_deps = match *method {
             Method::Everything => true,
             Method::Required { dev_deps, .. } => dev_deps,
         };
@@ -683,7 +683,7 @@ impl Context {
         // Second, ignoring dependencies that should not be compiled for this
         // platform
         let deps = deps.filter(|d| {
-            match method {
+            match *method {
                 Method::Required{target_platform: Some(ref platform), ..} => {
                     d.is_active_for_platform(platform)
                 },

src/cargo/ops/resolve.rs

@@ -103,7 +103,7 @@ pub fn resolve_with_previous<'a>(registry: &mut PackageRegistry,
         None => summary,
     };
 
-    let mut resolved = try!(resolver::resolve(&summary, method, registry));
+    let mut resolved = try!(resolver::resolve(&summary, &method, registry));
     match previous {
         Some(r) => resolved.copy_metadata(r),
         None => {}

tests/resolve.rs

@@ -16,7 +16,7 @@ fn resolve<R: Registry>(pkg: PackageId, deps: Vec<Dependency>,
                         -> CargoResult<Vec<PackageId>> {
     let summary = Summary::new(pkg, deps, HashMap::new()).unwrap();
     let method = Method::Everything;
-    Ok(try!(resolver::resolve(&summary, method, registry)).iter().map(|p| {
+    Ok(try!(resolver::resolve(&summary, &method, registry)).iter().map(|p| {
         p.clone()
     }).collect())
 }