Skip to content

Support invoke of function pointer type arg - #90270

Merged
steveharter merged 5 commits into
dotnet:mainfrom
steveharter:FcnPtrFix
Aug 14, 2023
Merged

Support invoke of function pointer type arg#90270
steveharter merged 5 commits into
dotnet:mainfrom
steveharter:FcnPtrFix

Conversation

@steveharter

@stevehartersteveharter commented Aug 9, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Addresses the main issue of #85028 which is the inability to invoke a method that has a function pointer type argument for CoreClr. Also adds support to Mono and fixes issue in NativeAot with returning function pointer types.

@stevehartersteveharter self-assigned this Aug 9, 2023
@ghost

ghost commented Aug 9, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

[verifying tests]

Author:steveharter
Assignees:steveharter
Labels:

area-System.Reflection

Milestone:-

@steveharter
steveharterforce-pushed the FcnPtrFix branch 2 times, most recently from a41e446 to 86cb0ebCompareAugust 10, 2023 16:58
@steveharter
steveharter marked this pull request as ready for review August 10, 2023 19:32
jkotas
jkotas reviewed Aug 10, 2023
View reviewed changes
Comment threadsrc/libraries/System.Reflection/tests/MethodInfoTests.cs Outdated
jkotas
jkotas reviewed Aug 11, 2023
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Outdated
jkotas
jkotas reviewed Aug 11, 2023
View reviewed changes
Comment threadsrc/libraries/System.Reflection/tests/MethodInfoTests.cs Outdated
@ivanpovazan

ivanpovazan commented Aug 11, 2023

Copy link
Copy Markdown
Member

Is this planned to be merged in by RC1 snap?
I am asking this because Mono support for function pointer introspection API is ready for review #89712 and I would like to synchronise and agree what should get merged in first to avoid any regressions.

ivanpovazan
ivanpovazan reviewed Aug 11, 2023
View reviewed changes
Comment threadsrc/libraries/System.Reflection/tests/MethodInfoTests.cs Outdated
@steveharter

steveharter commented Aug 11, 2023

Copy link
Copy Markdown
ContributorAuthor

Is this planned to be merged in by RC1 snap?

I'll hold off on this PR until the Mono function pointer introspection PR is in.

@ivanpovazan

ivanpovazan commented Aug 11, 2023

Copy link
Copy Markdown
Member

I'll hold off on this PR until the #89712 is in.

Thank you. Done!

@vargaz

vargaz commented Aug 13, 2023

Copy link
Copy Markdown
Contributor

The mono changes look ok.

@jkotas

jkotas commented Aug 14, 2023

Copy link
Copy Markdown
Member

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Aug 14, 2023

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

jkotas
jkotas approved these changes Aug 14, 2023
View reviewed changes

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. (Please check that the tests are passing on native AOT before merging - I have triggered extra-platforms run.)

This was referenced Aug 14, 2023
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Open
Closed
Closed
Closed
steveharter
steveharter commented Aug 14, 2023
View reviewed changes
Comment threadsrc/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/DynamicInvokeInfo.cs
Debug.Assert(type.IsPointer);
obj = Pointer.Box((void*)Unsafe.As<byte, IntPtr>(ref byref), type);
}
else if ((_returnTransform & Transform.FunctionPointer) != 0)

@stevehartersteveharterAug 14, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Without this change, a checked build would throw in the else statement below where it tried to box _returnType.

@steveharter

steveharter commented Aug 14, 2023

Copy link
Copy Markdown
ContributorAuthor

LGTM. (Please check that the tests are passing on native AOT before merging - I have triggered extra-platforms run.)

Also verified locally with both release and checked runtime build.

Did update #90376 based on CI failures. Other failures appear unrelated.

@steveharter
steveharter merged commit c6f64ab into dotnet:mainAug 14, 2023
@steveharter
steveharter deleted the FcnPtrFix branch August 14, 2023 15:24
@stevehartersteveharter mentioned this pull request Aug 14, 2023
Closed
@ghostghost locked as resolved and limited conversation to collaborators Sep 13, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ivanpovazanivanpovazanivanpovazan left review comments
@jkotasjkotasjkotas approved these changes
@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner
@buyaa-nbuyaa-nAwaiting requested review from buyaa-n
@marek-safarmarek-safarAwaiting requested review from marek-safar
@vargazvargazAwaiting requested review from vargaz
@lambdageeklambdageekAwaiting requested review from lambdageek
@SamMonoRTSamMonoRTAwaiting requested review from SamMonoRT
@thaystgthaystgAwaiting requested review from thaystgthaystg is a code owner

Assignees

@stevehartersteveharter

Labels

area-System.Reflection

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@steveharter@ivanpovazan@vargaz@jkotas