debugreturn.h should not macroize C++ keywords

[GrabYourPitchforks]

The reserved keyword return is macroized in debugreturn.h:

runtime/src/coreclr/inc/debugreturn.h

Line 25 in 3c3155d

#define return return

runtime/src/coreclr/inc/debugreturn.h

Lines 93 to 102 in 3c3155d

	// Unfortunately, the only way to make this work is to #define all return statements --
	// even the ones at global scope. This actually generates better code that appears.
	// The call is dead, and does not appear in the generated code, even in a checked
	// build. (And, in fastchecked, there is no penalty at all.)
	//
	#ifdef _MSC_VER
	#define return if (0 && __ReturnOK::safe_to_return()) { } else return
	#else // _MSC_VER
	#define return for (;1;__ReturnOK::safe_to_return()) return
	#endif // _MSC_VER

This is preventing import of some STL headers, causing build failures in PRs such as #67464. Static analyzers like https://rules.sonarsource.com/cpp/RSPEC-5266 catch this as bad practice, and MSVC also flags this as an error.

Consider using a different name, such as CHECKED_RETURN, instead of piggybacking on the reserved keyword return.

[jkotas]

Note that we do this in non-shipping debug/checked builds only.

[jkotas]

Consider using a different name, such as CHECKED_RETURN, instead of piggybacking on the reserved keyword return.

The whole point of this hack is to catch bad accidental incorrect uses of the return statements. If we were to use different name, this hack would lose its point.

I guess the proper fix would require getting rid of the macros that prohibit use of the return statements.

[GrabYourPitchforks]

Thanks for the context. I was looking around at other patterns that might work, such as clever use of dtors, but couldn't find anything that worked as well as the #define trick we're using.

The page https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/docs/coding-guidelines/clr-code-guide.md#214-dont-do-nonlocal-returns-from-within-gcprotect-blocks contains this text.

Why is GCPROTECT not implemented via a C++ smart pointer? The GCPROTECT macro originates in .NET Framework v1. All error handling was done explicitly at that time, without any use C++ exception handling or stack allocated holders.

Is there a technical reason forbidding reimplementing this functionality using C++ idioms like RAII for these scenarios, which would allow non-local returns? I can imagine that this would cause a decent amount of churn, assuming it's even pursuable.

Edit: I guess one can always do something silly with C++ lambdas if so inspired, which would cause the return to exit only the inner lambda, not the enclosing function. Changing the type away from "void" will cause return to generate a compiler error. Big downside is that F10 breaks in the debugger. :(

#define GCPROTECT_BEGIN() [&]() [[msvc::forceinline]] -> void {
#define GCPROTECT_END() }();

[jkotas]

The GCPROTECT macro was reimplemented using standard C++ idioms already. You can delete DEBUG_ASSURE_NO_RETURN_BEGIN use from it and also the quoted text from clr-code-guide.md.

However, we have number of other key macros that still use DEBUG_ASSURE_NO_RETURN_BEGIN (e.g. HELPER_METHOD_FRAME macros) that are hard to reimplement using C++ idioms.

C++ lambdas

FWIW, C++ lamdas come with significant code quality hit. We have tried to replace some macros in the GC with lamdas some time ago. We had to abandon the idea due to unacceptable performance impact.

[AaronRobinsonMSFT]

I guess the proper fix would require getting rid of the macros that prohibit use of the return statements.

@jkotas and @janvorli Thoughts on removing this compile time check? I agree it is unfortunate, but there is very little active development in this area anymore.

[jkotas]

there is very little active development in this area anymore.

I believe that some VM code that has to follow these no-return rules is being changed like every week.

Removing this compile time check will turn compile error into some sort of runtime crash. You can try to introduce a bad return inside HELPER_METHOD_FRAME and then try to debug the runtime crash to see what the experience looks like with this compile check deleted.

[mangod9]

@AaronRobinsonMSFT probably something which can be moved out of 8?