Skip to content

Conversation

@BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Dec 9, 2022

If we are calling an open instance delegate, where the target method is on a valuetype, we will need to unbox this pointer.

Fixes #79354

If we are calling an open instance delegate, where the target method is on a valuetype, we will need to unbox this pointer.
If we are calling a method of a valuetype, then we already know `this` pointer is an instantiation of a valuetype.
@BrzVlad BrzVlad requested a review from vargaz as a code owner December 9, 2022 10:51
@ghost ghost assigned BrzVlad Dec 9, 2022
@ghost
Copy link

ghost commented Dec 9, 2022

Tagging subscribers to this area: @BrzVlad
See info in area-owners.md if you want to be subscribed.

Issue Details

If we are calling an open instance delegate, where the target method is on a valuetype, we will need to unbox this pointer.

Fixes #79354

Author: BrzVlad
Assignees: -
Labels:

area-Codegen-Interpreter-mono

Milestone: -

@BrzVlad
Copy link
Member Author

BrzVlad commented Dec 9, 2022

@vargaz The new test case fails on fullaot with :

Unhandled Exception:
System.ExecutionEngineException: Attempting to JIT compile method '(wrapper delegate-invoke) System.ValueTuple3<string, int, string> <Module>:invoke_callvirt_ValueTuple3<string, int, string>_IGetContents (IGetContents)' while running in aot-only mode.

Changing to use Tuple instead of ValueTuple didn't have any effect. Should I disable this test on fullaot or something else ?

@vargaz
Copy link
Contributor

vargaz commented Dec 9, 2022

It can be disabled, full aot can't handle some open delegate scenarios.


// replace the MonoDelegate* on the stack with 'this' pointer
if (m_class_is_valuetype (this_arg->vtable->klass) && m_class_is_valuetype (cmethod->method->klass)) {
if (m_class_is_valuetype (cmethod->method->klass)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The first check was not needed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

To me it seems redundant. I can't think why it would be needed. If you can think on why it would be wrong let me know. I don't plan to backport this part of the change though.

@curia-damiano
Copy link

Hi all,
Thank you for your support!
I see this PR is merged in .NET 7, but I can't see the exact version.
In which version will it be published? Do you have also an idea when?

@ghost ghost locked as resolved and limited conversation to collaborators Feb 8, 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.

AutoMapper crashes on .NET 6 iOS MAUI application mapping IEnumerable properties

3 participants