Skip to content

Commit

Permalink
Auto merge of #55617 - oli-obk:stacker, r=<try>
Browse files Browse the repository at this point in the history 
Prevent compiler stack overflow for deeply recursive code

I was unable to write a test that

1. runs in under 1s
2. overflows on my machine without this patch

The following reproduces the issue, but I don't think it's sensible to include a test that takes 30s to compile. We can now easily squash newly appearing overflows by the strategic insertion of calls to `ensure_sufficient_stack`.

```rust
// compile-pass

#![recursion_limit="1000000"]

macro_rules! chain {
    (EE $e:expr) => {$e.sin()};
    (RECURSE $i:ident $e:expr) => {chain!($i chain!($i chain!($i chain!($i $e))))};
    (Z $e:expr) => {chain!(RECURSE EE $e)};
    (Y $e:expr) => {chain!(RECURSE Z $e)};
    (X $e:expr) => {chain!(RECURSE Y $e)};
    (A $e:expr) => {chain!(RECURSE X $e)};
    (B $e:expr) => {chain!(RECURSE A $e)};
    (C $e:expr) => {chain!(RECURSE B $e)};
    // causes overflow on x86_64 linux
    // less than 1 second until overflow on test machine
    // after overflow has been fixed, takes 30s to compile :/
    (D $e:expr) => {chain!(RECURSE C $e)};
    (E $e:expr) => {chain!(RECURSE D $e)};
    (F $e:expr) => {chain!(RECURSE E $e)};
    // more than 10 seconds
    (G $e:expr) => {chain!(RECURSE F $e)};
    (H $e:expr) => {chain!(RECURSE G $e)};
    (I $e:expr) => {chain!(RECURSE H $e)};
    (J $e:expr) => {chain!(RECURSE I $e)};
    (K $e:expr) => {chain!(RECURSE J $e)};
    (L $e:expr) => {chain!(RECURSE L $e)};
}

fn main() {
    let x = chain!(D 42.0_f32);
}
```

fixes #55471
fixes #41884
fixes #40161
fixes #34844
fixes #32594

cc @alexcrichton @rust-lang/compiler

I looked at all code that checks the recursion limit and inserted stack growth calls where appropriate.
  • Loading branch information
@bors
bors committed Nov 13, 2018
2 parents 485397e + a4ebef0 commit 5009042
Show file tree
Hide file tree
Showing 18 changed files with 274 additions and 210 deletions.
12 changes: 12 additions & 0 deletions src/Cargo.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1904,6 +1904,7 @@ dependencies = [
"scoped-tls 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
"serialize 0.0.0",
"smallvec 0.6.5 (registry+https://github.com/rust-lang/crates.io-index)",
"stacker 0.1.4 (git+https://github.com/oli-obk/stacker.git)",
"syntax 0.0.0",
"syntax_pos 0.0.0",
"tempfile 3.0.3 (registry+https://github.com/rust-lang/crates.io-index)",
Expand Down Expand Up @@ -2662,6 +2663,16 @@ name = "stable_deref_trait"
version = "1.1.0"
source = "registry+https://github.com/rust-lang/crates.io-index"

[[package]]
name = "stacker"
version = "0.1.4"
source = "git+https://github.com/oli-obk/stacker.git#30b32ea0514bac734539eccd4157e6d8684abcb6"
dependencies = [
"cc 1.0.25 (registry+https://github.com/rust-lang/crates.io-index)",
"cfg-if 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)",
"libc 0.2.43 (registry+https://github.com/rust-lang/crates.io-index)",
]

[[package]]
name = "std"
version = "0.0.0"
Expand Down Expand Up @@ -3369,6 +3380,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
"checksum smallvec 0.6.5 (registry+https://github.com/rust-lang/crates.io-index)" = "153ffa32fd170e9944f7e0838edf824a754ec4c1fc64746fcc9fe1f8fa602e5d"
"checksum socket2 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)" = "c4d11a52082057d87cb5caa31ad812f4504b97ab44732cd8359df2e9ff9f48e7"
"checksum stable_deref_trait 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "ffbc596e092fe5f598b12ef46cc03754085ac2f4d8c739ad61c4ae266cc3b3fa"
"checksum stacker 0.1.4 (git+https://github.com/oli-obk/stacker.git)" = "<none>"
"checksum string_cache 0.7.3 (registry+https://github.com/rust-lang/crates.io-index)" = "25d70109977172b127fe834e5449e5ab1740b9ba49fa18a2020f509174f25423"
"checksum string_cache_codegen 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)" = "35293b05cf1494e8ddd042a7df6756bf18d07f42d234f32e71dce8a7aabb0191"
"checksum string_cache_shared 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)" = "b1884d1bc09741d466d9b14e6d37ac89d6909cbcac41dd9ae982d4d063bbedfc"
Expand Down
1 change: 1 addition & 0 deletions src/librustc/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ byteorder = { version = "1.1", features = ["i128"]}
chalk-engine = { version = "0.8.0", default-features=false }
rustc_fs_util = { path = "../librustc_fs_util" }
smallvec = { version = "0.6.5", features = ["union"] }
stacker = { git = "https://github.com/oli-obk/stacker.git" }

# Note that these dependencies are a lie, they're just here to get linkage to
# work.
Expand Down
257 changes: 130 additions & 127 deletions src/librustc/hir/lowering.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ use hir::GenericArg;
use lint::builtin::{self, PARENTHESIZED_PARAMS_IN_TYPES_AND_MODULES,
ELIDED_LIFETIMES_IN_PATHS};
use middle::cstore::CrateStore;
use middle::recursion_limit::ensure_sufficient_stack;
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::indexed_vec::IndexVec;
use rustc_data_structures::thin_vec::ThinVec;
Expand Down Expand Up @@ -3659,8 +3660,8 @@ impl<'a> LoweringContext<'a> {
})
}

fn lower_expr(&mut self, e: &Expr) -> hir::Expr {
let kind = match e.node {
fn lower_expr_kind(&mut self, e: &Expr) -> hir::ExprKind {
match e.node {
ExprKind::Box(ref inner) => hir::ExprKind::Box(P(self.lower_expr(inner))),
ExprKind::ObsoleteInPlace(..) => {
self.sess.abort_if_errors();
Expand Down Expand Up @@ -3959,19 +3960,11 @@ impl<'a> LoweringContext<'a> {
let struct_path = self.std_path(e.span, &struct_path, None, is_unit);
let struct_path = hir::QPath::Resolved(None, P(struct_path));

let LoweredNodeId { node_id, hir_id } = self.lower_node_id(e.id);

return hir::Expr {
id: node_id,
hir_id,
node: if is_unit {
if is_unit {
hir::ExprKind::Path(struct_path)
} else {
hir::ExprKind::Struct(struct_path, fields, None)
},
span: e.span,
attrs: e.attrs.clone(),
};
}
}
ExprKind::Path(ref qself, ref path) => {
let qpath = self.lower_qpath(
Expand Down Expand Up @@ -4050,18 +4043,7 @@ impl<'a> LoweringContext<'a> {
fields.iter().map(|x| self.lower_field(x)).collect(),
maybe_expr.as_ref().map(|x| P(self.lower_expr(x))),
),
ExprKind::Paren(ref ex) => {
let mut ex = self.lower_expr(ex);
// include parens in span, but only if it is a super-span.
if e.span.contains(ex.span) {
ex.span = e.span;
}
// merge attributes into the inner expression.
let mut attrs = e.attrs.clone();
attrs.extend::<Vec<_>>(ex.attrs.into());
ex.attrs = attrs;
return ex;
}
ExprKind::Paren(_) => bug!("parens are handled in `lower_expr`"),

ExprKind::Yield(ref opt_expr) => {
self.is_generator = true;
Expand Down Expand Up @@ -4173,6 +4155,128 @@ impl<'a> LoweringContext<'a> {
loop_expr
}

ExprKind::ForLoop(..) => bug!(),

// Desugar ExprKind::Try
// From: `<expr>?`
ExprKind::Try(ref sub_expr) => {
// to:
//
// match Try::into_result(<expr>) {
// Ok(val) => #[allow(unreachable_code)] val,
// Err(err) => #[allow(unreachable_code)]
// // If there is an enclosing `catch {...}`
// break 'catch_target Try::from_error(From::from(err)),
// // Otherwise
// return Try::from_error(From::from(err)),
// }

let unstable_span =
self.allow_internal_unstable(CompilerDesugaringKind::QuestionMark, e.span);

// Try::into_result(<expr>)
let discr = {
// expand <expr>
let sub_expr = self.lower_expr(sub_expr);

let path = &["ops", "Try", "into_result"];
let path = P(self.expr_std_path(
unstable_span, path, None, ThinVec::new()));
P(self.expr_call(e.span, path, hir_vec![sub_expr]))
};

// #[allow(unreachable_code)]
let attr = {
// allow(unreachable_code)
let allow = {
let allow_ident = Ident::from_str("allow").with_span_pos(e.span);
let uc_ident = Ident::from_str("unreachable_code").with_span_pos(e.span);
let uc_nested = attr::mk_nested_word_item(uc_ident);
attr::mk_list_item(e.span, allow_ident, vec![uc_nested])
};
attr::mk_spanned_attr_outer(e.span, attr::mk_attr_id(), allow)
};
let attrs = vec![attr];

// Ok(val) => #[allow(unreachable_code)] val,
let ok_arm = {
let val_ident = self.str_to_ident("val");
let val_pat = self.pat_ident(e.span, val_ident);
let val_expr = P(self.expr_ident_with_attrs(
e.span,
val_ident,
val_pat.id,
ThinVec::from(attrs.clone()),
));
let ok_pat = self.pat_ok(e.span, val_pat);

self.arm(hir_vec![ok_pat], val_expr)
};

// Err(err) => #[allow(unreachable_code)]
// return Try::from_error(From::from(err)),
let err_arm = {
let err_ident = self.str_to_ident("err");
let err_local = self.pat_ident(e.span, err_ident);
let from_expr = {
let path = &["convert", "From", "from"];
let from = P(self.expr_std_path(
e.span, path, None, ThinVec::new()));
let err_expr = self.expr_ident(e.span, err_ident, err_local.id);

self.expr_call(e.span, from, hir_vec![err_expr])
};
let from_err_expr =
self.wrap_in_try_constructor("from_error", from_expr, unstable_span);
let thin_attrs = ThinVec::from(attrs);
let catch_scope = self.catch_scopes.last().map(|x| *x);
let ret_expr = if let Some(catch_node) = catch_scope {
P(self.expr(
e.span,
hir::ExprKind::Break(
hir::Destination {
label: None,
target_id: Ok(catch_node),
},
Some(from_err_expr),
),
thin_attrs,
))
} else {
P(self.expr(e.span, hir::ExprKind::Ret(Some(from_err_expr)), thin_attrs))
};

let err_pat = self.pat_err(e.span, err_local);
self.arm(hir_vec![err_pat], ret_expr)
};

hir::ExprKind::Match(
discr,
hir_vec![err_arm, ok_arm],
hir::MatchSource::TryDesugar,
)
}

ExprKind::Mac(_) => panic!("Shouldn't exist here"),
}
}


fn lower_expr(&mut self, e: &Expr) -> hir::Expr {
// parens and for loops have some custom desugaring going on where the node ids aren't
// just lowered from the expression
let kind = match e.node {
ExprKind::Paren(ref ex) => {
let mut ex = ensure_sufficient_stack(|| self.lower_expr(ex));
// include parens in span, but only if it is a super-span.
if e.span.contains(ex.span) {
ex.span = e.span;
}
// merge attributes into the inner expression.
ex.attrs.extend(e.attrs.iter().cloned());
return ex;
},

// Desugar ExprForLoop
// From: `[opt_ident]: for <pat> in <head> <body>`
ExprKind::ForLoop(ref pat, ref head, ref body, opt_label) => {
Expand Down Expand Up @@ -4341,109 +4445,8 @@ impl<'a> LoweringContext<'a> {
let block = P(self.block_all(e.span, hir_vec![let_stmt], Some(result)));
// add the attributes to the outer returned expr node
return self.expr_block(block, e.attrs.clone());
}

// Desugar ExprKind::Try
// From: `<expr>?`
ExprKind::Try(ref sub_expr) => {
// to:
//
// match Try::into_result(<expr>) {
// Ok(val) => #[allow(unreachable_code)] val,
// Err(err) => #[allow(unreachable_code)]
// // If there is an enclosing `catch {...}`
// break 'catch_target Try::from_error(From::from(err)),
// // Otherwise
// return Try::from_error(From::from(err)),
// }

let unstable_span =
self.allow_internal_unstable(CompilerDesugaringKind::QuestionMark, e.span);

// Try::into_result(<expr>)
let discr = {
// expand <expr>
let sub_expr = self.lower_expr(sub_expr);

let path = &["ops", "Try", "into_result"];
let path = P(self.expr_std_path(
unstable_span, path, None, ThinVec::new()));
P(self.expr_call(e.span, path, hir_vec![sub_expr]))
};

// #[allow(unreachable_code)]
let attr = {
// allow(unreachable_code)
let allow = {
let allow_ident = Ident::from_str("allow").with_span_pos(e.span);
let uc_ident = Ident::from_str("unreachable_code").with_span_pos(e.span);
let uc_nested = attr::mk_nested_word_item(uc_ident);
attr::mk_list_item(e.span, allow_ident, vec![uc_nested])
};
attr::mk_spanned_attr_outer(e.span, attr::mk_attr_id(), allow)
};
let attrs = vec![attr];

// Ok(val) => #[allow(unreachable_code)] val,
let ok_arm = {
let val_ident = self.str_to_ident("val");
let val_pat = self.pat_ident(e.span, val_ident);
let val_expr = P(self.expr_ident_with_attrs(
e.span,
val_ident,
val_pat.id,
ThinVec::from(attrs.clone()),
));
let ok_pat = self.pat_ok(e.span, val_pat);

self.arm(hir_vec![ok_pat], val_expr)
};

// Err(err) => #[allow(unreachable_code)]
// return Try::from_error(From::from(err)),
let err_arm = {
let err_ident = self.str_to_ident("err");
let err_local = self.pat_ident(e.span, err_ident);
let from_expr = {
let path = &["convert", "From", "from"];
let from = P(self.expr_std_path(
e.span, path, None, ThinVec::new()));
let err_expr = self.expr_ident(e.span, err_ident, err_local.id);

self.expr_call(e.span, from, hir_vec![err_expr])
};
let from_err_expr =
self.wrap_in_try_constructor("from_error", from_expr, unstable_span);
let thin_attrs = ThinVec::from(attrs);
let catch_scope = self.catch_scopes.last().map(|x| *x);
let ret_expr = if let Some(catch_node) = catch_scope {
P(self.expr(
e.span,
hir::ExprKind::Break(
hir::Destination {
label: None,
target_id: Ok(catch_node),
},
Some(from_err_expr),
),
thin_attrs,
))
} else {
P(self.expr(e.span, hir::ExprKind::Ret(Some(from_err_expr)), thin_attrs))
};

let err_pat = self.pat_err(e.span, err_local);
self.arm(hir_vec![err_pat], ret_expr)
};

hir::ExprKind::Match(
discr,
hir_vec![err_arm, ok_arm],
hir::MatchSource::TryDesugar,
)
}

ExprKind::Mac(_) => panic!("Shouldn't exist here"),
},
_ => ensure_sufficient_stack(|| self.lower_expr_kind(e)),
};

let LoweredNodeId { node_id, hir_id } = self.lower_node_id(e.id);
Expand Down
1 change: 1 addition & 0 deletions src/librustc/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ extern crate serialize as rustc_serialize; // used by deriving
extern crate rustc_apfloat;
extern crate byteorder;
extern crate backtrace;
extern crate stacker;

#[macro_use]
extern crate smallvec;
Expand Down
26 changes: 20 additions & 6 deletions src/librustc/middle/recursion_limit.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,32 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Recursion limit.
//
// There are various parts of the compiler that must impose arbitrary limits
// on how deeply they recurse to prevent stack overflow. Users can override
// this via an attribute on the crate like `#![recursion_limit="22"]`. This pass
// just peeks and looks for that attribute.
//! Recursion limit.
//!
//! There are various parts of the compiler that must impose arbitrary limits
//! on how deeply they recurse to prevent stack overflow. Users can override
//! this via an attribute on the crate like `#![recursion_limit="22"]`. This pass
//! just peeks and looks for that attribute.

use session::Session;
use syntax::ast;

use rustc_data_structures::sync::Once;

const RED_ZONE: usize = 100*1024; // 100k
const STACK_PER_RECURSION: usize = 1 * 1024 * 1024; // 1MB

/// Grows the stack on demand to prevent stack overflow. Call this in strategic locations
/// to "break up" recursive calls. E.g. almost any call to `visit_expr` or equivalent can benefit
/// from this.
///
/// Should not be sprinkled around carelessly, as it causes a little bit of overhead.
pub fn ensure_sufficient_stack<R, F: FnOnce() -> R>(
f: F
) -> R {
stacker::maybe_grow(RED_ZONE, STACK_PER_RECURSION, f)
}

pub fn update_limits(sess: &Session, krate: &ast::Crate) {
update_limit(sess, krate, &sess.recursion_limit, "recursion_limit",
"recursion limit", 64);
Expand Down
Loading

0 comments on commit 5009042

Please sign in to comment.