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 assert in Thread::VirtualUnwindCallFrame on ARM64 #102350

Merged
merged 1 commit into from
May 17, 2024

Conversation

janvorli
Copy link
Member

My recent change to fix windows arm64 unwinding for native code has started to trigger an assert in jitstress tests. The problem is that the codeinfo that is passed to the Thread::VirtualUnwindCallFrame in case of managed frames was created from an unadjusted PC, while the check that we use to verify the codeinfo validity in debug builds gets the function entry from the OS using the adjusted address.
The fix is to perform the adjustment only when codeinfo is not passed in.

Close #102337

My recent change to fix windows arm64 unwinding for native code has
started to trigger an assert in jitstress tests. The problem is that the
codeinfo that is passed to the Thread::VirtualUnwindCallFrame in case of
managed frames was created from an unadjusted PC, while the check that
we use to verify the codeinfo validity in debug builds gets the function
entry from the OS using the adjusted address.
The fix is to perform the adjustment only when codeinfo is not passed
in.
@janvorli janvorli requested a review from jkotas May 16, 2024 23:22
@janvorli janvorli self-assigned this May 16, 2024
Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@janvorli
Copy link
Member Author

janvorli commented May 16, 2024

@jkotas I was originally considering instantiating the CodeInfo from PC adjusted in the same manner, but there are too many places where that happens.
Moreover, some of those don't have an information about whether the PC was unwound to a call.

@janvorli
Copy link
Member Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@@ -554,7 +554,11 @@ PCODE Thread::VirtualUnwindCallFrame(T_CONTEXT* pContext,
PT_RUNTIME_FUNCTION pFunctionEntry;

#if !defined(TARGET_UNIX) && defined(TARGET_ARM64)
if ((pContext->ContextFlags & CONTEXT_UNWOUND_TO_CALL) != 0)
// We don't adjust the control PC when we have a code info, as the code info is always created from an unadjusted one
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the unadjusted code info going to work well for the unwind? In other words, can we hit the bug that you have fixed when the code info is passed in?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code info is passed in only for managed frames. If it was not the case, the code info would not be valid and the assert I am fixing would be firing.
Return address in managed code is never out of the bounds of the method containing the call, otherwise unwinding managed code would be crashing / hanging on arm64 Windows both with and without the new EH.

@janvorli janvorli merged commit 727f32b into dotnet:main May 17, 2024
110 of 114 checks passed
@janvorli janvorli deleted the fix-assert-in-virtualunwindcallframe branch May 17, 2024 09:35
@jakobbotsch
Copy link
Member

Any idea what causes jitstress in particular to hit this issue?

@janvorli
Copy link
Member Author

@jakobbotsch the way how the managed method gets covered by multiple RUNTIME_FUNCTIONS seems to be jitstress specific.

@jakobbotsch
Copy link
Member

Ah, this is likely our unwind stress mode then. In that mode we split the function into multiple fragments even when the function doesn't exceed the max fragment size.

Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
My recent change to fix windows arm64 unwinding for native code has
started to trigger an assert in jitstress tests. The problem is that the
codeinfo that is passed to the Thread::VirtualUnwindCallFrame in case of
managed frames was created from an unadjusted PC, while the check that
we use to verify the codeinfo validity in debug builds gets the function
entry from the OS using the adjusted address.
The fix is to perform the adjustment only when codeinfo is not passed
in.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants