From 6eca743859ab502cc6eae59e9a7617495dac051e Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Sun, 10 Nov 2024 18:36:21 -0800 Subject: [PATCH 1/2] Ignore modopts/modreqs for `UnsafeAccessor` field targets The generated IL remains correct and doesn't require a .volatile prefix for the generated accessor. This is based off of what Roslyn currently emits if a getter was available. --- src/coreclr/vm/prestub.cpp | 2 +- .../UnsafeAccessors/UnsafeAccessorsTests.cs | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/prestub.cpp b/src/coreclr/vm/prestub.cpp index 0ad45e1110ceb..e494c74667cba 100644 --- a/src/coreclr/vm/prestub.cpp +++ b/src/coreclr/vm/prestub.cpp @@ -1497,7 +1497,7 @@ namespace TokenPairList list { nullptr }; MetaSig::CompareState state{ &list }; - state.IgnoreCustomModifiers = false; + state.IgnoreCustomModifiers = true; if (!DoesFieldMatchUnsafeAccessorDeclaration(cxt, pField, state)) continue; diff --git a/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs b/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs index 72821f396b4f2..f4efeacf80fe9 100644 --- a/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs +++ b/src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs @@ -328,6 +328,30 @@ public static void Verify_AccessAllFields_CorElementType() extern static ref delegate* GetFPtr(ref AllFields f); } + // Contains fields that have modopts/modreqs + struct FieldsWithModifiers + { + private static volatile int s_vInt; + private volatile int _vInt; + } + + [Fact] + public static void Verify_AccessFieldsWithModifiers() + { + Console.WriteLine($"Running {nameof(Verify_AccessFieldsWithModifiers)}"); + + FieldsWithModifiers fieldsWithModifiers = default; + + GetStaticVolatileInt(ref fieldsWithModifiers) = default; + GetVolatileInt(ref fieldsWithModifiers) = default; + + [UnsafeAccessor(UnsafeAccessorKind.StaticField, Name="s_vInt")] + extern static ref int GetStaticVolatileInt(ref FieldsWithModifiers f); + + [UnsafeAccessor(UnsafeAccessorKind.Field, Name="_vInt")] + extern static ref int GetVolatileInt(ref FieldsWithModifiers f); + } + [Fact] public static void Verify_AccessStaticMethodClass() { From 8b02937ac58252d479da3bfb3804146e9019d323 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Mon, 11 Nov 2024 08:02:01 -0800 Subject: [PATCH 2/2] Update mono --- src/mono/mono/metadata/class.c | 2 +- src/mono/mono/metadata/marshal-lightweight.c | 14 ++++++------- src/mono/mono/metadata/metadata-internals.h | 8 ++++++- src/mono/mono/metadata/metadata.c | 22 +++++++------------- 4 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index abd0948e3a2f5..c74c5c12e00cf 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -2580,7 +2580,7 @@ mono_class_get_field_from_name_full (MonoClass *klass, const char *name, MonoTyp MonoClassField *gfield = mono_metadata_get_corresponding_field_from_generic_type_definition (field); g_assert (gfield != NULL); MonoType *field_type = gfield->type; - if (!mono_metadata_type_equal_full (type, field_type, TRUE)) + if (!mono_metadata_type_equal_full (type, field_type, MONO_TYPE_EQ_FLAGS_SIG_ONLY)) continue; } return field; diff --git a/src/mono/mono/metadata/marshal-lightweight.c b/src/mono/mono/metadata/marshal-lightweight.c index 9829c95232bae..d79919ec3623d 100644 --- a/src/mono/mono/metadata/marshal-lightweight.c +++ b/src/mono/mono/metadata/marshal-lightweight.c @@ -2305,8 +2305,8 @@ emit_unsafe_accessor_field_wrapper (MonoMethodBuilder *mb, gboolean inflate_gene } MonoClassField *target_field = mono_class_get_field_from_name_full (target_class, member_name, NULL); - if (target_field == NULL || !mono_metadata_type_equal_full (target_field->type, m_class_get_byval_arg (mono_class_from_mono_type_internal (ret_type)), TRUE)) { - mono_mb_emit_exception_full (mb, "System", "MissingFieldException", + if (target_field == NULL || !mono_metadata_type_equal_full (target_field->type, m_class_get_byval_arg (mono_class_from_mono_type_internal (ret_type)), MONO_TYPE_EQ_FLAGS_SIG_ONLY | MONO_TYPE_EQ_FLAG_IGNORE_CMODS)) { + mono_mb_emit_exception_full (mb, "System", "MissingFieldException", g_strdup_printf("No '%s' in '%s'. Or the type of '%s' doesn't match", member_name, m_class_get_name (target_class), member_name)); return; } @@ -2403,7 +2403,7 @@ inflate_method (MonoClass *klass, MonoMethod *method, MonoMethod *accessor_metho 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; } @@ -2425,13 +2425,13 @@ emit_unsafe_accessor_ctor_wrapper (MonoMethodBuilder *mb, gboolean inflate_gener mono_mb_emit_exception_full (mb, "System", "BadImageFormatException", "Invalid usage of UnsafeAccessorAttribute."); return; } - + MonoClass *target_class = mono_class_from_mono_type_internal (target_type); ERROR_DECL(find_method_error); MonoMethodSignature *member_sig = ctor_sig_from_accessor_sig (mb, sig); - + MonoClass *in_class = target_class; MonoMethod *target_method = mono_unsafe_accessor_find_ctor (in_class, member_sig, target_class, find_method_error); @@ -2506,7 +2506,7 @@ emit_unsafe_accessor_method_wrapper (MonoMethodBuilder *mb, gboolean inflate_gen emit_missing_method_error (mb, find_method_error, member_name); return; } - + g_assert (target_method->klass == target_class); emit_unsafe_accessor_ldargs (mb, sig, !hasthis ? 1 : 0); @@ -2733,7 +2733,7 @@ emit_swift_lowered_struct_load (MonoMethodBuilder *mb, MonoMethodSignature *csig } } -/* Swift struct lowering handling causes csig to have additional arguments. +/* Swift struct lowering handling causes csig to have additional arguments. * This function returns the index of the argument in the csig that corresponds to the argument in the original signature. */ static int diff --git a/src/mono/mono/metadata/metadata-internals.h b/src/mono/mono/metadata/metadata-internals.h index 54d85997d7af4..3f0345f881185 100644 --- a/src/mono/mono/metadata/metadata-internals.h +++ b/src/mono/mono/metadata/metadata-internals.h @@ -1021,8 +1021,14 @@ mono_type_stack_size_internal (MonoType *t, int *align, gboolean allow_open); MONO_API void mono_type_get_desc (GString *res, MonoType *type, mono_bool include_namespace); +enum { + MONO_TYPE_EQ_FLAGS_NONE = 0, + MONO_TYPE_EQ_FLAGS_SIG_ONLY = 1, + MONO_TYPE_EQ_FLAG_IGNORE_CMODS = 2, +}; + gboolean -mono_metadata_type_equal_full (MonoType *t1, MonoType *t2, gboolean signature_only); +mono_metadata_type_equal_full (MonoType *t1, MonoType *t2, int flags); MonoMarshalSpec * mono_metadata_parse_marshal_spec_full (MonoImage *image, MonoImage *parent_image, const char *ptr); diff --git a/src/mono/mono/metadata/metadata.c b/src/mono/mono/metadata/metadata.c index a9b291bcadc54..91da538ff585a 100644 --- a/src/mono/mono/metadata/metadata.c +++ b/src/mono/mono/metadata/metadata.c @@ -45,11 +45,6 @@ typedef struct { MonoGenericContext context; } MonoInflatedMethodSignature; -enum { - MONO_TYPE_EQ_FLAGS_SIG_ONLY = 1, - MONO_TYPE_EQ_FLAG_IGNORE_CMODS = 2, -}; - static gboolean do_mono_metadata_parse_type (MonoType *type, MonoImage *m, MonoGenericContainer *container, gboolean transient, const char *ptr, const char **rptr, MonoError *error); @@ -2936,7 +2931,7 @@ aggregate_modifiers_equal (gconstpointer ka, gconstpointer kb) for (int i = 0; i < amods1->count; ++i) { if (amods1->modifiers [i].required != amods2->modifiers [i].required) return FALSE; - if (!mono_metadata_type_equal_full (amods1->modifiers [i].type, amods2->modifiers [i].type, TRUE)) + if (!mono_metadata_type_equal_full (amods1->modifiers [i].type, amods2->modifiers [i].type, MONO_TYPE_EQ_FLAGS_SIG_ONLY)) return FALSE; } return TRUE; @@ -5936,24 +5931,23 @@ do_mono_metadata_type_equal (MonoType *t1, MonoType *t2, int equiv_flags) gboolean mono_metadata_type_equal (MonoType *t1, MonoType *t2) { - return do_mono_metadata_type_equal (t1, t2, 0); + return do_mono_metadata_type_equal (t1, t2, MONO_TYPE_EQ_FLAGS_NONE); } /** * mono_metadata_type_equal_full: * \param t1 a type * \param t2 another type - * \param signature_only if signature only comparison should be made + * \param flags flags used to modify comparison logic * - * Determine if \p t1 and \p t2 are signature compatible if \p signature_only is TRUE, otherwise - * behaves the same way as mono_metadata_type_equal. - * The function mono_metadata_type_equal(a, b) is just a shortcut for mono_metadata_type_equal_full(a, b, FALSE). - * \returns TRUE if \p t1 and \p t2 are equal taking \p signature_only into account. + * Determine if \p t1 and \p t2 are compatible based on the supplied flags. + * The function mono_metadata_type_equal(a, b) is just a shortcut for mono_metadata_type_equal_full(a, b, MONO_TYPE_EQ_FLAGS_NONE). + * \returns TRUE if \p t1 and \p t2 are equal. */ gboolean -mono_metadata_type_equal_full (MonoType *t1, MonoType *t2, gboolean signature_only) +mono_metadata_type_equal_full (MonoType *t1, MonoType *t2, int flags) { - return do_mono_metadata_type_equal (t1, t2, signature_only ? MONO_TYPE_EQ_FLAGS_SIG_ONLY : 0); + return do_mono_metadata_type_equal (t1, t2, flags); } enum {