Fix ValueType.GetHashCode not calling overriden method on nested field by huoyaoyuan · Pull Request #98754 · dotnet/runtime

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

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

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

 {
- 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.

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

 {
- 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

 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

 }

- 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

ghost 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

Comment threadsrc/coreclr/vm/comutilnative.cpp Outdated

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
Closed
[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

Comment threadsrc/coreclr/nativeaot/System.Private.CoreLib/src/System/ValueType.cs Outdated

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-analysisBot mentioned this pull request Feb 25, 2024
[mt][browser] HttpClient_CancelInDifferentThread failing with operation cancelled #98101
Closed

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:mainFeb 26, 2024

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

build-analysisBot mentioned this pull request Feb 28, 2024
profiler\\multiple\\multiple\\multiple.cmd failing on windows arm64 #98817
Closed

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

Navigation Menu

Uh oh!

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

Fix ValueType.GetHashCode not calling overriden method on nested field#98754
jkotas merged 11 commits into
dotnet:mainfrom
huoyaoyuan:valuetype-hashcode-fix

huoyaoyuan commented Feb 21, 2024

Uh oh!

Uh oh!

MichalStrehovskyFeb 23, 2024

Uh oh!

huoyaoyuanFeb 23, 2024

Uh oh!

MichalStrehovskyFeb 23, 2024

Uh oh!

Uh oh!

jkotasFeb 23, 2024

Uh oh!

jkotasFeb 24, 2024

Uh oh!

jkotasFeb 24, 2024

Uh oh!

huoyaoyuan commented Feb 25, 2024

Uh oh!

jkotas commented Feb 25, 2024

Uh oh!

jkotasFeb 25, 2024

Uh oh!

huoyaoyuan commented Feb 26, 2024

Uh oh!

jkotas left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

Uh oh!

Conversation

huoyaoyuan commented Feb 21, 2024

Uh oh!

Uh oh!

MichalStrehovskyFeb 23, 2024

Choose a reason for hiding this comment

Uh oh!

huoyaoyuanFeb 23, 2024

Choose a reason for hiding this comment

Uh oh!

MichalStrehovskyFeb 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkotasFeb 23, 2024

Choose a reason for hiding this comment

Uh oh!

jkotasFeb 24, 2024

Choose a reason for hiding this comment

Uh oh!

jkotasFeb 24, 2024

Choose a reason for hiding this comment

Uh oh!

huoyaoyuan commented Feb 25, 2024

Uh oh!

jkotas commented Feb 25, 2024

Uh oh!

jkotasFeb 25, 2024

Choose a reason for hiding this comment

Uh oh!

huoyaoyuan commented Feb 26, 2024

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants