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.

Sign up for GitHub

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 incorrect assumption around the presence of ICF frames in EH codebase for 64-bit targets #34526

Merged
merged 1 commit into from
Apr 6, 2020
Merged

Fix incorrect assumption around the presence of ICF frames in EH codebase for 64-bit targets #34526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 7 additions & 25 deletions src/coreclr/src/vm/exceptionhandling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,10 +1104,8 @@ ProcessCLRException(IN PEXCEPTION_RECORD pExceptionRecord

CLRUnwindStatus status;

#ifdef USE_PER_FRAME_PINVOKE_INIT
// Refer to comment in ProcessOSExceptionNotification about ICF and codegen difference.
InlinedCallFrame *pICFSetAsLimitFrame = NULL;
#endif // USE_PER_FRAME_PINVOKE_INIT

status = pTracker->ProcessOSExceptionNotification(
pExceptionRecord,
Expand All @@ -1116,11 +1114,8 @@ ProcessCLRException(IN PEXCEPTION_RECORD pExceptionRecord
dwExceptionFlags,
sf,
pThread,
STState
#ifdef USE_PER_FRAME_PINVOKE_INIT
, (PVOID)pICFSetAsLimitFrame
#endif // USE_PER_FRAME_PINVOKE_INIT
);
STState,
(PVOID)pICFSetAsLimitFrame);

if (FirstPassComplete == status)
{
Expand Down Expand Up @@ -1223,7 +1218,6 @@ ProcessCLRException(IN PEXCEPTION_RECORD pExceptionRecord


CONSISTENCY_CHECK(pLimitFrame > dac_cast<PTR_VOID>(GetSP(pContextRecord)));
#ifdef USE_PER_FRAME_PINVOKE_INIT
if (pICFSetAsLimitFrame != NULL)
{
_ASSERTE(pICFSetAsLimitFrame == pLimitFrame);
Expand All @@ -1235,7 +1229,6 @@ ProcessCLRException(IN PEXCEPTION_RECORD pExceptionRecord
// the next pinvoke callsite does not see the frame as active.
pICFSetAsLimitFrame->Reset();
}
#endif // USE_PER_FRAME_PINVOKE_INIT

pThread->SetFrame(pLimitFrame);

Expand Down Expand Up @@ -1715,11 +1708,8 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification(
DWORD dwExceptionFlags,
StackFrame sf,
Thread* pThread,
StackTraceState STState
#ifdef USE_PER_FRAME_PINVOKE_INIT
, PVOID pICFSetAsLimitFrame
#endif // USE_PER_FRAME_PINVOKE_INIT
)
StackTraceState STState,
PVOID pICFSetAsLimitFrame)
{
CONTRACTL
{
Expand Down Expand Up @@ -1785,10 +1775,8 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification(
this->m_EnclosingClauseInfoForGCReporting.SetEnclosingClauseCallerSP(uCallerSP);
}

#ifdef USE_PER_FRAME_PINVOKE_INIT
// Refer to detailed comment below.
PTR_Frame pICFForUnwindTarget = NULL;
#endif // USE_PER_FRAME_PINVOKE_INIT

CheckForRudeAbort(pThread, fIsFirstPass);

Expand Down Expand Up @@ -1817,15 +1805,12 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification(

while (((UINT_PTR)pFrame) < uCallerSP)
{
#ifdef USE_PER_FRAME_PINVOKE_INIT
// InlinedCallFrames (ICF) are allocated, initialized and linked to the Frame chain
// by the code generated by the JIT for a method containing a PInvoke.
//
// On X64, JIT generates code to dynamically link and unlink the ICF around
// each PInvoke call. On ARM, on the other hand, JIT's codegen, in context of ICF,
// is more inline with X86 and thus, it links in the ICF at the start of the method
// and unlinks it towards the method end. Thus, ICF is present on the Frame chain
// at any given point so long as the method containing the PInvoke is on the stack.
// JIT generates code that links in the ICF at the start of the method and unlinks it towards
// the method end. Thus, ICF is present on the Frame chain at any given point so long as the
// method containing the PInvoke is on the stack.
//
// Now, if the method containing ICF catches an exception, we will reset the Frame chain
// with the LimitFrame, that is computed below, after the catch handler returns. Since this
Expand Down Expand Up @@ -1895,7 +1880,6 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification(
}
}
}
#endif // USE_PER_FRAME_PINVOKE_INIT

cfThisFrame.CheckGSCookies();

Expand Down Expand Up @@ -2040,7 +2024,6 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification(

if (fTargetUnwind && (status == SecondPassComplete))
{
#ifdef USE_PER_FRAME_PINVOKE_INIT
// If we have got a ICF to set as the LimitFrame, do that now.
// The Frame chain is still intact and would be updated using
// the LimitFrame (done after the catch handler returns).
Expand All @@ -2052,7 +2035,6 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification(
m_pLimitFrame = pICFForUnwindTarget;
pICFSetAsLimitFrame = (PVOID)pICFForUnwindTarget;
}
#endif // USE_PER_FRAME_PINVOKE_INIT

// Since second pass is complete and we have reached
// the frame containing the catch funclet, reset the enclosing
Expand Down
10 changes: 2 additions & 8 deletions src/coreclr/src/vm/exceptionhandling.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@
#include "eexcp.h"
#include "exstatecommon.h"

#if defined(TARGET_ARM) || defined(TARGET_X86)
#define USE_PER_FRAME_PINVOKE_INIT
Copy link
Member

Choose a reason for hiding this comment

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

This will need matching changes in the JIT as well. You should see a functional test failures without matching changes in the JIT.

Copy link
Contributor Author

@fadimounir fadimounir Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The JIT today links/unlinks the ICF in the method prolog/epilog, and activates/deactivates the ICF around the pinvoke call. It does this for both 32-bits and 64-bits. The fix here is to match the behavior in the EH codebase.

#endif // TARGET_ARM || TARGET_X86

// This address lies in the NULL pointer partition of the process memory.
// Accessing it will result in AV.
#define INVALID_RESUME_ADDRESS 0x000000000000bad0
Expand Down Expand Up @@ -195,10 +191,8 @@ class ExceptionTracker
DWORD dwExceptionFlags,
StackFrame sf,
Thread* pThread,
StackTraceState STState
#ifdef USE_PER_FRAME_PINVOKE_INIT
, PVOID pICFSetAsLimitFrame
#endif // USE_PER_FRAME_PINVOKE_INIT
StackTraceState STState,
PVOID pICFSetAsLimitFrame
);

CLRUnwindStatus ProcessExplicitFrame(
Expand Down