-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Enable interpreter stack overflow handling #122204
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
Enable interpreter stack overflow handling #122204
Conversation
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds stack overflow handling for the interpreter stack, which was previously not being handled, causing CoreCLR stack overflow tests to fail. The changes implement checks to detect when a called method frame would exceed the interpreter stack limits and trigger appropriate stack overflow handling.
Key Changes:
- Added
HandleInterpreterStackOverflowfunction to properly handle stack overflow exceptions in the interpreter - Added stack overflow checks before allocating new interpreter frames
- Fixed
Thread::VirtualUnwindToFirstManagedCallFrameto properly detect and unwind interpreter frames
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/vm/stackwalk.cpp | Added logic to detect interpreter frames during stack unwinding and set context appropriately when unwinding past the topmost interpreter frame |
| src/coreclr/vm/interpexec.cpp | Added HandleInterpreterStackOverflow function and stack overflow checks at frame entry points; reorganized IP initialization to occur before stack overflow checks |
davidwrighton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine for StackOverflow situations caused on the interpreter stack, but what about when the thread OS stack is in trouble. Do we want to consider comparing the InterpMethodContextFrame's address to GetThread()->GetCachedStackSufficientExecutionLimit()and doing something there? We have several alloca's in the interpreter codebase, all of which are technically capable of causing overflows.
That already worked - it triggers real stack overflow and gets handled properly. One of the stack overflow tests hits that. |
Stack overflow of the interpreter stack was not being handled yet. The stack overflow coreclr test was failing due to that. This change adds checks when a called method frame would not fit the interpreter stack and fires the stack overflow handling in that case. I also had to fix the Thread::UnwindToFirstManagedFrame to work for the interpreter frames. Before that, it would not detect them and it would unwind through the InterpExecMethod and further native frames.
8c81e27 to
e497a3c
Compare
Stack overflow of the interpreter stack was not being handled yet. The stack overflow coreclr test was failing due to that.
This change adds checks when a called method frame would not fit the interpreter stack and fires the stack overflow handling in that case.
I also had to fix the Thread::UnwindToFirstManagedFrame to work for the interpreter frames. Before that, it would not detect them and it would unwind through the InterpExecMethod and further native frames.