Remove most usages of the macro-based "don't return in this block" hack #113856
Conversation
…ugh those macros are automatically removed by the constructor
…ring that we don't do a return to ensure we call a function in the success case.
… a no-return scope
…R scope as nothing in the uninstall requires manual unwinding.
…view to not allow people to return out of a contract block.
There was a problem hiding this comment.
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).
| @@ -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); | |||
There was a problem hiding this comment.
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.
src/coreclr/inc/contract.h
Outdated
There was a problem hiding this comment.
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).
|
|
||
| #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) |
There was a problem hiding this comment.
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.
…er as it shouldn't happen in that case for unwinding.
…alled while a C++ exception is in flight
|
My suggestion would be:
|
…ansitionHolder by a runtime assert instead of a compile-time error.
…ast of the OK_TO_RETURN macros
|
/ba-g failures unrelated |
We have hack in the CoreCLR codebase where we define the
returnkeyword as a macro with aforloop that depends on typedefs to enable blocking returns from within specific blocks of code.This has caused issues that have had to be fixed in the following PRs:
#67464
#109756
#113279
Many of these blocks of code can be adjusted slightly to not require this mechanism to act correctly or already do not require it. Also, some usages are just dead code entirely.
This PR does not remove the mechanism as it does not remove all usages of it. The final usage (as mentioned in #67470) is in the support logic for HelperMethodFrames. As we are removing HMFs from the runtime (#95695), I figure that we can remove this mechanism after we remove HMFs instead of trying to figure out how to re-implement the HMF logic such that returns are safe.