Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[mono] Disable gsharing when Unsafe.ReadUnaligned/WriteUnaligned () i… #89417

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

vargaz
Copy link
Contributor

@vargaz vargaz commented Jul 25, 2023

…s used with generic structures.

Fixes #89398.

For a method like

    static void Write<T>(ref byte b, T value) => Unsafe.WriteUnaligned<T>(ref b, value);

And an instance Write<GStruct<string>>, generic sharing will create a Write<T_INST> instance where T_INST is constrained to GStruct<T_REF>. The JIT currently calls
mini_get_underlying_type () in many places which transform T_INST into GStruct<T_REF>.
This causes problems at runtime in the generic sharing code, which expects to find T_INST.
I.e. inflate_info () can inflate T_INST to GStruct<string>, but it can't inflate
GStruct<T_REF> to GStruct<string>.

As a workaround, disable gsharing in (some) of these cases.

@vargaz
Copy link
Contributor Author

vargaz commented Jul 25, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…s used with generic structures.

Fixes dotnet#89398.

For a method like
```
    static void Write<T>(ref byte b, T value) => Unsafe.WriteUnaligned<T>(ref b, value);
```
And an instance ```Write<GStruct<string>>```, generic sharing will create a ```Write<T_INST>```
instance where T_INST is constrained to GStruct<T_REF>. The JIT currently calls
```mini_get_underlying_type ()``` in many places which transform T_INST into GStruct<T_REF>.
This causes problems at runtime in the generic sharing code, which expects to find T_INST.
I.e. ```inflate_info ()``` can inflate ```T_INST``` to ```GStruct<string>```, but it can't inflate
```GStruct<T_REF>``` to ```GStruct<string>```.

As a workaround, disable gsharing in (some) of these cases.
@vargaz
Copy link
Contributor Author

vargaz commented Jul 25, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 89417 in repo dotnet/runtime

@vargaz
Copy link
Contributor Author

vargaz commented Jul 25, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 89417 in repo dotnet/runtime

@vargaz
Copy link
Contributor Author

vargaz commented Jul 25, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 89417 in repo dotnet/runtime

@vargaz
Copy link
Contributor Author

vargaz commented Jul 25, 2023

/azp run runtime-wasm

1 similar comment
@vargaz
Copy link
Contributor Author

vargaz commented Jul 25, 2023

/azp run runtime-wasm

@azure-pipelines
Copy link

No commit pushedDate could be found for PR 89417 in repo dotnet/runtime

1 similar comment
@azure-pipelines
Copy link

No commit pushedDate could be found for PR 89417 in repo dotnet/runtime

@lambdageek
Copy link
Member

lambdageek commented Jul 25, 2023

Is this going to disable sharing for every generic method that has a Span<T> argument? nevermind, I thought this was in method-to-ir, but it's just intrinsics.

Should this be in method-to-ir? this mini_get_underlying_type issue seems like it would show up in other places, too

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

@vargaz can you make a GH Issue about the general problem. If we get this kind of crash in other situations, it would be good to have something that explains what is happening.

@lewing lewing merged commit 0be3889 into dotnet:main Jul 25, 2023
97 of 100 checks passed
@vargaz vargaz deleted the fix-89398 branch July 25, 2023 21:28
@ghost ghost locked as resolved and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wasm] AOT: System.Runtime.CompilerServices.Unsafe.Tests - assertion - src/mono/mono/metadata/object.c:926
3 participants