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][interp] Fix execution of delegate invoke wrapper with interpreter #111310

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Jan 11, 2025

The wrapper was relatively recently changed to icall into mono_get_addr_compiled_method in order to obtain a native function pointer to call using calli. This is incorrect on interpreter where we expect an InterpMethod*. This commit adds a new opcode instead, that on jit it goes through the same icall path, while on interpeter in similarly computes the appropiate method to call.

On a separate track, it might be useful to investigate whether the necessary delegate invoke wrapper should have been present in the aot image and not be executed with the interpreter in the first place.

Likely regressed after #83461.

Should fix #110995.

The wrapper was relatively recently changed to icall into mono_get_addr_compiled_method in order to obtain a native function pointer to call using calli. This is incorrect on interpreter where we expect an `InterpMethod*`. This commit adds a new opcode instead, that on jit it goes through the same icall path, while on interpeter in similarly computes the appropiate method to call.

On a separate track, it might be useful to investigate whether the necessary delegate invoke wrapper should have been present in the aot image and not be executed with the interpreter in the first place.
Copy link
Member

@kotlarmilos kotlarmilos left a comment

Choose a reason for hiding this comment

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

LGTM!

Do you want to add a test case in src/tests/JIT/Methodical/delegate/VirtualDelegate.cs?

@BrzVlad
Copy link
Member Author

BrzVlad commented Jan 14, 2025

@kotlarmilos Unfortunately this scenario is not easily tested given this path is not hit in interp only. One would need a very specific aot/interp transition, that we currently don't have testing support for.

@BrzVlad BrzVlad merged commit 5a395ed into dotnet:main Jan 15, 2025
66 of 69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS app crashes when calling GetValue on Virtual Property
2 participants