Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[mono] Use correct cast_class for IntPtr[] #103841

Merged
merged 7 commits into from
Jun 26, 2024

Conversation

steveisok
Copy link
Member

This change fixes the behavior to be like modern .NET instead of .NET Framework.

Fixes #97145

This change fixes the behavior to be like modern .NET instead of .NET Framework.

Fixes dotnet#97145
Copy link
Member

@kg kg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right, but I'm not 100% certain.

@lambdageek
Copy link
Member

I wonder if there's any tests that capture the modern behavior that we previously disabled in Mono.

@kg
Copy link
Member

kg commented Jun 21, 2024

cc @BrzVlad is this going to cause any problems for interp?

@steveisok
Copy link
Member Author

This seems to have crashed on ios/tvos device runs. I ran @lambdageek's sample locally at least works for iossimulator. I'll try on a local device and see if I can get any info out of it.

@@ -1039,18 +1039,14 @@ class_composite_fixup_cast_class (MonoClass *klass, gboolean for_ptr)
case MONO_TYPE_U2:
klass->cast_class = mono_defaults.int16_class;
break;
case MONO_TYPE_U4:
#if TARGET_SIZEOF_VOID_P == 4
case MONO_TYPE_I:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MONO_TYPE_BOOLEAN -> byte_class normalization above looks suspect as well. I do not think regular CoreCLR does normalization like that anywhere.

Copy link
Member

@lambdageek lambdageek Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas I think the bool -> int8 fixup comes from when I misread I.8.7.2:

A location type T is compatible-with a location type U if and only if one of the following holds.

  1. T and U are not managed pointer types and T is compatible-with U according to the definition in §I.8.7.1.
  2. T and U are both managed pointer types and T is pointer-element-compatible-with U.
    A managed pointer type T is pointer-element-compatible-with a managed pointer type U if and
    only if T has verification type V and U has verification type W and V is identical to W.

The verification type of a bool is defined in I.8.7 as

  1. If the reduced type of T is:
    a. int8 or bool, then its verification type is int8.

The problem is I think I applied the verification type rules to unmanaged pointer types (ie bool*) rather than to managed pointer types (ie bool&)

@steveisok
Copy link
Member Author

Weird, local device runs are fine.

@steveisok
Copy link
Member Author

Appears there's some infra issues with the tvos and ios device legs. I validated system.runtime tests on a local device.

@steveisok steveisok merged commit 5a0eb6e into dotnet:main Jun 26, 2024
119 of 126 checks passed
@steveisok steveisok deleted the fix-intptr-cast branch June 26, 2024 20:47
@github-actions github-actions bot locked and limited conversation to collaborators Jul 27, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mono] Incorrect cast_class for IntPtr[]
5 participants