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]: test_0_ldvirtftn_generic_method in generics.cs crash in Full AOT + Interp mode. #89076

Closed
lateralusX opened this issue Jul 18, 2023 · 0 comments · Fixed by #89142
Closed
Assignees

Comments

@lateralusX
Copy link
Member

lateralusX commented Jul 18, 2023

Description

Running one of the old Mono runtime regressions tests, generics.cs under Full AOT + Interp crash in test_0_ldvirtftn_generic_method with the following callstack:

interp_init_delegate(_MonoDelegate * del, MonoDelegateTrampInfo * * out_info, _MonoErrorInternal * error) Line 1795    C
mini_init_delegate(_MonoDelegateHandle delegate, _MonoObjectHandle target, void * addr, _MonoMethod * method, _MonoErrorInternal * error) Line 3968    C
mono_delegate_ctor(_MonoObjectHandle this_obj, _MonoObjectHandle target, void * addr, _MonoMethod * method, _MonoErrorInternal * error) Line 7659    C
ves_icall_mono_delegate_ctor(_MonoObject * this_obj_raw, _MonoObject * target_raw, void * addr) Line 1527    C
mono_aot_corlibjit_code_start()    Unknown
generics_Tests_ldvirtftn_T_REF()    Unknown
generics_Tests_test_0_ldvirtftn_generic_method()    Unknown
mono_aot_corlibjit_code_start()    Unknown

Happens because address is pointing into out static_rgctx_trampolines, but there is no method for the delegate, that will trigger fallback into interp_init_delegate that in turn assumes that method_ptr is an interpreter method, https://github.com/dotnet/runtime/blob/main/src/mono/mono/mini/interp/interp.c#L1749, but since this is not an interpreter method, it will trigger the crash in above callstack.

One potential fix is to do a more precise condition in https://github.com/dotnet/runtime/blob/main/src/mono/mono/mini/mini-runtime.c#L4031 like:

if (mono_use_interpreter && !mono_aot_only)

That will make sure not to go through mini_get_interp_callbacks ()->init_delegate when we run in Full AOT + Interp mode, the condition could potentially be made a little more precise to handle this specific case where we already have a working delegate method_ptr in the AOT image (we should favor anything in the AOT image before fallback to interpreter), but maybe we would need to hit init_delegate in some other scenarios running Full AOT + Interp.

Reproduction Steps

Run generics.cs runtime test using Mono Full AOT + Interp mode, https://github.com/dotnet/runtime/blob/main/src/mono/mono/mini/generics.cs

I have only tested it on Windows x64, but will most likely repro on other platforms.

There have also been reports of a similar crash when using Delegate.CreateDelegate under Full AOT + Interp.

Expected behavior

No crash.

Actual behavior

Crash.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 18, 2023
vargaz added a commit to vargaz/runtime that referenced this issue Jul 18, 2023
mini_init_delegate () needs to do a reverse lookup from address
to method, and its possible for the address to be a static rgctx
trampoline if the address is the result of mono_ldftn ().

Fixes dotnet#89076.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 18, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 19, 2023
vargaz added a commit that referenced this issue Jul 19, 2023
)

mini_init_delegate () needs to do a reverse lookup from address
to method, and its possible for the address to be a static rgctx
trampoline if the address is the result of mono_ldftn ().

Fixes #89076.
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 19, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants