[wasm] pinvoke improvements part 2 (#98250)

* InlineArray is supported in wasm pinvokes, and in general works correctly in mono marshaling now (it was broken on all targets)
* fixed arrays are supported in wasm pinvokes
*struct arguments and return values are supported in more wasm pinvoke scenarios
* wasm struct scalarization is more intelligent about noticing padding and size mismatches in structs (without this, some inlinearrays would be scalarized incorrectly)
* Prevent infinite recursion in build task's TypeToChar
* expand WBT coverage

Author: kg

src/mono/mono/metadata/class-init.c

@@ -2272,6 +2272,7 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_
 				}
 
 				size = mono_type_size (field->type, &align);
+				// keep in sync with marshal.c mono_marshal_load_type_info
 				if (m_class_is_inlinearray (klass)) {
 					// Limit the max size of array instance to 1MiB
 					const guint32 struct_max_size = 1024 * 1024;

src/mono/mono/metadata/icall.c

@@ -3028,7 +3028,7 @@ ves_icall_RuntimeType_GetNamespace (MonoQCallTypeHandle type_handle, MonoObjectH
 
 	MonoClass *klass = mono_class_from_mono_type_internal (type);
 	MonoClass *elem;
-	while (!m_class_is_enumtype (klass) && 
+	while (!m_class_is_enumtype (klass) &&
 		!mono_class_is_nullable (klass) &&
 		(klass != (elem = m_class_get_element_class (klass))))
 		klass = elem;

src/mono/mono/metadata/marshal.c

@@ -3294,7 +3294,7 @@ mono_emit_marshal (EmitMarshalContext *m, int argnum, MonoType *t,
 		return mono_emit_disabled_marshal (m, argnum, t, spec, conv_arg, conv_arg_type, action);
 
 	return mono_component_marshal_ilgen()->emit_marshal_ilgen(m, argnum, t, spec, conv_arg, conv_arg_type, action, get_marshal_cb());
-} 
+}
 
 static void
 mono_marshal_set_callconv_for_type(MonoType *type, MonoMethodSignature *csig, gboolean *skip_gc_trans /*out*/)
@@ -3577,7 +3577,7 @@ mono_marshal_get_native_wrapper (MonoMethod *method, gboolean check_exceptions,
 
 	if (G_UNLIKELY (pinvoke && mono_method_has_unmanaged_callers_only_attribute (method))) {
 		/*
-		 * In AOT mode and embedding scenarios, it is possible that the icall is not registered in the runtime doing the AOT compilation. 
+		 * In AOT mode and embedding scenarios, it is possible that the icall is not registered in the runtime doing the AOT compilation.
 		 * Emit a wrapper that throws a NotSupportedException.
 		 */
 		get_marshal_cb ()->mb_emit_exception (mb, "System", "NotSupportedException", "Method canot be marked with both DllImportAttribute and UnmanagedCallersOnlyAttribute");
@@ -3757,7 +3757,7 @@ mono_marshal_get_native_wrapper (MonoMethod *method, gboolean check_exceptions,
 	}
 
 	goto leave;
-	
+
 	emit_exception_for_error:
 		mono_error_cleanup (emitted_error);
 		info = mono_wrapper_info_create (mb, WRAPPER_SUBTYPE_NONE);
@@ -5231,7 +5231,7 @@ mono_marshal_get_unsafe_accessor_wrapper (MonoMethod *accessor_method, MonoUnsaf
 
 	if (member_name == NULL && kind != MONO_UNSAFE_ACCESSOR_CTOR)
 		member_name = accessor_method->name;
-	
+
 	/*
 	 * Check cache
 	 */
@@ -5827,11 +5827,20 @@ mono_marshal_load_type_info (MonoClass* klass)
 			continue;
 		}
 
+		size = mono_marshal_type_size (field->type, info->fields [j].mspec,
+						&align, TRUE, m_class_is_unicode (klass));
+
+		// Keep in sync with class-init.c mono_class_layout_fields
+		if (m_class_is_inlinearray (klass)) {
+			// Limit the max size of array instance to 1MiB
+			const int struct_max_size = 1024 * 1024;
+			size *= m_class_inlinearray_value (klass);
+			g_assert ((size > 0) && (size <= struct_max_size));
+		}
+
 		switch (layout) {
 		case TYPE_ATTRIBUTE_AUTO_LAYOUT:
 		case TYPE_ATTRIBUTE_SEQUENTIAL_LAYOUT:
-			size = mono_marshal_type_size (field->type, info->fields [j].mspec,
-						       &align, TRUE, m_class_is_unicode (klass));
 			align = m_class_get_packing_size (klass) ? MIN (m_class_get_packing_size (klass), align): align;
 			min_align = MAX (align, min_align);
 			info->fields [j].offset = info->native_size;
@@ -5840,8 +5849,6 @@ mono_marshal_load_type_info (MonoClass* klass)
 			info->native_size = info->fields [j].offset + size;
 			break;
 		case TYPE_ATTRIBUTE_EXPLICIT_LAYOUT:
-			size = mono_marshal_type_size (field->type, info->fields [j].mspec,
-						       &align, TRUE, m_class_is_unicode (klass));
 			min_align = MAX (align, min_align);
 			info->fields [j].offset = m_field_get_offset (field) - MONO_ABI_SIZEOF (MonoObject);
 			info->native_size = MAX (info->native_size, info->fields [j].offset + size);

src/mono/mono/mini/aot-runtime-wasm.c

@@ -15,8 +15,12 @@
 #ifdef HOST_WASM
 
 static char
-type_to_c (MonoType *t)
+type_to_c (MonoType *t, gboolean *is_byref_return)
 {
+	g_assert (t);
+
+	if (is_byref_return)
+		*is_byref_return = 0;
 	if (m_type_is_byref (t))
 		return 'I';
 
@@ -48,7 +52,7 @@ type_to_c (MonoType *t)
 		return 'L';
 	case MONO_TYPE_VOID:
 		return 'V';
-	case MONO_TYPE_VALUETYPE:
+	case MONO_TYPE_VALUETYPE: {
 		if (m_class_is_enumtype (t->data.klass)) {
 			t = mono_class_enum_basetype_internal (t->data.klass);
 			goto handle_enum;
@@ -60,13 +64,27 @@ type_to_c (MonoType *t)
 		// FIXME: Handle the scenario where there are fields of struct types that contain no members
 		MonoType *scalar_vtype;
 		if (mini_wasm_is_scalar_vtype (t, &scalar_vtype))
-			return type_to_c (scalar_vtype);
+			return type_to_c (scalar_vtype, NULL);
+
+		if (is_byref_return)
+			*is_byref_return = 1;
 
 		return 'I';
-	case MONO_TYPE_GENERICINST:
-		if (m_class_is_valuetype (t->data.klass))
+	}
+	case MONO_TYPE_GENERICINST: {
+		if (m_class_is_valuetype (t->data.klass)) {
+			MonoType *scalar_vtype;
+			if (mini_wasm_is_scalar_vtype (t, &scalar_vtype))
+				return type_to_c (scalar_vtype, NULL);
+
+			if (is_byref_return)
+				*is_byref_return = 1;
+
 			return 'S';
+		}
+
 		return 'I';
+	}
 	default:
 		g_warning ("CANT TRANSLATE %s", mono_type_full_name (t));
 		return 'X';
@@ -140,18 +158,29 @@ gpointer
 mono_wasm_get_interp_to_native_trampoline (MonoMethodSignature *sig)
 {
 	char cookie [32];
-	int c_count;
+	int c_count, offset = 1;
+	gboolean is_byref_return = 0;
+
+	memset (cookie, 0, 32);
+	cookie [0] = type_to_c (sig->ret, &is_byref_return);
 
-	c_count = sig->param_count + sig->hasthis + 1;
+	c_count = sig->param_count + sig->hasthis + is_byref_return + 1;
 	g_assert (c_count < sizeof (cookie)); //ensure we don't overflow the local
 
-	cookie [0] = type_to_c (sig->ret);
-	if (sig->hasthis)
-		cookie [1] = 'I';
+	if (is_byref_return) {
+		cookie[0] = 'V';
+		// return value address goes in arg0
+		cookie[1] = 'I';
+		offset += 1;
+	}
+	if (sig->hasthis) {
+		// thisptr goes in arg0/arg1 depending on return type
+		cookie [offset] = 'I';
+		offset += 1;
+	}
 	for (int i = 0; i < sig->param_count; ++i) {
-		cookie [1 + sig->hasthis + i] = type_to_c (sig->params [i]);
+		cookie [offset + i] = type_to_c (sig->params [i], NULL);
 	}
-	cookie [c_count] = 0;
 
 	void *p = mono_wasm_interp_to_native_callback (cookie);
 	if (!p)

src/mono/mono/mini/interp/interp.c

@@ -1340,7 +1340,10 @@ typedef enum {
 	PINVOKE_ARG_R8 = 3,
 	PINVOKE_ARG_R4 = 4,
 	PINVOKE_ARG_VTYPE = 5,
-	PINVOKE_ARG_SCALAR_VTYPE = 6
+	PINVOKE_ARG_SCALAR_VTYPE = 6,
+	// This isn't ifdefed so it's easier to write code that handles it without sprinkling
+	//  800 ifdefs in this file
+	PINVOKE_ARG_WASM_VALUETYPE_RESULT = 7,
 } PInvokeArgType;
 
 typedef struct {
@@ -1436,6 +1439,7 @@ get_build_args_from_sig_info (MonoMemoryManager *mem_manager, MonoMethodSignatur
 			ilen++;
 			break;
 		case MONO_TYPE_GENERICINST: {
+			// FIXME: Should mini_wasm_is_scalar_vtype stuff go in here?
 			MonoClass *container_class = type->data.generic_class->container_class;
 			type = m_class_get_byval_arg (container_class);
 			goto retry;
@@ -1473,11 +1477,32 @@ get_build_args_from_sig_info (MonoMemoryManager *mem_manager, MonoMethodSignatur
 		case MONO_TYPE_CLASS:
 		case MONO_TYPE_OBJECT:
 		case MONO_TYPE_STRING:
+			info->ret_pinvoke_type = PINVOKE_ARG_INT;
+			break;
+#if SIZEOF_VOID_P == 8
+		case MONO_TYPE_I8:
+		case MONO_TYPE_U8:
+#endif
+			info->ret_pinvoke_type = PINVOKE_ARG_INT;
+			break;
+#if SIZEOF_VOID_P == 4
 		case MONO_TYPE_I8:
 		case MONO_TYPE_U8:
+			info->ret_pinvoke_type = PINVOKE_ARG_INT;
+			break;
+#endif
 		case MONO_TYPE_VALUETYPE:
 		case MONO_TYPE_GENERICINST:
 			info->ret_pinvoke_type = PINVOKE_ARG_INT;
+#ifdef HOST_WASM
+			// This ISSTRUCT check is important, because the type could be an enum
+			if (MONO_TYPE_ISSTRUCT (info->ret_mono_type)) {
+				// The return type was already filtered previously, so if we get here
+				//  we're returning a struct byref instead of as a scalar
+				info->ret_pinvoke_type = PINVOKE_ARG_WASM_VALUETYPE_RESULT;
+				info->ilen++;
+			}
+#endif
 			break;
 		case MONO_TYPE_R4:
 		case MONO_TYPE_R8:
@@ -1503,6 +1528,15 @@ build_args_from_sig (InterpMethodArguments *margs, MonoMethodSignature *sig, Bui
 	margs->ilen = info->ilen;
 	margs->flen = info->flen;
 
+	size_t int_i = 0;
+	size_t int_f = 0;
+
+	if (info->ret_pinvoke_type == PINVOKE_ARG_WASM_VALUETYPE_RESULT) {
+		// Allocate an empty arg0 for the address of the return value
+		// info->ilen was already increased earlier
+		int_i++;
+	}
+
 	if (margs->ilen > 0) {
 		if (margs->ilen <= 8)
 			margs->iargs = margs->iargs_buf;
@@ -1517,9 +1551,6 @@ build_args_from_sig (InterpMethodArguments *margs, MonoMethodSignature *sig, Bui
 			margs->fargs = g_malloc0 (sizeof (double) * margs->flen);
 	}
 
-	size_t int_i = 0;
-	size_t int_f = 0;
-
 	for (int i = 0; i < sig->param_count; i++) {
 		guint32 offset = get_arg_offset (frame->imethod, sig, i);
 		stackval *sp_arg = STACK_ADD_BYTES (frame->stack, offset);
@@ -1578,6 +1609,15 @@ build_args_from_sig (InterpMethodArguments *margs, MonoMethodSignature *sig, Bui
 	}
 
 	switch (info->ret_pinvoke_type) {
+	case PINVOKE_ARG_WASM_VALUETYPE_RESULT:
+		// We pass the return value address in arg0 so fill it in, we already
+		//  reserved space for it earlier.
+		g_assert (frame->retval);
+		margs->iargs[0] = (gpointer*)frame->retval;
+		// The return type is void so retval should be NULL
+		margs->retval = NULL;
+		margs->is_float_ret = 0;
+		break;
 	case PINVOKE_ARG_INT:
 		margs->retval = (gpointer*)frame->retval;
 		margs->is_float_ret = 0;
@@ -1795,8 +1835,10 @@ ves_pinvoke_method (
 	g_free (ccontext.stack);
 #else
 	// Only the vt address has been returned, we need to copy the entire content on interp stack
-	if (!context->has_resume_state && MONO_TYPE_ISSTRUCT (call_info->ret_mono_type))
-		stackval_from_data (call_info->ret_mono_type, frame.retval, (char*)frame.retval->data.p, sig->pinvoke && !sig->marshalling_disabled);
+	if (!context->has_resume_state && MONO_TYPE_ISSTRUCT (call_info->ret_mono_type)) {
+		if (call_info->ret_pinvoke_type != PINVOKE_ARG_WASM_VALUETYPE_RESULT)
+			stackval_from_data (call_info->ret_mono_type, frame.retval, (char*)frame.retval->data.p, sig->pinvoke && !sig->marshalling_disabled);
+	}
 
 	if (margs.iargs != margs.iargs_buf)
 		g_free (margs.iargs);

src/mono/mono/mini/mini-wasm.c

@@ -75,17 +75,23 @@ get_storage (MonoType *type, MonoType **etype, gboolean is_return)
 	case MONO_TYPE_R8:
 		return ArgOnStack;
 
-	case MONO_TYPE_GENERICINST:
+	case MONO_TYPE_GENERICINST: {
 		if (!mono_type_generic_inst_is_valuetype (type))
 			return ArgOnStack;
 
 		if (mini_is_gsharedvt_variable_type (type))
 			return ArgGsharedVTOnStack;
-		/* fall through */
+
+		if (mini_wasm_is_scalar_vtype (type, etype))
+			return ArgVtypeAsScalar;
+
+		return is_return ? ArgValuetypeAddrInIReg : ArgValuetypeAddrOnStack;
+	}
 	case MONO_TYPE_VALUETYPE:
 	case MONO_TYPE_TYPEDBYREF: {
 		if (mini_wasm_is_scalar_vtype (type, etype))
 			return ArgVtypeAsScalar;
+
 		return is_return ? ArgValuetypeAddrInIReg : ArgValuetypeAddrOnStack;
 	}
 	case MONO_TYPE_VAR:
@@ -771,7 +777,12 @@ mini_wasm_is_scalar_vtype (MonoType *type, MonoType **etype)
 		if (nfields > 1)
 			return FALSE;
 		MonoType *t = mini_get_underlying_type (field->type);
-		if (MONO_TYPE_ISSTRUCT (t)) {
+		int align, field_size = mono_type_size (t, &align);
+		// inlinearray and fixed both work by having a single field that is bigger than its element type.
+		// we also don't want to scalarize a struct that has padding in its metadata, even if it would fit.
+		if (field_size != size) {
+			return FALSE;
+		} else if (MONO_TYPE_ISSTRUCT (t)) {
 			if (!mini_wasm_is_scalar_vtype (t, etype))
 				return FALSE;
 		} else if (!((MONO_TYPE_IS_PRIMITIVE (t) || MONO_TYPE_IS_REFERENCE (t) || MONO_TYPE_IS_POINTER (t)))) {