Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wasm] Implement initial support for WasmImportLinkage in the AppBuilders #94615

Merged
merged 13 commits into from
Nov 21, 2023

Conversation

lewing
Copy link
Member

@lewing lewing commented Nov 10, 2023

In response to #93823 this is a rough partial fix for #94513, #86984, and #86985
With the default settings this works in wasi-wasm but emscripten will complain if you don't enable unresolved symbols on browser-wasm

Using

    [WasmImportLinkage]
    [DllImport("extism:host/user")]
    public static extern void do_something();

Will generate

(type $i64_i32_=>_i64 (func (param i64 i32) (result i64)))
 (type $i32_i64_i64_i32_=>_none (func (param i32 i64 i64 i32)))
 (import "wasi_snapshot_preview1" "sock_accept" (func $fimport$0 (param i32 i32 i32) (result i32)))
 (import "extism:host/user" "do_something" (func $fimport$1))
 (import "wasi_snapshot_preview1" "args_get" (func $fimport$2 (param i32 i32) (result i32)))

@lewing lewing added this to the 9.0.0 milestone Nov 15, 2023
@lewing lewing changed the title [wasi][wasm] Implement initial support for WasmImportLinkage to the AppBuilders [wasm] Implement initial support for WasmImportLinkage to the AppBuilders Nov 16, 2023
@lewing lewing added os-wasi Related to WASI variant of arch-wasm os-browser Browser variant of arch-wasm arch-wasm WebAssembly architecture labels Nov 16, 2023
@ghost
Copy link

ghost commented Nov 16, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

After #94615 merged this is a quick hacky fix for #94513, #86984, and #86985
With the default settings this works in wasi-wasm but emscripten will complain if you don't enable unresolved symbols on browser-wasm

Using

    [WasmImportLinkage]
    [DllImport("extism:host/user")]
    public static extern void do_something();

Will generate

(type $i64_i32_=>_i64 (func (param i64 i32) (result i64)))
 (type $i32_i64_i64_i32_=>_none (func (param i32 i64 i64 i32)))
 (import "wasi_snapshot_preview1" "sock_accept" (func $fimport$0 (param i32 i32 i32) (result i32)))
 (import "extism:host/user" "do_something" (func $fimport$1))
 (import "wasi_snapshot_preview1" "args_get" (func $fimport$2 (param i32 i32) (result i32)))
Author: lewing
Assignees: lewing
Labels:

arch-wasm, area-Build-mono, os-wasi, os-browser

Milestone: 9.0.0

@lewing
Copy link
Member Author

lewing commented Nov 16, 2023

added export support (edited to reflect the current state)

 
        [UnmanagedCallersOnly(EntryPoint = "display_meaning")]
        public static void DisplayMeaningExport(int meaning)
        {
            Console.WriteLine($"The meaning of life is {meaning}");
        }
 (export "display_meaning" (func $13484))

There is currently an issue with when invoking the export from unmanged when there is no managed delegate pointing to it that will not be dealt with in this PR. A temporary workaround is shown in the sample app

   Intptr workaround = (IntPtr)(delegate* unmanaged<int,void>)&display_meaning;  // needs to be in method loaded by interp prior to unmanaged call 

@SingleAccretion
Copy link
Contributor

added export support using WasmImportLinkage

The attribute was not meant to enable exports. They are meant to be enabled by simply using EntryPoint on UCO-adorned methods.

@just-ero
Copy link
Contributor

I apologize upfront if this is already up for consideration or an entirely inappropriate suggestion;

I think I would personally prefer if it were just

[WasmImport("extism:host/user")]
public static extern void do_something();

@lewing
Copy link
Member Author

lewing commented Nov 16, 2023

added export support using WasmImportLinkage

The attribute was not meant to enable exports. They are meant to be enabled by simply using EntryPoint on UCO-adorned methods.

I'm aware, this is explicitly, as mentioned in the description, a quick hack to test out the changes.

@lewing lewing marked this pull request as ready for review November 21, 2023 02:10
@RReverser
Copy link

Oh this is awesome. I just finished rewriting bindings in our project for .NET 8, looks like I'll have to rewrite them again, but this removes some of the hacks I had to do so it's a win.

How can one try this PR out? Will it be available via dotnet workload update for wasi-experimental or do I need to pull it explicitly somehow?

@lewing
Copy link
Member Author

lewing commented Nov 21, 2023

The packages will need to be built in an official build then flow through the darc update process (hours, sometimes longer) until they hit https://github.com/dotnet/installer then you should be able to follow the instructions on the front page to download the latest 9.0-alpha.1 build (make sure to add the nuget feeds too) and then the workload commands should work.

@lewing
Copy link
Member Author

lewing commented Nov 21, 2023

I apologize upfront if this is already up for consideration or an entirely inappropriate suggestion;

I think I would personally prefer if it were just

[WasmImport("extism:host/user")]
public static extern void do_something();

The API discussion is here #93824

@lewing lewing changed the title [wasm] Implement initial support for WasmImportLinkage to the AppBuilders [wasm] Implement initial support for WasmImportLinkage in the AppBuilders Nov 21, 2023
@RReverser
Copy link

then you should be able to follow the instructions on the front page to download the latest 9.0-alpha.1 build (make sure to add the nuget feeds too

Ah, in that case I'll have to wait I guess. I thought it might be possible to get wasi-experimental workload updates independently from .NET itself for production .NET 8 use too, not just via alpha channel.

@lewing
Copy link
Member Author

lewing commented Nov 22, 2023

There is the possibility that we could consider servicing some of these fixes in net8.0

@RReverser
Copy link

That would be amazing. Having to manually wrap imports/exports via custom C file is the biggest remaining hurdle for us after migrating from Wasi.Sdk to .NET 8 WASI support, and it's a shame that this fix juuuust didn't make the release cut.

As we're providing a sort of Wasm hosting, where we provide a project template and users do dotnet publish themselves, we can't ask all end users to install an alpha version of the SDK, so it's either hoping that improvements like this will be backported to .NET 8 channel, or waiting for next release version of the SDK again.

@silesmo
Copy link

silesmo commented Nov 22, 2023

There is the possibility that we could consider servicing some of these fixes in net8.0

That would be great!

@RReverser
Copy link

Btw, one more issue we were seeing is that for UnmanagedCallersOnly .NET complains that custom structs, even with fixed layout, are not blittable and can't be used as return type or arguments of UCO entry points.

Structs have well-defined ABI in Wasm C ABI, so this shouldn't be a problem and structs with fixed layout should be allowed like they are on other platforms.

At the very least it would be useful to allow a struct with a single field, since Wasm C ABI says those have the same ABI as their field. This is useful for creating typechecked handles where a struct just has a single integer field.

@lewing
Copy link
Member Author

lewing commented Nov 23, 2023

Btw, one more issue we were seeing is that for UnmanagedCallersOnly .NET complains that custom structs, even with fixed layout, are not blittable and can't be used as return type or arguments of UCO entry points.

Structs have well-defined ABI in Wasm C ABI, so this shouldn't be a problem and structs with fixed layout should be allowed like they are on other platforms.

At the very least it would be useful to allow a struct with a single field, since Wasm C ABI says those have the same ABI as their field. This is useful for creating typechecked handles where a struct just has a single integer field.

@kg has been looking into this I'd recommend opening an issue here where we can discuss it

@lewing
Copy link
Member Author

lewing commented Nov 23, 2023

There is the possibility that we could consider servicing some of these fixes in net8.0

That would be great!

Just to set expectations the official leading edge will always be main here and I can help you test against that but we may have flexibility in servicing targeted changes (like this one)

@jkotas
Copy link
Member

jkotas commented Nov 23, 2023

I think it is unlikely that addition of WasmImportLinkageAttribute would be approved for servicing. The default bar for servicing is bug fixes only. WasmImportLinkageAttribute is a new feature.

@lewing
Copy link
Member Author

lewing commented Nov 23, 2023

I think it is unlikely that addition of WasmImportLinkageAttribute would be approved for servicing. The default bar for servicing is bug fixes only. WasmImportLinkageAttribute is a new feature.

As written this change supports any Attribute named WasmImportLinkageAttribute it also supports valid uses of EntryPoint on UCO that were broken before (with user reports) and there are first party considerations and it is a build task specific to wasm, what are your concerns?

@silesmo
Copy link

silesmo commented Nov 23, 2023

Hello!
Am I missing something when trying to use this with the nightly dotnet9 alpha?
I have installed the alpha and then when I try to install the wasi-experimental workload I get the following:
image

@lewing
Copy link
Member Author

lewing commented Nov 23, 2023

Hello! Am I missing something when trying to use this with the nightly dotnet9 alpha? I have installed the alpha and then when I try to install the wasi-experimental workload I get the following: image

There have been several large changes recently (specifically around the aot cross compiler), we will take a look

cc @steveisok @radical

@lewing
Copy link
Member Author

lewing commented Nov 23, 2023

There have been several large changes recently (specifically around the aot cross compiler), we will take a look

cc @steveisok @radical

#95170

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono os-browser Browser variant of arch-wasm os-wasi Related to WASI variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants