From e87edfa34c0a28e9d1f2843765c10bb7bb437815 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 20 Sep 2022 12:22:22 -0700 Subject: [PATCH 1/3] Preserve last error for patchpoint helpers In stress modes (and eventually perhaps in normal uses) the jit may insert patchpoint helper calls in regions where last error is live. So the helpers need to preserve last error. Because some invocations of the helpers may transition to OSR methods instead of returning, we can't use the normal macros for this. Fixes #75828. --- src/coreclr/vm/jithelpers.cpp | 192 +++++++++++++++++++--------------- 1 file changed, 107 insertions(+), 85 deletions(-) diff --git a/src/coreclr/vm/jithelpers.cpp b/src/coreclr/vm/jithelpers.cpp index 86503f3d0efa53..358af1bb2bbec2 100644 --- a/src/coreclr/vm/jithelpers.cpp +++ b/src/coreclr/vm/jithelpers.cpp @@ -4916,6 +4916,9 @@ HCIMPLEND void JIT_Patchpoint(int* counter, int ilOffset) { + // BEGIN_PRESERVE_LAST_ERROR; + DWORD __dwLastError = ::GetLastError(); + // This method may not return normally STATIC_CONTRACT_GC_NOTRIGGER; STATIC_CONTRACT_MODE_COOPERATIVE; @@ -4955,7 +4958,8 @@ void JIT_Patchpoint(int* counter, int ilOffset) { LOG((LF_TIEREDCOMPILATION, LL_INFO1000, "Jit_Patchpoint: invalid patchpoint [%d] (0x%p) in Method=0x%pM (%s::%s) at offset %d\n", ppId, ip, pMD, pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName, ilOffset)); - return; + + goto DONE; } // See if we have an OSR method for this patchpoint. @@ -4983,7 +4987,7 @@ void JIT_Patchpoint(int* counter, int ilOffset) { LOG((LF_TIEREDCOMPILATION, LL_INFO10, "Jit_Patchpoint: ignoring patchpoint [%d] (0x%p) in Method=0x%pM (%s::%s) at offset %d\n", ppId, ip, pMD, pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName, ilOffset)); - return; + goto DONE; } #endif @@ -5024,7 +5028,7 @@ void JIT_Patchpoint(int* counter, int ilOffset) // Defer, if we haven't yet reached the limit if (hitCount < hitLimit) { - return; + goto DONE; } // Third, make sure no other thread is trying to create the OSR method. @@ -5032,7 +5036,7 @@ void JIT_Patchpoint(int* counter, int ilOffset) if ((oldFlags & PerPatchpointInfo::patchpoint_triggered) == PerPatchpointInfo::patchpoint_triggered) { LOG((LF_TIEREDCOMPILATION, LL_INFO1000, "Jit_Patchpoint: AWAITING OSR method for patchpoint [%d] (0x%p)\n", ppId, ip)); - return; + goto DONE; } LONG newFlags = oldFlags | PerPatchpointInfo::patchpoint_triggered; @@ -5041,7 +5045,7 @@ void JIT_Patchpoint(int* counter, int ilOffset) if (!triggerTransition) { LOG((LF_TIEREDCOMPILATION, LL_INFO1000, "Jit_Patchpoint: (lost race) AWAITING OSR method for patchpoint [%d] (0x%p)\n", ppId, ip)); - return; + goto DONE; } // Time to create the OSR method. @@ -5071,7 +5075,7 @@ void JIT_Patchpoint(int* counter, int ilOffset) " marking patchpoint invalid for Method=0x%pM il offset %d\n", ip, pMD, ilOffset); InterlockedOr(&ppInfo->m_flags, (LONG)PerPatchpointInfo::patchpoint_invalid); - return; + goto DONE; } // We've successfully created the osr method; make it available. @@ -5083,115 +5087,126 @@ void JIT_Patchpoint(int* counter, int ilOffset) // If we get here, we have code to transition to... _ASSERTE(osrMethodCode != NULL); - Thread *pThread = GetThread(); + { + Thread *pThread = GetThread(); #ifdef FEATURE_HIJACK - // We can't crawl the stack of a thread that currently has a hijack pending - // (since the hijack routine won't be recognized by any code manager). So we - // Undo any hijack, the EE will re-attempt it later. - pThread->UnhijackThread(); + // We can't crawl the stack of a thread that currently has a hijack pending + // (since the hijack routine won't be recognized by any code manager). So we + // Undo any hijack, the EE will re-attempt it later. + pThread->UnhijackThread(); #endif - // Find context for the original method - CONTEXT *pFrameContext = NULL; + // Find context for the original method + CONTEXT *pFrameContext = NULL; #if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) - DWORD contextSize = 0; - ULONG64 xStateCompactionMask = 0; - DWORD contextFlags = CONTEXT_FULL; - if (Thread::AreCetShadowStacksEnabled()) - { - xStateCompactionMask = XSTATE_MASK_CET_U; - contextFlags |= CONTEXT_XSTATE; - } + DWORD contextSize = 0; + ULONG64 xStateCompactionMask = 0; + DWORD contextFlags = CONTEXT_FULL; + if (Thread::AreCetShadowStacksEnabled()) + { + xStateCompactionMask = XSTATE_MASK_CET_U; + contextFlags |= CONTEXT_XSTATE; + } - // The initialize call should fail but return contextSize - BOOL success = g_pfnInitializeContext2 ? - g_pfnInitializeContext2(NULL, contextFlags, NULL, &contextSize, xStateCompactionMask) : - InitializeContext(NULL, contextFlags, NULL, &contextSize); + // The initialize call should fail but return contextSize + BOOL success = g_pfnInitializeContext2 ? + g_pfnInitializeContext2(NULL, contextFlags, NULL, &contextSize, xStateCompactionMask) : + InitializeContext(NULL, contextFlags, NULL, &contextSize); - _ASSERTE(!success && (GetLastError() == ERROR_INSUFFICIENT_BUFFER)); + _ASSERTE(!success && (GetLastError() == ERROR_INSUFFICIENT_BUFFER)); - PVOID pBuffer = _alloca(contextSize); - success = g_pfnInitializeContext2 ? - g_pfnInitializeContext2(pBuffer, contextFlags, &pFrameContext, &contextSize, xStateCompactionMask) : - InitializeContext(pBuffer, contextFlags, &pFrameContext, &contextSize); - _ASSERTE(success); + PVOID pBuffer = _alloca(contextSize); + success = g_pfnInitializeContext2 ? + g_pfnInitializeContext2(pBuffer, contextFlags, &pFrameContext, &contextSize, xStateCompactionMask) : + InitializeContext(pBuffer, contextFlags, &pFrameContext, &contextSize); + _ASSERTE(success); #else // TARGET_WINDOWS && TARGET_AMD64 - CONTEXT frameContext; - frameContext.ContextFlags = CONTEXT_FULL; - pFrameContext = &frameContext; + CONTEXT frameContext; + frameContext.ContextFlags = CONTEXT_FULL; + pFrameContext = &frameContext; #endif // TARGET_WINDOWS && TARGET_AMD64 - // Find context for the original method - RtlCaptureContext(pFrameContext); + // Find context for the original method + RtlCaptureContext(pFrameContext); #if defined(TARGET_WINDOWS) && defined(TARGET_AMD64) - if (Thread::AreCetShadowStacksEnabled()) - { - pFrameContext->ContextFlags |= CONTEXT_XSTATE; - SetXStateFeaturesMask(pFrameContext, xStateCompactionMask); - SetSSP(pFrameContext, _rdsspq()); - } + if (Thread::AreCetShadowStacksEnabled()) + { + pFrameContext->ContextFlags |= CONTEXT_XSTATE; + SetXStateFeaturesMask(pFrameContext, xStateCompactionMask); + SetSSP(pFrameContext, _rdsspq()); + } #endif // TARGET_WINDOWS && TARGET_AMD64 - // Walk back to the original method frame - pThread->VirtualUnwindToFirstManagedCallFrame(pFrameContext); + // Walk back to the original method frame + pThread->VirtualUnwindToFirstManagedCallFrame(pFrameContext); - // Remember original method FP and SP because new method will inherit them. - UINT_PTR currentSP = GetSP(pFrameContext); - UINT_PTR currentFP = GetFP(pFrameContext); + // Remember original method FP and SP because new method will inherit them. + UINT_PTR currentSP = GetSP(pFrameContext); + UINT_PTR currentFP = GetFP(pFrameContext); - // We expect to be back at the right IP - if ((UINT_PTR)ip != GetIP(pFrameContext)) - { - // Should be fatal - STRESS_LOG2(LF_TIEREDCOMPILATION, LL_FATALERROR, "Jit_Patchpoint: patchpoint (0x%p) TRANSITION" - " unexpected context IP 0x%p\n", ip, GetIP(pFrameContext)); - EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE); - } + // We expect to be back at the right IP + if ((UINT_PTR)ip != GetIP(pFrameContext)) + { + // Should be fatal + STRESS_LOG2(LF_TIEREDCOMPILATION, LL_FATALERROR, "Jit_Patchpoint: patchpoint (0x%p) TRANSITION" + " unexpected context IP 0x%p\n", ip, GetIP(pFrameContext)); + EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE); + } - // Now unwind back to the original method caller frame. - EECodeInfo callerCodeInfo(GetIP(pFrameContext)); - ULONG_PTR establisherFrame = 0; - PVOID handlerData = NULL; - RtlVirtualUnwind(UNW_FLAG_NHANDLER, callerCodeInfo.GetModuleBase(), GetIP(pFrameContext), callerCodeInfo.GetFunctionEntry(), - pFrameContext, &handlerData, &establisherFrame, NULL); + // Now unwind back to the original method caller frame. + EECodeInfo callerCodeInfo(GetIP(pFrameContext)); + ULONG_PTR establisherFrame = 0; + PVOID handlerData = NULL; + RtlVirtualUnwind(UNW_FLAG_NHANDLER, callerCodeInfo.GetModuleBase(), GetIP(pFrameContext), callerCodeInfo.GetFunctionEntry(), + pFrameContext, &handlerData, &establisherFrame, NULL); - // Now, set FP and SP back to the values they had just before this helper was called, - // since the new method must have access to the original method frame. - // - // TODO: if we access the patchpointInfo here, we can read out the FP-SP delta from there and - // use that to adjust the stack, likely saving some stack space. + // Now, set FP and SP back to the values they had just before this helper was called, + // since the new method must have access to the original method frame. + // + // TODO: if we access the patchpointInfo here, we can read out the FP-SP delta from there and + // use that to adjust the stack, likely saving some stack space. #if defined(TARGET_AMD64) - // If calls push the return address, we need to simulate that here, so the OSR - // method sees the "expected" SP misalgnment on entry. - _ASSERTE(currentSP % 16 == 0); - currentSP -= 8; + // If calls push the return address, we need to simulate that here, so the OSR + // method sees the "expected" SP misalgnment on entry. + _ASSERTE(currentSP % 16 == 0); + currentSP -= 8; #if defined(TARGET_WINDOWS) - DWORD64 ssp = GetSSP(pFrameContext); - if (ssp != 0) - { - SetSSP(pFrameContext, ssp - 8); - } + DWORD64 ssp = GetSSP(pFrameContext); + if (ssp != 0) + { + SetSSP(pFrameContext, ssp - 8); + } #endif // TARGET_WINDOWS - pFrameContext->Rbp = currentFP; + pFrameContext->Rbp = currentFP; #endif // TARGET_AMD64 - SetSP(pFrameContext, currentSP); + SetSP(pFrameContext, currentSP); - // Note we can get here w/o triggering, if there is an existing OSR method and - // we hit the patchpoint. - const int transitionLogLevel = isNewMethod ? LL_INFO10 : LL_INFO1000; - LOG((LF_TIEREDCOMPILATION, transitionLogLevel, "Jit_Patchpoint: patchpoint [%d] (0x%p) TRANSITION to ip 0x%p\n", ppId, ip, osrMethodCode)); + // Note we can get here w/o triggering, if there is an existing OSR method and + // we hit the patchpoint. + const int transitionLogLevel = isNewMethod ? LL_INFO10 : LL_INFO1000; + LOG((LF_TIEREDCOMPILATION, transitionLogLevel, "Jit_Patchpoint: patchpoint [%d] (0x%p) TRANSITION to ip 0x%p\n", ppId, ip, osrMethodCode)); - // Install new entry point as IP - SetIP(pFrameContext, osrMethodCode); + // Install new entry point as IP + SetIP(pFrameContext, osrMethodCode); - // Transition! - ClrRestoreNonvolatileContext(pFrameContext); + // Restore last error (since call below does not return) + // END_PRESERVE_LAST_ERROR; + ::SetLastError(__dwLastError); + + // Transition! + ClrRestoreNonvolatileContext(pFrameContext); + } + + DONE: + + // END_PRESERVE_LAST_ERROR; + ::SetLastError(__dwLastError); } // Jit helper invoked at a partial compilation patchpoint. @@ -5205,6 +5220,9 @@ void JIT_Patchpoint(int* counter, int ilOffset) // void JIT_PartialCompilationPatchpoint(int ilOffset) { + // BEGIN_PRESERVE_LAST_ERROR; + DWORD __dwLastError = ::GetLastError(); + // This method will not return normally STATIC_CONTRACT_GC_NOTRIGGER; STATIC_CONTRACT_MODE_COOPERATIVE; @@ -5356,6 +5374,10 @@ void JIT_PartialCompilationPatchpoint(int ilOffset) // Install new entry point as IP SetIP(&frameContext, osrMethodCode); + // Restore last error (since call below does not return) + // END_PRESERVE_LAST_ERROR; + ::SetLastError(__dwLastError); + // Transition! RtlRestoreContext(&frameContext, NULL); } From 51bb3e72fdfad0a93ea8bd2d626944f3144e2a94 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 20 Sep 2022 13:43:06 -0700 Subject: [PATCH 2/3] fix build issues, review feedback --- src/coreclr/vm/jithelpers.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/coreclr/vm/jithelpers.cpp b/src/coreclr/vm/jithelpers.cpp index 358af1bb2bbec2..873ebce421cb02 100644 --- a/src/coreclr/vm/jithelpers.cpp +++ b/src/coreclr/vm/jithelpers.cpp @@ -4917,7 +4917,7 @@ HCIMPLEND void JIT_Patchpoint(int* counter, int ilOffset) { // BEGIN_PRESERVE_LAST_ERROR; - DWORD __dwLastError = ::GetLastError(); + DWORD dwLastError = ::GetLastError(); // This method may not return normally STATIC_CONTRACT_GC_NOTRIGGER; @@ -4932,6 +4932,8 @@ void JIT_Patchpoint(int* counter, int ilOffset) LoaderAllocator* allocator = pMD->GetLoaderAllocator(); OnStackReplacementManager* manager = allocator->GetOnStackReplacementManager(); PerPatchpointInfo * ppInfo = manager->GetPerPatchpointInfo(ip); + PCODE osrMethodCode = NULL; + bool isNewMethod = false; // In the current prototype, counter is shared by all patchpoints // in a method, so no matter what happens below, we don't want to @@ -4963,8 +4965,7 @@ void JIT_Patchpoint(int* counter, int ilOffset) } // See if we have an OSR method for this patchpoint. - PCODE osrMethodCode = ppInfo->m_osrMethodCode; - bool isNewMethod = false; + osrMethodCode = ppInfo->m_osrMethodCode; if (osrMethodCode == NULL) { @@ -5197,7 +5198,7 @@ void JIT_Patchpoint(int* counter, int ilOffset) // Restore last error (since call below does not return) // END_PRESERVE_LAST_ERROR; - ::SetLastError(__dwLastError); + ::SetLastError(dwLastError); // Transition! ClrRestoreNonvolatileContext(pFrameContext); @@ -5206,7 +5207,7 @@ void JIT_Patchpoint(int* counter, int ilOffset) DONE: // END_PRESERVE_LAST_ERROR; - ::SetLastError(__dwLastError); + ::SetLastError(dwLastError); } // Jit helper invoked at a partial compilation patchpoint. @@ -5221,7 +5222,7 @@ void JIT_Patchpoint(int* counter, int ilOffset) void JIT_PartialCompilationPatchpoint(int ilOffset) { // BEGIN_PRESERVE_LAST_ERROR; - DWORD __dwLastError = ::GetLastError(); + DWORD dwLastError = ::GetLastError(); // This method will not return normally STATIC_CONTRACT_GC_NOTRIGGER; @@ -5376,7 +5377,7 @@ void JIT_PartialCompilationPatchpoint(int ilOffset) // Restore last error (since call below does not return) // END_PRESERVE_LAST_ERROR; - ::SetLastError(__dwLastError); + ::SetLastError(dwLastError); // Transition! RtlRestoreContext(&frameContext, NULL); From 7d398e2101d5645560cb8f1abd4eec5e52bba567 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 21 Sep 2022 11:00:53 -0700 Subject: [PATCH 3/3] add test case --- .../JitBlue/Runtime_75828/Runtime_75828.cs | 30 +++++++++++++++++++ .../Runtime_75828/Runtime_75828.csproj | 9 ++++++ 2 files changed, 39 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_75828/Runtime_75828.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_75828/Runtime_75828.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75828/Runtime_75828.cs b/src/tests/JIT/Regression/JitBlue/Runtime_75828/Runtime_75828.cs new file mode 100644 index 00000000000000..714359398eb1e8 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75828/Runtime_75828.cs @@ -0,0 +1,30 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.InteropServices; + +// Verify that the Jit_Patchpoint helper inserted for OSR preserves last error + +class Runtime_75828 +{ + public static int Main() + { + Marshal.SetLastSystemError(42); + + int expected = 5_000_000 + 42; + + int result = 0; + for (int i = 0; i < 10_000_000; i++) + { + result += i % 2; + } + + result += Marshal.GetLastSystemError(); + + Console.WriteLine($"got {result} expected {expected}"); + + return result == expected ? 100 : -1; + } +} + diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_75828/Runtime_75828.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_75828/Runtime_75828.csproj new file mode 100644 index 00000000000000..f492aeac9d056b --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_75828/Runtime_75828.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + \ No newline at end of file