From 4af9a470092a6bb539282352033419f60c405998 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 7 Mar 2025 14:05:10 -0800 Subject: [PATCH 01/11] Remove unused frame type --- src/coreclr/vm/FrameTypes.h | 3 --- src/coreclr/vm/frames.h | 51 ------------------------------------- 2 files changed, 54 deletions(-) diff --git a/src/coreclr/vm/FrameTypes.h b/src/coreclr/vm/FrameTypes.h index 59e1c56356e80e..a75ac7cbdc973c 100644 --- a/src/coreclr/vm/FrameTypes.h +++ b/src/coreclr/vm/FrameTypes.h @@ -47,8 +47,5 @@ FRAME_TYPE_NAME(DebuggerClassInitMarkFrame) FRAME_TYPE_NAME(DebuggerExitFrame) FRAME_TYPE_NAME(DebuggerU2MCatchHandlerFrame) FRAME_TYPE_NAME(ExceptionFilterFrame) -#if defined(_DEBUG) -FRAME_TYPE_NAME(AssumeByrefFromJITStackFrame) -#endif // _DEBUG #undef FRAME_TYPE_NAME diff --git a/src/coreclr/vm/frames.h b/src/coreclr/vm/frames.h index be519af3b9b239..d335c4aeb14f30 100644 --- a/src/coreclr/vm/frames.h +++ b/src/coreclr/vm/frames.h @@ -2819,36 +2819,6 @@ class ExceptionFilterFrame : public Frame #endif }; -#ifdef _DEBUG -// We use IsProtectedByGCFrame to check if some OBJECTREF pointers are protected -// against GC. That function doesn't know if a byref is from managed stack thus -// protected by JIT. AssumeByrefFromJITStackFrame is used to bypass that check if an -// OBJECTRef pointer is passed from managed code to an FCall and it's in stack. - -typedef DPTR(class AssumeByrefFromJITStackFrame) PTR_AssumeByrefFromJITStackFrame; - -class AssumeByrefFromJITStackFrame : public Frame -{ -public: -#ifndef DACCESS_COMPILE - AssumeByrefFromJITStackFrame(OBJECTREF *pObjRef) : Frame(FrameIdentifier::AssumeByrefFromJITStackFrame) - { - m_pObjRef = pObjRef; - } -#endif - - BOOL Protects_Impl(OBJECTREF *ppORef) - { - LIMITED_METHOD_CONTRACT; - return ppORef == m_pObjRef; - } - -private: - OBJECTREF *m_pObjRef; -}; //AssumeByrefFromJITStackFrame - -#endif //_DEBUG - //------------------------------------------------------------------------ // These macros GC-protect OBJECTREF pointers on the EE's behalf. // In between these macros, the GC can move but not discard the protected @@ -2977,27 +2947,6 @@ class AssumeByrefFromJITStackFrame : public Frame #define ASSERT_ADDRESS_IN_STACK(address) _ASSERTE (Thread::IsAddressInCurrentStack (address)); -#if defined (_DEBUG) && !defined (DACCESS_COMPILE) -#define ASSUME_BYREF_FROM_JIT_STACK_BEGIN(__objRef) \ - /* make sure we are only called inside an FCall */ \ - if (__me == 0) {}; \ - /* make sure the address is in stack. If the address is an interior */ \ - /*pointer points to GC heap, the FCall still needs to protect it explicitly */ \ - ASSERT_ADDRESS_IN_STACK (__objRef); \ - do { \ - AssumeByrefFromJITStackFrame __dummyAssumeByrefFromJITStackFrame ((__objRef)); \ - __dummyAssumeByrefFromJITStackFrame.Push (); \ - /* work around unreachable code warning */ \ - if (true) { DEBUG_ASSURE_NO_RETURN_BEGIN(GC_PROTECT) - -#define ASSUME_BYREF_FROM_JIT_STACK_END() \ - DEBUG_ASSURE_NO_RETURN_END(GC_PROTECT) } \ - __dummyAssumeByrefFromJITStackFrame.Pop(); } while(0) -#else //defined (_DEBUG) && !defined (DACCESS_COMPILE) -#define ASSUME_BYREF_FROM_JIT_STACK_BEGIN(__objRef) -#define ASSUME_BYREF_FROM_JIT_STACK_END() -#endif //defined (_DEBUG) && !defined (DACCESS_COMPILE) - void ComputeCallRefMap(MethodDesc* pMD, GCRefMapBuilder * pBuilder, bool isDispatchCell); From 512c3389a5546f9de9f50877c4d6904d1e17f99d Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 7 Mar 2025 14:07:50 -0800 Subject: [PATCH 02/11] Remove "debug noreturn" from GCPROTECT. GCFrame instances set up through those macros are automatically removed by the constructor --- src/coreclr/vm/frames.h | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/coreclr/vm/frames.h b/src/coreclr/vm/frames.h index d335c4aeb14f30..ef46f59f2f5ece 100644 --- a/src/coreclr/vm/frames.h +++ b/src/coreclr/vm/frames.h @@ -2889,8 +2889,7 @@ class ExceptionFilterFrame : public Frame (OBJECTREF*)&(ObjRefStruct), \ sizeof(ObjRefStruct)/sizeof(OBJECTREF), \ FALSE); \ - /* work around unreachable code warning */ \ - if (true) { DEBUG_ASSURE_NO_RETURN_BEGIN(GCPROTECT) + { #define GCPROTECT_BEGIN_THREAD(pThread, ObjRefStruct) do { \ GCFrame __gcframe( \ @@ -2898,16 +2897,14 @@ class ExceptionFilterFrame : public Frame (OBJECTREF*)&(ObjRefStruct), \ sizeof(ObjRefStruct)/sizeof(OBJECTREF), \ FALSE); \ - /* work around unreachable code warning */ \ - if (true) { DEBUG_ASSURE_NO_RETURN_BEGIN(GCPROTECT) + { #define GCPROTECT_ARRAY_BEGIN(ObjRefArray,cnt) do { \ GCFrame __gcframe( \ (OBJECTREF*)&(ObjRefArray), \ cnt * sizeof(ObjRefArray) / sizeof(OBJECTREF), \ FALSE); \ - /* work around unreachable code warning */ \ - if (true) { DEBUG_ASSURE_NO_RETURN_BEGIN(GCPROTECT) + { #define GCPROTECT_BEGININTERIOR(ObjRefStruct) do { \ /* work around Wsizeof-pointer-div warning as we */ \ @@ -2917,20 +2914,18 @@ class ExceptionFilterFrame : public Frame (OBJECTREF*)&(ObjRefStruct), \ subjectSize/sizeof(OBJECTREF), \ TRUE); \ - /* work around unreachable code warning */ \ - if (true) { DEBUG_ASSURE_NO_RETURN_BEGIN(GCPROTECT) + { #define GCPROTECT_BEGININTERIOR_ARRAY(ObjRefArray,cnt) do { \ GCFrame __gcframe( \ (OBJECTREF*)&(ObjRefArray), \ cnt, \ TRUE); \ - /* work around unreachable code warning */ \ - if (true) { DEBUG_ASSURE_NO_RETURN_BEGIN(GCPROTECT) + { #define GCPROTECT_END() \ - DEBUG_ASSURE_NO_RETURN_END(GCPROTECT) } \ + } \ } while(0) From 0f07989948387a899cb6a4c93a0f362b9604f2c1 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Fri, 7 Mar 2025 15:18:56 -0800 Subject: [PATCH 03/11] Actually check for if we have an exception in flight instead of requiring that we don't do a return to ensure we call a function in the success case. --- src/coreclr/vm/exceptmacros.h | 7 +++---- src/coreclr/vm/threads.h | 11 ++--------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/src/coreclr/vm/exceptmacros.h b/src/coreclr/vm/exceptmacros.h index 2c11c059853ba9..2bc215d4a4c07e 100644 --- a/src/coreclr/vm/exceptmacros.h +++ b/src/coreclr/vm/exceptmacros.h @@ -500,12 +500,11 @@ void COMPlusCooperativeTransitionHandler(Frame* pFrame); { \ MAKE_CURRENT_THREAD_AVAILABLE(); \ BEGIN_GCX_ASSERT_PREEMP; \ - CoopTransitionHolder __CoopTransition(CURRENT_THREAD); \ - DEBUG_ASSURE_NO_RETURN_BEGIN(COOP_TRANSITION) + { \ + CoopTransitionHolder __CoopTransition(CURRENT_THREAD); #define COOPERATIVE_TRANSITION_END() \ - DEBUG_ASSURE_NO_RETURN_END(COOP_TRANSITION) \ - __CoopTransition.SuppressRelease(); \ + } \ END_GCX_ASSERT_PREEMP; \ } diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 48580a13278432..25f15f03c586c2 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -111,6 +111,7 @@ #ifndef __threads_h__ #define __threads_h__ +#include #include "vars.hpp" #include "util.hpp" #include "eventstore.hpp" @@ -5239,17 +5240,9 @@ class CoopTransitionHolder ~CoopTransitionHolder() { WRAPPER_NO_CONTRACT; - if (m_pFrame != NULL) + if (std::uncaught_exception()) COMPlusCooperativeTransitionHandler(m_pFrame); } - - void SuppressRelease() - { - LIMITED_METHOD_CONTRACT; - // FRAME_TOP and NULL must be distinct values. - // static_assert_no_msg(FRAME_TOP_VALUE != NULL); - m_pFrame = NULL; - } }; // -------------------------------------------------------------------------------- From aa8002c495064c6fa211a493cdaefc0be5ac6798 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 24 Mar 2025 14:04:41 -0700 Subject: [PATCH 04/11] Create a separate "preserve last error" holder type to allow removing a no-return scope --- src/coreclr/inc/clrhost.h | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/coreclr/inc/clrhost.h b/src/coreclr/inc/clrhost.h index a1d22b6ee269d1..05bbefa4ff0338 100644 --- a/src/coreclr/inc/clrhost.h +++ b/src/coreclr/inc/clrhost.h @@ -29,16 +29,29 @@ using std::nothrow; #define _DEBUG_IMPL 1 #endif +struct PreserveLastErrorHolder +{ + PreserveLastErrorHolder() + { + m_dwLastError = ::GetLastError(); + } + + ~PreserveLastErrorHolder() + { + ::SetLastError(m_dwLastError); + } +private: + DWORD m_dwLastError; +}; + + #define BEGIN_PRESERVE_LAST_ERROR \ { \ - DWORD __dwLastError = ::GetLastError(); \ - DEBUG_ASSURE_NO_RETURN_BEGIN(PRESERVE_LAST_ERROR); \ - { + PreserveLastErrorHolder __preserveLastErrorHolder; \ + { #define END_PRESERVE_LAST_ERROR \ - } \ - DEBUG_ASSURE_NO_RETURN_END(PRESERVE_LAST_ERROR); \ - ::SetLastError(__dwLastError); \ + } \ } // From 24e6f3485d2fbba068f80ee8db9898ff5b25701e Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 24 Mar 2025 14:07:23 -0700 Subject: [PATCH 05/11] Allow returning from within an (UN)INSTALL_UNWIND_AND_CONTINUE_HANDLER scope as nothing in the uninstall requires manual unwinding. --- src/coreclr/vm/exceptmacros.h | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/coreclr/vm/exceptmacros.h b/src/coreclr/vm/exceptmacros.h index 2bc215d4a4c07e..fda507bd4464a4 100644 --- a/src/coreclr/vm/exceptmacros.h +++ b/src/coreclr/vm/exceptmacros.h @@ -342,8 +342,7 @@ VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHar bool __fExceptionCaught = false; \ SCAN_EHMARKER(); \ if (true) PAL_CPP_TRY { \ - SCAN_EHMARKER_TRY(); \ - DEBUG_ASSURE_NO_RETURN_BEGIN(IUACH) + SCAN_EHMARKER_TRY(); #define INSTALL_UNWIND_AND_CONTINUE_HANDLER \ INSTALL_UNWIND_AND_CONTINUE_HANDLER_EX \ @@ -358,11 +357,9 @@ VOID DECLSPEC_NORETURN DispatchManagedException(PAL_SEHException& ex, bool isHar bool __fExceptionCaught = false; \ SCAN_EHMARKER(); \ if (true) PAL_CPP_TRY { \ - SCAN_EHMARKER_TRY(); \ - DEBUG_ASSURE_NO_RETURN_BEGIN(IUACH); + SCAN_EHMARKER_TRY(); #define UNINSTALL_UNWIND_AND_CONTINUE_HANDLER_EX(nativeRethrow) \ - DEBUG_ASSURE_NO_RETURN_END(IUACH) \ SCAN_EHMARKER_END_TRY(); \ } \ PAL_CPP_CATCH_NON_DERIVED_NOARG (const std::bad_alloc&) \ From f581f237806ccf564886b69deac4ed324eece2e3 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Mon, 24 Mar 2025 14:12:45 -0700 Subject: [PATCH 06/11] Don't block "return" in contracts at compile time, just rely on PR review to not allow people to return out of a contract block. --- src/coreclr/inc/contract.h | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/coreclr/inc/contract.h b/src/coreclr/inc/contract.h index a8f3f6f47c2263..e855d66ec14f20 100644 --- a/src/coreclr/inc/contract.h +++ b/src/coreclr/inc/contract.h @@ -1160,7 +1160,6 @@ typedef __SafeToUsePostCondition __PostConditionOK; Contract::RanPostconditions ___ran(__FUNCTION__); \ Contract::Operation ___op = Contract::Setup; \ BOOL ___contract_enabled = FALSE; \ - DEBUG_ASSURE_NO_RETURN_BEGIN(CONTRACT) \ ___contract_enabled = Contract::EnforceContract(); \ enum {___disabled = 0}; \ if (!___contract_enabled) \ @@ -1181,10 +1180,8 @@ typedef __SafeToUsePostCondition __PostConditionOK; } \ else \ { \ - DEBUG_OK_TO_RETURN_BEGIN(CONTRACT) \ ___run_return: \ return _returnexp; \ - DEBUG_OK_TO_RETURN_END(CONTRACT) \ } \ } \ if (0) \ @@ -1226,7 +1223,6 @@ typedef __SafeToUsePostCondition __PostConditionOK; Contract::Returner<_returntype> ___returner(RETVAL); \ Contract::RanPostconditions ___ran(__FUNCTION__); \ Contract::Operation ___op = Contract::Setup; \ - DEBUG_ASSURE_NO_RETURN_BEGIN(CONTRACT) \ BOOL ___contract_enabled = Contract::EnforceContract(); \ enum {___disabled = 0}; \ { \ @@ -1244,10 +1240,8 @@ typedef __SafeToUsePostCondition __PostConditionOK; } \ else \ { \ - DEBUG_OK_TO_RETURN_BEGIN(CONTRACT) \ ___run_return: \ return _returnexp; \ - DEBUG_OK_TO_RETURN_END(CONTRACT) \ } \ } \ if (0) \ @@ -1679,7 +1673,6 @@ class AutoCleanupContractViolationHolder : ContractViolationHolder __violationHolder_onlyOneAllowedPerScope; \ __violationHolder_onlyOneAllowedPerScope.Enter(); \ - DEBUG_ASSURE_NO_RETURN_BEGIN(CONTRACT) \ // Use this to jump out prematurely from a violation. Used for EH // when the function might not return From 6cc3ecd6b3f51b2159276cab58e995684e11c1c0 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 25 Mar 2025 10:55:28 -0700 Subject: [PATCH 07/11] Adjust one usage of BEGIN/END_PRESERVE_LAST_ERROR to not use the holder as it shouldn't happen in that case for unwinding. --- src/coreclr/vm/threadsuspend.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 1b1ce6536f392c..1f08a27a5a0f53 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -2264,7 +2264,8 @@ Exit: ; void Thread::HandleThreadAbort () { - BEGIN_PRESERVE_LAST_ERROR; + // Only preserve the last error when we aren't throwing an exception for ThreadAbort. + DWORD lastError = GetLastError(); STATIC_CONTRACT_THROWS; STATIC_CONTRACT_GC_TRIGGERS; @@ -2322,7 +2323,7 @@ void Thread::HandleThreadAbort () } } - END_PRESERVE_LAST_ERROR; + ::SetLastError(lastError); } void Thread::PreWorkForThreadAbort() @@ -5731,7 +5732,7 @@ BOOL CheckActivationSafePoint(SIZE_T ip) // The criteria for safe activation is to be running managed code. // Also we are not interested in handling interruption if we are already in preemptive mode nor if we are single stepping - BOOL isActivationSafePoint = pThread != NULL && + BOOL isActivationSafePoint = pThread != NULL && (pThread->m_StateNC & Thread::TSNC_DebuggerIsStepping) == 0 && pThread->PreemptiveGCDisabled() && ExecutionManager::IsManagedCode(ip); From 42fb90e58679b29f2b67db6a02093ecf0a7043fc Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 25 Mar 2025 10:56:31 -0700 Subject: [PATCH 08/11] Add assert that CoopTransitionHolder isn't being used in a function called while a C++ exception is in flight --- src/coreclr/vm/threads.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 25f15f03c586c2..697cada66c6944 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -5235,6 +5235,7 @@ class CoopTransitionHolder : m_pFrame(pThread->m_pFrame) { LIMITED_METHOD_CONTRACT; + _ASSERTE(!std::uncaught_exception()); } ~CoopTransitionHolder() From 666af8e044966f2ebc821924ba02c6e591c150ed Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 25 Mar 2025 13:44:05 -0700 Subject: [PATCH 09/11] Remove PRESERVE_LAST_ERROR macros and enforce correct usage of CoopTransitionHolder by a runtime assert instead of a compile-time error. --- src/coreclr/inc/clrhost.h | 10 ---------- src/coreclr/vm/callcounting.cpp | 4 +--- src/coreclr/vm/codeman.cpp | 12 +++--------- src/coreclr/vm/dllimport.cpp | 12 +++--------- src/coreclr/vm/excep.cpp | 7 ++++--- src/coreclr/vm/exceptmacros.h | 1 + src/coreclr/vm/jithelpers.cpp | 19 ++++++------------- src/coreclr/vm/prestub.cpp | 8 ++------ src/coreclr/vm/stubhelpers.cpp | 14 +++----------- src/coreclr/vm/threads.h | 12 ++++++++++-- src/coreclr/vm/threadsuspend.cpp | 23 +++++++++-------------- 11 files changed, 42 insertions(+), 80 deletions(-) diff --git a/src/coreclr/inc/clrhost.h b/src/coreclr/inc/clrhost.h index 05bbefa4ff0338..8d8309f8187bdb 100644 --- a/src/coreclr/inc/clrhost.h +++ b/src/coreclr/inc/clrhost.h @@ -44,16 +44,6 @@ struct PreserveLastErrorHolder DWORD m_dwLastError; }; - -#define BEGIN_PRESERVE_LAST_ERROR \ - { \ - PreserveLastErrorHolder __preserveLastErrorHolder; \ - { - -#define END_PRESERVE_LAST_ERROR \ - } \ - } - // // TRASH_LASTERROR macro sets bogus last error in debug builds to help find places that fail to save it // diff --git a/src/coreclr/vm/callcounting.cpp b/src/coreclr/vm/callcounting.cpp index 3b2df30ed4cfcc..dfa23e90f4e8d9 100644 --- a/src/coreclr/vm/callcounting.cpp +++ b/src/coreclr/vm/callcounting.cpp @@ -760,7 +760,7 @@ PCODE CallCountingManager::OnCallCountThresholdReached(TransitionBlock *transiti PCODE codeEntryPoint = 0; - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; MAKE_CURRENT_THREAD_AVAILABLE(); @@ -823,8 +823,6 @@ PCODE CallCountingManager::OnCallCountThresholdReached(TransitionBlock *transiti frame->Pop(CURRENT_THREAD); - END_PRESERVE_LAST_ERROR; - return codeEntryPoint; } diff --git a/src/coreclr/vm/codeman.cpp b/src/coreclr/vm/codeman.cpp index 12b02c90a39ba4..6a41d89d2c994a 100644 --- a/src/coreclr/vm/codeman.cpp +++ b/src/coreclr/vm/codeman.cpp @@ -2522,7 +2522,7 @@ extern "C" PT_RUNTIME_FUNCTION GetRuntimeFunctionCallback(IN ULONG ControlPc PT_RUNTIME_FUNCTION prf = NULL; // We must preserve this so that GCStress=4 eh processing doesnt kill last error. - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; #ifdef ENABLE_CONTRACTS // Some 64-bit OOM tests use the hosting interface to re-enter the CLR via @@ -2545,8 +2545,6 @@ extern "C" PT_RUNTIME_FUNCTION GetRuntimeFunctionCallback(IN ULONG ControlPc LOG((LF_EH, LL_INFO1000000, "GetRuntimeFunctionCallback(%p) returned %p\n", ControlPc, prf)); - END_PRESERVE_LAST_ERROR; - return prf; } #endif // FEATURE_EH_FUNCLETS @@ -4379,7 +4377,7 @@ extern "C" void GetRuntimeStackWalkInfo(IN ULONG64 ControlPc, WRAPPER_NO_CONTRACT; - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; if (pModuleBase) *pModuleBase = 0; @@ -4392,7 +4390,7 @@ extern "C" void GetRuntimeStackWalkInfo(IN ULONG64 ControlPc, #if defined(DACCESS_COMPILE) GetUnmanagedStackWalkInfo(ControlPc, pModuleBase, pFuncEntry); #endif // DACCESS_COMPILE - goto Exit; + return; } if (pModuleBase) @@ -4404,10 +4402,6 @@ extern "C" void GetRuntimeStackWalkInfo(IN ULONG64 ControlPc, { *pFuncEntry = (UINT_PTR)(PT_RUNTIME_FUNCTION)codeInfo.GetFunctionEntry(); } - -Exit: - ; - END_PRESERVE_LAST_ERROR; } #endif // FEATURE_EH_FUNCLETS diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index f070d81f95980a..7f16554ce79196 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -5828,7 +5828,7 @@ EXTERN_C LPVOID STDCALL NDirectImportWorker(NDirectMethodDesc* pMD) { LPVOID ret = NULL; - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; CONTRACTL { @@ -5879,8 +5879,6 @@ EXTERN_C LPVOID STDCALL NDirectImportWorker(NDirectMethodDesc* pMD) UNINSTALL_UNWIND_AND_CONTINUE_HANDLER; UNINSTALL_MANAGED_EXCEPTION_DISPATCHER; - END_PRESERVE_LAST_ERROR; - return ret; } @@ -5891,7 +5889,7 @@ EXTERN_C LPVOID STDCALL NDirectImportWorker(NDirectMethodDesc* pMD) EXTERN_C void STDCALL VarargPInvokeStubWorker(TransitionBlock * pTransitionBlock, VASigCookie *pVASigCookie, MethodDesc *pMD) { - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; STATIC_CONTRACT_THROWS; STATIC_CONTRACT_GC_TRIGGERS; @@ -5915,13 +5913,11 @@ EXTERN_C void STDCALL VarargPInvokeStubWorker(TransitionBlock * pTransitionBlock GetILStubForCalli(pVASigCookie, pMD); pFrame->Pop(CURRENT_THREAD); - - END_PRESERVE_LAST_ERROR; } EXTERN_C void STDCALL GenericPInvokeCalliStubWorker(TransitionBlock * pTransitionBlock, VASigCookie * pVASigCookie, PCODE pUnmanagedTarget) { - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; STATIC_CONTRACT_THROWS; STATIC_CONTRACT_GC_TRIGGERS; @@ -5944,8 +5940,6 @@ EXTERN_C void STDCALL GenericPInvokeCalliStubWorker(TransitionBlock * pTransitio GetILStubForCalli(pVASigCookie, NULL); pFrame->Pop(CURRENT_THREAD); - - END_PRESERVE_LAST_ERROR; } PCODE GetILStubForCalli(VASigCookie *pVASigCookie, MethodDesc *pMD) diff --git a/src/coreclr/vm/excep.cpp b/src/coreclr/vm/excep.cpp index 769c142e25888a..3359b4e8f3208d 100644 --- a/src/coreclr/vm/excep.cpp +++ b/src/coreclr/vm/excep.cpp @@ -5896,9 +5896,10 @@ bool IsGcMarker(CONTEXT* pContext, EXCEPTION_RECORD *pExceptionRecord) { // GCStress processing can disturb last error, so preserve it. BOOL res; - BEGIN_PRESERVE_LAST_ERROR; - res = OnGcCoverageInterrupt(pContext); - END_PRESERVE_LAST_ERROR; + { + PreserveLastErrorHolder preserveLastError; + res = OnGcCoverageInterrupt(pContext); + } if (res) { return true; diff --git a/src/coreclr/vm/exceptmacros.h b/src/coreclr/vm/exceptmacros.h index fda507bd4464a4..ff3eafd842da4c 100644 --- a/src/coreclr/vm/exceptmacros.h +++ b/src/coreclr/vm/exceptmacros.h @@ -501,6 +501,7 @@ void COMPlusCooperativeTransitionHandler(Frame* pFrame); CoopTransitionHolder __CoopTransition(CURRENT_THREAD); #define COOPERATIVE_TRANSITION_END() \ + __CoopTransition.SuppressRelease(); \ } \ END_GCX_ASSERT_PREEMP; \ } diff --git a/src/coreclr/vm/jithelpers.cpp b/src/coreclr/vm/jithelpers.cpp index 28ac102756360d..9bdca9d4d90d8a 100644 --- a/src/coreclr/vm/jithelpers.cpp +++ b/src/coreclr/vm/jithelpers.cpp @@ -2139,7 +2139,7 @@ extern "C" VOID JIT_PInvokeEndRarePath(); void JIT_PInvokeEndRarePath() { - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; Thread *thread = GetThread(); @@ -2164,8 +2164,6 @@ void JIT_PInvokeEndRarePath() } thread->m_pFrame->Pop(thread); - - END_PRESERVE_LAST_ERROR; } /*************************************************************/ @@ -2203,7 +2201,7 @@ void JIT_RareDisableHelper() // in the first phase. // - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; Thread *thread = GetThread(); // We execute RareDisablePreemptiveGC manually before checking any abort conditions @@ -2225,8 +2223,6 @@ void JIT_RareDisableHelper() END_QCALL; thread->DisablePreemptiveGC(); } - - END_PRESERVE_LAST_ERROR; } FCIMPL0(INT32, JIT_GetCurrentManagedThreadId) @@ -2654,7 +2650,7 @@ static PCODE PatchpointRequiredPolicy(TransitionBlock* pTransitionBlock, int* co extern "C" void JIT_PatchpointWorkerWorkerWithPolicy(TransitionBlock * pTransitionBlock) { - // BEGIN_PRESERVE_LAST_ERROR; + // Manually preserve the last error as we may not return normally from this method. DWORD dwLastError = ::GetLastError(); // This method may not return normally @@ -2819,7 +2815,6 @@ extern "C" void JIT_PatchpointWorkerWorkerWithPolicy(TransitionBlock * pTransiti #endif // Restore last error (since call below does not return) - // END_PRESERVE_LAST_ERROR; ::SetLastError(dwLastError); // Transition! @@ -2827,8 +2822,6 @@ extern "C" void JIT_PatchpointWorkerWorkerWithPolicy(TransitionBlock * pTransiti } DONE: - - // END_PRESERVE_LAST_ERROR; ::SetLastError(dwLastError); } @@ -3384,21 +3377,21 @@ HCIMPL3_RAW(void, JIT_ReversePInvokeEnterTrackTransitions, ReversePInvokeFrame* frame->pMD = pMD; Thread* thread = GetThreadNULLOk(); - + // If a thread instance exists and is in the // correct GC mode attempt a quick transition. if (thread != NULL && !thread->PreemptiveGCDisabled()) { frame->currentThread = thread; - + #ifdef PROFILING_SUPPORTED if (CORProfilerTrackTransitions()) { ProfilerUnmanagedToManagedTransitionMD(frame->pMD, COR_PRF_TRANSITION_CALL); } #endif - + // Manually inline the fast path in Thread::DisablePreemptiveGC(). thread->m_fPreemptiveGCDisabled.StoreWithoutBarrier(1); if (g_TrapReturningThreads != 0) diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index 6458e86998ce71..6277c7f26fb305 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -2622,7 +2622,7 @@ extern "C" PCODE STDCALL PreStubWorker(TransitionBlock* pTransitionBlock, Method { PCODE pbRetVal = (PCODE)NULL; - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; STATIC_CONTRACT_THROWS; STATIC_CONTRACT_GC_TRIGGERS; @@ -2735,8 +2735,6 @@ extern "C" PCODE STDCALL PreStubWorker(TransitionBlock* pTransitionBlock, Method POSTCONDITION(pbRetVal != NULL); - END_PRESERVE_LAST_ERROR; - return pbRetVal; } @@ -3163,7 +3161,7 @@ EXTERN_C PCODE STDCALL ExternalMethodFixupWorker(TransitionBlock * pTransitionBl PCODE pCode = (PCODE)NULL; - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; MAKE_CURRENT_THREAD_AVAILABLE(); @@ -3490,8 +3488,6 @@ EXTERN_C PCODE STDCALL ExternalMethodFixupWorker(TransitionBlock * pTransitionBl pEMFrame->Pop(CURRENT_THREAD); // Pop the ExternalMethodFrame from the frame stack - END_PRESERVE_LAST_ERROR; - return pCode; } diff --git a/src/coreclr/vm/stubhelpers.cpp b/src/coreclr/vm/stubhelpers.cpp index c3ec58f129b753..d5121fc7f062df 100644 --- a/src/coreclr/vm/stubhelpers.cpp +++ b/src/coreclr/vm/stubhelpers.cpp @@ -420,7 +420,7 @@ FCIMPL1(void*, StubHelpers::GetDelegateTarget, DelegateObject *pThisUNSAFE) PCODE pEntryPoint = (PCODE)NULL; #ifdef _DEBUG - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; #endif CONTRACTL @@ -442,10 +442,6 @@ FCIMPL1(void*, StubHelpers::GetDelegateTarget, DelegateObject *pThisUNSAFE) pEntryPoint = orefThis->GetMethodPtrAux(); -#ifdef _DEBUG - END_PRESERVE_LAST_ERROR; -#endif - return (PVOID)pEntryPoint; } FCIMPLEND @@ -485,7 +481,7 @@ extern "C" void QCALLTYPE StubHelpers_ThrowInteropParamException(INT resID, INT #ifdef PROFILING_SUPPORTED extern "C" void* QCALLTYPE StubHelpers_ProfilerBeginTransitionCallback(MethodDesc* pTargetMD) { - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; QCALL_CONTRACT; @@ -495,14 +491,12 @@ extern "C" void* QCALLTYPE StubHelpers_ProfilerBeginTransitionCallback(MethodDes END_QCALL; - END_PRESERVE_LAST_ERROR; - return pTargetMD; } extern "C" void QCALLTYPE StubHelpers_ProfilerEndTransitionCallback(MethodDesc* pTargetMD) { - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; QCALL_CONTRACT; @@ -511,8 +505,6 @@ extern "C" void QCALLTYPE StubHelpers_ProfilerEndTransitionCallback(MethodDesc* ProfilerUnmanagedToManagedTransitionMD(pTargetMD, COR_PRF_TRANSITION_RETURN); END_QCALL; - - END_PRESERVE_LAST_ERROR; } #endif // PROFILING_SUPPORTED diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index 697cada66c6944..bde77a84ea0471 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -5235,15 +5235,23 @@ class CoopTransitionHolder : m_pFrame(pThread->m_pFrame) { LIMITED_METHOD_CONTRACT; - _ASSERTE(!std::uncaught_exception()); } ~CoopTransitionHolder() { WRAPPER_NO_CONTRACT; - if (std::uncaught_exception()) + _ASSERTE(m_pFrame == nullptr || std::uncaught_exception()); + if (m_pFrame != nullptr) COMPlusCooperativeTransitionHandler(m_pFrame); } + + void SuppressRelease() + { + LIMITED_METHOD_CONTRACT; + // FRAME_TOP and NULL must be distinct values. + // static_assert_no_msg(FRAME_TOP_VALUE != NULL); + m_pFrame = nullptr; + } }; // -------------------------------------------------------------------------------- diff --git a/src/coreclr/vm/threadsuspend.cpp b/src/coreclr/vm/threadsuspend.cpp index 1f08a27a5a0f53..6a6f3e1ff51564 100644 --- a/src/coreclr/vm/threadsuspend.cpp +++ b/src/coreclr/vm/threadsuspend.cpp @@ -2105,7 +2105,7 @@ extern void WaitForEndOfShutdown(); // why we Enable GC here! void Thread::RareDisablePreemptiveGC() { - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; CONTRACTL { NOTHROW; @@ -2115,7 +2115,7 @@ void Thread::RareDisablePreemptiveGC() if (IsAtProcessExit()) { - goto Exit; + return; } _ASSERTE (m_fPreemptiveGCDisabled); @@ -2127,7 +2127,7 @@ void Thread::RareDisablePreemptiveGC() if (!GCHeapUtilities::IsGCHeapInitialized()) { - goto Exit; + return; } if (ThreadStore::HoldingThreadStore(this)) @@ -2138,7 +2138,7 @@ void Thread::RareDisablePreemptiveGC() // may want to enter coop mode, but we have a number of other scenarios where TSL is acquired // in coop mode or when TSL-owning thread tries to swtch to coop. // We will handle all cases in the same way - by allowing the thread into coop mode without a wait. - goto Exit; + return; } STRESS_LOG1(LF_SYNC, LL_INFO1000, "RareDisablePreemptiveGC: entering. Thread state = %x\n", m_State.Load()); @@ -2257,9 +2257,6 @@ void Thread::RareDisablePreemptiveGC() } STRESS_LOG0(LF_SYNC, LL_INFO1000, "RareDisablePreemptiveGC: leaving\n"); - -Exit: ; - END_PRESERVE_LAST_ERROR; } void Thread::HandleThreadAbort () @@ -2642,9 +2639,8 @@ extern "C" PCONTEXT __stdcall GetCurrentSavedRedirectContext() PCONTEXT pContext; - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; pContext = GetThread()->GetSavedRedirectContext(); - END_PRESERVE_LAST_ERROR; return pContext; } @@ -2689,7 +2685,8 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) // We must preserve this in case we've interrupted an IL pinvoke stub before it // was able to save the error. - DWORD dwLastError = GetLastError(); // BEGIN_PRESERVE_LAST_ERROR + // Manually preserve it as we don't return from this function normally. + DWORD dwLastError = GetLastError(); Thread *pThread = GetThread(); @@ -2775,7 +2772,7 @@ void __stdcall Thread::RedirectedHandledJITCase(RedirectReason reason) pThread->UnmarkRedirectContextInUse(pCtx); LOG((LF_SYNC, LL_INFO1000, "Resuming execution with RtlRestoreContext\n")); - SetLastError(dwLastError); // END_PRESERVE_LAST_ERROR + SetLastError(dwLastError); __asan_handle_no_return(); #ifdef TARGET_X86 @@ -4869,7 +4866,7 @@ void STDCALL OnHijackWorker(HijackArgs * pArgs) CONTRACTL_END; #ifdef HIJACK_NONINTERRUPTIBLE_THREADS - BEGIN_PRESERVE_LAST_ERROR; + PreserveLastErrorHolder preserveLastError; Thread *thread = GetThread(); @@ -4902,8 +4899,6 @@ void STDCALL OnHijackWorker(HijackArgs * pArgs) #endif // _DEBUG frame.Pop(); - - END_PRESERVE_LAST_ERROR; #else PORTABILITY_ASSERT("OnHijackWorker not implemented on this platform."); #endif // HIJACK_NONINTERRUPTIBLE_THREADS From c36e9d80ee832ab06769ab4bcf23fe6b81703412 Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 25 Mar 2025 14:15:02 -0700 Subject: [PATCH 10/11] Remove some now-unneeded ASSURE_NO_RETURN_END macros and remove the last of the OK_TO_RETURN macros --- src/coreclr/inc/contract.h | 4 +--- src/coreclr/inc/debugreturn.h | 9 --------- src/coreclr/vm/fcall.h | 10 ---------- 3 files changed, 1 insertion(+), 22 deletions(-) diff --git a/src/coreclr/inc/contract.h b/src/coreclr/inc/contract.h index e855d66ec14f20..44be057ffc7cf5 100644 --- a/src/coreclr/inc/contract.h +++ b/src/coreclr/inc/contract.h @@ -1454,8 +1454,7 @@ typedef __SafeToUsePostCondition __PostConditionOK; #endif // __FORCE_NORUNTIME_CONTRACTS__ -#define CONTRACT_END CONTRACTL_END \ - DEBUG_ASSURE_NO_RETURN_END(CONTRACT) \ +#define CONTRACT_END CONTRACTL_END // The final expression in the RETURN macro deserves special explanation (or something.) @@ -1680,7 +1679,6 @@ class AutoCleanupContractViolationHolder : ContractViolationHolder 190024315) #define DEBUG_ASSURE_SAFE_TO_RETURN TRUE @@ -114,9 +108,6 @@ typedef __SafeToReturn __ReturnOK; #define DEBUG_ASSURE_NO_RETURN_BEGIN(arg) { #define DEBUG_ASSURE_NO_RETURN_END(arg) } -#define DEBUG_OK_TO_RETURN_BEGIN(arg) { -#define DEBUG_OK_TO_RETURN_END(arg) } - #endif // defined(_DEBUG) && !defined(JIT_BUILD) && (!defined(_MSC_FULL_VER) || _MSC_FULL_VER > 190024315) #endif // !_PREFAST_ diff --git a/src/coreclr/vm/fcall.h b/src/coreclr/vm/fcall.h index 0b362c0cc71696..74082e7ce78cef 100644 --- a/src/coreclr/vm/fcall.h +++ b/src/coreclr/vm/fcall.h @@ -542,13 +542,9 @@ LPVOID __FCThrow(LPVOID me, enum RuntimeExceptionKind reKind, UINT resID, LPCWST #if defined(_PREFAST_) #define FORLAZYMACHSTATE_BEGINLOOP(x) x #define FORLAZYMACHSTATE_ENDLOOP(x) - #define FORLAZYMACHSTATE_DEBUG_OK_TO_RETURN_BEGIN - #define FORLAZYMACHSTATE_DEBUG_OK_TO_RETURN_END #else #define FORLAZYMACHSTATE_BEGINLOOP(x) x do #define FORLAZYMACHSTATE_ENDLOOP(x) while(x) - #define FORLAZYMACHSTATE_DEBUG_OK_TO_RETURN_BEGIN DEBUG_OK_TO_RETURN_BEGIN(LAZYMACHSTATE) - #define FORLAZYMACHSTATE_DEBUG_OK_TO_RETURN_END DEBUG_OK_TO_RETURN_END(LAZYMACHSTATE) #endif // BEGIN: before gcpoll @@ -558,10 +554,6 @@ LPVOID __FCThrow(LPVOID me, enum RuntimeExceptionKind reKind, UINT resID, LPCWST // END: after gcpoll //__fcallGcCanTrigger.Leave(__FUNCTION__, __FILE__, __LINE__); -// We have to put DEBUG_OK_TO_RETURN_BEGIN around the FORLAZYMACHSTATE -// to allow the HELPER_FRAME to be installed inside an SO_INTOLERANT region -// which does not allow a return. The return is used by FORLAZYMACHSTATE -// to capture the state, but is not an actual return, so it is ok. #define HELPER_METHOD_FRAME_BEGIN_EX_BODY(ret, helperFrame, gcpoll, allowGC) \ FORLAZYMACHSTATE_BEGINLOOP(int alwaysZero = 0;) \ { \ @@ -569,9 +561,7 @@ LPVOID __FCThrow(LPVOID me, enum RuntimeExceptionKind reKind, UINT resID, LPCWST PERMIT_HELPER_METHOD_FRAME_BEGIN(); \ CHECK_HELPER_METHOD_FRAME_PERMITTED(); \ helperFrame; \ - FORLAZYMACHSTATE_DEBUG_OK_TO_RETURN_BEGIN; \ FORLAZYMACHSTATE(CAPTURE_STATE(__helperframe.MachineState(), ret);) \ - FORLAZYMACHSTATE_DEBUG_OK_TO_RETURN_END; \ INDEBUG(__helperframe.SetAddrOfHaveCheckedRestoreState(&__haveCheckedRestoreState)); \ DEBUG_ASSURE_NO_RETURN_BEGIN(HELPER_METHOD_FRAME); \ INCONTRACT(FCallGCCanTrigger::Enter()); From 0af13da14cf9b0060e82b0a2f2307e7499c5b62d Mon Sep 17 00:00:00 2001 From: Jeremy Koritzinsky Date: Tue, 25 Mar 2025 14:28:53 -0700 Subject: [PATCH 11/11] PR feedback --- src/coreclr/vm/threads.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/threads.h b/src/coreclr/vm/threads.h index bde77a84ea0471..afbc74f239c7d9 100644 --- a/src/coreclr/vm/threads.h +++ b/src/coreclr/vm/threads.h @@ -5240,7 +5240,7 @@ class CoopTransitionHolder ~CoopTransitionHolder() { WRAPPER_NO_CONTRACT; - _ASSERTE(m_pFrame == nullptr || std::uncaught_exception()); + _ASSERTE_MSG(m_pFrame == nullptr || std::uncaught_exception(), "Early return from JIT/EE interface method"); if (m_pFrame != nullptr) COMPlusCooperativeTransitionHandler(m_pFrame); }