Ignore modopts/modreqs for UnsafeAccessor field targets (dotnet#109694

)

* Ignore modopts/modreqs for `UnsafeAccessor` field targets

The generated IL remains correct and doesn't require a
.volatile prefix for the accessor. This is based off of what Roslyn
currently emits if an accessor was manually written.

src/coreclr/vm/prestub.cpp

@@ -1498,7 +1498,7 @@ namespace
 
             TokenPairList list { nullptr };
             MetaSig::CompareState state{ &list };
-            state.IgnoreCustomModifiers = false;
+            state.IgnoreCustomModifiers = true;
             if (!DoesFieldMatchUnsafeAccessorDeclaration(cxt, pField, state))
                 continue;
 

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;

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

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);

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 {

src/tests/baseservices/compilerservices/UnsafeAccessors/UnsafeAccessorsTests.cs

@@ -328,6 +328,30 @@ public static void Verify_AccessAllFields_CorElementType()
         extern static ref delegate*<void> 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()
     {