[wasm] p/invoke and interop improvements #94446

kg · 2023-11-07T02:29:31Z

Refactor logging in the wasm build tasks to use a LogAdapter helper class that can either write to stdout/stderr or via the msbuild logging APIs
Refactor ManagedToNativeGenerator so that it can run in either in-process or out-of-process modes (but it still runs in-process right now). This will enable us to pivot to out-of-process in order to pick up function pointer support later on.
Add support for function pointer types via reflection (so that we can still build and run on net4x for visual studio). Right now the underlying libraries are too old for this support to work, but once the libraries are updated it will.
Add basic support for structs as arguments
Implement scalar struct behavior from the wasm C ABI (structs containing one scalar are passed by-value instead of by-reference) in various places in the build tasks, interpreter, and AOT compiler
Remove a use of varargs pinvoke from SystemNative because it isn't compatible with the WASM C ABI (it's replaced with two non-varargs pinvokes)
Generate more accurate C signatures for pinvokes

kg · 2023-11-07T02:30:03Z

Checkpoint Checkpoint Checkpoint Update tests; make fn pointers explicitly blittable on net8 Checkpoint Checkpoint Update test assertions to handle out-of-process line breaking Csproj no longer needed Repair merge damage Blittable structs Fix broken WBT tests Checkpoint wasm ABI Wasm ABI in SignatureMapper Wasm ABI fixes Add test covering most of the weird parts of the wasm ABI (doesn't currently pass) Checkpoint (this is broken) Revert transform.c changes Split test into both AOT and interp variants Checkpoint Checkpoint: scalarized calls seem to work now but the return value causes crashes Update build_args_from_sig to properly support scalarized struct parameters on wasm Fix scalarized struct return values in interp Add missing file

…chitectures, so change to an approach that always works

… etc

kg · 2024-01-18T21:25:53Z

It looks like SBRP hasn't updated since Jan 5. Is there something I need to do to make the flow happen? Should I just manually do it in this PR? #96989 happened, which I thought would have updated it to a more recent commit, but it didn't. @mthalman

EDIT: Looking closer, it looks like the timing was just slightly off, so it missed two deps updates and my PR in SBRP, so I guess that flow was correct.

kg · 2024-02-02T19:10:33Z

@BrzVlad @kotlarmilos @vargaz Any of you feel comfortable reviewing the interp/type system changes here? They're nuanced enough I want to make sure someone with expertise looks them over.

kotlarmilos

LGTM!

kotlarmilos · 2024-02-05T10:56:55Z

src/mono/mono/mini/mini-llvm.c

@@ -4948,6 +4948,8 @@ process_call (EmitContext *ctx, MonoBasicBlock *bb, LLVMBuilderRef *builder_ref,
 		if (!addresses [call->inst.dreg])
 			addresses [call->inst.dreg] = build_alloca_address (ctx, sig->ret);
 		emit_store (builder, lcall, convert_full (ctx, addresses [call->inst.dreg]->value, pointer_type (LLVMTypeOf (lcall)), FALSE), is_volatile);
+		load_name = "wasm_vtype_as_scalar";


Should this be WASM only?

src/mono/mono/mini/aot-runtime-wasm.c

src/mono/mono/mini/mini-llvm.c

src/mono/mono/mini/interp/interp.c

Address PR feedback

lewing · 2024-02-09T02:09:42Z

src/native/libs/System.Native/pal_string.h

+/**
+ * Two specialized overloads for use from Interop.Sys, because these two signatures are not equivalent
+ * on some architectures (like 64-bit WebAssembly)
+*/
+
+PALEXPORT int32_t SystemNative_SNPrintF_1S(char* string, int32_t size, const char* format, char* str);
+
+PALEXPORT int32_t SystemNative_SNPrintF_1I(char* string, int32_t size, const char* format, int arg);


@AaronRobinsonMSFT how did you handle this in runtimelab ?

This is the correct way to handle this scenario. Native varargs at the interop boundary have narrow support in .NET and are exclusive to the Windows platform and typically only for the C++/CLI scenario. See #48796 for broader support.

The approach taken here is to provide an unmanaged export that managed code can call safely which then forwards to a native varargs function. This ensures the unmanaged calling convention is handled correctly during the P/Invoke and correctly at the dispatch to the unmanaged varargs.

It is ugly and not what anyone really wants, but the cost of supporting native varargs is expensive to support. This comes up rarely relative to other interop scenarios so just hasn't been prioritized. The aforementioned issue mentions some of the costs and relevant parties that would need to respond to support.

Makes perfect sense, just wanted to verify.

lewing · 2024-02-09T02:40:13Z

@kg please double check the conflict resolution with https://github.com/dotnet/runtime/pull/97643/files#r1483684742 in mind

kg · 2024-02-09T02:50:15Z

@kg please double check the conflict resolution with https://github.com/dotnet/runtime/pull/97643/files#r1483684742 in mind

Looks right to me, and tests should fail if it's wrong. Thanks for sorting out the merge

kg · 2024-02-09T22:17:26Z

S.T.J failures are known, merging

am11 · 2024-02-10T13:43:55Z

eng/Versions.props

    <SystemComponentModelAnnotationsVersion>5.0.0</SystemComponentModelAnnotationsVersion>
    <SystemDataSqlClientVersion>4.8.6</SystemDataSqlClientVersion>
    <SystemDrawingCommonVersion>8.0.0</SystemDrawingCommonVersion>
    <SystemIOFileSystemAccessControlVersion>5.0.0</SystemIOFileSystemAccessControlVersion>
    <SystemMemoryVersion>4.5.5</SystemMemoryVersion>
    <SystemReflectionMetadataVersion>9.0.0-alpha.1.24072.1</SystemReflectionMetadataVersion>
    <!-- Keep toolset versions in sync with dotnet/msbuild and dotnet/sdk -->
-    <SystemReflectionMetadataToolsetVersion>7.0.0</SystemReflectionMetadataToolsetVersion>
-    <SystemReflectionMetadataLoadContextVersion>7.0.0</SystemReflectionMetadataLoadContextVersion>
+    <SystemReflectionMetadataToolsetVersion>8.0.0</SystemReflectionMetadataToolsetVersion>


Based on the comment above, this toolset version should not be updated without coordination with msbuild and sdk repos.

@ViktorHofer, FYI, this is blocking the codeflow dotnet/sdk#38665 (while previous change made it to the next phases without problems dotnet/sdk#38553). We can either revert this or update https://github.com/dotnet/sdk/blob/b32df9afff77e8f1afbd417e7c70841867cb6cdc/src/Tasks/Microsoft.NET.Build.Tasks/Microsoft.NET.Build.Tasks.csproj#L75 to 8.0 (if we choose the latter, we can close #96809).

I coordinated with people from the msbuild team at multiple points while developing this along with the SBRP team, so I'm sorry that something fell through the cracks here. Is there a specific step we missed?

I see now from looking through the replies to Larry's comment on this issue that you told us there. For some reason, GitHub did not send me a notification for your comment, so I didn't see it :( I guess in the future blocking issues like that need to be PR comments instead of replies.

My preference would definitely be to take the bump in the sdk. VS 17.9 is where these changes will ship and it has taken the update.

cc @ViktorHofer @marcpopMSFT

NP, #96809 is tracking it, but it seems (almost) resolved by your PR. Once confirmed, we can just update sdk's Microsoft.NET.Build.Tasks.csproj#L74-L76 to 8.0 (now that all the related toolings are using 8.0; msbuild, HostModel and VS).

I think for this to work, sdk would need to upgrade its .NET Framework leg to a VS 17.9 Preview image (for the updated msbuild binding redirects that allow to unify to an 8.0.0.0 assembly version). I'm not sure if such an image is already available. Worst case, we would need to revert this dependency bump here. @lewing can you please make sure that this dependency flow issue gets resolved: dotnet/sdk#38665?

Correct me if I'm wrong but the sdk build should be fine, it'd just need the testing legs to use VS17.9 Preview right? That should already be the case: https://github.com/dotnet/sdk/blob/0962c1f89f5daf924a9fe876c80e80b0bde63b0d/.vsts-ci.yml#L108-L111

I'll try bumping the .csproj in the sdk PR and see what happens.

Correct. Binding redirect unification at run time.

Looks like it worked :)

ghost assigned kg Nov 7, 2023

dotnet-issue-labeler bot added the area-Build-mono label Nov 7, 2023

This was referenced Nov 16, 2023

[wasm][debugger] Debugger tests getting randomly aborted on CI #89409

Open

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

kg mentioned this pull request Nov 17, 2023

[wasm] Wasm C ABI correctness for "singleton" structs in the mono AOT compiler and interpreter #94895

Open

build-analysis bot mentioned this pull request Nov 18, 2023

Tracking issue for CI build timeouts #76454

Closed

kg added 2 commits November 20, 2023 17:48

Repair merge damage

86b5cdd

kg force-pushed the pinvoke-srm-1 branch from 13f22bf to 86b5cdd Compare November 21, 2023 03:53

lewing mentioned this pull request Nov 21, 2023

[mono][wasm] wasm interop tracking #95084

Closed

7 tasks

Merge branch 'main' into pinvoke-srm-1

a355079

build-analysis bot mentioned this pull request Nov 27, 2023

[mono][tvos] OOM in System.IO.Tests.MemoryStreamTests #92467

Closed

Run M2NG in-process again

13157f3

build-analysis bot mentioned this pull request Nov 29, 2023

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

Open

2 tasks

kg added 11 commits November 29, 2023 18:34

Our current approach to SystemNative_SNPrintF is incorrect on some ar…

11c3d7a

…chitectures, so change to an approach that always works

Support initonly fields in blittable structs in PInvokeTableGenerator…

0d413b2

… etc

Merge remote-tracking branch 'upstream/main' into pinvoke-srm-1

a46ddd9

Work around our rampant use of warnaserror

51274ed

Revert most csproj changes

bf9439d

Better solution for warnaserror (probably)

6f49b91

Restore old EnumCalendarInfo since we're in-process again

c25be75

Revert fn ptr test changes

65275b6

Maybe fix blazor WBTs

f47f8ea

Make LogAdapter Info high priority so it appears in failed build logs

835e278

Hack to make skiasharp build

d8ac1a9

build-analysis bot mentioned this pull request Dec 5, 2023

[wasm] Playwright failing #94240

Closed

Merge remote-tracking branch 'upstream/main' into pinvoke-srm-1

f40b16a

This was referenced Dec 6, 2023

Crash in Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.WorkItemExecution #90019

Open

[8.0] Failing to build native components #94823

Closed

This was referenced Jan 18, 2024

BasicEventSourceTests.TestsManifestGeneration test failing in CI #97103

Closed

Tests crashing in CI with no dump: exit code 137 means SIGKILL Killed #97049

Closed

Merge remote-tracking branch 'upstream/main' into pinvoke-srm-1

c6a809a

kg requested review from maraf, akoeplinger and ilonatommy as code owners January 24, 2024 23:42

Merge remote-tracking branch 'upstream/main' into pinvoke-srm-1

47478c5

kotlarmilos reviewed Feb 5, 2024

View reviewed changes

vargaz reviewed Feb 5, 2024

View reviewed changes

src/mono/mono/mini/aot-runtime-wasm.c Outdated Show resolved Hide resolved

vargaz reviewed Feb 5, 2024

View reviewed changes

src/mono/mono/mini/mini-llvm.c Outdated Show resolved Hide resolved

vargaz reviewed Feb 5, 2024

View reviewed changes

src/mono/mono/mini/interp/interp.c Outdated Show resolved Hide resolved

kg added 3 commits February 5, 2024 11:51

Merge remote-tracking branch 'upstream/main' into pinvoke-srm-1

2a57464

Address PR feedback

b1e64a0

Repair merge damage

1ba9222

Address PR feedback

lewing reviewed Feb 9, 2024

View reviewed changes

Merge branch 'main' into pinvoke-srm-1

5f75189

lewing approved these changes Feb 9, 2024

View reviewed changes

kg merged commit ed9f475 into dotnet:main Feb 9, 2024
188 of 191 checks passed

am11 reviewed Feb 10, 2024

View reviewed changes

ViktorHofer mentioned this pull request Feb 12, 2024

[main] Update dependencies from dotnet/runtime dotnet/sdk#38665

Merged

akoeplinger mentioned this pull request Feb 12, 2024

Update pinned System.Reflection.Metadata and System.Collections.Immutable versions to 8.0.0 #96809

Closed

github-actions bot locked and limited conversation to collaborators Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm] p/invoke and interop improvements #94446

[wasm] p/invoke and interop improvements #94446

kg commented Nov 7, 2023 •

edited

Loading

kg commented Nov 7, 2023

kg commented Jan 18, 2024 •

edited

Loading

kg commented Feb 2, 2024

kotlarmilos left a comment

kotlarmilos Feb 5, 2024

lewing Feb 9, 2024

AaronRobinsonMSFT Feb 9, 2024

lewing Feb 9, 2024

lewing commented Feb 9, 2024

kg commented Feb 9, 2024

kg commented Feb 9, 2024

am11 Feb 10, 2024

kg Feb 10, 2024

kg Feb 10, 2024

lewing Feb 10, 2024 •

edited

Loading

am11 Feb 10, 2024

ViktorHofer Feb 11, 2024 •

edited

Loading

akoeplinger Feb 12, 2024

ViktorHofer Feb 12, 2024 •

edited

Loading

akoeplinger Feb 12, 2024

[wasm] p/invoke and interop improvements #94446

[wasm] p/invoke and interop improvements #94446

Conversation

kg commented Nov 7, 2023 • edited Loading

kg commented Nov 7, 2023

kg commented Jan 18, 2024 • edited Loading

kg commented Feb 2, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewing commented Feb 9, 2024

kg commented Feb 9, 2024

kg commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewing Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kg commented Nov 7, 2023 •

edited

Loading

kg commented Jan 18, 2024 •

edited

Loading

lewing Feb 10, 2024 •

edited

Loading

ViktorHofer Feb 11, 2024 •

edited

Loading

ViktorHofer Feb 12, 2024 •

edited

Loading