Fix ValueType.GetHashCode not calling overriden method on nested field #98754

huoyaoyuan · 2024-02-21T13:20:28Z

This is indeed a bug, since GetHashCode can return different result when Equals is returning true.

src/coreclr/vm/comutilnative.cpp

MichalStrehovsky · 2024-02-23T10:38:05Z

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


I think GetHashCodeImpl can now be inlined into GetHashCode.

The only reason we had GetHashCodeImpl was because it was emulating CoreCLR behavior that did not call the GetHashCode on the field type (even if it had one) but instead used the fallback algorithm.

Is it feasible to use System.HashCode for NativeAOT here? Will it bring unintended dependency or complexity?

I think it would be fine to use System.HashCode. This was mirroring the CoreCLR-JIT logic but now it diverged because that's how forks usually end up.

If we really wanted to, we could probably unify this to the point of sharing a part of this file, maybe with an ifdef or two. From a quick look GetHashCodeStrategy could be implemented using NativeAOT's __GetFieldHelper.

src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs

jkotas · 2024-02-23T16:26:58Z

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


Suggested change

hashCode.Add(this.GetMethodTable()->HashCode);

hashCode.Add((IntPtr)this.GetMethodTable());

to match CoreCLR

jkotas · 2024-02-24T05:34:07Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs

@@ -150,7 +137,9 @@ private unsafe int RegularGetValueTypeHashCode(ref byte data, int numFields)
                }
                else if (fieldType->IsPrimitive)
                {
-                    hashCode = FastGetValueTypeHashCodeHelper(fieldType, ref fieldData);
+                    HashCode hash = default;


The method should take ref HashCode argument instead of creating a new instance here.

I meant change RegularGetValueTypeHashCode to take ref HashCode argument and delete HashCode hash = default; that this comment is attached to.

huoyaoyuan · 2024-02-25T08:22:47Z

Noticed that the test case doesn't actually cover this case. It should not match the default comparison/hashcode strategy of nested field.

Using manual version of int Abs to avoid introducing any dependency.

jkotas · 2024-02-25T14:42:49Z

Noticed that the test case doesn't actually cover this case. It should not match the default comparison/hashcode strategy of nested field

What is the test case that you are talking about?

Using manual version of int Abs to avoid introducing any dependency.

Nit: Dependencies like this are not a problem in the tests.

jkotas · 2024-02-25T14:46:00Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs

        }

-        private static unsafe int FastGetValueTypeHashCodeHelper(MethodTable* type, ref byte data)
+        private static unsafe void AddHashCodeForField(ref HashCode hashCode, MethodTable* type, ref byte data)


This method name is a bit misleading. It cannot be used to add hashcode for any field.

I like the Span returning method that you had here before better.

huoyaoyuan · 2024-02-26T04:49:13Z

What is the test case that you are talking about?

Describing the failing case: GetHashCode of top-level struct fails to call GetHashCode on top.field, but running the fallback strategy on top.field instead, which end up calls default strategy on top.field.nested.
If top.field.GetHashCode behaves the same as default strategy of top.field.nested, the test won't fail.
So, it's better to implement top.field.GetHashCode different than top.field.nested.GetHashCode.

Nit: Dependencies like this are not a problem in the tests.

The original reproduction uses StringComparer.OrdinalIgnoreCase. I think using numbers only is more clear here.

jkotas

Thanks

huoyaoyuan added 3 commits February 21, 2024 19:59

Handle overridden value type method

1ae14c2

Call virtual for NativeAot

d5a84eb

Add test

80235f7

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 21, 2024

huoyaoyuan commented Feb 21, 2024

View reviewed changes

src/coreclr/vm/comutilnative.cpp Outdated Show resolved Hide resolved

huoyaoyuan added area-VM-coreclr community-contribution Indicates that the PR has been added by a community member and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 21, 2024

This was referenced Feb 21, 2024

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

[browser][MT] Assert failed: Cannot find Promise for JSHandle -2 #98406

Closed

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

huoyaoyuan added 2 commits February 22, 2024 19:11

Change to box

33e0177

Fix test

d6b8287

huoyaoyuan marked this pull request as ready for review February 23, 2024 10:31

huoyaoyuan requested a review from MichalStrehovsky as a code owner February 23, 2024 10:31

MichalStrehovsky reviewed Feb 23, 2024

View reviewed changes

Fold GetHashCodeImpl

758dead

jkotas reviewed Feb 23, 2024

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs Outdated Show resolved Hide resolved

jkotas reviewed Feb 23, 2024

View reviewed changes

NativeAOT cleanup

3219160

This was referenced Feb 24, 2024

[browser] RuntimeError: memory access out of bounds #96546

Closed

WasmTestOnBrowser-System.Runtime.InteropServices.JavaScript.Tests time out #97112

Closed

[browser][MT] Unhandled JS error #97546

Closed

jkotas reviewed Feb 24, 2024

View reviewed changes

huoyaoyuan added 3 commits February 24, 2024 21:03

Don't create duplicated HashCode

19e5618

Pass ref HashCode to RegularGetValueTypeHashCode

aea4b67

Update test to not use default equals of nested field

2d27a42

build-analysis bot mentioned this pull request Feb 25, 2024

[mt][browser] HttpClient_CancelInDifferentThread failing with operation cancelled #98101

Open

jkotas reviewed Feb 25, 2024

View reviewed changes

Return span from method instead

b6fabf5

jkotas approved these changes Feb 26, 2024

View reviewed changes

jkotas merged commit a71cc52 into dotnet:main Feb 26, 2024
151 of 153 checks passed

huoyaoyuan deleted the valuetype-hashcode-fix branch February 26, 2024 19:25

build-analysis bot mentioned this pull request Feb 28, 2024

profiler\\multiple\\multiple\\multiple.cmd failing on windows arm64 #98817

Closed

github-actions bot locked and limited conversation to collaborators Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ValueType.GetHashCode not calling overriden method on nested field #98754

Fix ValueType.GetHashCode not calling overriden method on nested field #98754

huoyaoyuan commented Feb 21, 2024

MichalStrehovsky Feb 23, 2024

huoyaoyuan Feb 23, 2024

MichalStrehovsky Feb 23, 2024

jkotas Feb 23, 2024

jkotas Feb 24, 2024

jkotas Feb 24, 2024

huoyaoyuan commented Feb 25, 2024

jkotas commented Feb 25, 2024

jkotas Feb 25, 2024

huoyaoyuan commented Feb 26, 2024

jkotas left a comment

	hashCode.Add(this.GetMethodTable()->HashCode);
	hashCode.Add((IntPtr)this.GetMethodTable());

Fix ValueType.GetHashCode not calling overriden method on nested field #98754

Fix ValueType.GetHashCode not calling overriden method on nested field #98754

Conversation

huoyaoyuan commented Feb 21, 2024

MichalStrehovsky Feb 23, 2024

Choose a reason for hiding this comment

huoyaoyuan Feb 23, 2024

Choose a reason for hiding this comment

MichalStrehovsky Feb 23, 2024

Choose a reason for hiding this comment

jkotas Feb 23, 2024

Choose a reason for hiding this comment

jkotas Feb 24, 2024

Choose a reason for hiding this comment

jkotas Feb 24, 2024

Choose a reason for hiding this comment

huoyaoyuan commented Feb 25, 2024

jkotas commented Feb 25, 2024

jkotas Feb 25, 2024

Choose a reason for hiding this comment

huoyaoyuan commented Feb 26, 2024

jkotas left a comment

Choose a reason for hiding this comment