[mono] Unify invoke code with coreclr by BrzVlad · Pull Request #72717 · dotnet/runtime

BrzVlad · 2022-07-23T11:03:29Z

Unification of invoke paths enables use of IL Emit Invoke also on mono.

InternalInvoke now receives arguments byref, enabling support for future APIs making use of this.

ghost · 2022-07-23T11:03:35Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

ghost · 2022-07-23T11:05:05Z

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

Issue Details

#68928

Author:	BrzVlad
Assignees:	BrzVlad
Labels:	`area-System.Reflection`
Milestone:	-

vargaz · 2022-07-24T11:18:46Z

Might be better to use the non _handle functions and MONO_HANDLE_PIN () in this function.

srxqds · 2022-08-04T13:40:23Z

hi, this will merge to rc1?

marek-safar · 2022-09-29T07:13:19Z

Could you look into fixing this for DynamicMethod as well?

BrzVlad · 2022-09-29T15:11:57Z

Could you look into fixing this for DynamicMethod as well?

Not sure what you mean by this. Most of the code for invoking a DynamicMethod is shared now.

marek-safar · 2022-11-14T14:40:38Z

Are these same as https://github.com/dotnet/runtime/blob/main/src/mono/System.Private.CoreLib/src/System/Nullable.Mono.cs#L17-L39 ?

No. Methods in Nullable.Mono.cs translate between normal boxed valuetypes and nullable structures, while these icalls translate between normal boxed valuetypes and boxed nullable structures.

This version doesn't receive a MonoArray of params as objects, rather a simple array of byrefs. There is also no need for special handling for nullables. The runtime invoke wrapper receives a boxed nullable, this conversion happens in managed code using ReboxFromNullable and ReboxToNullable. This change might have some side effects on other API making use of the runtime invoke wrapper!

Behavior was changed recently.

Aside from perf optimization, this allows the usage of LocalAppContextSwitches for invoke tests. Before this change, invoke machinery was initialized before the appcontext properties were set, failing to detect ForceInterpretedInvoke and ForceEmitInvoke properties. CoreCLR also hardcodes string type param.

This API already receives a boxed nullable now.

CoreCLR achieves this via NextCallReturnAddress. Add another intrinsic to be used for mono.

Remove special handling for nullables and throwing of NullByRefReturnException.

Which was still used only by mono_runtime_invoke_array

Nullable<T> only has 1 param constructor that creates a boxed vtype

With same behavior as before, except we have to handle conversions between nullable and boxedvt ourserlves, since it is no longer done by the runtime invoke wrapper.

… method overrides Seems to be possible via reflection, ex System.Reflection.Emit.Tests.MethodBuilderDefineMethodOverride.DefineMethodOverride_CalledTwiceWithSameBodies_Works

BrzVlad · 2022-11-17T18:06:57Z

can we merge this ?

ghost assigned BrzVlad Jul 23, 2022

BrzVlad added the area-System.Reflection label Jul 23, 2022

vargaz reviewed Jul 24, 2022
View reviewed changes

Comment threadsrc/mono/mono/metadata/icall.c Outdated

vargaz reviewed Jul 24, 2022
View reviewed changes

Comment threadsrc/mono/mono/metadata/icall.c Outdated

vargaz reviewed Jul 24, 2022
View reviewed changes

Comment threadsrc/mono/mono/metadata/icall.c Outdated

vargaz reviewed Jul 24, 2022
View reviewed changes

Comment threadsrc/mono/mono/metadata/icall.c Outdated

vargaz reviewed Jul 24, 2022
View reviewed changes

Comment threadsrc/mono/mono/metadata/icall.c Outdated

BrzVlad force-pushed the feature-il-invoke branch from 6c9caa3 to dd5ab0eCompare July 25, 2022 07:47

BrzVlad force-pushed the feature-il-invoke branch from dd5ab0e to 2d374e5Compare August 22, 2022 13:39

joperezr added this to the 8.0.0 milestone Aug 24, 2022

BrzVlad force-pushed the feature-il-invoke branch from d256579 to 63ede38Compare September 6, 2022 09:05

BrzVlad force-pushed the feature-il-invoke branch 3 times, most recently from 061bb7a to d0d180bCompare September 14, 2022 09:39

BrzVlad force-pushed the feature-il-invoke branch 2 times, most recently from a76abee to c0268a5Compare September 29, 2022 06:58

BrzVlad marked this pull request as ready for review September 29, 2022 15:14

BrzVlad requested review from SamMonoRT, lambdageek and thaystg as code owners September 29, 2022 15:14

vargaz reviewed Nov 11, 2022
View reviewed changes

Comment threadsrc/mono/mono/metadata/icall.c Outdated

BrzVlad force-pushed the feature-il-invoke branch from 5300688 to 1ed045dCompare November 14, 2022 14:22

marek-safar reviewed Nov 14, 2022
View reviewed changes

build-analysisBot mentioned this pull request Nov 14, 2022
System.IO.Pipes.Tests.NamedPipeTest_Specific.ClientConnectAsync_Cancel_With_InfiniteTimeout failing on Libraries Test Run release coreclr windows x86 Release#69101
Open

BrzVlad added 19 commits November 15, 2022 11:27

[mono] Add icalls to be used by new invoke machinery
17c93bd

[mono] Fix IsInstanceOfType check
c6b7015

[mono] Reuse most of the managed invoke path used by coreclr
6c2169f

[mono] Throw NRE for a null byref parameter
6501ef0
Behavior was changed recently.

[tests] Enable reflection invoke tests on mono
e9a4c57

[mono] Remove nullable handling case
5757632
This API already receives a boxed nullable now.

[mono] Disable inlining into Invoke stubs
6375397
CoreCLR achieves this via NextCallReturnAddress. Add another intrinsic to be used for mono.

[mono] Fix handling of nullables when invoking from debugger
f71f9ed

[mono] Update code for dyn runtime invokes and llvmonly invoke
c6dc7f3
Remove special handling for nullables and throwing of NullByRefReturnException.

[mono] Remove old invoke code
0e5ddb4
Which was still used only by mono_runtime_invoke_array

[mono] Move invoke code back in object.c
4f4cde9

[mono] Simplify nullable ctor case
ec1dcb9
Nullable<T> only has 1 param constructor that creates a boxed vtype

[mono] Extract invoke code to be reused by mono_runtime_invoke_array
16aeff6

[mono] Add back support for the embedding api
4f30ce5
With same behavior as before, except we have to handle conversions between nullable and boxedvt ourserlves, since it is no longer done by the runtime invoke wrapper.

[mono] Don't trigger ambiguous method exception when having duplicate…
2c03493
… method overrides Seems to be possible via reflection, ex System.Reflection.Emit.Tests.MethodBuilderDefineMethodOverride.DefineMethodOverride_CalledTwiceWithSameBodies_Works

Disable some warnings
ecbf3c8

Disable tests on wasm aot since they rely on stacktraces
93ca85f

BrzVlad force-pushed the feature-il-invoke branch from 1ed045d to 93ca85fCompare November 15, 2022 09:27

vargaz approved these changes Nov 17, 2022
View reviewed changes

BrzVlad merged commit aa012f4 into dotnet:mainNov 18, 2022

BrzVlad mentioned this pull request Nov 30, 2022
[mono][interp] Speed up invoke path #79029
Closed

BrzVlad mentioned this pull request Dec 13, 2022
[mono] Fix passing of NULL via mono_runtime_invoke to method expecting nullable #79597
Merged

ghost locked as resolved and limited conversation to collaborators Dec 18, 2022

Navigation Menu

Uh oh!

[mono] Unify invoke code with coreclr - #72717

[mono] Unify invoke code with coreclr#72717
BrzVlad merged 19 commits into
dotnet:mainfrom
BrzVlad:feature-il-invoke

BrzVlad commented Jul 23, 2022•
edited
Loading

Uh oh!

ghost commented Jul 23, 2022

Uh oh!

ghost commented Jul 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vargazJul 24, 2022

Uh oh!

Uh oh!

srxqds commented Aug 4, 2022

Uh oh!

marek-safar commented Sep 29, 2022

Uh oh!

BrzVlad commented Sep 29, 2022

Uh oh!

Uh oh!

marek-safarNov 14, 2022

Uh oh!

BrzVladNov 14, 2022

Uh oh!

BrzVlad commented Nov 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

BrzVlad commented Jul 23, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 23, 2022

Uh oh!

ghost commented Jul 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vargazJul 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

srxqds commented Aug 4, 2022

Uh oh!

marek-safar commented Sep 29, 2022

Uh oh!

BrzVlad commented Sep 29, 2022

Uh oh!

Uh oh!

marek-safarNov 14, 2022

Choose a reason for hiding this comment

Uh oh!

BrzVladNov 14, 2022

Choose a reason for hiding this comment

Uh oh!

BrzVlad commented Nov 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

BrzVlad commented Jul 23, 2022•
edited
Loading