[mono] Add SwiftError support for Swift reverse pinvokes #101122

jkurdek · 2024-04-16T15:27:29Z

Adds support for SwiftError in Swift reverse pinvokes. With this changes basic pinvokes using primitive types and SwiftSelf/SwiftError arguments should work. Contributes to #100010

Works on arm64/amd64.

kotlarmilos

Thanks, looks great! Do you plan to include the amd64 support in this PR?

kotlarmilos · 2024-04-16T15:33:45Z

src/mono/mono/mini/mini-arm64.c

+			code = emit_strx (code, ARMREG_R21, ARMREG_IP0, 0);
+		} else if (cfg->method->wrapper_type == MONO_WRAPPER_NATIVE_TO_MANAGED) {
+			code = emit_ldrx (code, ARMREG_R21, cfg->arch.swift_error_var->inst_basereg, GTMREG_TO_INT (cfg->arch.swift_error_var->inst_offset));
+			code = emit_ldrx (code, ARMREG_R21, ARMREG_R21, 0);


Please add a comment here for clarity.

kotlarmilos · 2024-04-16T15:40:27Z

src/mono/mono/mini/mini-arm64.c

+				}
+				if (cfg->method->wrapper_type == MONO_WRAPPER_NATIVE_TO_MANAGED) {
+					code = emit_ldrx (code, ARMREG_IP0, cfg->arch.swift_error_var->inst_basereg, GTMREG_TO_INT (cfg->arch.swift_error_var->inst_offset));
+					code = emit_strx (code, ARMREG_R21, ARMREG_IP0, 0);


Does this align with the CoreCLR implementation? It seems intuitive to load swifterror reg into the SwiftError argument, but I wonder if such cases are expected.

For the CoreCLR implementation, RyuJIT does not load the error register's current value into the SwiftError arg upon method entry. The JIT implementation represents the error value by creating a SwiftError "pseudo-local", and converts all usages of the SwiftError* out parameter into usages of the local's address (see #100692). This approach allows us to be quite liberal in optimizing uses of the SwiftError* parameter (for example, we can promote the SwiftError::Value member to its own local, and enregister it), but with our implementation, I believe the initial error value upon method entry is undefined.

@kotlarmilos do we want to remove loading swifterror reg into the SwiftError to align?

I believe it's beneficial to align with the CoreCLR implementation. However, since it is undefined and if there are no performance implications, we can leave it as is.

amanasifkhalid

LGTM so far -- thanks!

amanasifkhalid · 2024-04-16T16:06:53Z

src/mono/mono/mini/mini-arm64.c

@@ -2887,7 +2887,8 @@ mono_arch_allocate_vars (MonoCompile *cfg)
 			offset += size;

 			cfg->arch.swift_error_var = ins;
-			cfg->used_int_regs |= 1 << ARMREG_R21;
+			if (cfg->method->wrapper_type == MONO_WRAPPER_MANAGED_TO_NATIVE)
+ 				cfg->used_int_regs |= 1 << ARMREG_R21;


It might be helpful to explain here that in the NATIVE_TO_MANAGED case, the error register is functioning as an extra return register, and thus isn't treated as callee-save.

matouskozak · 2024-04-18T08:17:14Z

Good job! Note for the CI test coverage, we have a good coverage for osx-x64 (mini and interpreter) on the runtime pipeline. For arm64, we only have interpreter coverage running on runtime-extra-platforms pipeline.

kotlarmilos · 2024-04-25T10:09:53Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-04-25T10:10:12Z

Azure Pipelines successfully started running 1 pipeline(s).

kotlarmilos

LGTM!

kotlarmilos · 2024-04-25T13:14:21Z

src/mono/mono/mini/mini-amd64.c

 					offset = ALIGN_TO (offset, sizeof (target_mgreg_t));
+					ins->inst_basereg = cfg->frame_reg;
+					ins->inst_offset = offset;


Why it doesn't include the ARGS_OFFSET offset between fp and the first argument in the callee?

ainfo->offset was always 0 here. So it was always allocated at ARGS_OFFSET from the beginning. I think that offset already includes information about the distance.

The Swift types are specified either at the beginning or the end of signature. We don't enforce strict signature rules, but it would be beneficial to make them more general if possible. Can it still function if SwiftError is specified as the last argument, using ainfo->offset instead of offset?

@amanasifkhalid, what are the limitations from the CoreCLR side?

We don't enforce any signature rules, either. The Swift special register types can appear anywhere in the signature's parameter list.

@kotlarmilos i think for that to work with ainfo->offset without any modifications, the error would have to be the first argument. We could probably update ainfo->offset in get_call_info so that it works for every position. I'm not sure what are the benefits of using ainfo->offset though, as the current solution works for every position.

Ok, sounds good. I forgot that the ainfo->offset is always 0 for swifterror in the get_call_info.

jkurdek · 2024-04-26T10:14:21Z

@lambdageek could you take a look?

lambdageek

looks good to me. although by this point you and Milos know way more about how the calling convention code works.

* initialized swift reverse pinvokes for mini jit arm64 * added comments * fixed reverse pinvokes error passing on arm64 * implemented swift reverse pinvoke error passing on amd64 * disable SwiftErrorHandling tests on mono interpreter for now * add comments

initialized swift reverse pinvokes for mini jit arm64

94ae2a4

jkurdek requested a review from kotlarmilos April 16, 2024 15:27

dotnet-issue-labeler bot added the area-VM-meta-mono label Apr 16, 2024

dotnet-policy-service bot assigned jkurdek Apr 16, 2024

kotlarmilos reviewed Apr 16, 2024

View reviewed changes

kotlarmilos requested review from matouskozak and amanasifkhalid April 16, 2024 15:41

amanasifkhalid reviewed Apr 16, 2024

View reviewed changes

This was referenced Apr 16, 2024

[wasm] System.Text.Json.Tests running out of memory #98578

Closed

[browser][MT] TIMED_OUT but 0 was expected #99888

Closed

System.Text.Json failing some large file tests #59678

Closed

added comments

8f8a71a

jkurdek changed the title ~~[Mono] Add basic support for Swift reverse pinvokes~~ [mono] Add SwiftError support for Swift reverse pinvokes Apr 17, 2024

Merge branch 'main' into swift-reverse-pinvokes

74c708a

This was referenced Apr 17, 2024

Timeout in System.Text.Json.Tests.Utf8JsonWriterTests.WriteNumbers #101193

Open

[browser][MT] RuntimeError: memory access out of bounds #96546

Open

jkurdek added 2 commits April 24, 2024 23:55

fixed reverse pinvokes error passing on arm64

97003cd

implemented swift reverse pinvoke error passing on amd64

1ba2a83

jkurdek marked this pull request as ready for review April 25, 2024 08:36

jkurdek requested review from lambdageek and steveisok as code owners April 25, 2024 08:36

build-analysis bot mentioned this pull request Apr 25, 2024

[WASI][AOT] LLVM ERROR: out of memory #101533

Open

disable SwiftErrorHandling tests on mono interpreter for now

32455a4

add comments

6e7620d

kotlarmilos approved these changes Apr 25, 2024

View reviewed changes

lambdageek reviewed May 6, 2024

View reviewed changes

jkurdek merged commit 5e949e1 into dotnet:main May 6, 2024
122 of 127 checks passed

kotlarmilos mentioned this pull request May 10, 2024

[mono] Implement UnmanagedCallersOnly support for Swift calling convention #100010

Closed

2 tasks

github-actions bot locked and limited conversation to collaborators Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mono] Add SwiftError support for Swift reverse pinvokes #101122

[mono] Add SwiftError support for Swift reverse pinvokes #101122

jkurdek commented Apr 16, 2024 •

edited

kotlarmilos left a comment

kotlarmilos Apr 16, 2024

kotlarmilos Apr 16, 2024

amanasifkhalid Apr 16, 2024 •

edited

jkurdek Apr 17, 2024

kotlarmilos Apr 17, 2024

amanasifkhalid left a comment

amanasifkhalid Apr 16, 2024

matouskozak commented Apr 18, 2024 •

edited

kotlarmilos commented Apr 25, 2024

azure-pipelines bot commented Apr 25, 2024

kotlarmilos left a comment

kotlarmilos Apr 25, 2024 •

edited

jkurdek Apr 25, 2024

kotlarmilos Apr 25, 2024

amanasifkhalid Apr 25, 2024

jkurdek Apr 25, 2024 •

edited

kotlarmilos Apr 26, 2024 •

edited

jkurdek commented Apr 26, 2024

lambdageek left a comment

[mono] Add SwiftError support for Swift reverse pinvokes #101122

[mono] Add SwiftError support for Swift reverse pinvokes #101122

Conversation

jkurdek commented Apr 16, 2024 • edited

kotlarmilos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanasifkhalid Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanasifkhalid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matouskozak commented Apr 18, 2024 • edited

kotlarmilos commented Apr 25, 2024

azure-pipelines bot commented Apr 25, 2024

kotlarmilos left a comment

Choose a reason for hiding this comment

kotlarmilos Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkurdek Apr 25, 2024 • edited

Choose a reason for hiding this comment

kotlarmilos Apr 26, 2024 • edited

Choose a reason for hiding this comment

jkurdek commented Apr 26, 2024

lambdageek left a comment

Choose a reason for hiding this comment

jkurdek commented Apr 16, 2024 •

edited

amanasifkhalid Apr 16, 2024 •

edited

matouskozak commented Apr 18, 2024 •

edited

kotlarmilos Apr 25, 2024 •

edited

jkurdek Apr 25, 2024 •

edited

kotlarmilos Apr 26, 2024 •

edited