Rust: regression: stack overflow on macosx with xcode 6.4
The build.rs of mach-test in the mach crate commit https://github.com/fitzgen/mach/commit/d31c809c22724c5dad198037ad03bfec7906e016 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: https://github.com/fitzgen/mach/pull/49
gnzlbg
All 24 comments
This is showing up in curl-rust as well -- https://github.com/alexcrichton/curl-rust/issues/234
(I haven't reproduced it myself yet)
alexcrichton
on 29 Oct 2018
cc @oli-obk, the stack trace in the curl issue looks like const eval things?
alexcrichton
on 29 Oct 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.
oli-obk
on 29 Oct 2018
er sorry I apologize, I misremembered the stack trace as being const related when in fact it's simply MIR construction related. My bad!
alexcrichton
on 29 Oct 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
on 29 Oct 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?
oli-obk
on 29 Oct 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
on 29 Oct 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.
nagisa
on 29 Oct 2018
Does this happen on beta?
pietroalbini
on 31 Oct 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
gnzlbg
on 31 Oct 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
on 1 Nov 2018
visited for triage. Looks like progress is moving forward at a good pace on PR #55617.
pnkfelix
on 8 Nov 2018
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.
pnkfelix
on 29 Nov 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
oli-obk
on 29 Nov 2018
Ping @oli-obk, any progress on this? Release date is approaching.
pietroalbini
on 3 Dec 2018
It is too risky to backport
We could double the stack size on beta to buy us another 6 weeks.
oli-obk
on 3 Dec 2018
The 2018 release will have this stack overflow (https://github.com/rust-lang/rust/pull/56468#issuecomment-443942583), but the following compilers won't.
oli-obk
on 4 Dec 2018
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
on 13 Dec 2018
T-compiler triage: assigning to self for looking into the oddity mentioned in the previous comment.
pnkfelix
on 20 Dec 2018
I have addressed the default stack size problem with https://github.com/rust-lang/rust/pull/56813
oli-obk
on 20 Dec 2018
Ping, what needs to be backported to beta?
pietroalbini
on 1 Jan 2019
Ping @rust-lang/compiler, what needs to be backported to beta?
pietroalbini
on 6 Jan 2019
https://github.com/rust-lang/rust/pull/56813 is what would be backported if we end up wanting to do a backport at all.
nagisa
on 6 Jan 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
on 11 Jan 2019
Related issues
overvenus
·
3Comments
SharplEr
·
3Comments
zhendongsu
·
3Comments
Robbepop
·
3Comments
wthrowe
·
3Comments
Most helpful comment
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.