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

Reflection Invoke share more coreclr/mono #60270

Merged

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Oct 11, 2021

Work to reuse error handling between coreclr/mono for Reflection.Invoke scenarios.

Fixes mono/mono#14962
Fixes mono/mono#14993
Fixes mono/mono#14998

/cc @jkoritzinsky @elinor-fung

@AaronRobinsonMSFT AaronRobinsonMSFT added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 11, 2021
@ghost
Copy link

ghost commented Oct 11, 2021

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

Issue Details

Work to reuse error handling between coreclr/mono for Reflection.Invoke scenarios.

Author: AaronRobinsonMSFT
Assignees: -
Labels:

* NO MERGE *, area-System.Reflection

Milestone: -

@AaronRobinsonMSFT
Copy link
Member Author

/cc @lambdageek @vargaz

@AaronRobinsonMSFT AaronRobinsonMSFT removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 13, 2021
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.

Mono LGTM

…hare

# Conflicts:
#	src/mono/mono/metadata/icall.c
#	src/mono/mono/metadata/object.c
@AaronRobinsonMSFT
Copy link
Member Author

@steveharter and @buyaa-n Any further thoughts on this change?

Copy link
Member

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 53e8c7c into dotnet:main Oct 16, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the reflection_invoke_share branch October 16, 2021 16:18
@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2021
Comment on lines -4896 to -4907
if (has_byref_nullables) {
/*
* The runtime invoke wrapper already converted byref nullables back,
* and stored them in pa, we just need to copy them back to the
* managed array.
*/
for (i = 0; i < mono_array_length_internal (params); i++) {
MonoType *t = sig->params [i];

if (m_type_is_byref (t) && t->type == MONO_TYPE_GENERICINST && mono_class_is_nullable (mono_class_from_mono_type_internal (t)))
mono_array_setref_internal (params, i, pa [i]);
}
Copy link
Member

Choose a reason for hiding this comment

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

@AaronRobinsonMSFT oops, I think this caused #67269

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
9 participants