Skip to content

Remove most usages of the macro-based "don't return in this block" hack #113856

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

Merged
merged 11 commits into from
Mar 26, 2025
Merged

Remove most usages of the macro-based "don't return in this block" hack #113856

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4af9a47
Remove unused frame type
jkoritzinsky Mar 7, 2025
512c338
Remove "debug noreturn" from GCPROTECT. GCFrame instances set up thro…
jkoritzinsky Mar 7, 2025
0f07989
Actually check for if we have an exception in flight instead of requi…
jkoritzinsky Mar 7, 2025
aa8002c
Create a separate "preserve last error" holder type to allow removing…
jkoritzinsky Mar 24, 2025
24e6f34
Allow returning from within an (UN)INSTALL_UNWIND_AND_CONTINUE_HANDLE…
jkoritzinsky Mar 24, 2025
f581f23
Don't block "return" in contracts at compile time, just rely on PR re…
jkoritzinsky Mar 24, 2025
6cc3ecd
Adjust one usage of BEGIN/END_PRESERVE_LAST_ERROR to not use the hold…
jkoritzinsky Mar 25, 2025
42fb90e
Add assert that CoopTransitionHolder isn't being used in a function c…
jkoritzinsky Mar 25, 2025
666af8e
Remove PRESERVE_LAST_ERROR macros and enforce correct usage of CoopTr…
jkoritzinsky Mar 25, 2025
c36e9d8
Remove some now-unneeded ASSURE_NO_RETURN_END macros and remove the l…
jkoritzinsky Mar 25, 2025
0af13da
PR feedback
jkoritzinsky Mar 25, 2025
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
23 changes: 13 additions & 10 deletions src/coreclr/inc/clrhost.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,20 @@ using std::nothrow;
#define _DEBUG_IMPL 1
#endif

#define BEGIN_PRESERVE_LAST_ERROR \
{ \
DWORD __dwLastError = ::GetLastError(); \
DEBUG_ASSURE_NO_RETURN_BEGIN(PRESERVE_LAST_ERROR); \
{

#define END_PRESERVE_LAST_ERROR \
} \
DEBUG_ASSURE_NO_RETURN_END(PRESERVE_LAST_ERROR); \
::SetLastError(__dwLastError); \
struct PreserveLastErrorHolder
{
PreserveLastErrorHolder()
{
m_dwLastError = ::GetLastError();
}

~PreserveLastErrorHolder()
{
::SetLastError(m_dwLastError);
}
private:
DWORD m_dwLastError;
};

//
// TRASH_LASTERROR macro sets bogus last error in debug builds to help find places that fail to save it
Expand Down
7 changes: 0 additions & 7 deletions src/coreclr/inc/contract.h
View file
Open in desktop
Copy link
Member Author

@jkoritzinsky jkoritzinsky Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Technically it is still unsafe to return from within a CONTRACT block. However, I believe that it is quite obvious that it is incorrect to do so, to the point that it would immediately pop out in code-review as an issue that must be fixed (to the point that I would expect AI code review tools like Copilot to flag it as suspicious).

Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand All @@ -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) \
Expand Down Expand Up @@ -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}; \
{ \
Expand All @@ -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) \
Expand Down Expand Up @@ -1679,7 +1673,6 @@ class AutoCleanupContractViolationHolder : ContractViolationHolder<VIOLATION_MAS
{ \
ContractViolationHolder<violationmask> __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
Expand Down
3 changes: 0 additions & 3 deletions src/coreclr/vm/FrameTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 1 addition & 3 deletions src/coreclr/vm/callcounting.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,7 +760,7 @@ PCODE CallCountingManager::OnCallCountThresholdReached(TransitionBlock *transiti

PCODE codeEntryPoint = 0;

BEGIN_PRESERVE_LAST_ERROR;
PreserveLastErrorHolder preserveLastError;

MAKE_CURRENT_THREAD_AVAILABLE();

Expand Down Expand Up @@ -823,8 +823,6 @@ PCODE CallCountingManager::OnCallCountThresholdReached(TransitionBlock *transiti

frame->Pop(CURRENT_THREAD);

END_PRESERVE_LAST_ERROR;

return codeEntryPoint;
}

Expand Down
12 changes: 3 additions & 9 deletions src/coreclr/vm/codeman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -4379,7 +4377,7 @@ extern "C" void GetRuntimeStackWalkInfo(IN ULONG64 ControlPc,

WRAPPER_NO_CONTRACT;

BEGIN_PRESERVE_LAST_ERROR;
PreserveLastErrorHolder preserveLastError;

if (pModuleBase)
*pModuleBase = 0;
Expand All @@ -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)
Expand All @@ -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

Expand Down
12 changes: 3 additions & 9 deletions src/coreclr/vm/dllimport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5828,7 +5828,7 @@ EXTERN_C LPVOID STDCALL NDirectImportWorker(NDirectMethodDesc* pMD)
{
LPVOID ret = NULL;

BEGIN_PRESERVE_LAST_ERROR;
PreserveLastErrorHolder preserveLastError;

CONTRACTL
{
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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)
Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/vm/excep.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
15 changes: 6 additions & 9 deletions src/coreclr/vm/exceptmacros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand All @@ -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);
Copy link
Member Author

@jkoritzinsky jkoritzinsky Mar 24, 2025

Choose a reason for hiding this comment

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

Nothing in the enclosing block in the INSTALL macros nor any code following in the UNINSTALL macros depend on us not returning from within this block.

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&) \
Expand Down Expand Up @@ -500,12 +497,12 @@ 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(); \
__CoopTransition.SuppressRelease(); \
} \
END_GCX_ASSERT_PREEMP; \
}

Expand Down
68 changes: 6 additions & 62 deletions src/coreclr/vm/frames.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -2919,25 +2889,22 @@ class AssumeByrefFromJITStackFrame : 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( \
pThread, \
(OBJECTREF*)&(ObjRefStruct), \
sizeof(ObjRefStruct)/sizeof(OBJECTREF), \
FALSE); \
/* work around unreachable code warning */ \
if (true) { DEBUG_ASSURE_NO_RETURN_BEGIN(GCPROTECT)
Copy link
Member

@janvorli janvorli Mar 24, 2025

Choose a reason for hiding this comment

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

A note - I believe this removal is correct because we have changed the GCFrame to call Pop from the destructor couple of years ago. The requirement for no return was already obsolete.

{

#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 */ \
Expand All @@ -2947,20 +2914,18 @@ class AssumeByrefFromJITStackFrame : 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)


Expand All @@ -2977,27 +2942,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);
Expand Down
Loading
Loading