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 6 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
25 changes: 19 additions & 6 deletions src/coreclr/inc/clrhost.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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); \
} \
}

//
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
14 changes: 5 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,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; \
}

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
11 changes: 2 additions & 9 deletions src/coreclr/vm/threads.h
View file
Open in desktop
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.

The changes in this file are the most "sketchy" to me as they add a dependency on the C++ Standard Library's exception APIs. As we already use C++ exceptions all over the VM, this isn't that out of the ordinary (and if we used a different mechanism, we wouldn't be considering using this API anyway).

Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@
#ifndef __threads_h__
#define __threads_h__

#include <exception>
#include "vars.hpp"
#include "util.hpp"
#include "eventstore.hpp"
Expand Down Expand Up @@ -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;
}
};

// --------------------------------------------------------------------------------
Expand Down
Loading