Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

regression: stack overflow on macosx with xcode 6.4 #55471

Closed
gnzlbg opened this issue Oct 29, 2018 · 24 comments · Fixed by #55617
Closed

regression: stack overflow on macosx with xcode 6.4 #55471

gnzlbg opened this issue Oct 29, 2018 · 24 comments · Fixed by #55617
Assignees
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Oct 29, 2018

The build.rs of mach-test in the mach crate commit fitzgen/mach@d31c809 crashes on MacOS X with a stack overflow when xcode 6.4 is used. This used to work, but is now failing on nightly, so this stack overflow is a regression. I don't have access to a MacOSX system with xcode 6.4 but travis reproduces this reliably. The PR in mach that works around this is: fitzgen/mach#49

@alexcrichton
Copy link
Member

This is showing up in curl-rust as well -- alexcrichton/curl-rust#234

(I haven't reproduced it myself yet)

@alexcrichton alexcrichton added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 29, 2018
@alexcrichton
Copy link
Member

cc @oli-obk, the stack trace in the curl issue looks like const eval things?

@oli-obk
Copy link
Contributor

oli-obk commented Oct 29, 2018

You mean because of the mir_const query call? That's a normal step in any compilation. It's just the MIR that const eval needs to run on. It will continue to get processed later, but it's already the regular MIR that you will get for any function.

@alexcrichton
Copy link
Member

er sorry I apologize, I misremembered the stack trace as being const related when in fact it's simply MIR construction related. My bad!

@oli-obk
Copy link
Contributor

oli-obk commented Oct 29, 2018

No worries. I'm knee deep in Mir building code right now anyway, I'll build a repro that I can run on x86 and see what it takes to remove a few intermediate stack frames or locals.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 29, 2018

Actually, we might want to fix this once and forever. I think we can change the recursion into a loop (well obviously, but I mean without making the code impossible to maintain). Cc @rust-lang/compiler do you think it's reasonable to attempt refactoring Mir building code to use stacks or should we patch this over and wait for guaranteed TCO?

@nagisa
Copy link
Member

nagisa commented Oct 29, 2018

We already have an outstanding issue to reduce the default stack size from 16MB back to 8MB (upping the stack to 16MB was a temporary-not-temporary workaround to a similar issue). Compiler should not restrict itself with code down to the fairly arbitrarily chosen size of the compiler’s stack.

@nagisa
Copy link
Member

nagisa commented Oct 29, 2018

That being said I do not believe it is sensible to single-out the MIR building code – there are quite a few other areas that are implemented recursively and could be problematic. I feel that we should simply implement some sort of dynamic stack sizing functionality for rustc. I don’t think it is necessary to make it able to reduce the stack – only increasing when the stack limit is reached ought to be fine IMO.

@pietroalbini
Copy link
Member

Does this happen on beta?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Oct 31, 2018

Does this happen on beta?

Yes, I've modified mach CI to test that, and it produces a stack overflow in beta as well: https://travis-ci.org/gnzlbg/mach/builds/448907619

@pietroalbini pietroalbini added regression-from-stable-to-beta Performance or correctness regression from stable to beta. I-nominated and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Oct 31, 2018
@nagisa
Copy link
Member

nagisa commented Nov 1, 2018

Discussed during T-compiler meeting. Broadly there was consensus on employing something like stacker crate to grow the stack as necessary.

@oli-obk “volunteered” to be the assignee and looking into implementing this.

P-high.

@nagisa nagisa added P-high High priority and removed I-nominated labels Nov 1, 2018
bors added a commit that referenced this issue Nov 6, 2018
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.
@pnkfelix
Copy link
Member

pnkfelix commented Nov 8, 2018

visited for triage. Looks like progress is moving forward at a good pace on PR #55617.

bors added a commit that referenced this issue Nov 13, 2018
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.
@pnkfelix
Copy link
Member

triage: progress still seems to continue on PR #55617. I am not clear whether @oli-obk plans to move forward with the idea of making a version that applies solely to Mac+Linux.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 29, 2018

No, @alexcrichton expressed strong discomfort with fixing it just for 2 of 3 tier 1 platforms. I am still attempting to get the full fix done this week

@pietroalbini
Copy link
Member

Ping @oli-obk, any progress on this? Release date is approaching.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 3, 2018

It is too risky to backport

We could double the stack size on beta to buy us another 6 weeks.

bors added a commit that referenced this issue Dec 4, 2018
Bump stack size to 32MB

cc #55471

A short term solution (really this time! Full fix being grown in #55617) for stack overflows due to deeply recursive syntax trees.

r? @nagisa

cc @alexcrichton @eddyb @michaelwoerister @pietroalbini
@oli-obk
Copy link
Contributor

oli-obk commented Dec 4, 2018

The 2018 release will have this stack overflow (#56468 (comment)), but the following compilers won't.

@pietroalbini pietroalbini removed this from the Rust 2018 Release milestone Dec 4, 2018
@pnkfelix
Copy link
Member

T-compiler triage: The problem might have been addressed in the short term via PRs #56467 and #56518. However, it is unclear whether PR #56467 was actually the right short term fix, because @gnzlbg reports that they still get a stack overflow on nightly unless they use RUST_MIN_STACK=32000000.

  • It seems super-weird that we successfully bumped the stack via that environment variable but not via the default value that was changed in the compiler. We probably need to look at that quickly, just to check if there's some underlying broader bug of which this is a symptom.

The longer-term fix is in-progress on PR #55617.

@pnkfelix
Copy link
Member

T-compiler triage: assigning to self for looking into the oddity mentioned in the previous comment.

@pnkfelix pnkfelix self-assigned this Dec 20, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Dec 20, 2018

I have addressed the default stack size problem with #56813

@pietroalbini
Copy link
Member

Ping, what needs to be backported to beta?

@pietroalbini
Copy link
Member

Ping @rust-lang/compiler, what needs to be backported to beta?

@nagisa
Copy link
Member

nagisa commented Jan 6, 2019

#56813 is what would be backported if we end up wanting to do a backport at all.

@nagisa
Copy link
Member

nagisa commented Jan 11, 2019

I’m going to close this issue because the regression itself is now gone. Just the PR is enough to track a more general fix.

@nagisa nagisa closed this as completed Jan 11, 2019
Centril added a commit to Centril/rust that referenced this issue Mar 18, 2020
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 rust-lang#55471
fixes rust-lang#41884
fixes rust-lang#40161
fixes rust-lang#34844
fixes rust-lang#32594

cc @alexcrichton @rust-lang/compiler

I looked at all code that checks the recursion limit and inserted stack growth calls where appropriate.
bors added a commit to rust-lang-ci/rust that referenced this issue May 7, 2020
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 rust-lang#55471
fixes rust-lang#41884
fixes rust-lang#40161
fixes rust-lang#34844
fixes rust-lang#32594

cc @alexcrichton @rust-lang/compiler

I looked at all code that checks the recursion limit and inserted stack growth calls where appropriate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants