From 3efef3d2e1a1df80cc82ba92ead292a17d9d16f5 Mon Sep 17 00:00:00 2001 From: Vlad Brezae Date: Fri, 1 Apr 2022 22:27:49 +0300 Subject: [PATCH] [mono] Fix static virtual calls to generic methods from gshared code (#66739) * [mono] Fix static virtual calls to generic methods from gshared code When the resolved method from the vtable is generic, we need to inflate it. For example, in the gshared method we are calling on `Interface.InterfaceMethod ()` (the type argument can either be fixed or determined based on the gshared method's context), and this method is resolved to generic method `Class.InterfaceMethod ()`. We then need to inflate the method and resolve `T` to `int`. * Enable tests * [mono][interp] Fix calls to static virtual methods via delegates * [mono][aot] Fix constrained + ldftn * [mono][gsharing] Fix constrained + ldftn from gshared method We can'd determine at compile time the static method if the constrained_class is a generic parameteri. We defer the computation of the method at runtime, in a similar fashion with calls. * Disable test suite on android It hits an issue in the android test runner --- src/mono/mono/mini/interp/interp.c | 2 +- src/mono/mono/mini/method-to-ir.c | 29 +++++++++++++++-------- src/mono/mono/mini/mini-generic-sharing.c | 25 ++++++++++++++++--- src/mono/mono/mini/mini.h | 4 +++- src/tests/issues.targets | 6 ++--- 5 files changed, 48 insertions(+), 18 deletions(-) diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c index ad6e3e7652d..dcaa0f13727 100644 --- a/src/mono/mono/mini/interp/interp.c +++ b/src/mono/mono/mini/interp/interp.c @@ -3778,7 +3778,7 @@ main_loop: del_imethod = mono_interp_get_imethod (mono_marshal_get_native_wrapper (del_imethod->method, FALSE, FALSE), error); mono_error_assert_ok (error); del->interp_invoke_impl = del_imethod; - } else if (del_imethod->method->flags & METHOD_ATTRIBUTE_VIRTUAL && !del->target && !m_class_is_valuetype (del_imethod->method->klass)) { + } else if ((m_method_is_virtual (del_imethod->method) && !m_method_is_static (del_imethod->method)) && !del->target && !m_class_is_valuetype (del_imethod->method->klass)) { // 'this' is passed dynamically, we need to recompute the target method // with each call del_imethod = get_virtual_method (del_imethod, LOCAL_VAR (call_args_offset + MINT_STACK_SLOT_SIZE, MonoObject*)->vtable); diff --git a/src/mono/mono/mini/method-to-ir.c b/src/mono/mono/mini/method-to-ir.c index 717e4eb54fd..ea951a7565b 100644 --- a/src/mono/mono/mini/method-to-ir.c +++ b/src/mono/mono/mini/method-to-ir.c @@ -11326,23 +11326,27 @@ mono_ldptr: case MONO_CEE_LDFTN: { MonoInst *argconst; MonoMethod *cil_method; + gboolean gshared_static_virtual = FALSE; cmethod = mini_get_method (cfg, method, n, NULL, generic_context); CHECK_CFG_ERROR; if (constrained_class) { - if (m_method_is_static (cmethod) && mini_class_check_context_used (cfg, constrained_class)) - // FIXME: - GENERIC_SHARING_FAILURE (CEE_LDFTN); - cmethod = get_constrained_method (cfg, image, n, cmethod, constrained_class, generic_context); - constrained_class = NULL; - CHECK_CFG_ERROR; + if (m_method_is_static (cmethod) && mini_class_check_context_used (cfg, constrained_class)) { + gshared_static_virtual = TRUE; + } else { + cmethod = get_constrained_method (cfg, image, n, cmethod, constrained_class, generic_context); + constrained_class = NULL; + CHECK_CFG_ERROR; + } + } else { + // we can't save token info if we have a constrained_class since + // n no longer represents the token for cmethod + mono_save_token_info (cfg, image, n, cmethod); } mono_class_init_internal (cmethod->klass); - mono_save_token_info (cfg, image, n, cmethod); - context_used = mini_method_check_context_used (cfg, cmethod); cil_method = cmethod; @@ -11356,7 +11360,7 @@ mono_ldptr: /* * Optimize the common case of ldftn+delegate creation */ - if ((sp > stack_start) && (next_ip + 4 < end) && ip_in_bb (cfg, cfg->cbb, next_ip) && (next_ip [0] == CEE_NEWOBJ)) { + if (!gshared_static_virtual && (sp > stack_start) && (next_ip + 4 < end) && ip_in_bb (cfg, cfg->cbb, next_ip) && (next_ip [0] == CEE_NEWOBJ)) { MonoMethod *ctor_method = mini_get_method (cfg, method, read32 (next_ip + 1), NULL, generic_context); if (ctor_method && (m_class_get_parent (ctor_method->klass) == mono_defaults.multicastdelegate_class)) { MonoInst *target_ins, *handle_ins; @@ -11434,7 +11438,12 @@ mono_ldptr: } } - argconst = emit_get_rgctx_method (cfg, context_used, cmethod, MONO_RGCTX_INFO_METHOD); + if (gshared_static_virtual) { + argconst = emit_get_rgctx_virt_method (cfg, -1, constrained_class, cmethod, MONO_RGCTX_INFO_VIRT_METHOD); + constrained_class = NULL; + } else { + argconst = emit_get_rgctx_method (cfg, context_used, cmethod, MONO_RGCTX_INFO_METHOD); + } ins = mono_emit_jit_icall (cfg, mono_ldftn, &argconst); *sp++ = ins; diff --git a/src/mono/mono/mini/mini-generic-sharing.c b/src/mono/mono/mini/mini-generic-sharing.c index 754e9bf8a4d..b009270d86f 100644 --- a/src/mono/mono/mini/mini-generic-sharing.c +++ b/src/mono/mono/mini/mini-generic-sharing.c @@ -702,6 +702,7 @@ inflate_info (MonoMemoryManager *mem_manager, MonoRuntimeGenericContextInfoTempl g_assert (is_ok (error)); return isig; } + case MONO_RGCTX_INFO_VIRT_METHOD: case MONO_RGCTX_INFO_VIRT_METHOD_CODE: case MONO_RGCTX_INFO_VIRT_METHOD_BOX_TYPE: { MonoJumpInfoVirtMethod *info = (MonoJumpInfoVirtMethod *)data; @@ -2250,6 +2251,7 @@ instantiate_info (MonoMemoryManager *mem_manager, MonoRuntimeGenericContextInfoT return mini_get_interp_callbacks ()->create_method_pointer_llvmonly (m, FALSE, error); } + case MONO_RGCTX_INFO_VIRT_METHOD: case MONO_RGCTX_INFO_VIRT_METHOD_CODE: { MonoJumpInfoVirtMethod *info = (MonoJumpInfoVirtMethod *)data; MonoClass *iface_class = info->method->klass; @@ -2270,10 +2272,24 @@ instantiate_info (MonoMemoryManager *mem_manager, MonoRuntimeGenericContextInfoT g_assert (m_class_get_vtable (info->klass)); method = m_class_get_vtable (info->klass) [ioffset + slot]; - method = mono_class_inflate_generic_method_checked (method, context, error); - return_val_if_nok (error, NULL); - if (mono_llvm_only) { + if (info->method->is_inflated) { + MonoGenericContext *method_ctx = mono_method_get_context (info->method); + if (method_ctx->method_inst != NULL) { + MonoGenericContext tmp_context; + tmp_context.class_inst = NULL; + tmp_context.method_inst = method_ctx->method_inst; + + // The resolved virtual method can be generic, in which case we need to inflate + // it with the type parameters that we are calling with. + method = mono_class_inflate_generic_method_checked (method, &tmp_context, error); + return_val_if_nok (error, NULL); + } + } + + if (oti->info_type == MONO_RGCTX_INFO_VIRT_METHOD) { + return method; + } else if (mono_llvm_only) { gpointer arg = NULL; addr = mini_llvmonly_load_method (method, FALSE, FALSE, &arg, error); @@ -2668,6 +2684,7 @@ mono_rgctx_info_type_to_str (MonoRgctxInfoType type) case MONO_RGCTX_INFO_BZERO: return "BZERO"; case MONO_RGCTX_INFO_NULLABLE_CLASS_BOX: return "NULLABLE_CLASS_BOX"; case MONO_RGCTX_INFO_NULLABLE_CLASS_UNBOX: return "NULLABLE_CLASS_UNBOX"; + case MONO_RGCTX_INFO_VIRT_METHOD: return "VIRT_METHOD"; case MONO_RGCTX_INFO_VIRT_METHOD_CODE: return "VIRT_METHOD_CODE"; case MONO_RGCTX_INFO_VIRT_METHOD_BOX_TYPE: return "VIRT_METHOD_BOX_TYPE"; case MONO_RGCTX_INFO_DELEGATE_TRAMP_INFO: return "DELEGATE_TRAMP_INFO"; @@ -2773,6 +2790,7 @@ info_equal (gpointer data1, gpointer data2, MonoRgctxInfoType info_type) case MONO_RGCTX_INFO_SIG_GSHAREDVT_IN_TRAMPOLINE_CALLI: case MONO_RGCTX_INFO_SIG_GSHAREDVT_OUT_TRAMPOLINE_CALLI: return data1 == data2; + case MONO_RGCTX_INFO_VIRT_METHOD: case MONO_RGCTX_INFO_VIRT_METHOD_CODE: case MONO_RGCTX_INFO_VIRT_METHOD_BOX_TYPE: { MonoJumpInfoVirtMethod *info1 = (MonoJumpInfoVirtMethod *)data1; @@ -2835,6 +2853,7 @@ mini_rgctx_info_type_to_patch_info_type (MonoRgctxInfoType info_type) return MONO_PATCH_INFO_METHOD; case MONO_RGCTX_INFO_DELEGATE_TRAMP_INFO: return MONO_PATCH_INFO_DELEGATE_TRAMPOLINE; + case MONO_RGCTX_INFO_VIRT_METHOD: case MONO_RGCTX_INFO_VIRT_METHOD_CODE: case MONO_RGCTX_INFO_VIRT_METHOD_BOX_TYPE: return MONO_PATCH_INFO_VIRT_METHOD; diff --git a/src/mono/mono/mini/mini.h b/src/mono/mono/mini/mini.h index b759b4222d1..0eaa4f9367e 100644 --- a/src/mono/mono/mini/mini.h +++ b/src/mono/mono/mini/mini.h @@ -1076,7 +1076,9 @@ typedef enum { /* The InterpMethod for a method */ MONO_RGCTX_INFO_INTERP_METHOD = 35, /* The llvmonly interp entry for a method */ - MONO_RGCTX_INFO_LLVMONLY_INTERP_ENTRY = 36 + MONO_RGCTX_INFO_LLVMONLY_INTERP_ENTRY = 36, + /* Same as VIRT_METHOD_CODE, but resolve MonoMethod* instead of code */ + MONO_RGCTX_INFO_VIRT_METHOD = 37 } MonoRgctxInfoType; /* How an rgctx is passed to a method */ diff --git a/src/tests/issues.targets b/src/tests/issues.targets index e6ca1d7c0f1..89a63c060e0 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -1526,9 +1526,6 @@ Doesn't pass after LLVM AOT compilation. - - Static virtual methods are not yet implemented in the Mono runtime. - Static virtual methods are not yet implemented in the Mono runtime. @@ -3783,6 +3780,9 @@ Cannot run multiple apps on Android for subprocesses + + https://github.com/dotnet/runtime/issues/67359 + -- GitLab