From 1ae14c25dd2a74ba2bc508ea0a6012961d3b4b37 Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Tue, 20 Feb 2024 21:51:55 +0800 Subject: [PATCH 01/11] Handle overridden value type method --- .../src/System/ValueType.cs | 10 ++++++-- src/coreclr/vm/comutilnative.cpp | 25 +++++++++++++++---- src/coreclr/vm/comutilnative.h | 2 +- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/ValueType.cs b/src/coreclr/System.Private.CoreLib/src/System/ValueType.cs index 78301866c36dce..1d431f05a5f2a6 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/ValueType.cs @@ -120,7 +120,7 @@ public override unsafe int GetHashCode() else { object thisRef = this; - switch (GetHashCodeStrategy(pMT, ObjectHandleOnStack.Create(ref thisRef), out uint fieldOffset, out uint fieldSize)) + switch (GetHashCodeStrategy(pMT, ObjectHandleOnStack.Create(ref thisRef), out uint fieldOffset, out uint fieldSize, out delegate* managed getHashCodeMethod)) { case ValueTypeHashCodeStrategy.ReferenceField: hashCode.Add(Unsafe.As(ref Unsafe.AddByteOffset(ref rawData, fieldOffset)).GetHashCode()); @@ -138,6 +138,11 @@ public override unsafe int GetHashCode() Debug.Assert(fieldSize != 0); hashCode.AddBytes(MemoryMarshal.CreateReadOnlySpan(ref Unsafe.AddByteOffset(ref rawData, fieldOffset), (int)fieldSize)); break; + + case ValueTypeHashCodeStrategy.ValueTypeOverride: + Debug.Assert(getHashCodeMethod != null); + hashCode.Add(getHashCodeMethod(ref Unsafe.AddByteOffset(ref rawData, fieldOffset))); + break; } } @@ -152,11 +157,12 @@ private enum ValueTypeHashCodeStrategy DoubleField, SingleField, FastGetHashCode, + ValueTypeOverride, } [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ValueType_GetHashCodeStrategy")] private static unsafe partial ValueTypeHashCodeStrategy GetHashCodeStrategy( - MethodTable* pMT, ObjectHandleOnStack objHandle, out uint fieldOffset, out uint fieldSize); + MethodTable* pMT, ObjectHandleOnStack objHandle, out uint fieldOffset, out uint fieldSize, out delegate* managed getHashCodeMethod); public override string? ToString() { diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 612cb9d72dc0dd..29eb6e5d579526 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -1703,9 +1703,10 @@ enum ValueTypeHashCodeStrategy DoubleField, SingleField, FastGetHashCode, + ValueTypeOverride, }; -static ValueTypeHashCodeStrategy GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize) +static ValueTypeHashCodeStrategy GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize, PCODE* method) { CONTRACTL { @@ -1774,8 +1775,21 @@ static ValueTypeHashCodeStrategy GetHashCodeStrategy(MethodTable* mt, QCall::Obj } else { - *fieldOffset += field->GetOffsetUnsafe(); - ret = GetHashCodeStrategy(fieldMT, objHandle, fieldOffset, fieldSize); + MethodTable* valueTypeMT = CoreLibBinder::GetClass(CLASS__VALUE_TYPE); + WORD slotGetHashCode = CoreLibBinder::GetMethod(METHOD__VALUE_TYPE__GET_HASH_CODE)->GetSlot(); + + if (HasOverriddenMethod(mt, valueTypeMT, slotGetHashCode)) + { + *fieldOffset += field->GetOffsetUnsafe(); + *method = mt->GetRestoredSlot(slotGetHashCode); + ret = ValueTypeHashCodeStrategy::ValueTypeOverride; + } + else + { + *fieldOffset += field->GetOffsetUnsafe(); + // fieldOffset is required since first field may not be at offset 0 with explicit layout + ret = GetHashCodeStrategy(fieldMT, objHandle, fieldOffset, fieldSize, method); + } } } } @@ -1785,18 +1799,19 @@ static ValueTypeHashCodeStrategy GetHashCodeStrategy(MethodTable* mt, QCall::Obj return ret; } -extern "C" INT32 QCALLTYPE ValueType_GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize) +extern "C" INT32 QCALLTYPE ValueType_GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize, PCODE * method) { QCALL_CONTRACT; ValueTypeHashCodeStrategy ret = ValueTypeHashCodeStrategy::None; *fieldOffset = 0; *fieldSize = 0; + *method = NULL; BEGIN_QCALL; - ret = GetHashCodeStrategy(mt, objHandle, fieldOffset, fieldSize); + ret = GetHashCodeStrategy(mt, objHandle, fieldOffset, fieldSize, method); END_QCALL; diff --git a/src/coreclr/vm/comutilnative.h b/src/coreclr/vm/comutilnative.h index a3c5ea65c3ca7c..6271b12b23fb00 100644 --- a/src/coreclr/vm/comutilnative.h +++ b/src/coreclr/vm/comutilnative.h @@ -252,7 +252,7 @@ class MethodTableNative { extern "C" BOOL QCALLTYPE MethodTable_AreTypesEquivalent(MethodTable* mta, MethodTable* mtb); extern "C" BOOL QCALLTYPE MethodTable_CanCompareBitsOrUseFastGetHashCode(MethodTable* mt); -extern "C" INT32 QCALLTYPE ValueType_GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize); +extern "C" INT32 QCALLTYPE ValueType_GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize, PCODE * method); class StreamNative { public: From d5a84ebf65f0dd5b8ba3d779dc162e2b7c086300 Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Wed, 21 Feb 2024 20:15:59 +0800 Subject: [PATCH 02/11] Call virtual for NativeAot --- .../nativeaot/System.Private.CoreLib/src/System/ValueType.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs index e8340e41191513..3b997ab758ed4f 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs @@ -164,7 +164,8 @@ private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields) var fieldValue = (ValueType)RuntimeImports.RhBox(fieldType, ref fieldData); if (fieldValue != null) { - hashCode = fieldValue.GetHashCodeImpl(); + // call virtual method to handle overriden case + hashCode = fieldValue.GetHashCode(); } else { From 80235f7b04673707272c2e1e8f2900f8df60b558 Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Wed, 21 Feb 2024 21:16:56 +0800 Subject: [PATCH 03/11] Add test --- .../System/ValueTypeTests.cs | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs index 92a2c006ce2042..72f1b6a15c512c 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs @@ -315,6 +315,21 @@ public static void StructContainsPointerNestedCompareTest() Assert.Equal(obj1.GetHashCode(), obj2.GetHashCode()); } + [Fact] + public static void StructWithNestedOverriddenNotBitwiseComparableNext() + { + StructWithNestedOverriddenNotBitwiseComparable obj1 = new StructWithNestedOverriddenNotBitwiseComparable(); + obj1.value1.value = 0.0; + obj1.value2.value = 1.0; + + StructWithNestedOverriddenNotBitwiseComparable obj2 = new StructWithNestedOverriddenNotBitwiseComparable(); + obj1.value1.value = -0.0; + obj1.value2.value = 1.0; + + Assert.True(obj1.Equals(obj2)); + Assert.Equal(obj1.GetHashCode(), obj2.GetHashCode()); + } + public struct S { public int x; @@ -413,5 +428,20 @@ public struct StructContainsPointerNested public object o; public StructNonOverriddenEqualsOrGetHasCode value; } + + public struct StructOverriddenNotBitwiseComparable + { + public double value; + + public override bool Equals(object obj) => obj is StructOverriddenNotBitwiseComparable other && value.Equals(other.value); + + public override int GetHashCode() => value.GetHashCode(); + } + + public struct StructWithNestedOverriddenNotBitwiseComparable + { + public StructOverriddenNotBitwiseComparable value1; + public StructOverriddenNotBitwiseComparable value2; + } } } From 33e0177248e204ed51d93e71e80a376e0ecf8bec Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Thu, 22 Feb 2024 19:11:46 +0800 Subject: [PATCH 04/11] Change to box --- .../src/System/ValueType.cs | 9 ++--- src/coreclr/vm/comutilnative.cpp | 34 ++++++++----------- src/coreclr/vm/comutilnative.h | 2 +- 3 files changed, 20 insertions(+), 25 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/ValueType.cs b/src/coreclr/System.Private.CoreLib/src/System/ValueType.cs index 1d431f05a5f2a6..f4c3acb31adf88 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/ValueType.cs @@ -120,7 +120,7 @@ public override unsafe int GetHashCode() else { object thisRef = this; - switch (GetHashCodeStrategy(pMT, ObjectHandleOnStack.Create(ref thisRef), out uint fieldOffset, out uint fieldSize, out delegate* managed getHashCodeMethod)) + switch (GetHashCodeStrategy(pMT, ObjectHandleOnStack.Create(ref thisRef), out uint fieldOffset, out uint fieldSize, out MethodTable* fieldMT)) { case ValueTypeHashCodeStrategy.ReferenceField: hashCode.Add(Unsafe.As(ref Unsafe.AddByteOffset(ref rawData, fieldOffset)).GetHashCode()); @@ -140,8 +140,9 @@ public override unsafe int GetHashCode() break; case ValueTypeHashCodeStrategy.ValueTypeOverride: - Debug.Assert(getHashCodeMethod != null); - hashCode.Add(getHashCodeMethod(ref Unsafe.AddByteOffset(ref rawData, fieldOffset))); + Debug.Assert(fieldMT != null); + // Box the field to handle complicated cases like mutable method and shared generic + hashCode.Add(RuntimeHelpers.Box(fieldMT, ref Unsafe.AddByteOffset(ref rawData, fieldOffset))?.GetHashCode() ?? 0); break; } } @@ -162,7 +163,7 @@ private enum ValueTypeHashCodeStrategy [LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ValueType_GetHashCodeStrategy")] private static unsafe partial ValueTypeHashCodeStrategy GetHashCodeStrategy( - MethodTable* pMT, ObjectHandleOnStack objHandle, out uint fieldOffset, out uint fieldSize, out delegate* managed getHashCodeMethod); + MethodTable* pMT, ObjectHandleOnStack objHandle, out uint fieldOffset, out uint fieldSize, out MethodTable* fieldMT); public override string? ToString() { diff --git a/src/coreclr/vm/comutilnative.cpp b/src/coreclr/vm/comutilnative.cpp index 29eb6e5d579526..6c7e2468d2744b 100644 --- a/src/coreclr/vm/comutilnative.cpp +++ b/src/coreclr/vm/comutilnative.cpp @@ -1706,7 +1706,7 @@ enum ValueTypeHashCodeStrategy ValueTypeOverride, }; -static ValueTypeHashCodeStrategy GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize, PCODE* method) +static ValueTypeHashCodeStrategy GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize, MethodTable** fieldMTOut) { CONTRACTL { @@ -1773,23 +1773,18 @@ static ValueTypeHashCodeStrategy GetHashCodeStrategy(MethodTable* mt, QCall::Obj *fieldSize = field->LoadSize(); ret = ValueTypeHashCodeStrategy::FastGetHashCode; } + else if (HasOverriddenMethod(fieldMT, + CoreLibBinder::GetClass(CLASS__VALUE_TYPE), + CoreLibBinder::GetMethod(METHOD__VALUE_TYPE__GET_HASH_CODE)->GetSlot())) + { + *fieldOffset += field->GetOffsetUnsafe(); + *fieldMTOut = fieldMT; + ret = ValueTypeHashCodeStrategy::ValueTypeOverride; + } else { - MethodTable* valueTypeMT = CoreLibBinder::GetClass(CLASS__VALUE_TYPE); - WORD slotGetHashCode = CoreLibBinder::GetMethod(METHOD__VALUE_TYPE__GET_HASH_CODE)->GetSlot(); - - if (HasOverriddenMethod(mt, valueTypeMT, slotGetHashCode)) - { - *fieldOffset += field->GetOffsetUnsafe(); - *method = mt->GetRestoredSlot(slotGetHashCode); - ret = ValueTypeHashCodeStrategy::ValueTypeOverride; - } - else - { - *fieldOffset += field->GetOffsetUnsafe(); - // fieldOffset is required since first field may not be at offset 0 with explicit layout - ret = GetHashCodeStrategy(fieldMT, objHandle, fieldOffset, fieldSize, method); - } + *fieldOffset += field->GetOffsetUnsafe(); + ret = GetHashCodeStrategy(fieldMT, objHandle, fieldOffset, fieldSize, fieldMTOut); } } } @@ -1799,19 +1794,18 @@ static ValueTypeHashCodeStrategy GetHashCodeStrategy(MethodTable* mt, QCall::Obj return ret; } -extern "C" INT32 QCALLTYPE ValueType_GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize, PCODE * method) +extern "C" INT32 QCALLTYPE ValueType_GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize, MethodTable** fieldMT) { QCALL_CONTRACT; ValueTypeHashCodeStrategy ret = ValueTypeHashCodeStrategy::None; *fieldOffset = 0; *fieldSize = 0; - *method = NULL; + *fieldMT = NULL; BEGIN_QCALL; - - ret = GetHashCodeStrategy(mt, objHandle, fieldOffset, fieldSize, method); + ret = GetHashCodeStrategy(mt, objHandle, fieldOffset, fieldSize, fieldMT); END_QCALL; diff --git a/src/coreclr/vm/comutilnative.h b/src/coreclr/vm/comutilnative.h index 6271b12b23fb00..0f305e0af90072 100644 --- a/src/coreclr/vm/comutilnative.h +++ b/src/coreclr/vm/comutilnative.h @@ -252,7 +252,7 @@ class MethodTableNative { extern "C" BOOL QCALLTYPE MethodTable_AreTypesEquivalent(MethodTable* mta, MethodTable* mtb); extern "C" BOOL QCALLTYPE MethodTable_CanCompareBitsOrUseFastGetHashCode(MethodTable* mt); -extern "C" INT32 QCALLTYPE ValueType_GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize, PCODE * method); +extern "C" INT32 QCALLTYPE ValueType_GetHashCodeStrategy(MethodTable* mt, QCall::ObjectHandleOnStack objHandle, UINT32* fieldOffset, UINT32* fieldSize, MethodTable** fieldMT); class StreamNative { public: From d6b8287dc01020838f003df59f430c374079b984 Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Fri, 23 Feb 2024 15:34:42 +0800 Subject: [PATCH 05/11] Fix test --- .../tests/System.Runtime.Tests/System/ValueTypeTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs index 72f1b6a15c512c..0cbb69fb92a3ab 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs @@ -316,15 +316,15 @@ public static void StructContainsPointerNestedCompareTest() } [Fact] - public static void StructWithNestedOverriddenNotBitwiseComparableNext() + public static void StructWithNestedOverriddenNotBitwiseComparableTest() { StructWithNestedOverriddenNotBitwiseComparable obj1 = new StructWithNestedOverriddenNotBitwiseComparable(); obj1.value1.value = 0.0; obj1.value2.value = 1.0; StructWithNestedOverriddenNotBitwiseComparable obj2 = new StructWithNestedOverriddenNotBitwiseComparable(); - obj1.value1.value = -0.0; - obj1.value2.value = 1.0; + obj2.value1.value = -0.0; + obj2.value2.value = 1.0; Assert.True(obj1.Equals(obj2)); Assert.Equal(obj1.GetHashCode(), obj2.GetHashCode()); From 758dead79a63b6c16a116c58b6a34b425ded9fd3 Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Fri, 23 Feb 2024 21:55:17 +0800 Subject: [PATCH 06/11] Fold GetHashCodeImpl --- .../src/System/ValueType.cs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs index 3b997ab758ed4f..f093c9128f24c1 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs @@ -95,21 +95,17 @@ public override unsafe bool Equals([NotNullWhen(true)] object? obj) public override unsafe int GetHashCode() { - int hashCode = (int)this.GetMethodTable()->HashCode; + HashCode hashCode = default; + hashCode.Add(this.GetMethodTable()->HashCode); - hashCode ^= GetHashCodeImpl(); - - return hashCode; - } - - private unsafe int GetHashCodeImpl() - { int numFields = __GetFieldHelper(GetNumFields, out _); if (numFields == UseFastHelper) - return FastGetValueTypeHashCodeHelper(this.GetMethodTable(), ref this.GetRawData()); + hashCode.Add(FastGetValueTypeHashCodeHelper(this.GetMethodTable(), ref this.GetRawData())); + + hashCode.Add(RegularGetValueTypeHashCode(ref this.GetRawData(), numFields)); - return RegularGetValueTypeHashCode(ref this.GetRawData(), numFields); + return hashCode.ToHashCode(); } private static unsafe int FastGetValueTypeHashCodeHelper(MethodTable* type, ref byte data) @@ -164,7 +160,6 @@ private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields) var fieldValue = (ValueType)RuntimeImports.RhBox(fieldType, ref fieldData); if (fieldValue != null) { - // call virtual method to handle overriden case hashCode = fieldValue.GetHashCode(); } else From 3219160254571c32b6ecb1dee0da93fd50b6d745 Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Sat, 24 Feb 2024 02:19:26 +0800 Subject: [PATCH 07/11] NativeAOT cleanup --- .../src/System/ValueType.cs | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs index f093c9128f24c1..c43383b47aa6e0 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs @@ -96,32 +96,23 @@ public override unsafe bool Equals([NotNullWhen(true)] object? obj) public override unsafe int GetHashCode() { HashCode hashCode = default; - hashCode.Add(this.GetMethodTable()->HashCode); + hashCode.Add((IntPtr)this.GetMethodTable()); int numFields = __GetFieldHelper(GetNumFields, out _); if (numFields == UseFastHelper) - hashCode.Add(FastGetValueTypeHashCodeHelper(this.GetMethodTable(), ref this.GetRawData())); - - hashCode.Add(RegularGetValueTypeHashCode(ref this.GetRawData(), numFields)); + hashCode.AddBytes(GetSpanForField(this.GetMethodTable(), ref this.GetRawData())); + else + hashCode.Add(RegularGetValueTypeHashCode(ref this.GetRawData(), numFields)); return hashCode.ToHashCode(); } - private static unsafe int FastGetValueTypeHashCodeHelper(MethodTable* type, ref byte data) + private static unsafe ReadOnlySpan GetSpanForField(MethodTable* type, ref byte data) { // Sanity check - if there are GC references, we should not be hashing bytes Debug.Assert(!type->ContainsGCPointers); - - int size = (int)type->ValueTypeSize; - int hashCode = 0; - - for (int i = 0; i < size / 4; i++) - { - hashCode ^= Unsafe.As(ref Unsafe.Add(ref data, i * 4)); - } - - return hashCode; + return new ReadOnlySpan(ref data, (int)type->ValueTypeSize); } private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields) @@ -146,7 +137,9 @@ private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields) } else if (fieldType->IsPrimitive) { - hashCode = FastGetValueTypeHashCodeHelper(fieldType, ref fieldData); + HashCode hash = default; + hash.AddBytes(GetSpanForField(fieldType, ref fieldData)); + hashCode = hash.ToHashCode(); } else if (fieldType->IsValueType) { From 19e5618e13c4b67dc33df12faa33102a0a83ac85 Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Sat, 24 Feb 2024 21:03:21 +0800 Subject: [PATCH 08/11] Don't create duplicated HashCode --- .../System.Private.CoreLib/src/System/ValueType.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs index c43383b47aa6e0..d0e4d677c56f18 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs @@ -101,18 +101,18 @@ public override unsafe int GetHashCode() int numFields = __GetFieldHelper(GetNumFields, out _); if (numFields == UseFastHelper) - hashCode.AddBytes(GetSpanForField(this.GetMethodTable(), ref this.GetRawData())); + AddHashCodeForField(ref hashCode, this.GetMethodTable(), ref this.GetRawData()); else hashCode.Add(RegularGetValueTypeHashCode(ref this.GetRawData(), numFields)); return hashCode.ToHashCode(); } - private static unsafe ReadOnlySpan GetSpanForField(MethodTable* type, ref byte data) + private static unsafe void AddHashCodeForField(ref HashCode hashCode, MethodTable* type, ref byte data) { // Sanity check - if there are GC references, we should not be hashing bytes Debug.Assert(!type->ContainsGCPointers); - return new ReadOnlySpan(ref data, (int)type->ValueTypeSize); + hashCode.AddBytes(new ReadOnlySpan(ref data, (int)type->ValueTypeSize)); } private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields) @@ -138,7 +138,7 @@ private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields) else if (fieldType->IsPrimitive) { HashCode hash = default; - hash.AddBytes(GetSpanForField(fieldType, ref fieldData)); + AddHashCodeForField(ref hash, fieldType, ref fieldData); hashCode = hash.ToHashCode(); } else if (fieldType->IsValueType) From aea4b67dd3ebaaea7542128f6e58f3c3fe80b2a1 Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Sun, 25 Feb 2024 16:13:12 +0800 Subject: [PATCH 09/11] Pass ref HashCode to RegularGetValueTypeHashCode --- .../src/System/ValueType.cs | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs index d0e4d677c56f18..96f689580694cb 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs @@ -103,7 +103,7 @@ public override unsafe int GetHashCode() if (numFields == UseFastHelper) AddHashCodeForField(ref hashCode, this.GetMethodTable(), ref this.GetRawData()); else - hashCode.Add(RegularGetValueTypeHashCode(ref this.GetRawData(), numFields)); + RegularGetValueTypeHashCode(ref hashCode, ref this.GetRawData(), numFields); return hashCode.ToHashCode(); } @@ -115,10 +115,8 @@ private static unsafe void AddHashCodeForField(ref HashCode hashCode, MethodTabl hashCode.AddBytes(new ReadOnlySpan(ref data, (int)type->ValueTypeSize)); } - private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields) + private unsafe void RegularGetValueTypeHashCode(ref HashCode hashCode, ref byte data, int numFields) { - int hashCode = 0; - // We only take the hashcode for the first non-null field. That's what the CLR does. for (int i = 0; i < numFields; i++) { @@ -129,17 +127,15 @@ private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields) if (fieldType->ElementType == EETypeElementType.Single) { - hashCode = Unsafe.As(ref fieldData).GetHashCode(); + hashCode.Add(Unsafe.As(ref fieldData)); } else if (fieldType->ElementType == EETypeElementType.Double) { - hashCode = Unsafe.As(ref fieldData).GetHashCode(); + hashCode.Add(Unsafe.As(ref fieldData)); } else if (fieldType->IsPrimitive) { - HashCode hash = default; - AddHashCodeForField(ref hash, fieldType, ref fieldData); - hashCode = hash.ToHashCode(); + AddHashCodeForField(ref hashCode, fieldType, ref fieldData); } else if (fieldType->IsValueType) { @@ -153,7 +149,7 @@ private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields) var fieldValue = (ValueType)RuntimeImports.RhBox(fieldType, ref fieldData); if (fieldValue != null) { - hashCode = fieldValue.GetHashCode(); + hashCode.Add(fieldValue); } else { @@ -166,7 +162,7 @@ private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields) object fieldValue = Unsafe.As(ref fieldData); if (fieldValue != null) { - hashCode = fieldValue.GetHashCode(); + hashCode.Add(fieldValue); } else { @@ -176,8 +172,6 @@ private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields) } break; } - - return hashCode; } } } From 2d27a429aeb6a285b52eb54acd9fe332c6ca8bd5 Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Sun, 25 Feb 2024 16:20:35 +0800 Subject: [PATCH 10/11] Update test to not use default equals of nested field --- .../System.Runtime.Tests/System/ValueTypeTests.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs index 0cbb69fb92a3ab..92c7000ed414d5 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs @@ -319,12 +319,12 @@ public static void StructContainsPointerNestedCompareTest() public static void StructWithNestedOverriddenNotBitwiseComparableTest() { StructWithNestedOverriddenNotBitwiseComparable obj1 = new StructWithNestedOverriddenNotBitwiseComparable(); - obj1.value1.value = 0.0; - obj1.value2.value = 1.0; + obj1.value1.value = 1; + obj1.value2.value = 0; StructWithNestedOverriddenNotBitwiseComparable obj2 = new StructWithNestedOverriddenNotBitwiseComparable(); - obj2.value1.value = -0.0; - obj2.value2.value = 1.0; + obj2.value1.value = -1; + obj2.value2.value = 0; Assert.True(obj1.Equals(obj2)); Assert.Equal(obj1.GetHashCode(), obj2.GetHashCode()); @@ -431,11 +431,11 @@ public struct StructContainsPointerNested public struct StructOverriddenNotBitwiseComparable { - public double value; + public int value; - public override bool Equals(object obj) => obj is StructOverriddenNotBitwiseComparable other && value.Equals(other.value); + public override bool Equals(object obj) => obj is StructOverriddenNotBitwiseComparable other && (value == other.value || value == -other.value); - public override int GetHashCode() => value.GetHashCode(); + public override int GetHashCode() => value < 0 ? -value : value; } public struct StructWithNestedOverriddenNotBitwiseComparable From b6fabf5daaafc7e95947d47814ac4fe3a955dc9f Mon Sep 17 00:00:00 2001 From: Huo Yaoyuan Date: Mon, 26 Feb 2024 21:24:59 +0800 Subject: [PATCH 11/11] Return span from method instead --- .../System.Private.CoreLib/src/System/ValueType.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs index 96f689580694cb..968e97c425cf81 100644 --- a/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs +++ b/src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs @@ -101,18 +101,18 @@ public override unsafe int GetHashCode() int numFields = __GetFieldHelper(GetNumFields, out _); if (numFields == UseFastHelper) - AddHashCodeForField(ref hashCode, this.GetMethodTable(), ref this.GetRawData()); + hashCode.AddBytes(GetSpanForField(this.GetMethodTable(), ref this.GetRawData())); else RegularGetValueTypeHashCode(ref hashCode, ref this.GetRawData(), numFields); return hashCode.ToHashCode(); } - private static unsafe void AddHashCodeForField(ref HashCode hashCode, MethodTable* type, ref byte data) + private static unsafe ReadOnlySpan GetSpanForField(MethodTable* type, ref byte data) { // Sanity check - if there are GC references, we should not be hashing bytes Debug.Assert(!type->ContainsGCPointers); - hashCode.AddBytes(new ReadOnlySpan(ref data, (int)type->ValueTypeSize)); + return new ReadOnlySpan(ref data, (int)type->ValueTypeSize); } private unsafe void RegularGetValueTypeHashCode(ref HashCode hashCode, ref byte data, int numFields) @@ -135,7 +135,7 @@ private unsafe void RegularGetValueTypeHashCode(ref HashCode hashCode, ref byte } else if (fieldType->IsPrimitive) { - AddHashCodeForField(ref hashCode, fieldType, ref fieldData); + hashCode.AddBytes(GetSpanForField(fieldType, ref fieldData)); } else if (fieldType->IsValueType) {