[interp] always require GC Unsafe mode in exception checkpoint

[lewurm]

Follow-up for #16955

/cc @BrzVlad

[BrzVlad]

Overall the code is a little fishy when it comes to this unsafe checking. Normally the wrapper does all the necessary transitions and we would know when to check and when not to check for exception inside the wrapper (I think this is how it works on jit). In the interpreter, I think we use this opcode for multiple scenarios, some of which require exception checking, some of which must not (and therefore the bug from the other PR).

Regardless, we should probably have a separate define, that does the state check, only for these icall opcodes. The check is redundant for other uses of EXCEPTION_CHECKPOINT (since we are by definition in unsafe state) and it might also confuse readers of code.

[lewurm]

You are right, the wrapper could do it:

mono/mono/metadata/marshal.c

Lines 3447 to 3449 in aeca4a6

	MonoMethod *
	mono_marshal_get_native_wrapper (MonoMethod *method, gboolean check_exceptions, gboolean aot)
	{

mono/mono/metadata/marshal-ilgen.c

Lines 2073 to 2074 in aeca4a6

	if (check_exceptions)
	emit_thread_interrupt_checkpoint (mb);

It looks like it would do the thread interruption regardless of the GC state?

mono/mono/metadata/marshal-ilgen.c

Lines 1188 to 1218 in aeca4a6

	static void
	emit_thread_interrupt_checkpoint_call (MonoMethodBuilder *mb, MonoJitICallId checkpoint_icall_id)
	{
	int pos_noabort, pos_noex;
	mono_mb_emit_byte (mb, MONO_CUSTOM_PREFIX);
	mono_mb_emit_byte (mb, CEE_MONO_LDPTR_INT_REQ_FLAG);
	mono_mb_emit_byte (mb, CEE_LDIND_U4);
	pos_noabort = mono_mb_emit_branch (mb, CEE_BRFALSE);
	mono_mb_emit_byte (mb, MONO_CUSTOM_PREFIX);
	mono_mb_emit_byte (mb, CEE_MONO_NOT_TAKEN);
	mono_mb_emit_icall_id (mb, checkpoint_icall_id);
	/* Throw the exception returned by the checkpoint function, if any */
	mono_mb_emit_byte (mb, CEE_DUP);
	pos_noex = mono_mb_emit_branch (mb, CEE_BRFALSE);
	mono_mb_emit_byte (mb, CEE_DUP);
	mono_mb_emit_ldflda (mb, MONO_STRUCT_OFFSET (MonoException, caught_in_unmanaged));
	mono_mb_emit_byte (mb, CEE_LDC_I4_1);
	mono_mb_emit_byte (mb, CEE_STIND_I4);
	mono_mb_emit_byte (mb, MONO_CUSTOM_PREFIX);
	mono_mb_emit_byte (mb, CEE_MONO_RETHROW);
	mono_mb_patch_branch (mb, pos_noex);
	mono_mb_emit_byte (mb, CEE_POP);
	mono_mb_patch_branch (mb, pos_noabort);
	}

Not sure how this can work in the JIT then. Anyway, we specifically ask to not do generate those checks when building the wrapper for interpreter uses:

mono/mono/mini/interp/interp.c

Line 510 in aeca4a6

virtual_method = mono_marshal_get_native_wrapper (virtual_method, FALSE, FALSE);

mono/mono/mini/interp/interp.c

Line 1683 in aeca4a6

target_method = mono_marshal_get_native_wrapper (target_method, FALSE, FALSE);

mono/mono/mini/interp/interp.c

Line 3460 in aeca4a6

child_frame.imethod = mono_interp_get_imethod (frame->imethod->domain, mono_marshal_get_native_wrapper (child_frame.imethod->method, FALSE, FALSE), error);

Edit: and in transform.c:

mono/mono/mini/interp/transform.c

Line 1702 in aeca4a6

target_method = mono_marshal_get_native_wrapper (target_method, FALSE, FALSE);

mono/mono/mini/interp/transform.c

Line 1707 in aeca4a6

target_method = mono_marshal_get_native_wrapper (target_method, FALSE, FALSE);

mono/mono/mini/interp/transform.c

Line 6720 in aeca4a6

nm = mono_marshal_get_native_wrapper (method, FALSE, FALSE);

[lewurm]

@monojenkins squash