Fix x86 InlinedCallFrame popping during EH #2215
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jkotas
reviewed
Jan 27, 2020
src/coreclr/src/vm/i386/excepx86.cpp
Outdated
| { | ||
| // The current explicit frame is below the resume esp and it is an inlined call frame with an active call. | ||
| // |
There was a problem hiding this comment.
Nit: You can fix typos in the comment below while you are on it:
unlined
pinovoke
jkotas
reviewed
Jan 27, 2020
src/coreclr/src/vm/i386/excepx86.cpp
Outdated
| @@ -1868,8 +1868,10 @@ NOINLINE LPVOID COMPlusEndCatchWorker(Thread * pThread) | |||
| // Sync managed exception state, for the managed thread, based upon any active exception tracker | |||
| pThread->SyncManagedExceptionState(fIsDebuggerHelperThread); | |||
|
|
|||
| if (InlinedCallFrame::FrameHasActiveCall(pThread->m_pFrame)) | |||
| if ((pThread->m_pFrame < esp) && InlinedCallFrame::FrameHasActiveCall(pThread->m_pFrame)) | |||
There was a problem hiding this comment.
How does this work?
I believe that we need to check whether the frame is within the method that we are resuming into, ie we need to check whether pFrame < callerESP.
There was a problem hiding this comment.
Hmm, you are right that this would not work for the case when the location we resume to is in the same function as the pinvoke frame. It works when it is anywhere up the call chain.
|
Could you please also delete the workaround for this bug from AssemblyNative::InternalLoad ? |
There is a bug in InlinedCallFrame popping on x86 during EH with R2R compiled code. The code is popping this frame if it is at the top of the explicit frames stack before executing catch handler. But it is missing a check to see if the frame was actually in the unwound part of the stack. So in some corner cases, it can end up popping a frame that belongs to an active pinvoke somewhere up the call chain. The fix is to do the popping only if the InlinedCallFrame is located at an address smaller than the resume ESP.
The correct ESP to use for detecting the InlinedCallFrame affected by the current exception handling is the the ESP of the caller of the method that contains the exception handler. I have moved the check to a different location where the CrawlFrame with the context of the exception handler is still available and we can unwind it to get the caller's SP.
janvorli
force-pushed
the
fix-x86-inlinedcallframe-pop
branch
from
7c32f98 to
54720ec
Compare
|
@jkotas I've removed the workaround for the issue, can you please take a look again? |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is a bug in InlinedCallFrame popping on x86 during EH with R2R
compiled code. The code is popping this frame if it is at the top of
the explicit frames stack before executing catch handler. But it
is missing a check to see if the frame was actually in the unwound part
of the stack. So in some corner cases, it can end up popping a frame
that belongs to an active pinvoke somewhere up the call chain.
The fix is to do the popping only if the InlinedCallFrame is located at
an address smaller than the resume ESP.
Close #2240