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 VS debugger compensation for the new EH #98847

Merged
merged 2 commits into from
Feb 23, 2024

Conversation

janvorli
Copy link
Member

In a recent fix, I've fixed a problem that VS debugger has with the new exception handling reporting correct address for a hardware exception on a stack trace. However, that has broken the
JIT/opt/Vectorization/UnrollEqualsStartsWith test when it is run with tiered compilation disabled. In that test, the faulting instruction is the first one in a method and the compensation for the old EH issue has moved the address to the previous method. And then a call to PreserveStackTrace that the new EH ends up calling due to the way it propagates exceptions over internal native boundaries ends up asserting down in the DebugStackTrace::GetStackFramesInternal call chain, because the compensated address and the MethodDesc attached to it in the stack frame didn't match.

This change moves the compensation to the
DebugStackTrace::GetStackFramesFromException and only for the DAC code. That way only the debugger gets the compensated value.

In a recent fix, I've fixed a problem that VS debugger has with the new
exception handling reporting correct address for a hardware exception on
a stack trace. However, that has broken the
JIT/opt/Vectorization/UnrollEqualsStartsWith test when it is run with
tiered compilation disabled. In that test, the faulting instruction is
the first one in a method and the compensation for the old EH issue has
moved the address to the previous method. And then a call to
PreserveStackTrace that the new EH ends up calling due to the way it
propagates exceptions over internal native boundaries ends up asserting
down in the DebugStackTrace::GetStackFramesInternal call chain, because
the compensated address and the MethodDesc attached to it in the stack
frame didn't match.

This change moves the compensation to the
DebugStackTrace::GetStackFramesFromException and only for the DAC code.
That way only the debugger gets the compensated value.
@janvorli janvorli added this to the 9.0.0 milestone Feb 23, 2024
@janvorli janvorli requested a review from jkotas February 23, 2024 09:36
@janvorli janvorli self-assigned this Feb 23, 2024
@janvorli
Copy link
Member Author

The CI failure is the #98817

@jkotas
Copy link
Member

jkotas commented Feb 23, 2024

The simulation of this bug seems to be causing a cascade of problems. Do you have a plan for deleting the simulation of this bug and fixing the tests?

@janvorli
Copy link
Member Author

@jkotas it will require fixing VS, not a test. The glass test has just uncovered the problem. I will work with VS folks to find out a fix that would still allow VS to work correctly with the older runtimes.

And I don't really see a cascade of problems stemming from the previous workaround. The original workaround was correct for debugger but it has caused trouble for the runtime stuff and this change moves the fix to a debugger only place which is more appropriate. I have not caught the runtime trouble during local testing of that fix as it occurs in a specific test only when tiered compilation is off.

The debugger also special-cases this for x64
@janvorli
Copy link
Member Author

@jkotas I have updated it to be x64 specific as the debugger also special cases this for x64 only.

@janvorli janvorli merged commit 42b5348 into dotnet:main Feb 23, 2024
111 checks passed
@janvorli janvorli deleted the fix-vs-debugger-compensation branch February 23, 2024 18:53
@github-actions github-actions bot locked and limited conversation to collaborators Mar 25, 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.

2 participants