[Mono] [swift-interop] Add support for reverse pinvoke argument lowering by jkurdek · Pull Request #104437 · dotnet/runtime

jkurdek · 2024-07-04T13:15:35Z

Adds support in Mono for passing Swift @frozen struct as arguments to UnmanagedCallersOnly callbacks. Changes do not include support for lowering return value structs. That will come in separate PR. Detailed description of struct lowering can be found here #102143.

Changes introduce struct lowering on IL level to native-to-managed wrapper. First, the wrapper's signature is modified according to the result of struct lowering algorithm. For structs passed by reference we replace the struct arg with a reference arg. For a lowered struct, struct argument is replaced by a series of up to 4 primitive arguments.

Then, during wrapper's IL code generation, additional code is emitted to load the new arguments at correct offsets.

Verified full AOT locally.

Contributes to #102077

jkurdek · 2024-07-08T12:15:50Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-07-08T12:16:12Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2024-07-08T21:12:22Z

Azure Pipelines successfully started running 1 pipeline(s).

src/mono/mono/metadata/marshal-lightweight.c

src/mono/mono/metadata/marshal.c

kotlarmilos · 2024-07-09T12:00:50Z

src/mono/mono/metadata/marshal.c

+		MonoClass *swift_self = mono_class_try_get_swift_self_class ();
+		MonoClass *swift_error = mono_class_try_get_swift_error_class ();
+		MonoClass *swift_indirect_result = mono_class_try_get_swift_indirect_result_class ();
+		swift_lowering = g_newa (SwiftPhysicalLowering, sig->param_count);


Where are these deallocated?

Good catch. Added deallocation for new_params. swift_lowering and swift_sig_to_csig_mp are allocated on stack, so we don't deallocate explicetely them right?

Yes. Why they are allocated differently?

swift_lowering and swift_sig_to_csig_mp are going to have exactly sig->param_count elements. In some places across marshalling code such arrays are allocated on stack.

runtime/src/mono/mono/metadata/marshal-lightweight.c

Line 2797 in f9eda07

tmp_locals = g_newa (int, sig->param_count);

swift_sig_to_csig_mp is an array of ints so I guess it ok. We could make the swift_lowering heap allocated, but I don't know whether this is necessary.

The new_params can be up to 4 times longer than theswift_lowering and swift_sig_to_csig_mp, as it includes the lowered params. So I following what was done in method-to-ir.

runtime/src/mono/mono/mini/method-to-ir.c

Line 7533 in f9eda07

GArray *new_params = g_array_sized_new (FALSE, FALSE, sizeof (MonoType*), n);

src/mono/mono/metadata/marshal.h

src/mono/mono/metadata/marshal-lightweight.c

matouskozak · 2024-07-10T12:39:24Z

src/mono/mono/metadata/marshal.c

+			}
+		}
+
+		csig = mono_metadata_signature_dup_new_params (NULL, NULL, get_method_image (method), csig, new_param_count, (MonoType**)new_params->data);


I'm unsure where we should allocate the new csig. For the direct p/invokes we allocate the temporary signature in mempool but I suppose this is not temporary so image is probably the right choice.

yea, I also think image is the right place.

hmm... actually... why not the mem manager of the method?

m_method_get_mem_manager (method). (That's usually the same as the image mempool, but for generic instances it will be the mempool of the whole set of images)

src/mono/mono/metadata/marshal-lightweight.c

lambdageek · 2024-07-10T15:41:56Z

src/mono/mono/metadata/marshal-lightweight.c

+			tmp_locals [i] = mono_emit_marshal (m, csig_argnum, t, mspecs [i + 1], 0,  &csig->params [csig_argnum], MARSHAL_ACTION_MANAGED_CONV_IN);
 		} else {
 			switch (t->type) {
+			case MONO_TYPE_VALUETYPE:


do you need a MONO_TYPE_GENERICINST case to handle SwiftSelf<T> or other generics?

I believe currently we do not plan to support SwiftSelf<T> in reverse pinvokes #103576 (comment)

cc: @kotlarmilos

#103576 (comment)

lambdageek · 2024-07-10T15:49:03Z

src/mono/mono/metadata/metadata.c

 */
 MonoMethodSignature*
-mono_metadata_signature_dup_new_params (MonoMemPool *mp, MonoMemoryManager *mem_manager, MonoMethodSignature *sig, uint32_t num_params, MonoType **new_params)
+mono_metadata_signature_dup_new_params (MonoMemPool *mp, MonoMemoryManager *mem_manager, MonoImage *image, MonoMethodSignature *sig, uint32_t num_params, MonoType **new_params)


I think we should try to move away from passing MonoImage* to allocation functions. you can pass the mempool of the image if you need to. or better to get the mem manager for the method definition, I think.

Reversed the change. Just for my education, why is passing MonoImage* a code smell?

lambdageek

mostly LGTM, but let's try to avoid adding a MonoImage* argument to the signature dup function. Using MonoImage* for allocation is a code smell.

Co-authored-by: Aleksey Kliger (λgeek) <[email protected]>

src/mono/mono/metadata/marshal.c

kotlarmilos

LGTM!

Add support for reverse pinvoke argument lowering

f4a08ad

jkurdek added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Jul 4, 2024

ghost added the area-VM-meta-mono label Jul 4, 2024

dotnet-policy-service bot assigned jkurdek Jul 4, 2024

This was referenced Jul 4, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

The job running on agent NetCore-Public ran longer than the maximum time #104044

Closed

jkurdek added 2 commits July 5, 2024 11:27

Fix ILGeneration tests errors

f113643

Fix build errors

ad59f6d

jkurdek force-pushed the reverse-pinvokes-args-lowering branch from 3fc4be0 to ad59f6d Compare July 5, 2024 11:37

Reworked structure reassemble algorithm to work on sig

d703585

This was referenced Jul 5, 2024

ProcessThreadTests.TestStartTimeProperty failed in CI #103448

Closed

[browser] test failure: Tests timed out. Killing driver service #103524

Open

jkurdek and others added 3 commits July 5, 2024 18:11

Fix x64 test errors

4af9745

Merge branch 'dotnet:main' into reverse-pinvokes-args-lowering

bbf5612

Add support for SwiftIndirectResult

b40a633

matouskozak mentioned this pull request Jul 8, 2024

Implement support for Swift frozen structs in Mono runtime #102077

Closed

6 tasks

jkurdek added 4 commits July 8, 2024 17:56

Remove duplicate call to lowering algorithm

6ebc9bd

Refactor emitting swift lowered struct

12ff9d8

Fix formatting

3912225

Remove extra newline

e6401cb

jkurdek changed the title ~~Add support for reverse pinvoke argument lowering~~ [Mono] [swift-interop] Add support for reverse pinvoke argument lowering Jul 8, 2024

Fix SwiftIndirectResult errors

f95d478

jkurdek removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Jul 8, 2024

kotlarmilos reviewed Jul 9, 2024

View reviewed changes

Address review comments

9e1f6e2

matouskozak reviewed Jul 10, 2024

View reviewed changes

src/mono/mono/metadata/marshal-lightweight.c Outdated Show resolved Hide resolved

Fix loop indent

b7e8f35

build-analysis bot mentioned this pull request Jul 10, 2024

[browser][MT] System.Net.Http.Functional.Tests failure ("Why are we setting the target on an unoccupied slot?") #104524

Open

matouskozak reviewed Jul 10, 2024

View reviewed changes

lambdageek reviewed Jul 10, 2024

View reviewed changes

src/mono/mono/metadata/marshal-lightweight.c Outdated Show resolved Hide resolved

lambdageek reviewed Jul 10, 2024

View reviewed changes

jkurdek and others added 2 commits July 11, 2024 09:51

Update src/mono/mono/metadata/marshal-lightweight.c

a176c16

Co-authored-by: Aleksey Kliger (λgeek) <[email protected]>

Remove passing MonoImage

cb449f9

BrzVlad reviewed Jul 11, 2024

View reviewed changes

src/mono/mono/metadata/marshal.c Outdated Show resolved Hide resolved

BrzVlad approved these changes Jul 11, 2024

View reviewed changes

jkurdek added 2 commits July 12, 2024 10:37

Fix formatting around conditional blocks

022eab3

Fix missing whitespace

96a2cb9

kotlarmilos self-requested a review July 12, 2024 14:36

kotlarmilos approved these changes Jul 12, 2024

View reviewed changes

Merge branch 'main' into reverse-pinvokes-args-lowering

903d7b6

build-analysis bot mentioned this pull request Jul 15, 2024

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

2 tasks

jkurdek merged commit 9d24b13 into dotnet:main Jul 15, 2024

jkurdek deleted the reverse-pinvokes-args-lowering branch July 15, 2024 13:40

github-actions bot locked and limited conversation to collaborators Aug 15, 2024

Conversation

jkurdek commented Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkurdek commented Jul 8, 2024

Uh oh!

azure-pipelines bot commented Jul 8, 2024

Uh oh!

azure-pipelines bot commented Jul 8, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdageek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kotlarmilos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jkurdek commented Jul 4, 2024 •

edited

Loading