From 8168b80a88cc157eacbbec34523c8e13adc3e816 Mon Sep 17 00:00:00 2001 From: fanyang-mono Date: Tue, 23 Apr 2024 14:17:45 -0400 Subject: [PATCH 1/4] Support non generic instance --- src/mono/mono/metadata/class.c | 2 +- src/mono/mono/metadata/marshal-lightweight.c | 65 +++++++++++++++++-- src/mono/mono/metadata/unsafe-accessor.c | 22 ++----- src/mono/mono/mini/method-to-ir.c | 2 +- .../UnsafeAccessorsTests.Generics.cs | 4 -- 5 files changed, 66 insertions(+), 29 deletions(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 6045628bf49e7..01888dfbabd57 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -5264,7 +5264,7 @@ mono_class_get_fields_internal (MonoClass *klass, gpointer *iter) * mono_class_get_methods: * \param klass the \c MonoClass to act on * - * This routine is an iterator routine for retrieving the fields in a class. + * This routine is an iterator routine for retrieving the methods in a class. * * You must pass a \c gpointer that points to zero and is treated as an opaque handle to * iterate over all of the elements. When no more values are diff --git a/src/mono/mono/metadata/marshal-lightweight.c b/src/mono/mono/metadata/marshal-lightweight.c index 6399b315cfd20..accf93f9a81ba 100644 --- a/src/mono/mono/metadata/marshal-lightweight.c +++ b/src/mono/mono/metadata/marshal-lightweight.c @@ -2371,6 +2371,28 @@ emit_missing_method_error (MonoMethodBuilder *mb, MonoError *failure, const char } } +static MonoMethodSignature * +update_signature (MonoMethod *accessor_method) +{ + MonoClass *accessor_method_class_instance = accessor_method->klass; + MonoClass *accessor_method_class = mono_class_is_ginst (accessor_method_class_instance) ? mono_class_get_generic_class (accessor_method_class_instance)->container_class : accessor_method_class_instance; + + const char *accessor_method_name = accessor_method->name; + + gpointer iter = NULL; + MonoMethod *m = NULL; + while ((m = mono_class_get_methods (accessor_method_class, &iter))) { + if (!m) + continue; + + if (strcmp (m->name, accessor_method_name)) + continue; + + return mono_metadata_signature_dup_full (get_method_image (m), mono_method_signature_internal (m)); + } + g_assert_not_reached (); +} + static void emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoGenericContext *ctx, MonoUnsafeAccessorKind kind, const char *member_name) { @@ -2389,12 +2411,17 @@ emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_m return; } - MonoMethodSignature *member_sig = ctor_sig_from_accessor_sig (mb, sig, ctx); - MonoClass *target_class = mono_class_from_mono_type_internal (target_type); ERROR_DECL(find_method_error); + if (accessor_method->is_inflated) { + sig = update_signature(accessor_method); + } + + MonoMethodSignature *member_sig = ctor_sig_from_accessor_sig (mb, sig, ctx); + MonoClass *in_class = mono_class_is_ginst (target_class) ? mono_class_get_generic_class (target_class)->container_class : target_class; + MonoMethod *target_method = mono_unsafe_accessor_find_ctor (in_class, member_sig, target_class, find_method_error); if (!is_ok (find_method_error) || target_method == NULL) { if (mono_error_get_error_code (find_method_error) == MONO_ERROR_GENERIC) @@ -2404,6 +2431,15 @@ emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_m mono_error_cleanup (find_method_error); return; } + + MonoGenericContext context = { NULL, NULL }; + if (mono_class_is_ginst (target_class)) + context.class_inst = mono_class_get_generic_class (target_class)->context.class_inst; + if (accessor_method->is_inflated) + context.method_inst = mono_method_get_context (accessor_method)->method_inst; + if ((context.class_inst != NULL) || (context.method_inst != NULL)) + target_method = mono_class_inflate_generic_method_checked (target_method, &context, find_method_error); + g_assert (target_method->klass == target_class); emit_unsafe_accessor_ldargs (mb, sig, 0); @@ -2425,11 +2461,9 @@ emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "Invalid usage of UnsafeAccessorAttribute."); return; } - gboolean hasthis = kind == MONO_UNSAFE_ACCESSOR_METHOD; - MonoType *target_type = sig->params[0]; - - MonoMethodSignature *member_sig = method_sig_from_accessor_sig (mb, hasthis, sig, ctx); + MonoType *target_type = sig->params[0]; + gboolean hasthis = kind == MONO_UNSAFE_ACCESSOR_METHOD; MonoClass *target_class = mono_class_from_mono_type_internal (target_type); if (hasthis && m_class_is_valuetype (target_class) && !m_type_is_byref (target_type)) { @@ -2438,7 +2472,14 @@ emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor } ERROR_DECL(find_method_error); + if (accessor_method->is_inflated) { + sig = update_signature(accessor_method); + } + + MonoMethodSignature *member_sig = method_sig_from_accessor_sig (mb, hasthis, sig, ctx); + MonoClass *in_class = mono_class_is_ginst (target_class) ? mono_class_get_generic_class (target_class)->container_class : target_class; + MonoMethod *target_method = NULL; if (!ctor_as_method) target_method = mono_unsafe_accessor_find_method (in_class, member_name, member_sig, target_class, find_method_error); @@ -2452,11 +2493,21 @@ emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor mono_error_cleanup (find_method_error); return; } + + MonoGenericContext context = { NULL, NULL }; + if (mono_class_is_ginst (target_class)) + context.class_inst = mono_class_get_generic_class (target_class)->context.class_inst; + if (accessor_method->is_inflated) + context.method_inst = mono_method_get_context (accessor_method)->method_inst; + if ((context.class_inst != NULL) || (context.method_inst != NULL)) + target_method = mono_class_inflate_generic_method_checked (target_method, &context, find_method_error); + if (!hasthis && target_method->klass != target_class) { emit_missing_method_error (mb, find_method_error, member_name); return; } - g_assert (target_method->klass == target_class); // are instance methods allowed to be looked up using inheritance? + + g_assert (target_method->klass == target_class); emit_unsafe_accessor_ldargs (mb, sig, !hasthis ? 1 : 0); diff --git a/src/mono/mono/metadata/unsafe-accessor.c b/src/mono/mono/metadata/unsafe-accessor.c index e7287a00c4617..584b291465869 100644 --- a/src/mono/mono/metadata/unsafe-accessor.c +++ b/src/mono/mono/metadata/unsafe-accessor.c @@ -14,7 +14,7 @@ #include "mono/metadata/class-internals.h" #include "mono/utils/mono-error-internals.h" #include "mono/metadata/unsafe-accessor.h" - +#include static MonoMethod * @@ -134,7 +134,10 @@ find_method_slow (MonoClass *klass, const char *name, const char *qname, const c return precise_match; } mono_error_set_generic_error (error, "System.Reflection", "AmbiguousMatchException", "Ambiguity in binding of UnsafeAccessorAttribute."); - return NULL; + result->i = -1; + result->m = NULL; + result->matched = FALSE; + return result; } matched = TRUE; result->i = i; @@ -176,23 +179,10 @@ find_method_in_class_unsafe_accessor (MonoClass *klass, const char *name, const if (!is_ok(error) && mono_error_get_error_code (error) == MONO_ERROR_GENERIC) return NULL; - int mcount = mono_class_get_method_count (klass); g_assert (result != NULL); if (result->matched) { - if (result->i < mcount) - return mono_class_get_method_by_index (from_class, result->i); - else if (result->m != NULL) { - // FIXME: metadata-update: hack - // it's from a metadata-update, probably - MonoMethod * m = mono_class_inflate_generic_method_full_checked ( - result->m, from_class, mono_class_get_context (from_class), error); - mono_error_assert_ok (error); - g_assert (m != NULL); - g_assert (m->klass == from_class); - g_assert (m->is_inflated); - return m; - } + return result->m; } g_free (result); diff --git a/src/mono/mono/mini/method-to-ir.c b/src/mono/mono/mini/method-to-ir.c index 8d569eae92a35..0212ba0d25f3c 100644 --- a/src/mono/mono/mini/method-to-ir.c +++ b/src/mono/mono/mini/method-to-ir.c @@ -7864,7 +7864,7 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b UNVERIFIED; if (!cfg->gshared) - g_assert (!mono_method_check_context_used (cmethod)); + g_assertf (!mono_method_check_context_used (cmethod), "cmethod is %s", mono_method_get_full_name (cmethod)); CHECK_STACK (n); diff --git a/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.Generics.cs b/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.Generics.cs index e4fb9593dd3ed..fd867ef222b69 100644 --- a/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.Generics.cs +++ b/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.Generics.cs @@ -140,7 +140,6 @@ sealed class Derived2 : GenericBase } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/89439", TestRuntimes.Mono)] public static void Verify_Generic_InheritanceMethodResolution() { string expect = "abc"; @@ -230,7 +229,6 @@ sealed class Accessors } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/89439", TestRuntimes.Mono)] public static void Verify_Generic_CallCtor() { Console.WriteLine($"Running {nameof(Verify_Generic_CallCtor)}"); @@ -312,7 +310,6 @@ public static void Verify_Generic_GenericTypeNonGenericInstanceMethod() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/89439", TestRuntimes.Mono)] public static void Verify_Generic_GenericTypeGenericInstanceMethod() { Console.WriteLine($"Running {nameof(Verify_Generic_GenericTypeGenericInstanceMethod)}"); @@ -366,7 +363,6 @@ public static void Verify_Generic_GenericTypeNonGenericStaticMethod() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/89439", TestRuntimes.Mono)] public static void Verify_Generic_GenericTypeGenericStaticMethod() { Console.WriteLine($"Running {nameof(Verify_Generic_GenericTypeGenericStaticMethod)}"); From a46b3759b63923b1294c03607444a5308af8eb36 Mon Sep 17 00:00:00 2001 From: fanyang-mono Date: Fri, 26 Apr 2024 11:35:51 -0400 Subject: [PATCH 2/4] Create helper function for shared code --- src/mono/mono/metadata/marshal-lightweight.c | 41 ++++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/src/mono/mono/metadata/marshal-lightweight.c b/src/mono/mono/metadata/marshal-lightweight.c index accf93f9a81ba..fbf14407a9f61 100644 --- a/src/mono/mono/metadata/marshal-lightweight.c +++ b/src/mono/mono/metadata/marshal-lightweight.c @@ -2393,6 +2393,27 @@ update_signature (MonoMethod *accessor_method) g_assert_not_reached (); } +static MonoClass * +get_container_class (MonoClass *klass) +{ + return mono_class_is_ginst (klass) ? mono_class_get_generic_class (klass)->container_class : klass; +} + +static MonoMethod * +inflate_method (MonoClass *klass, MonoMethod *method, MonoMethod *accessor_method, MonoError *error) +{ + MonoMethod *result = method; + MonoGenericContext context = { NULL, NULL }; + if (mono_class_is_ginst (klass)) + context.class_inst = mono_class_get_generic_class (klass)->context.class_inst; + if (accessor_method->is_inflated) + context.method_inst = mono_method_get_context (accessor_method)->method_inst; + if ((context.class_inst != NULL) || (context.method_inst != NULL)) + result = mono_class_inflate_generic_method_checked (method, &context, error); + + return result; +} + static void emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_method, MonoMethodSignature *sig, MonoGenericContext *ctx, MonoUnsafeAccessorKind kind, const char *member_name) { @@ -2420,7 +2441,7 @@ emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_m MonoMethodSignature *member_sig = ctor_sig_from_accessor_sig (mb, sig, ctx); - MonoClass *in_class = mono_class_is_ginst (target_class) ? mono_class_get_generic_class (target_class)->container_class : target_class; + MonoClass *in_class = get_container_class (target_class); MonoMethod *target_method = mono_unsafe_accessor_find_ctor (in_class, member_sig, target_class, find_method_error); if (!is_ok (find_method_error) || target_method == NULL) { @@ -2432,13 +2453,7 @@ emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_m return; } - MonoGenericContext context = { NULL, NULL }; - if (mono_class_is_ginst (target_class)) - context.class_inst = mono_class_get_generic_class (target_class)->context.class_inst; - if (accessor_method->is_inflated) - context.method_inst = mono_method_get_context (accessor_method)->method_inst; - if ((context.class_inst != NULL) || (context.method_inst != NULL)) - target_method = mono_class_inflate_generic_method_checked (target_method, &context, find_method_error); + target_method = inflate_method (target_class, target_method, accessor_method, find_method_error); g_assert (target_method->klass == target_class); @@ -2478,7 +2493,7 @@ emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor MonoMethodSignature *member_sig = method_sig_from_accessor_sig (mb, hasthis, sig, ctx); - MonoClass *in_class = mono_class_is_ginst (target_class) ? mono_class_get_generic_class (target_class)->container_class : target_class; + MonoClass *in_class = get_container_class (target_class); MonoMethod *target_method = NULL; if (!ctor_as_method) @@ -2494,13 +2509,7 @@ emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor return; } - MonoGenericContext context = { NULL, NULL }; - if (mono_class_is_ginst (target_class)) - context.class_inst = mono_class_get_generic_class (target_class)->context.class_inst; - if (accessor_method->is_inflated) - context.method_inst = mono_method_get_context (accessor_method)->method_inst; - if ((context.class_inst != NULL) || (context.method_inst != NULL)) - target_method = mono_class_inflate_generic_method_checked (target_method, &context, find_method_error); + target_method = inflate_method (target_class, target_method, accessor_method, find_method_error); if (!hasthis && target_method->klass != target_class) { emit_missing_method_error (mb, find_method_error, member_name); From a380cdea76b5e55ab1717c4aeae91e33e2360440 Mon Sep 17 00:00:00 2001 From: fanyang-mono Date: Fri, 26 Apr 2024 12:57:39 -0400 Subject: [PATCH 3/4] Use an existing API --- src/mono/mono/metadata/marshal-lightweight.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/mono/mono/metadata/marshal-lightweight.c b/src/mono/mono/metadata/marshal-lightweight.c index fbf14407a9f61..95ccd524efde2 100644 --- a/src/mono/mono/metadata/marshal-lightweight.c +++ b/src/mono/mono/metadata/marshal-lightweight.c @@ -2375,7 +2375,7 @@ static MonoMethodSignature * update_signature (MonoMethod *accessor_method) { MonoClass *accessor_method_class_instance = accessor_method->klass; - MonoClass *accessor_method_class = mono_class_is_ginst (accessor_method_class_instance) ? mono_class_get_generic_class (accessor_method_class_instance)->container_class : accessor_method_class_instance; + MonoClass *accessor_method_class = mono_class_get_generic_type_definition (accessor_method_class_instance); const char *accessor_method_name = accessor_method->name; @@ -2393,12 +2393,6 @@ update_signature (MonoMethod *accessor_method) g_assert_not_reached (); } -static MonoClass * -get_container_class (MonoClass *klass) -{ - return mono_class_is_ginst (klass) ? mono_class_get_generic_class (klass)->container_class : klass; -} - static MonoMethod * inflate_method (MonoClass *klass, MonoMethod *method, MonoMethod *accessor_method, MonoError *error) { @@ -2441,7 +2435,7 @@ emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor_m MonoMethodSignature *member_sig = ctor_sig_from_accessor_sig (mb, sig, ctx); - MonoClass *in_class = get_container_class (target_class); + MonoClass *in_class = mono_class_get_generic_type_definition (target_class); MonoMethod *target_method = mono_unsafe_accessor_find_ctor (in_class, member_sig, target_class, find_method_error); if (!is_ok (find_method_error) || target_method == NULL) { @@ -2493,7 +2487,7 @@ emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, MonoMethod *accessor MonoMethodSignature *member_sig = method_sig_from_accessor_sig (mb, hasthis, sig, ctx); - MonoClass *in_class = get_container_class (target_class); + MonoClass *in_class = mono_class_get_generic_type_definition (target_class); MonoMethod *target_method = NULL; if (!ctor_as_method) From 468d481bbb76dafe847a57c3e2ee734337412240 Mon Sep 17 00:00:00 2001 From: fanyang-mono Date: Fri, 26 Apr 2024 15:50:06 -0400 Subject: [PATCH 4/4] Add an assert --- src/mono/mono/metadata/marshal-lightweight.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/mono/mono/metadata/marshal-lightweight.c b/src/mono/mono/metadata/marshal-lightweight.c index 95ccd524efde2..245b5dbe57250 100644 --- a/src/mono/mono/metadata/marshal-lightweight.c +++ b/src/mono/mono/metadata/marshal-lightweight.c @@ -2404,6 +2404,7 @@ inflate_method (MonoClass *klass, MonoMethod *method, MonoMethod *accessor_metho context.method_inst = mono_method_get_context (accessor_method)->method_inst; if ((context.class_inst != NULL) || (context.method_inst != NULL)) result = mono_class_inflate_generic_method_checked (method, &context, error); + mono_error_assert_ok (error); return result; }