diff options
| -rw-r--r-- | src/coreclr/inc/corinfo.h | 5 | ||||
| -rw-r--r-- | src/coreclr/jit/importer.cpp | 50 | ||||
| -rw-r--r-- | src/coreclr/jit/lower.cpp | 21 | ||||
| -rw-r--r-- | src/coreclr/vm/exceptionhandling.cpp | 41 | ||||
| -rw-r--r-- | src/coreclr/vm/i386/excepx86.cpp | 21 | ||||
| -rw-r--r-- | src/tests/baseservices/exceptions/exceptioninterop/ExceptionInterop.cs | 34 |
6 files changed, 46 insertions, 126 deletions
diff --git a/src/coreclr/inc/corinfo.h b/src/coreclr/inc/corinfo.h index 9cfbb21fffe..5e97c53adb2 100644 --- a/src/coreclr/inc/corinfo.h +++ b/src/coreclr/inc/corinfo.h @@ -3244,9 +3244,4 @@ public: // #define IMAGE_REL_BASED_REL_THUMB_MOV32_PCREL 0x14 -/**********************************************************************************/ -#ifdef TARGET_64BIT -#define USE_PER_FRAME_PINVOKE_INIT -#endif - #endif // _COR_INFO_H_ diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index bdf66cc22ee..6fbafbcc993 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -8428,43 +8428,41 @@ bool Compiler::impCanPInvokeInlineCallSite(BasicBlock* block) return true; } -#ifdef USE_PER_FRAME_PINVOKE_INIT - // For platforms that use per-P/Invoke InlinedCallFrame initialization, - // we can't inline P/Invokes inside of try blocks where we can resume execution in the same function. - // The runtime can correctly unwind out of an InlinedCallFrame and out of managed code. However, - // it cannot correctly unwind out of an InlinedCallFrame and stop at that frame without also unwinding - // at least one managed frame. In particular, the runtime struggles to restore non-volatile registers - // from the top-most unmanaged call before the InlinedCallFrame. As a result, the runtime does not support - // re-entering the same method frame as the InlinedCallFrame after an exception in unmanaged code. +#ifdef TARGET_64BIT + // On 64-bit platforms, we disable pinvoke inlining inside of try regions. + // Note that this could be needed on other architectures too, but we + // haven't done enough investigation to know for sure at this point. + // + // Here is the comment from JIT64 explaining why: + // [VSWhidbey: 611015] - because the jitted code links in the + // Frame (instead of the stub) we rely on the Frame not being + // 'active' until inside the stub. This normally happens by the + // stub setting the return address pointer in the Frame object + // inside the stub. On a normal return, the return address + // pointer is zeroed out so the Frame can be safely re-used, but + // if an exception occurs, nobody zeros out the return address + // pointer. Thus if we re-used the Frame object, it would go + // 'active' as soon as we link it into the Frame chain. + // + // Technically we only need to disable PInvoke inlining if we're + // in a handler or if we're in a try body with a catch or + // filter/except where other non-handler code in this method + // might run and try to re-use the dirty Frame object. + // + // A desktop test case where this seems to matter is + // jit\jit64\ebvts\mcpp\sources2\ijw\__clrcall\vector_ctor_dtor.02\deldtor_clr.exe if (block->hasTryIndex()) { // This does not apply to the raw pinvoke call that is inside the pinvoke // ILStub. In this case, we have to inline the raw pinvoke call into the stub, // otherwise we would end up with a stub that recursively calls itself, and end // up with a stack overflow. - // This works correctly because the runtime never emits a catch block in a managed-to-native - // IL stub. If the runtime ever emits a catch block into a managed-to-native stub when using - // P/Invoke helpers, this condition will need to be revisited. if (opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB) && opts.ShouldUsePInvokeHelpers()) { return true; } - // Check if this block's try block or any containing try blocks have catch handlers. - // If any of the containing try blocks have catch handlers, - // we cannot inline a P/Invoke for reasons above. If the handler is a fault or finally handler, - // we can inline a P/Invoke into this block in the try since the code will not resume execution - // in the same method after throwing an exception if only fault or finally handlers are executed. - for (unsigned int ehIndex = block->getTryIndex(); ehIndex != EHblkDsc::NO_ENCLOSING_INDEX; - ehIndex = ehGetEnclosingTryIndex(ehIndex)) - { - if (ehGetDsc(ehIndex)->HasCatchHandler()) - { - return false; - } - } - - return true; + return false; } #endif // TARGET_64BIT diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index d995e6c4ce0..9753615d620 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4281,7 +4281,6 @@ GenTree* Lowering::CreateFrameLinkUpdate(FrameLinkAction action) // Return Value: // none // -// See the usages for USE_PER_FRAME_PINVOKE_INIT for more information. void Lowering::InsertPInvokeMethodProlog() { noway_assert(comp->info.compUnmanagedCallCountWithGCTransition); @@ -4378,16 +4377,13 @@ void Lowering::InsertPInvokeMethodProlog() // -------------------------------------------------------- // On 32-bit targets, CORINFO_HELP_INIT_PINVOKE_FRAME initializes the PInvoke frame and then pushes it onto // the current thread's Frame stack. On 64-bit targets, it only initializes the PInvoke frame. - // As a result, don't push the frame onto the frame stack here for any 64-bit targets CLANG_FORMAT_COMMENT_ANCHOR; #ifdef TARGET_64BIT -#ifdef USE_PER_FRAME_PINVOKE_INIT - // For IL stubs, we push the frame once even when we're doing per-pinvoke init. if (comp->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB)) -#endif // USE_PER_FRAME_PINVOKE_INIT { - // Push a frame. The init routine sets InlinedCallFrame's m_pNext, so we just set the thread's top-of-stack + // Push a frame - if we are NOT in an IL stub, this is done right before the call + // The init routine sets InlinedCallFrame's m_pNext, so we just set the thead's top-of-stack GenTree* frameUpd = CreateFrameLinkUpdate(PushFrame); firstBlockRange.InsertBefore(insertionPoint, LIR::SeqTree(comp, frameUpd)); ContainCheckStoreIndir(frameUpd->AsStoreInd()); @@ -4447,10 +4443,9 @@ void Lowering::InsertPInvokeMethodEpilog(BasicBlock* returnBB DEBUGARG(GenTree* // this in the epilog for IL stubs; for non-IL stubs the frame is popped after every PInvoke call. CLANG_FORMAT_COMMENT_ANCHOR; -#ifdef USE_PER_FRAME_PINVOKE_INIT - // For IL stubs, we push the frame once even when we're doing per-pinvoke init +#ifdef TARGET_64BIT if (comp->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB)) -#endif // USE_PER_FRAME_PINVOKE_INIT +#endif // TARGET_64BIT { GenTree* frameUpd = CreateFrameLinkUpdate(PopFrame); returnBlockRange.InsertBefore(insertionPoint, LIR::SeqTree(comp, frameUpd)); @@ -4606,7 +4601,7 @@ void Lowering::InsertPInvokeCallProlog(GenTreeCall* call) // contains PInvokes; on 64-bit targets this is necessary in non-stubs. CLANG_FORMAT_COMMENT_ANCHOR; -#ifdef USE_PER_FRAME_PINVOKE_INIT +#ifdef TARGET_64BIT if (!comp->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB)) { // Set the TCB's frame to be the one we just created. @@ -4618,7 +4613,7 @@ void Lowering::InsertPInvokeCallProlog(GenTreeCall* call) BlockRange().InsertBefore(insertBefore, LIR::SeqTree(comp, frameUpd)); ContainCheckStoreIndir(frameUpd->AsStoreInd()); } -#endif // USE_PER_FRAME_PINVOKE_INIT +#endif // TARGET_64BIT // IMPORTANT **** This instruction must be the last real instruction **** // It changes the thread's state to Preemptive mode @@ -4684,7 +4679,7 @@ void Lowering::InsertPInvokeCallEpilog(GenTreeCall* call) // this happens after every PInvoke call in non-stubs. 32-bit targets instead mark the frame as inactive. CLANG_FORMAT_COMMENT_ANCHOR; -#ifdef USE_PER_FRAME_PINVOKE_INIT +#ifdef TARGET_64BIT if (!comp->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_IL_STUB)) { tree = CreateFrameLinkUpdate(PopFrame); @@ -4708,7 +4703,7 @@ void Lowering::InsertPInvokeCallEpilog(GenTreeCall* call) BlockRange().InsertBefore(insertionPoint, constantZero, storeCallSiteTracker); ContainCheckStoreLoc(storeCallSiteTracker); -#endif // USE_PER_FRAME_PINVOKE_INIT +#endif // TARGET_64BIT } //------------------------------------------------------------------------ diff --git a/src/coreclr/vm/exceptionhandling.cpp b/src/coreclr/vm/exceptionhandling.cpp index 5ec98e6f8ac..21fe4076938 100644 --- a/src/coreclr/vm/exceptionhandling.cpp +++ b/src/coreclr/vm/exceptionhandling.cpp @@ -16,7 +16,6 @@ #include "virtualcallstub.h" #include "utilcode.h" #include "interoplibinterface.h" -#include "corinfo.h" #if defined(TARGET_X86) #define USE_CURRENT_CONTEXT_IN_FILTER @@ -1777,10 +1776,8 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification( // 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 platforms where USE_PER_FRAME_PINVOKE_INIT is not defined, - // the 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 + // 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 @@ -1818,16 +1815,13 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification( // below the callerSP for which we will invoke ExceptionUnwind. // // Thus, ICF::ExceptionUnwind should not do anything significant. If any of these assumptions - // break, then the next best thing will be to make the JIT link/unlink the frame dynamically + // break, then the next best thing will be to make the JIT link/unlink the frame dynamically. // - // If the current method executing is from precompiled ReadyToRun code, each PInvoke is wrapped - // by calls to the JIT_PInvokeBegin and JIT_PInvokeEnd helpers, - // which push and pop the ICF to the current thread. The ICF is not - // linked during the method prolog, and unlinked at the epilog. + // If the current method executing is from precompiled ReadyToRun code, then the above is no longer + // applicable because each PInvoke is wrapped by calls to the JIT_PInvokeBegin and JIT_PInvokeEnd + // helpers, which push and pop the ICF to the current thread. Unlike jitted code, the ICF is not + // linked during the method prolog, and unlinked at the epilog (it looks more like the X64 case). // In that case, we need to unlink the ICF during unwinding here. - // On platforms where USE_PER_FRAME_PINVOKE_INIT is defined, the JIT generates code that links in - // the ICF immediately before and after a PInvoke in non-IL-stubs, like ReadyToRun. - // See the usages for USE_PER_FRAME_PINVOKE_INIT for more information. if (fTargetUnwind && (pFrame->GetVTablePtr() == InlinedCallFrame::GetMethodFrameVPtr())) { @@ -1836,12 +1830,8 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification( // // 1) ICF address is higher than the current frame's SP (which we get from DispatcherContext), AND // 2) ICF address is below callerSP. - // 3) ICF is active. - // - IL stubs link the frame in for the whole stub, so if an exception is thrown during marshalling, - // the ICF will be on the frame chain and inactive. - if ((GetSP(pDispatcherContext->ContextRecord) < (TADDR)pICF) - && ((UINT_PTR)pICF < uCallerSP) - && InlinedCallFrame::FrameHasActiveCall(pICF)) + if ((GetSP(pDispatcherContext->ContextRecord) < (TADDR)pICF) && + ((UINT_PTR)pICF < uCallerSP)) { pICFForUnwindTarget = pFrame; @@ -1850,18 +1840,9 @@ CLRUnwindStatus ExceptionTracker::ProcessOSExceptionNotification( // to the JIT_PInvokeBegin and JIT_PInvokeEnd helpers, which push and pop the ICF on the thread. The // ICF is not linked at the method prolog and unlined at the epilog when running R2R code. Since the // JIT_PInvokeEnd helper will be skipped, we need to unlink the ICF here. If the executing method - // has another pinvoke, it will re-link the ICF again when the JIT_PInvokeBegin helper is called. + // has another pinovoke, it will re-link the ICF again when the JIT_PInvokeBegin helper is called - TADDR returnAddress = ((InlinedCallFrame*)pFrame)->m_pCallerReturnAddress; -#ifdef USE_PER_FRAME_PINVOKE_INIT - // If we're setting up the frame for each P/Invoke for the given platform, - // then we do this for all P/Invokes except ones in IL stubs. - if (!ExecutionManager::GetCodeMethodDesc(returnAddress)->IsILStub()) -#else - // If we aren't setting up the frame for each P/Invoke (instead setting up once per method), - // then ReadyToRun code is the only code using the per-P/Invoke logic. - if (ExecutionManager::IsReadyToRunCode(returnAddress)) -#endif + if (ExecutionManager::IsReadyToRunCode(((InlinedCallFrame*)pFrame)->m_pCallerReturnAddress)) { pICFForUnwindTarget = pICFForUnwindTarget->Next(); } diff --git a/src/coreclr/vm/i386/excepx86.cpp b/src/coreclr/vm/i386/excepx86.cpp index 54576261f2e..68c68f7f258 100644 --- a/src/coreclr/vm/i386/excepx86.cpp +++ b/src/coreclr/vm/i386/excepx86.cpp @@ -28,7 +28,6 @@ #include "eeconfig.h" #include "vars.hpp" #include "generics.h" -#include "corinfo.h" #include "asmconstants.h" #include "virtualcallstub.h" @@ -2971,8 +2970,6 @@ void ResumeAtJitEH(CrawlFrame* pCf, // Check that the InlinedCallFrame is in the method with the exception handler. There can be other // InlinedCallFrame somewhere up the call chain that is not related to the current exception // handling. - - // See the usages for USE_PER_FRAME_PINVOKE_INIT for more information. #ifdef DEBUG TADDR handlerFrameSP = pCf->GetRegisterSet()->SP; @@ -2985,22 +2982,10 @@ void ResumeAtJitEH(CrawlFrame* pCf, NULL /* StackwalkCacheUnwindInfo* */); _ASSERTE(unwindSuccess); - if (((TADDR)pThread->m_pFrame < pCf->GetRegisterSet()->SP)) + if (((TADDR)pThread->m_pFrame < pCf->GetRegisterSet()->SP) && ExecutionManager::IsReadyToRunCode(((InlinedCallFrame*)pThread->m_pFrame)->m_pCallerReturnAddress)) { - TADDR returnAddress = ((InlinedCallFrame*)pThread->m_pFrame)->m_pCallerReturnAddress; -#ifdef USE_PER_FRAME_PINVOKE_INIT - // If we're setting up the frame for each P/Invoke for the given platform, - // then we do this for all P/Invokes except ones in IL stubs. - if (!ExecutionManager::GetCodeMethodDesc(returnAddress)->IsILStub()) -#else - // If we aren't setting up the frame for each P/Invoke (instead setting up once per method), - // then ReadyToRun code is the only code using the per-P/Invoke logic. - if (ExecutionManager::IsReadyToRunCode(returnAddress)) -#endif - { - _ASSERTE((TADDR)pThread->m_pFrame >= handlerFrameSP); - pThread->m_pFrame->Pop(pThread); - } + _ASSERTE((TADDR)pThread->m_pFrame >= handlerFrameSP); + pThread->m_pFrame->Pop(pThread); } } diff --git a/src/tests/baseservices/exceptions/exceptioninterop/ExceptionInterop.cs b/src/tests/baseservices/exceptions/exceptioninterop/ExceptionInterop.cs index f0f479dfc55..1c6f43e12e4 100644 --- a/src/tests/baseservices/exceptions/exceptioninterop/ExceptionInterop.cs +++ b/src/tests/baseservices/exceptions/exceptioninterop/ExceptionInterop.cs @@ -122,38 +122,4 @@ public unsafe static class ExceptionInterop Assert.True(caughtException); } - - [Fact] - [PlatformSpecific(TestPlatforms.Windows)] - [SkipOnMono("Exception interop not supported on Mono.")] - public static void ThrowNativeExceptionInFrameWithFinallyCatchInOuterFrame() - { - bool caughtException = false; - try - { - ThrowInFrameWithFinally(); - } - catch - { - caughtException = true; - } - - Assert.True(caughtException); - - [MethodImpl(MethodImplOptions.NoInlining)] - static void ThrowInFrameWithFinally() - { - try - { - ThrowException(); - } - finally - { - // Try calling another P/Invoke in the finally block before the catch - // to make sure we have everything set up - // to recover from the exceptional control flow. - NativeFunction(); - } - } - } } |