From be18dfb2e39ee6bbd9c0988fbab4605a9b43ae26 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Mon, 17 Jun 2024 17:58:21 -0700 Subject: [PATCH 1/4] InlineArray's Equals and GetHashCode throw The default Equals() and GetHashCode() throw for types marked with InlineArrayAttribute. --- .../Runtime/CompilerHelpers/ThrowHelpers.cs | 5 ++++ .../ValueTypeGetFieldHelperMethodOverride.cs | 13 ++++++++++ src/coreclr/vm/comutilnative.cpp | 8 ++++-- .../src/Resources/Strings.resx | 3 +++ src/mono/mono/metadata/icall.c | 10 +++++++ .../InlineArray/InlineArrayValid.cs | 26 +++++++++++++++++++ 6 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs index 55adc4b896824..ae0b6c50b138d 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs @@ -132,5 +132,10 @@ public static void ThrowArgumentOutOfRangeException() { throw new ArgumentOutOfRangeException(); } + + public static void ThrowInvalidOperationInlineArrayEqualsGetHashCode() + { + throw new InvalidOperationException(SR.InvalidOperation_InlineArrayEqualsGetHashCode); + } } } diff --git a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs index a4c967fbf967a..37ead3c78ff8d 100644 --- a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs +++ b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs @@ -61,6 +61,19 @@ public override MethodIL EmitIL() ILEmitter emitter = new ILEmitter(); + if (_owningType is MetadataType mdType) + { + // Types marked as InlineArray aren't supported by + // the built-in Equals() or GetHashCode(). + if (mdType.IsInlineArray) + { + var stream = emitter.NewCodeStream(); + MethodDesc invalidOp = Context.GetHelperEntryPoint("ThrowHelpers", "ThrowInvalidOperationInlineArrayEqualsGetHashCode"); + stream.EmitCallThrowHelper(emitter, invalidOp); + return emitter.Link(this); + } + } + TypeDesc methodTableType = Context.SystemModule.GetKnownType("Internal.Runtime", "MethodTable"); MethodDesc methodTableOfMethod = methodTableType.GetKnownMethod("Of", null); diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 1b768de4f92f5..0eab25e446bca 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -1624,7 +1624,8 @@ BOOL CanCompareBitsOrUseFastGetHashCode(MethodTable* mt) } if (mt->ContainsPointers() - || mt->IsNotTightlyPacked()) + || mt->IsNotTightlyPacked() + || mt->GetClass()->IsInlineArray()) { mt->SetHasCheckedCanCompareBitsOrUseFastGetHashCode(); return FALSE; @@ -1677,7 +1678,7 @@ BOOL CanCompareBitsOrUseFastGetHashCode(MethodTable* mt) return canCompareBitsOrUseFastGetHashCode; } -extern "C" BOOL QCALLTYPE MethodTable_CanCompareBitsOrUseFastGetHashCode(MethodTable * mt) +extern "C" BOOL QCALLTYPE MethodTable_CanCompareBitsOrUseFastGetHashCode(MethodTable* mt) { QCALL_CONTRACT; @@ -1685,6 +1686,9 @@ extern "C" BOOL QCALLTYPE MethodTable_CanCompareBitsOrUseFastGetHashCode(MethodT BEGIN_QCALL; + if (mt->GetClass()->IsInlineArray()) + COMPlusThrow(kInvalidOperationException, W("InvalidOperation_InlineArrayEqualsGetHashCode")); + ret = CanCompareBitsOrUseFastGetHashCode(mt); END_QCALL; diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index 79653d2eb426a..270bdca69d511 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -2641,6 +2641,9 @@ Failed to compare two elements in the array. + + Calling built-in Equals() or GetHashCode() on type marked as InlineArray is invalid. + Type definition of the method is complete. diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index be76808491569..459891b5a3cb1 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -1252,6 +1252,11 @@ ves_icall_System_ValueType_InternalGetHashCode (MonoObjectHandle this_obj, MonoA klass = mono_handle_class (this_obj); + if (m_class_is_inlinearray (klass)) { + mono_error_set_invalid_operation (error, "Calling built-in GetHashCode() on type marked as InlineArray is invalid."); + return FALSE; + } + if (mono_class_num_fields (klass) == 0) return result; @@ -1327,6 +1332,11 @@ ves_icall_System_ValueType_Equals (MonoObjectHandle this_obj, MonoObjectHandle t klass = mono_handle_class (this_obj); + if (m_class_is_inlinearray (klass)) { + mono_error_set_invalid_operation (error, "Calling built-in Equals() on type marked as InlineArray is invalid."); + return FALSE; + } + if (m_class_is_enumtype (klass) && mono_class_enum_basetype_internal (klass) && mono_class_enum_basetype_internal (klass)->type == MONO_TYPE_I4) return *(gint32*)mono_handle_get_data_unsafe (this_obj) == *(gint32*)mono_handle_get_data_unsafe (that); diff --git a/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs b/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs index 885d2a08d8c91..f64718b3c43fe 100644 --- a/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs +++ b/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs @@ -393,4 +393,30 @@ public static void MonoGCDescOpt() Assert.Equal(i + 1, holder.arr[i].s); } } + + struct StructHasInlineArrayField + { + FourtyTwoBytes _field; + } + + [Fact] + public static void InlineArrayEqualsGetHashCode_Fails() + { + Console.WriteLine($"{nameof(InlineArrayEqualsGetHashCode_Fails)}..."); + + Assert.Throws(() => + { + new FourtyTwoBytes().Equals(new FourtyTwoBytes()); + }); + + Assert.Throws(() => + { + new StructHasInlineArrayField().Equals(new StructHasInlineArrayField()); + }); + + Assert.Throws(() => + { + new FourtyTwoBytes().GetHashCode(); + }); + } } From 8713bbdfd557ca0e631ea616bc3df2c9b2219aab Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Mon, 17 Jun 2024 20:11:25 -0700 Subject: [PATCH 2/4] Switch to NotSupportedException. --- .../src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs | 4 ++-- .../IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs | 2 +- src/coreclr/vm/comutilnative.cpp | 2 +- .../System.Private.CoreLib/src/Resources/Strings.resx | 6 +++--- src/mono/mono/metadata/icall.c | 4 ++-- .../Loader/classloader/InlineArray/InlineArrayValid.cs | 6 +++--- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs index ae0b6c50b138d..c71b79f8dc0f9 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/CompilerHelpers/ThrowHelpers.cs @@ -133,9 +133,9 @@ public static void ThrowArgumentOutOfRangeException() throw new ArgumentOutOfRangeException(); } - public static void ThrowInvalidOperationInlineArrayEqualsGetHashCode() + public static void ThrowNotSupportedInlineArrayEqualsGetHashCode() { - throw new InvalidOperationException(SR.InvalidOperation_InlineArrayEqualsGetHashCode); + throw new NotSupportedException(SR.NotSupported_InlineArrayEqualsGetHashCode); } } } diff --git a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs index 37ead3c78ff8d..d9cf3009ec50f 100644 --- a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs +++ b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs @@ -68,7 +68,7 @@ public override MethodIL EmitIL() if (mdType.IsInlineArray) { var stream = emitter.NewCodeStream(); - MethodDesc invalidOp = Context.GetHelperEntryPoint("ThrowHelpers", "ThrowInvalidOperationInlineArrayEqualsGetHashCode"); + MethodDesc invalidOp = Context.GetHelperEntryPoint("ThrowHelpers", "ThrowNotSupportedInlineArrayEqualsGetHashCode"); stream.EmitCallThrowHelper(emitter, invalidOp); return emitter.Link(this); } diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 0eab25e446bca..aa53b29041bb4 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -1687,7 +1687,7 @@ extern "C" BOOL QCALLTYPE MethodTable_CanCompareBitsOrUseFastGetHashCode(MethodT BEGIN_QCALL; if (mt->GetClass()->IsInlineArray()) - COMPlusThrow(kInvalidOperationException, W("InvalidOperation_InlineArrayEqualsGetHashCode")); + COMPlusThrow(kNotSupportedException, W("NotSupported_InlineArrayEqualsGetHashCode")); ret = CanCompareBitsOrUseFastGetHashCode(mt); diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index 270bdca69d511..1f1669a04359c 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -2641,9 +2641,6 @@ Failed to compare two elements in the array. - - Calling built-in Equals() or GetHashCode() on type marked as InlineArray is invalid. - Type definition of the method is complete. @@ -3032,6 +3029,9 @@ Illegal one-byte branch at position: {0}. Requested branch was: {1}. + + Calling built-in Equals() or GetHashCode() on type marked as InlineArray is invalid. + Mutating a key collection derived from a dictionary is not allowed. diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index 459891b5a3cb1..e1d9bacb5f7ee 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -1253,7 +1253,7 @@ ves_icall_System_ValueType_InternalGetHashCode (MonoObjectHandle this_obj, MonoA klass = mono_handle_class (this_obj); if (m_class_is_inlinearray (klass)) { - mono_error_set_invalid_operation (error, "Calling built-in GetHashCode() on type marked as InlineArray is invalid."); + mono_error_set_not_supported (error, "Calling built-in GetHashCode() on type marked as InlineArray is invalid."); return FALSE; } @@ -1333,7 +1333,7 @@ ves_icall_System_ValueType_Equals (MonoObjectHandle this_obj, MonoObjectHandle t klass = mono_handle_class (this_obj); if (m_class_is_inlinearray (klass)) { - mono_error_set_invalid_operation (error, "Calling built-in Equals() on type marked as InlineArray is invalid."); + mono_error_set_not_supported (error, "Calling built-in Equals() on type marked as InlineArray is invalid."); return FALSE; } diff --git a/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs b/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs index f64718b3c43fe..75a62d0b5afe7 100644 --- a/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs +++ b/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs @@ -404,17 +404,17 @@ public static void InlineArrayEqualsGetHashCode_Fails() { Console.WriteLine($"{nameof(InlineArrayEqualsGetHashCode_Fails)}..."); - Assert.Throws(() => + Assert.Throws(() => { new FourtyTwoBytes().Equals(new FourtyTwoBytes()); }); - Assert.Throws(() => + Assert.Throws(() => { new StructHasInlineArrayField().Equals(new StructHasInlineArrayField()); }); - Assert.Throws(() => + Assert.Throws(() => { new FourtyTwoBytes().GetHashCode(); }); From 37127112eb44cdb4dba309042b9b4d5b36e7e8d5 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Mon, 17 Jun 2024 20:15:35 -0700 Subject: [PATCH 3/4] Change name of local. --- .../IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs index d9cf3009ec50f..babe171490aa0 100644 --- a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs +++ b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs @@ -68,8 +68,8 @@ public override MethodIL EmitIL() if (mdType.IsInlineArray) { var stream = emitter.NewCodeStream(); - MethodDesc invalidOp = Context.GetHelperEntryPoint("ThrowHelpers", "ThrowNotSupportedInlineArrayEqualsGetHashCode"); - stream.EmitCallThrowHelper(emitter, invalidOp); + MethodDesc thrower = Context.GetHelperEntryPoint("ThrowHelpers", "ThrowNotSupportedInlineArrayEqualsGetHashCode"); + stream.EmitCallThrowHelper(emitter, thrower); return emitter.Link(this); } } From 2fb5476a4126c00edc98d5ba9b9f4265dfe7affe Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Tue, 18 Jun 2024 08:45:20 -0700 Subject: [PATCH 4/4] Review feedback. Add test for types where their layout is the same whether InlineArray is provided or not. --- .../TypeSystem/IL/Stubs/ComparerIntrinsics.cs | 3 ++ .../InlineArray/InlineArrayValid.cs | 44 ++++++++++++++++--- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs index cbb1c37c5ae6e..8835a98168582 100644 --- a/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs +++ b/src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs @@ -240,6 +240,9 @@ public static bool CanCompareValueTypeBits(MetadataType type, MethodDesc objectE if (type.ContainsGCPointers) return false; + if (type.IsInlineArray) + return false; + if (type.IsGenericDefinition) return false; diff --git a/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs b/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs index 75a62d0b5afe7..a41f493bf046b 100644 --- a/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs +++ b/src/tests/Loader/classloader/InlineArray/InlineArrayValid.cs @@ -41,7 +41,7 @@ public unsafe class Validate { // ====================== SizeOf ============================================================== [InlineArray(42)] - struct FourtyTwoBytes + struct FortyTwoBytes { byte b; } @@ -50,11 +50,20 @@ struct FourtyTwoBytes public static void Sizeof() { Console.WriteLine($"{nameof(Sizeof)}..."); - Assert.Equal(42, sizeof(FourtyTwoBytes)); + Assert.Equal(42, sizeof(FortyTwoBytes)); Assert.Equal(84, sizeof(MyArray)); } // ====================== OneElement ========================================================== + // These types are interesting since their layouts are + // identical with or without the InlineArrayAttribute. + + [InlineArray(1)] + struct OneInt + { + public int i; + } + [InlineArray(1)] struct OneObj { @@ -65,6 +74,7 @@ struct OneObj public static void OneElement() { Console.WriteLine($"{nameof(OneElement)}..."); + Assert.Equal(sizeof(int), sizeof(OneInt)); Assert.Equal(sizeof(nint), sizeof(OneObj)); } @@ -394,9 +404,14 @@ public static void MonoGCDescOpt() } } - struct StructHasInlineArrayField + struct StructHasFortyTwoBytesField { - FourtyTwoBytes _field; + FortyTwoBytes _field; + } + + struct StructHasOneIntField + { + OneInt _field; } [Fact] @@ -406,17 +421,32 @@ public static void InlineArrayEqualsGetHashCode_Fails() Assert.Throws(() => { - new FourtyTwoBytes().Equals(new FourtyTwoBytes()); + new OneInt().Equals(new OneInt()); + }); + + Assert.Throws(() => + { + new StructHasOneIntField().Equals(new StructHasOneIntField()); + }); + + Assert.Throws(() => + { + new FortyTwoBytes().Equals(new FortyTwoBytes()); + }); + + Assert.Throws(() => + { + new StructHasFortyTwoBytesField().Equals(new StructHasFortyTwoBytesField()); }); Assert.Throws(() => { - new StructHasInlineArrayField().Equals(new StructHasInlineArrayField()); + new OneInt().GetHashCode(); }); Assert.Throws(() => { - new FourtyTwoBytes().GetHashCode(); + new FortyTwoBytes().GetHashCode(); }); } }