[mono][interp] Fix unboxing during CALLI (#59747)

* [tests] Re-enable test * [interp] Fix unboxing during CALLI On interp we were storing function pointers as a simple InterpMethod. The problem is that, when loading an InterpMethod with a ldvirtftn opcode, the resulting method might need to have its this pointer unboxed, so this information needs to be embedded into the function pointer. We achieve this by tagging the InterpMethod whether we need to perform unboxing as part of calling it.
dotnet · Oct 9, 2021 · 9fbc728 · 9fbc728
1 parent 3de6207
commit 9fbc728
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 35 deletions.
diff --git a/src/mono/mono/mini/interp/interp-internals.h b/src/mono/mono/mini/interp/interp-internals.h
@@ -97,6 +97,10 @@ typedef enum {
 
 #define PROFILE_INTERP 0
 
+#define INTERP_IMETHOD_TAG_UNBOX(im) ((gpointer)((mono_u)(im) | 1))
+#define INTERP_IMETHOD_IS_TAGGED_UNBOX(im) ((mono_u)(im) & 1)
+#define INTERP_IMETHOD_UNTAG_UNBOX(im) ((InterpMethod*)((mono_u)(im) & ~1))
+
 /* 
  * Structure representing a method transformed for the interpreter 
  */
@@ -125,6 +129,7 @@ struct InterpMethod {
 	MonoType **param_types;
 	MonoJitInfo *jinfo;
 	MonoFtnDesc *ftndesc;
+	MonoFtnDesc *ftndesc_unbox;
 
 	guint32 locals_size;
 	guint32 alloca_size;

diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c
@@ -1679,7 +1679,7 @@ interp_init_delegate (MonoDelegate *del, MonoError *error)
 
 /* Convert a function pointer for a managed method to an InterpMethod* */
 static InterpMethod*
-ftnptr_to_imethod (gpointer addr)
+ftnptr_to_imethod (gpointer addr, gboolean *need_unbox)
 {
 	InterpMethod *imethod;
 
@@ -1690,44 +1690,59 @@ ftnptr_to_imethod (gpointer addr)
 		g_assert (ftndesc);
 		g_assert (ftndesc->method);
 
-		imethod = ftndesc->interp_method;
-		if (!imethod) {
+		if (!ftndesc->interp_method) {
 			imethod = mono_interp_get_imethod (ftndesc->method, error);
 			mono_error_assert_ok (error);
 			mono_memory_barrier ();
+			// FIXME Handle unboxing here ?
 			ftndesc->interp_method = imethod;
 		}
+		*need_unbox = INTERP_IMETHOD_IS_TAGGED_UNBOX (ftndesc->interp_method);
+		imethod = INTERP_IMETHOD_UNTAG_UNBOX (ftndesc->interp_method);
 	} else {
 		/* Function pointers are represented by their InterpMethod */
-		imethod = (InterpMethod*)addr;
+		*need_unbox = INTERP_IMETHOD_IS_TAGGED_UNBOX (addr);
+		imethod = INTERP_IMETHOD_UNTAG_UNBOX (addr);
 	}
 	return imethod;
 }
 
 static gpointer
-imethod_to_ftnptr (InterpMethod *imethod)
+imethod_to_ftnptr (InterpMethod *imethod, gboolean need_unbox)
 {
 	if (mono_llvm_only) {
 		ERROR_DECL (error);
 		/* Function pointers are represented by a MonoFtnDesc structure */
-		MonoFtnDesc *ftndesc = imethod->ftndesc;
-		if (!ftndesc) {
-			ftndesc = mini_llvmonly_load_method_ftndesc (imethod->method, FALSE, FALSE, error);
+		MonoFtnDesc **ftndesc_p;
+		if (need_unbox)
+			ftndesc_p = &imethod->ftndesc_unbox;
+		else
+			ftndesc_p = &imethod->ftndesc;
+		if (!*ftndesc_p) {
+			MonoFtnDesc *ftndesc = mini_llvmonly_load_method_ftndesc (imethod->method, FALSE, need_unbox, error);
 			mono_error_assert_ok (error);
+			if (need_unbox)
+				ftndesc->interp_method = INTERP_IMETHOD_TAG_UNBOX (imethod);
+			else
+				ftndesc->interp_method = imethod;
 			mono_memory_barrier ();
-			imethod->ftndesc = ftndesc;
+			*ftndesc_p = ftndesc;
 		}
-		return ftndesc;
+		return *ftndesc_p;
 	} else {
-		return imethod;
+		if (need_unbox)
+			return INTERP_IMETHOD_TAG_UNBOX (imethod);
+		else
+			return imethod;
 	}
 }
 
 static void
 interp_delegate_ctor (MonoObjectHandle this_obj, MonoObjectHandle target, gpointer addr, MonoError *error)
 {
+	gboolean need_unbox;
 	/* addr is the result of an LDFTN opcode */
-	InterpMethod *imethod = ftnptr_to_imethod (addr);
+	InterpMethod *imethod = ftnptr_to_imethod (addr, &need_unbox);
 
 	if (!(imethod->method->flags & METHOD_ATTRIBUTE_STATIC)) {
 		MonoMethod *invoke = mono_get_delegate_invoke_internal (mono_handle_class (this_obj));
@@ -3484,12 +3499,10 @@ interp_exec_method (InterpFrame *frame, ThreadContext *context, FrameClauseArgs
 			goto call;
 		}
 		MINT_IN_CASE(MINT_CALLI) {
-			MonoMethodSignature *csignature;
-
-			csignature = (MonoMethodSignature*)frame->imethod->data_items [ip [4]];
+			gboolean need_unbox;
 
 			/* In mixed mode, stay in the interpreter for simplicity even if there is an AOT version of the callee */
-			cmethod = ftnptr_to_imethod (LOCAL_VAR (ip [2], gpointer));
+			cmethod = ftnptr_to_imethod (LOCAL_VAR (ip [2], gpointer), &need_unbox);
 
 			if (cmethod->method->flags & METHOD_ATTRIBUTE_PINVOKE_IMPL) {
 				cmethod = mono_interp_get_imethod (mono_marshal_get_native_wrapper (cmethod->method, FALSE, FALSE), error);
@@ -3499,15 +3512,11 @@ interp_exec_method (InterpFrame *frame, ThreadContext *context, FrameClauseArgs
 			return_offset = ip [1];
 			call_args_offset = ip [3];
 
-			if (csignature->hasthis) {
+			if (need_unbox) {
 				MonoObject *this_arg = LOCAL_VAR (call_args_offset, MonoObject*); 
-
-				if (m_class_is_valuetype (this_arg->vtable->klass)) {
-					gpointer unboxed = mono_object_unbox_internal (this_arg);
-					LOCAL_VAR (call_args_offset, gpointer) = unboxed;
-				}
+				LOCAL_VAR (call_args_offset, gpointer) = mono_object_unbox_internal (this_arg);
 			}
-			ip += 5;
+			ip += 4;
 
 			goto call;
 		}
@@ -6508,17 +6517,18 @@ MINT_IN_CASE(MINT_BRTRUE_I8_SP) ZEROP_SP(gint64, !=); MINT_IN_BREAK;
 		MINT_IN_CASE(MINT_LDFTN) {
 			InterpMethod *m = (InterpMethod*)frame->imethod->data_items [ip [2]];
 
-			LOCAL_VAR (ip [1], gpointer) = imethod_to_ftnptr (m);
+			LOCAL_VAR (ip [1], gpointer) = imethod_to_ftnptr (m, FALSE);
 			ip += 3;
 			MINT_IN_BREAK;
 		}
 		MINT_IN_CASE(MINT_LDVIRTFTN) {
-			InterpMethod *m = (InterpMethod*)frame->imethod->data_items [ip [3]];
+			InterpMethod *virtual_method = (InterpMethod*)frame->imethod->data_items [ip [3]];
 			MonoObject *o = LOCAL_VAR (ip [2], MonoObject*);
 			NULL_CHECK (o);
 
-			m = get_virtual_method (m, o->vtable);
-			LOCAL_VAR (ip [1], gpointer) = imethod_to_ftnptr (m);
+			InterpMethod *res_method = get_virtual_method (virtual_method, o->vtable);
+			gboolean need_unbox = m_class_is_valuetype (res_method->method->klass) && !m_class_is_valuetype (virtual_method->method->klass);
+			LOCAL_VAR (ip [1], gpointer) = imethod_to_ftnptr (res_method, need_unbox);
 			ip += 4;
 			MINT_IN_BREAK;
 		}
@@ -6529,7 +6539,7 @@ MINT_IN_CASE(MINT_BRTRUE_I8_SP) ZEROP_SP(gint64, !=); MINT_IN_BREAK;
 
 			InterpMethod *m = mono_interp_get_imethod (cmethod, error);
 			mono_error_assert_ok (error);
-			LOCAL_VAR (ip [1], gpointer) = imethod_to_ftnptr (m);
+			LOCAL_VAR (ip [1], gpointer) = imethod_to_ftnptr (m, FALSE);
 			ip += 3;
 			MINT_IN_BREAK;
 		}
@@ -6711,7 +6721,7 @@ MINT_IN_CASE(MINT_BRTRUE_I8_SP) ZEROP_SP(gint64, !=); MINT_IN_BREAK;
 				mono_error_assert_ok (error);
 			}
 			g_assert (del->interp_method);
-			LOCAL_VAR (ip [1], gpointer) = imethod_to_ftnptr (del->interp_method);
+			LOCAL_VAR (ip [1], gpointer) = imethod_to_ftnptr (del->interp_method, FALSE);
 			ip += 3;
 			MINT_IN_BREAK;
 		}

diff --git a/src/mono/mono/mini/interp/mintops.def b/src/mono/mono/mini/interp/mintops.def
@@ -664,7 +664,7 @@ OPDEF(MINT_CALL, "call", 4, 1, 1, MintOpMethodToken)
 OPDEF(MINT_CALLVIRT, "callvirt", 4, 1, 1, MintOpMethodToken)
 OPDEF(MINT_CALLVIRT_FAST, "callvirt.fast", 5, 1, 1, MintOpMethodToken)
 OPDEF(MINT_CALL_DELEGATE, "call.delegate", 5, 1, 1, MintOpTwoShorts)
-OPDEF(MINT_CALLI, "calli", 5, 1, 2, MintOpMethodToken)
+OPDEF(MINT_CALLI, "calli", 4, 1, 2, MintOpNoArgs)
 OPDEF(MINT_CALLI_NAT, "calli.nat", 8, 1, 2, MintOpMethodToken)
 OPDEF(MINT_CALLI_NAT_DYNAMIC, "calli.nat.dynamic", 5, 1, 2, MintOpMethodToken)
 OPDEF(MINT_CALLI_NAT_FAST, "calli.nat.fast", 7, 1, 2, MintOpMethodToken)

diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c
@@ -3511,7 +3511,6 @@ interp_transform_call (TransformData *td, MonoMethod *method, MonoMethod *target
 				interp_add_ins (td, MINT_CALLI);
 				interp_ins_set_dreg (td->last_ins, dreg);
 				interp_ins_set_sregs2 (td->last_ins, fp_sreg, MINT_CALL_ARGS_SREG);
-				td->last_ins->data [0] = get_data_item_index (td, (void *)csignature);
 			}
 		} else {
 			InterpMethod *imethod = mono_interp_get_imethod (target_method, error);

diff --git a/src/mono/mono/mini/mini.h b/src/mono/mono/mini/mini.h
@@ -1144,7 +1144,7 @@ typedef struct
 	gpointer addr;
 	gpointer arg;
 	MonoMethod *method;
-	/* InterpMethod* */
+	/* Tagged InterpMethod* */
 	gpointer interp_method;
 } MonoFtnDesc;
 

diff --git a/src/tests/issues.targets b/src/tests/issues.targets
@@ -1556,9 +1556,6 @@
         <ExcludeList Include = "$(XunitTestBinBase)/JIT/jit64/verif/sniff/fg/ver_fg_13/**">
             <Issue>https://github.com/dotnet/runtime/issues/54396</Issue>
         </ExcludeList>
-        <ExcludeList Include = "$(XunitTestBinBase)/JIT/Methodical/VT/callconv/_il_reljumper3/**">
-            <Issue>https://github.com/dotnet/runtime/issues/54374</Issue>
-        </ExcludeList>
         <ExcludeList Include = "$(XunitTestBinBase)/JIT/Methodical/NaN/r4nanconv_il_d/**">
             <Issue>https://github.com/dotnet/runtime/issues/54381</Issue>
         </ExcludeList>