-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Cranelift: Doesn't compile anymore on latest Windows Nightly #8919
Comments
This might just be a temporary / general issue with the latest Windows nightly compiler. This does not reproduce on any other target, not even the GNU variant. |
Would you be able to run a bisection to determine what caused this? The |
It's pretty late here, I'll look into it tomorrow. |
I was not able to reproduce it outside of the Github Actions, so I'll bisect it there manually now. |
The regression happened between |
The only thing that stands out to me is some changes to LLD and the patchable function entries. |
I was able to reproduce it locally and run cargo bisect-rustc on it. The regression is in rust-lang/rust@99f77a2 which is rust-lang/rust#127076 |
Thanks for investigating! Nothing obvious there and my hunch would be rust-lang/rust#126970 but even then it's probably a case where we had a really deep stack before and just happened to overflow now. Since you're able to reproduce locally are you able to capture a backtrace to see where the stack overflow is coming from? |
This is how you can reproduce it (with a sufficiently up to date Windows MSVC nightly toolchain):
what's interesting is that this does NOT reproduce it (which is slightly different for cargo in how it's able to reuse dependencies of the host vs. the target):
|
Mmmh, that does not look super healthy regardless of whether it's a slight regression. But I believe you are right. The PR you linked does inline the |
cc @jameysharp |
This recursion certainly could be rewritten to use an explicit stack. Its depth is proportional to the number of constraints in the left-hand side of a rule, so I didn't expect it to be a problem. It would be nice to figure out which rule it was generating code for at the time. (It must be a mid-end optimization rule if It's also worth noting that since build scripts are compiled in debug mode, rustc is building this without optimization. |
In bytecodealliance#8919 we learned that `Codegen::emit_block` can overrun the stack when generating Rust source for deeply-nested blocks. Stack usage can be worse than one might expect because ISLE is called from a build script, and those are always built in debug mode. This commit avoids the problem by using an explicit heap-allocated stack instead of recursion. I recommend turning on the "ignore whitespace" option in your diff tool of choice when reviewing this commit. The amount of stack space that this recursive function uses is affected by the version of rustc used to compile it, and the amount of stack space available depends on the host platform. As a result, this was only observed on Windows with a recent nightly version of rustc, but it could eventually affect other platforms or compiler versions, especially as ISLE rules grow in complexity. Note that there are several other places in ISLE which recurse over the user-provided rules. This commit does not change those, but they could also become a problem in the future.
@CryZe, could you test whether #8935 fixes this issue for you? There are several other parts of ISLE which also recurse over the structure of a user-provided rule, so I'm concerned that we may have this problem again in the future. But |
The PR does indeed seem to fix the issue. Thanks for working on it :) |
* ISLE: Make block codegen non-recursive In #8919 we learned that `Codegen::emit_block` can overrun the stack when generating Rust source for deeply-nested blocks. Stack usage can be worse than one might expect because ISLE is called from a build script, and those are always built in debug mode. This commit avoids the problem by using an explicit heap-allocated stack instead of recursion. I recommend turning on the "ignore whitespace" option in your diff tool of choice when reviewing this commit. The amount of stack space that this recursive function uses is affected by the version of rustc used to compile it, and the amount of stack space available depends on the host platform. As a result, this was only observed on Windows with a recent nightly version of rustc, but it could eventually affect other platforms or compiler versions, especially as ISLE rules grow in complexity. Note that there are several other places in ISLE which recurse over the user-provided rules. This commit does not change those, but they could also become a problem in the future. * Review comments: make stack manipulation local to emit_block
I forgot to mark that PR as fixing this, so now that it's merged I'll close this issue. |
* ISLE: Make block codegen non-recursive In bytecodealliance#8919 we learned that `Codegen::emit_block` can overrun the stack when generating Rust source for deeply-nested blocks. Stack usage can be worse than one might expect because ISLE is called from a build script, and those are always built in debug mode. This commit avoids the problem by using an explicit heap-allocated stack instead of recursion. I recommend turning on the "ignore whitespace" option in your diff tool of choice when reviewing this commit. The amount of stack space that this recursive function uses is affected by the version of rustc used to compile it, and the amount of stack space available depends on the host platform. As a result, this was only observed on Windows with a recent nightly version of rustc, but it could eventually affect other platforms or compiler versions, especially as ISLE rules grow in complexity. Note that there are several other places in ISLE which recurse over the user-provided rules. This commit does not change those, but they could also become a problem in the future. * Review comments: make stack manipulation local to emit_block
* ISLE: Make block codegen non-recursive In bytecodealliance#8919 we learned that `Codegen::emit_block` can overrun the stack when generating Rust source for deeply-nested blocks. Stack usage can be worse than one might expect because ISLE is called from a build script, and those are always built in debug mode. This commit avoids the problem by using an explicit heap-allocated stack instead of recursion. I recommend turning on the "ignore whitespace" option in your diff tool of choice when reviewing this commit. The amount of stack space that this recursive function uses is affected by the version of rustc used to compile it, and the amount of stack space available depends on the host platform. As a result, this was only observed on Windows with a recent nightly version of rustc, but it could eventually affect other platforms or compiler versions, especially as ISLE rules grow in complexity. Note that there are several other places in ISLE which recurse over the user-provided rules. This commit does not change those, but they could also become a problem in the future. * Review comments: make stack manipulation local to emit_block
* ISLE: Make block codegen non-recursive In #8919 we learned that `Codegen::emit_block` can overrun the stack when generating Rust source for deeply-nested blocks. Stack usage can be worse than one might expect because ISLE is called from a build script, and those are always built in debug mode. This commit avoids the problem by using an explicit heap-allocated stack instead of recursion. I recommend turning on the "ignore whitespace" option in your diff tool of choice when reviewing this commit. The amount of stack space that this recursive function uses is affected by the version of rustc used to compile it, and the amount of stack space available depends on the host platform. As a result, this was only observed on Windows with a recent nightly version of rustc, but it could eventually affect other platforms or compiler versions, especially as ISLE rules grow in complexity. Note that there are several other places in ISLE which recurse over the user-provided rules. This commit does not change those, but they could also become a problem in the future. * Review comments: make stack manipulation local to emit_block Co-authored-by: Jamey Sharp <jsharp@fastly.com>
Quite likely related: rust-lang/rust#128422 That's fundamentally a |
@sunshowers I don't think the fix above would address that issue unfortunately -- Jamey's change removes recursion (in favor of iteration with an explicit stack) in the ISLE DSL-to-Rust compiler, but the generated Rust code that causes rust-lang/rust#128422 will be exactly the same. (They're certainly related in the sense that the matcher decision tree gets quite deep!) |
That makes sense! Thanks @cfallin. |
Versions and Environment
Cranelift version or commit:
cranelift-codegen v0.104.3
as well ascranelift-codegen v0.109.0
Operating system: Windows
Architecture: x86_64
Rust: x86_64-pc-windows-msvc
latest update on 2024-07-08, rust version 1.81.0-nightly (20ae37c18 2024-07-07)
Actually seems to have been happening since at least:
latest update on 2024-06-30, rust version 1.81.0-nightly (ba1d7f4a0 2024-06-29)
Last time I saw it working was:
latest update on 2024-06-24, rust version 1.81.0-nightly (bcf94dec5 2024-06-23)
Extra Info
The text was updated successfully, but these errors were encountered: