rustc built by MIR overflows its stack for crates with very deep ASTs. #35408
Comments
eddyb
added
I-nominated
regression-from-stable-to-nightly
|
triage: P-high |
[MIR] Add Storage{Live,Dead} statements to emit llvm.lifetime.{start,end}.
Storage live ranges are tracked for all MIR variables and temporaries with a drop scope.
`StorageLive` is lowered to `llvm.lifetime.start` and `StorageDead` to `llvm.lifetime.end`.
There are some improvements possible here, such as:
* pack multiple storage liveness statements by using the index of first local + `u64` bitset
* enforce that locals are not directly accessed outside their storage live range
* shrink storage live ranges for never-borrowed locals to initialization -> last use
* emit storage liveness statements for *all* temporaries
* however, the remaining ones are *always* SSA immediates, so they'd be noop in MIR trans
* could have a flag on the temporary that its storage is irrelevant (a la C's old `register`)
* would also deny borrows if necessary
* this seems like an overcompliation and with packing & optimizations it may be pointless
Even in the current state, it helps stage2 `rustc` compile `boiler` without overflowing (see #35408).
A later addition fixes #26764 and closes #27372 by emitting `.section` directives for dylib metadata to avoid them being allocated into memory or read as `.note`. For this PR, those bugs were tripping valgrind.
arielb1
added
regression-from-stable-to-beta
|
May be fixed by #35764, we should do a crater run after that's merged. |
|
Actually, how are we going to do a crater run if we don't have an old trans switch? I suppose before-after on that PR might suffice (and it would catch other problems wrt drop flags). |
|
@eddyb well we can certainly just check the results for |
|
@nikomatsakis My manual check suggests it's fixed, but I want to see it on crater before closing. |
|
Crater report shows it didn't actually get fixed. |
|
@eddyb were you able to observe it failing before? |
|
@nikomatsakis Yes, and a stage2 build of #35764 was the first time I saw it work again. EDIT: Just tested again, stage1 overflows its stack (being built by beta), stage2 doesn't. |
|
@brson's beta crater runs reveals that @nagisa's If it is the change to remove filling drop from |
|
Its probably caused by the auto generated trie in We should really investigate that arbitrary sized initializers do not break On Aug 27, 2016 5:25 AM, "Eduard-Mihai Burtescu" notifications@github.com
|
|
For all its worth I coud not reproduce the issue with nightly from 08-04, but could reproduce with stage2 build of rustc built from my i128 PR. EDIT: the stack looks like this: |
|
What a waste of time, old trans is missing %self = alloca %"hir::lowering::LoweringContext"*, align 8
%e = alloca %"12.syntax::ast::Expr"*, align 8While MIR trans... well, MIR trans has What I missed the first time was that So the remaining theory is that there's just too many |
|
Something I noticed while looking at #36023 which may be relevant: we seem to be emitting |
|
@eddyb do you think that having more lifetime-start calls would cause problems? I guess it's possible. |
brson
added
the
I-crash
|
Heads up, I got danburkert/procinfo-rs#15 merged with a workaround for this issue in |
|
@rust-lang/compiler @eddyb please retriage this one. When we made it P-medium it was because we thought it was fixed, but it is not. |
|
Note that it was fixed (or did not manifest on |
|
@kamalmarhubi What value of |
Sure. The region hierarchy for match-expressions has the bindings destroyed only on the exit block of the match, so for example fn example(ex: Box<Result<[u32; 64], [i32; 64]>>) {
match *ex {
Ok(a) => {},
Err(b) => {}
}
}Translates to Here the live-ranges for |
|
@arielb1 Last I looked the numbers seemed to fit the non-overlapping situation, but maybe I wasn't paying enough attention. It'd be great if we can get a win from changing that? |
|
triage: P-high We should at least figure out if something changed since the last time we looked, though the solution seems pretty unclear. |
rust-highfive
added
P-high
|
All functions in librustc with with stack frames over 4kB: |
|
And in |
|
@arielb1 Nice! Could that be used on a stable release, prior to MIR trans being on by default? |
|
cd /tmp
cargo new foo
cd foo
echo 'procinfo = "=0.3.0"' >> Cargo.toml
rustup run <toolchain> cargo buildHere are the results I get:
So latest nightly is not reproing, but it looks like there has been a serious perf regression since 1.13. It also looks like 1.14 will have the issue if it isn't fixed before the release (which would be a regression from 1.13). |
|
cc @eddyb @nikomatsakis any updates? This is still tagged P-high so please circle back to it soon. |
|
No changes since 11/17. Still P-high. @nikomatsakis @eddyb please re-triage. |
|
triage: P-medium Still worthy of investigation -- and I hope to do so post-xmas -- but not a burning priority. More of a long-standing issue we need to improve bit by bit. |
rust-highfive
added
P-medium
|
There's been mucho activity investigating stack blowup issues on #40883. @arielb1 believes that the LLVM fixes linked from #40883 (comment) will be necessary to resolve these problems for us. |
|
I believe this should be fixed with the fix to #40883. |
|
This can probably be closed now. |
These examples have been found on crater:
boiler(failure)procinfo(failure)marksman_escape(failure)These reproduce with
stage2but notstage1, unlessstage0is passed-Z orbit, which means a compiler built by MIR trans will use more stack space than old trans did.For
procinfo, there are just over 1000 frames, usually in theNodeIdAssigner, my guess is that its AST has a depth 500-700 in places (it's usingnom, which might cause such extreme nesting).cc @rust-lang/compiler
The text was updated successfully, but these errors were encountered: