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

Fix unwound stack range check for the new EH #99041

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

janvorli
Copy link
Member

With the old EH, the unwound stack range limits are using SP for each frame on x64 and a caller SP on arm64, arm, riscv64 and longsoon. The ExceptionTracker::IsInStackRegionUnwoundBySpecifiedException takes that into consideration when detecting whether a frame with a specific SP was already unwound or not.
With the new EH though, the range is always based on the actual SP of the frame, so the check was being done incorrectly on non-x64 architectures.
That lead to a crash in a test that has dynamic method code on stack and its frame was unwound. The bug has caused that method to not to be reported to GC, so the dynamic method was destroyed by GC. And later on, another GC stack walk has crashed while trying to get info on the method.
This change fixes it by making the check with new EH enabled the same for all architectures, the same as it was for x64 in the old EH.

Close #98914

With the old EH, the unwound stack range limits are using SP for each
frame on x64 and a caller SP on arm64, arm, riscv64 and longsoon.
The ExceptionTracker::IsInStackRegionUnwoundBySpecifiedException takes
that into consideration when detecting whether a frame with a specific
SP was already unwound or not.
With the new EH though, the range is always based on the actual SP of
the frame, so the check was being done incorrectly on non-x64
architectures.
That lead to a crash in a test that has dynamic method code on stack
and its frame was unwound. The bug has caused that method to not to be
reported to GC, so the dynamic method was destroyed by GC. And later on,
another GC stack walk has crashed while trying to get info on the
method.
This change fixes it by making the check with new EH enabled the same
for all architectures, the same as it was for x64 in the old EH.

Close dotnet#98914
@janvorli janvorli added this to the 9.0.0 milestone Feb 28, 2024
@janvorli janvorli requested a review from jkotas February 28, 2024 13:31
@janvorli janvorli self-assigned this Feb 28, 2024
@janvorli
Copy link
Member Author

/azp run runtime-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@janvorli
Copy link
Member Author

/azp run runtime-coreclr crossgen2

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@janvorli
Copy link
Member Author

/backport to release/9.0-preview2

Copy link
Contributor

Started backporting to release/9.0-preview2: https://github.com/dotnet/runtime/actions/runs/8081325683

@janvorli
Copy link
Member Author

The CI failures are known - the #98817

@janvorli janvorli merged commit da808b0 into dotnet:main Feb 28, 2024
198 of 202 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure: Regressions/coreclr/GitHub_45929/test45929/test45929.cmd
2 participants