From 3adc3c69603a2c3d27c762504d4523d1397960b9 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Mon, 8 Jul 2024 09:48:23 -0400 Subject: [PATCH 1/9] Call composite_type_to_reduced_element_type all the time --- src/mono/mono/metadata/class.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 043338c14899d9..b379f637999a7f 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -4284,13 +4284,9 @@ mono_class_is_assignable_from_general (MonoClass *klass, MonoClass *oklass, gboo MonoClass *eclass; MonoClass *eoclass; - if (signature_assignment) { - eclass = composite_type_to_reduced_element_type (klass); - eoclass = composite_type_to_reduced_element_type (oklass); - } else { - eclass = m_class_get_cast_class (klass); - eoclass = m_class_get_cast_class (oklass); - } + + eclass = composite_type_to_reduced_element_type (klass); + eoclass = composite_type_to_reduced_element_type (oklass); *result = (eclass == eoclass); return; From ccb82a15fb480968e6b95d1217e9acec63ced0e6 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Mon, 8 Jul 2024 15:00:40 -0400 Subject: [PATCH 2/9] Work path to set values --- .../tests/System.Runtime.Tests/System/ArrayTests.cs | 1 - src/mono/System.Private.CoreLib/src/System/Array.Mono.cs | 7 +------ src/mono/mono/metadata/icall.c | 5 ----- 3 files changed, 1 insertion(+), 12 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs index 998094377be4f4..ccaba707130386 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ArrayTests.cs @@ -1599,7 +1599,6 @@ public static void Copy_SourceAndDestinationNeverConvertible_ThrowsArrayTypeMism } [Fact] - [SkipOnMono("https://github.com/dotnet/runtime/issues/104197")] public static unsafe void Copy_CompatiblePointers() { // Can copy between compatible pointers diff --git a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs index cded34be47b1f8..ac1a31a9c94088 100644 --- a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs @@ -169,6 +169,7 @@ private static void CopySlow(Array sourceArray, int sourceIndex, Array destinati if (reliable) { if (!dst_type.Equals(src_type) && + !(dst_type.IsPointer && src_type.IsPointer) && !(dst_type.IsPrimitive && src_type.IsPrimitive && CanChangePrimitive(ObjectHandleOnStack.Create(ref dst_type), ObjectHandleOnStack.Create(ref src_type), true))) { @@ -329,9 +330,6 @@ private unsafe nint GetFlattenedIndex(ReadOnlySpan indices) internal object? InternalGetValue(nint index) { - if (GetType().GetElementType()!.IsPointer) - throw new NotSupportedException(SR.NotSupported_Type); - Array self = this; object? res = null; GetValueImpl(ObjectHandleOnStack.Create(ref self), ObjectHandleOnStack.Create(ref res), (int)index); @@ -340,9 +338,6 @@ private unsafe nint GetFlattenedIndex(ReadOnlySpan indices) internal void InternalSetValue(object? value, nint index) { - if (GetType().GetElementType()!.IsPointer) - throw new NotSupportedException(SR.NotSupported_Type); - Array self = this; SetValueImpl(ObjectHandleOnStack.Create(ref self), ObjectHandleOnStack.Create(ref value), (int)index); } diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index 67cd6470df19db..a30dda9fffb0c3 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -199,11 +199,6 @@ ves_icall_System_Array_GetValueImpl (MonoObjectHandleOnStack array_handle, MonoO MonoClass * const array_class = mono_object_class (array); MonoClass * const element_class = m_class_get_element_class (array_class); - if (m_class_is_native_pointer (element_class)) { - mono_error_set_not_supported (error, NULL); - return; - } - if (m_class_is_valuetype (element_class)) { gsize element_size = mono_array_element_size (array_class); gpointer element_address = mono_array_addr_with_size_fast (array, element_size, (gsize)pos); From 047d4ad7717b08b38dbf4babcd3e76ca30dff59a Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Tue, 9 Jul 2024 10:59:34 -0400 Subject: [PATCH 3/9] Revert slow path changes - make fast path work --- src/mono/System.Private.CoreLib/src/System/Array.Mono.cs | 1 - src/mono/mono/metadata/class.c | 4 ++++ src/mono/mono/metadata/icall.c | 5 +---- src/mono/mono/metadata/object.c | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs index ac1a31a9c94088..c85c6d4b89efc5 100644 --- a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs @@ -169,7 +169,6 @@ private static void CopySlow(Array sourceArray, int sourceIndex, Array destinati if (reliable) { if (!dst_type.Equals(src_type) && - !(dst_type.IsPointer && src_type.IsPointer) && !(dst_type.IsPrimitive && src_type.IsPrimitive && CanChangePrimitive(ObjectHandleOnStack.Create(ref dst_type), ObjectHandleOnStack.Create(ref src_type), true))) { diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index b379f637999a7f..5fab235e63173a 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -3582,6 +3582,10 @@ mono_class_is_subclass_of_internal (MonoClass *klass, MonoClass *klassc, gboolean check_interfaces) { MONO_REQ_GC_UNSAFE_MODE; + + if (mono_class_is_pointer(klass) && mono_class_is_pointer(klassc)) + return TRUE; + /* FIXME test for interfaces with variant generic arguments */ if (check_interfaces) { mono_class_init_internal (klass); diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index a30dda9fffb0c3..acef2351a8b44e 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -199,7 +199,7 @@ ves_icall_System_Array_GetValueImpl (MonoObjectHandleOnStack array_handle, MonoO MonoClass * const array_class = mono_object_class (array); MonoClass * const element_class = m_class_get_element_class (array_class); - if (m_class_is_valuetype (element_class)) { + if (m_class_is_valuetype (element_class) || mono_class_is_pointer (element_class)) { gsize element_size = mono_array_element_size (array_class); gpointer element_address = mono_array_addr_with_size_fast (array, element_size, (gsize)pos); MonoObject *res = mono_value_box_checked (element_class, element_address, error); @@ -871,9 +871,6 @@ ves_icall_System_Array_FastCopy (MonoObjectHandleOnStack source_handle, int sour /* It's only safe to copy between arrays if we can ensure the source will always have a subtype of the destination. We bail otherwise. */ if (!mono_class_is_subclass_of_internal (src_class, dest_class, FALSE)) return FALSE; - - if (m_class_is_native_pointer (src_class) || m_class_is_native_pointer (dest_class)) - return FALSE; } if (m_class_is_valuetype (dest_class)) { diff --git a/src/mono/mono/metadata/object.c b/src/mono/mono/metadata/object.c index 2a0e9e9834e094..1c1a632a4003e7 100644 --- a/src/mono/mono/metadata/object.c +++ b/src/mono/mono/metadata/object.c @@ -6529,7 +6529,7 @@ mono_value_box_handle (MonoClass *klass, gpointer value, MonoError *error) error_init (error); - g_assert (m_class_is_valuetype (klass)); + g_assert (m_class_is_valuetype (klass) || mono_class_is_pointer (klass)); g_assert (value != NULL); if (G_UNLIKELY (m_class_is_byreflike (klass))) { char *full_name = mono_type_get_full_name (klass); From 6494a32fb841434950f54bc430d2d9a0610d6057 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Tue, 9 Jul 2024 11:55:21 -0400 Subject: [PATCH 4/9] Removed wrong NSE checks --- src/mono/System.Private.CoreLib/src/System/Array.Mono.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs index c85c6d4b89efc5..cded34be47b1f8 100644 --- a/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs +++ b/src/mono/System.Private.CoreLib/src/System/Array.Mono.cs @@ -329,6 +329,9 @@ private unsafe nint GetFlattenedIndex(ReadOnlySpan indices) internal object? InternalGetValue(nint index) { + if (GetType().GetElementType()!.IsPointer) + throw new NotSupportedException(SR.NotSupported_Type); + Array self = this; object? res = null; GetValueImpl(ObjectHandleOnStack.Create(ref self), ObjectHandleOnStack.Create(ref res), (int)index); @@ -337,6 +340,9 @@ private unsafe nint GetFlattenedIndex(ReadOnlySpan indices) internal void InternalSetValue(object? value, nint index) { + if (GetType().GetElementType()!.IsPointer) + throw new NotSupportedException(SR.NotSupported_Type); + Array self = this; SetValueImpl(ObjectHandleOnStack.Create(ref self), ObjectHandleOnStack.Create(ref value), (int)index); } From 5ea2e7696d7fccab8e8573b16f7b2fed43ad9646 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Tue, 9 Jul 2024 14:14:53 -0400 Subject: [PATCH 5/9] Guard against incompatible types --- src/mono/mono/metadata/icall.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index acef2351a8b44e..187d57f885814e 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -867,6 +867,9 @@ ves_icall_System_Array_FastCopy (MonoObjectHandleOnStack source_handle, int sour if (m_class_is_valuetype (dest_class) || m_class_is_enumtype (dest_class) || m_class_is_valuetype (src_class) || m_class_is_valuetype (src_class)) return FALSE; + + if (!mono_class_is_assignable_from_internal(dest_class, src_class)) + return FALSE; /* It's only safe to copy between arrays if we can ensure the source will always have a subtype of the destination. We bail otherwise. */ if (!mono_class_is_subclass_of_internal (src_class, dest_class, FALSE)) From 5ff76e596b5b904c7bec16d788ec80aa33033d7d Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Tue, 9 Jul 2024 16:24:17 -0400 Subject: [PATCH 6/9] Feedback --- src/mono/mono/metadata/class.c | 3 --- src/mono/mono/metadata/icall.c | 15 +++++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/mono/mono/metadata/class.c b/src/mono/mono/metadata/class.c index 5fab235e63173a..2117c94c06a840 100644 --- a/src/mono/mono/metadata/class.c +++ b/src/mono/mono/metadata/class.c @@ -3583,9 +3583,6 @@ mono_class_is_subclass_of_internal (MonoClass *klass, MonoClass *klassc, { MONO_REQ_GC_UNSAFE_MODE; - if (mono_class_is_pointer(klass) && mono_class_is_pointer(klassc)) - return TRUE; - /* FIXME test for interfaces with variant generic arguments */ if (check_interfaces) { mono_class_init_internal (klass); diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index 187d57f885814e..c0d381ea32f6ec 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -868,12 +868,15 @@ ves_icall_System_Array_FastCopy (MonoObjectHandleOnStack source_handle, int sour m_class_is_valuetype (src_class) || m_class_is_valuetype (src_class)) return FALSE; - if (!mono_class_is_assignable_from_internal(dest_class, src_class)) - return FALSE; - - /* It's only safe to copy between arrays if we can ensure the source will always have a subtype of the destination. We bail otherwise. */ - if (!mono_class_is_subclass_of_internal (src_class, dest_class, FALSE)) - return FALSE; + if (mono_class_is_pointer (dest_class) && mono_class_is_pointer (src_class)) { + /* if we're copying between two arrays of pointers, only allow it if both dest_class is assignable from src_class (checked above, and src_class is assignable from dest_class). This should only be true if both src_class and dest_class have a common cast_class. (for example: int*[] and uint*[] are ok, but void*[] and int*[] are not)). */ + if (!mono_class_is_assignable_from_internal (src_class, dest_class, FALSE)) + return FALSE; + } else { + /* It's only safe to copy between arrays if we can ensure the source will always have a subtype of the destination. We bail otherwise. */ + if (!mono_class_is_subclass_of_internal (src_class, dest_class, FALSE)) + return FALSE; + } } if (m_class_is_valuetype (dest_class)) { From 461b0fd254f2242aa8b1f9bf3eafd5ac297d5ab8 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Tue, 9 Jul 2024 16:49:27 -0400 Subject: [PATCH 7/9] Mistaken param --- src/mono/mono/metadata/icall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index c0d381ea32f6ec..8069eb89e1378f 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -870,7 +870,7 @@ ves_icall_System_Array_FastCopy (MonoObjectHandleOnStack source_handle, int sour if (mono_class_is_pointer (dest_class) && mono_class_is_pointer (src_class)) { /* if we're copying between two arrays of pointers, only allow it if both dest_class is assignable from src_class (checked above, and src_class is assignable from dest_class). This should only be true if both src_class and dest_class have a common cast_class. (for example: int*[] and uint*[] are ok, but void*[] and int*[] are not)). */ - if (!mono_class_is_assignable_from_internal (src_class, dest_class, FALSE)) + if (!mono_class_is_assignable_from_internal (src_class, dest_class)) return FALSE; } else { /* It's only safe to copy between arrays if we can ensure the source will always have a subtype of the destination. We bail otherwise. */ From 1f08798d1357100c51670a45be6d5ba9a9b3fcda Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Tue, 9 Jul 2024 17:45:52 -0400 Subject: [PATCH 8/9] Flip params --- src/mono/mono/metadata/icall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index 8069eb89e1378f..b03d69e0ff949c 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -870,7 +870,7 @@ ves_icall_System_Array_FastCopy (MonoObjectHandleOnStack source_handle, int sour if (mono_class_is_pointer (dest_class) && mono_class_is_pointer (src_class)) { /* if we're copying between two arrays of pointers, only allow it if both dest_class is assignable from src_class (checked above, and src_class is assignable from dest_class). This should only be true if both src_class and dest_class have a common cast_class. (for example: int*[] and uint*[] are ok, but void*[] and int*[] are not)). */ - if (!mono_class_is_assignable_from_internal (src_class, dest_class)) + if (!mono_class_is_assignable_from_internal (dest_class, src_class)) return FALSE; } else { /* It's only safe to copy between arrays if we can ensure the source will always have a subtype of the destination. We bail otherwise. */ From e6690489a78207028bf46bd80ec76fa4146b9227 Mon Sep 17 00:00:00 2001 From: Steve Pfister Date: Tue, 9 Jul 2024 20:22:54 -0400 Subject: [PATCH 9/9] Do assignable check if src or dest is a pointer type --- src/mono/mono/metadata/icall.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index b03d69e0ff949c..43f3d32653f321 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -868,8 +868,8 @@ ves_icall_System_Array_FastCopy (MonoObjectHandleOnStack source_handle, int sour m_class_is_valuetype (src_class) || m_class_is_valuetype (src_class)) return FALSE; - if (mono_class_is_pointer (dest_class) && mono_class_is_pointer (src_class)) { - /* if we're copying between two arrays of pointers, only allow it if both dest_class is assignable from src_class (checked above, and src_class is assignable from dest_class). This should only be true if both src_class and dest_class have a common cast_class. (for example: int*[] and uint*[] are ok, but void*[] and int*[] are not)). */ + if (mono_class_is_pointer (dest_class) || mono_class_is_pointer (src_class)) { + /* if we're copying between at least one array of pointers, only allow it if both dest_class is assignable from src_class (checked above, and src_class is assignable from dest_class). This should only be true if both src_class and dest_class have a common cast_class. (for example: int*[] and uint*[] are ok, but void*[] and int*[] are not)). */ if (!mono_class_is_assignable_from_internal (dest_class, src_class)) return FALSE; } else {