Incorrect marshalling of Bool return types in new exception handling

[filipnavara]

The managed prototypes for RhpSfiInit, RhpSfiNext and few other methods specify UnmanagedType.Bool for the return values:

runtime/src/coreclr/System.Private.CoreLib/src/System/Runtime/ExceptionServices/InternalCalls.cs

Lines 15 to 21 in 4ca1691

	[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "SfiInit")]
	[return: MarshalAs(UnmanagedType.Bool)]
	internal static unsafe partial bool RhpSfiInit(ref StackFrameIterator pThis, void* pStackwalkCtx, [MarshalAs(UnmanagedType.Bool)] bool instructionFault, bool* fIsExceptionIntercepted);
	[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "SfiNext")]
	[return: MarshalAs(UnmanagedType.Bool)]
	internal static unsafe partial bool RhpSfiNext(ref StackFrameIterator pThis, uint* uExCollideClauseIdx, bool* fUnwoundReversePInvoke, bool* fIsExceptionIntercepted);

This is a four byte Win32 BOOL type. However, the native code uses 1-byte C++ bool type.

In the debug version of the runtime with MSVC compiler the contracts often trash the high bytes of the RAX/EAX register and thus an incorrect return value is marshalled.

/cc @janvorli

[janvorli]

In the debug version of the runtime with MSVC compiler the contracts often trash the high bytes of the RAX/EAX register and thus an incorrect return value is marshalled.

@filipnavara thank you for spotting this. I am surprised though that you have seen a problem occur, as I've never hit it in my testing that's always with debug / checked build.

[filipnavara]

It's probably dependent on a lot of factors including compiler versions. I initially spotted it when testing my win-x86 branch with funclets where the compiler was more prone to generate the patterns that only restored al register. That said, I was regularly comparing the runs with win-x64 and at least on one occasion I hit the problem there as well.

I don't remember which test hit the scenario but I was running the NativeAOT Exceptions test on CoreCLR which is normally excluded in the runs. That test explicitly runs throw new ...() inside the Main method and expects to hit the unhandled exception handler. It depends on the SfiNext return value to be correct to terminate the exception dispatch loop in the right place.