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 all 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
11 changes: 1 addition & 10 deletions src/coreclr/inc/contract.h
View file
Open in desktop
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 @@ -1460,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.)
Expand Down Expand Up @@ -1679,15 +1672,13 @@ 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
#define RESET_CONTRACT_VIOLATION() \
__violationHolder_onlyOneAllowedPerScope.Leave(); \

#define END_CONTRACT_VIOLATION \
DEBUG_ASSURE_NO_RETURN_END(CONTRACT) \
__violationHolder_onlyOneAllowedPerScope.Leave(); \
} \

Expand Down
9 changes: 0 additions & 9 deletions src/coreclr/inc/debugreturn.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@
#define DEBUG_ASSURE_NO_RETURN_BEGIN(arg) { char* __noReturnInThisBlock_##arg = ::new (nothrow) char[1];
#define DEBUG_ASSURE_NO_RETURN_END(arg) ::delete[] __noReturnInThisBlock_##arg; }

#define DEBUG_OK_TO_RETURN_BEGIN(arg) { ::delete[] __noReturnInThisBlock_##arg;
#define DEBUG_OK_TO_RETURN_END(arg) __noReturnInThisBlock_##arg = ::new (nothrow) char[1]; }

#define DEBUG_ASSURE_SAFE_TO_RETURN TRUE
#define return return

Expand Down Expand Up @@ -104,19 +101,13 @@ typedef __SafeToReturn __ReturnOK;
#define DEBUG_ASSURE_NO_RETURN_BEGIN(arg) { typedef __YouCannotUseAReturnStatementHere __ReturnOK; if (0 && __ReturnOK::used()) { } else {
#define DEBUG_ASSURE_NO_RETURN_END(arg) } }

#define DEBUG_OK_TO_RETURN_BEGIN(arg) { typedef __SafeToReturn __ReturnOK; if (0 && __ReturnOK::used()) { } else {
#define DEBUG_OK_TO_RETURN_END(arg) } }

#else // defined(_DEBUG) && !defined(JIT_BUILD) && (!defined(_MSC_FULL_VER) || _MSC_FULL_VER > 190024315)

#define DEBUG_ASSURE_SAFE_TO_RETURN TRUE

#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_
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

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
10 changes: 0 additions & 10 deletions src/coreclr/vm/fcall.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -558,20 +554,14 @@ 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;) \
{ \
INDEBUG(static BOOL __haveCheckedRestoreState = FALSE;) \
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());
Expand Down
Loading
Loading