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

[API Proposal]: WasmImportLinkageAttribute to control wasm module names #93824

Open
AaronRobinsonMSFT opened this issue Oct 22, 2023 · 20 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices community-contribution Indicates that the PR has been added by a community member
Milestone

Comments

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Oct 22, 2023

Background and motivation

This PR proposes a new attribute to be used to control the Wasm import module and function names. In Wasm when a function is imported it can be specified as coming from a named module using this WAT

(import "hellowasi" "reverse" (func $reverse (type 4)))

The first use of this would be in the experimental NativeAOT-LLVM backend, but the desire is to have something that works for Mono and NativeAOT-LLVM. Something that we can use to proceed with Wasm Components and WIT binding via source gen for both Mono and NativeAOT-LLVM.

DllImportAttribute has a Value and EntryPoint which can be used to define the names, but to preserve existing semantics including static linking for DllImport("*") and DirectPInvoke we cannot simply say that we should always create a Wasm import if the DllImport Value is not *. The presence of this new WasmImportLinkage will distinguish these use cases and the attribute has no properties.

Please see dotnet/runtimelab#2414 for how we got here and in particular, dotnet/runtimelab#2414 (comment).

Other issues and PRs of note on this subject:

dotnet/runtimelab#1390
dotnet/runtimelab#1845
dotnet/runtimelab#2410
dotnet/runtimelab#2383

Proposed PR #93823

API Proposal

namespace System.Runtime.InteropServices;

[AttributeUsage(AttributeTargets.Method, Inherited = false)]
public sealed class WasmImportLinkageAttribute : Attribute
{
    public WasmImportLinkageAttribute() { }
}

API Usage

[WasmImportLinkage]
[DllImport("library")]
static extern void Func();

Alternative Designs

It has been noted (dotnet/runtimelab#2414 (comment)) that the original WasmImport suggestion was a potentially confusing name as the Import suffix can infer it's use should be as an alternative to DllImport but this is not the case, so some other alternatives that I could think of:

WasmDynamicLinkAttribute
WasmImportLinkageAttribute - Updated this proposal
WasmModuleLinkAttribute
LlvmWasmImportAttribute (not good for Mono interpreter perhaps)
WasmNamedModuleAttribute

Risks

No response

@AaronRobinsonMSFT AaronRobinsonMSFT added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 22, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 22, 2023
@ghost
Copy link

ghost commented Oct 22, 2023

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

Issue Details

Background and motivation

This PR proposes a new attribute to be used to control the Wasm import module and function names. In Wasm when a function is imported it can be specified as coming from a named module using this WAT

(import "hellowasi" "reverse" (func $reverse (type 4)))

The first use of this would be in the experimental NativeAOT-LLVM backend, but the desire is to have something that works for Mono and NativeAOT-LLVM. Something that we can use to proceed with Wasm Components and WIT binding via source gen for both Mono and NativeAOT-LLVM.

DllImportAttribute has a Value and EntryPoint which can be used to define the names, but to preserve existing semantics including static linking for DllImport("*") and DirectPInvoke we cannot simply say that we should always create a Wasm import if the DllImport Value is not *. The presence of this new WasmImport will distinguish these use cases and the attribute has no properties.

Please see dotnet/runtimelab#2414 for how we got here and in particular, dotnet/runtimelab#2414 (comment).

Other issues and PRs of note on this subject:

dotnet/runtimelab#1390
dotnet/runtimelab#1845
dotnet/runtimelab#2410
dotnet/runtimelab#2383

API Proposal

namespace System.Runtime.InteropServices;

[AttributeUsage(AttributeTargets.Method, Inherited = false)]
public sealed class WasmImportAttribute : Attribute
{
    public WasmImportAttribute() { }
}

API Usage

[WasmImport]
[DllImport("library")]
static extern void Func();

Alternative Designs

It has been noted (dotnet/runtimelab#2414 (comment)) that WasmImport is a potentially confusing name as the Import suffix can infer it's use should be as an alternative to DllImport but this is not the case, so some other alternatives that I could think of:

WasmDynamicLink
WasmImportLink
WasmModuleLink
LlvmWasmImport (not good for Mono interpreter perhaps)
WasmNamedModule

Risks

No response

Author: AaronRobinsonMSFT
Assignees: -
Labels:

api-suggestion, area-System.Runtime.InteropServices

Milestone: -

@AaronRobinsonMSFT AaronRobinsonMSFT added community-contribution Indicates that the PR has been added by a community member and removed untriaged New issue has not been triaged by the area owner labels Oct 22, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 9.0.0 milestone Oct 22, 2023
@AaronRobinsonMSFT
Copy link
Member Author

/cc @yowl

@AaronRobinsonMSFT
Copy link
Member Author

/cc @lambdageek @pavelsavara @dotnet/nativeaot-llvm

@lewing
Copy link
Member

lewing commented Oct 22, 2023

cc @maraf @radekdoulik @ivanpovazan

@AaronRobinsonMSFT
Copy link
Member Author

My preference would be WasmImportLinkAttribute. I like that because it has the WasmImport prefix which calls to mind DllImport, followed by the Link, which helps describe its practical utility.

@jkoritzinsky
Copy link
Member

2 thoughts:

If (as mentioned in the runtimelab issue and comments) this attribute is more similar to DllImportSearchPathsAttrbute, would we want to consider a form that could be applied at a type/module/assembly level as well where the module name is specified in the attribute?

Not really API Review related, but would we want to have LibraryImport automatically propagate this attribute to the inner DllImport? My initial thought is yes, but just wanted to cover my bases.

@SingleAccretion
Copy link
Contributor

If (as mentioned in the runtimelab issue and comments) this attribute is more similar to DllImportSearchPathsAttrbute, would we want to consider a form that could be applied at a type/module/assembly level as well where the module name is specified in the attribute?

We don't have a concrete need for that right now, I suppose.

One of the uses for the attribute would be in generated source code, where I don't think the assembly-wide version could be used due to its potential impact on other PI declarations.

Not really API Review related, but would we want to have LibraryImport automatically propagate this attribute to the inner DllImport? My initial thought is yes, but just wanted to cover my bases.

Correct, this would need to be propagated.

I don't have preferences on naming aside from "WASM import" staying as that terminology is used by the WASM specification and related tooling.

@jkotas
Copy link
Member

jkotas commented Oct 22, 2023

WasmImportLinkAttribute

WasmImportLinkageAttribute? (Inspired by extern "C" linkage }

a form that could be applied at a type/module/assembly level as well where the module name is specified in the attribute

I do not think we need this. Similar interop attributes (e.g. UnmanagedCallConvAttribute) do not have it either. DefaultDllImportSearchPathsAttribute is an exception and it only allows Method and Assembly targets.

@yowl
Copy link
Contributor

yowl commented Oct 22, 2023

My preference would be WasmImportLinkAttribute. I like that because it has the WasmImport prefix which calls to mind DllImport, followed by the Link, which helps describe its practical utility.

I agree it is one of the better suggestions.

@AaronRobinsonMSFT
Copy link
Member Author

WasmImportLinkAttribute

WasmImportLinkageAttribute? (Inspired by extern "C" linkage }

+1

@AaronRobinsonMSFT
Copy link
Member Author

Unless there is any other discussion or thoughts, I am going to move this to ready for approval? Any concerns?

/cc @yowl @jkotas @pavelsavara @SingleAccretion

@SingleAccretion
Copy link
Contributor

Semantics LGTM. Are we unified on the name - WasmImportAttribute or WasmImportLinkageAttribute?

(My preference is towards the latter)

@AaronRobinsonMSFT
Copy link
Member Author

(My preference is towards the latter)

Same. I updated the proposal and bolded this alternative. I will be in the API review and push in that direction unless there is concern here. Also I am happy to update the proposal after I hear from @yowl.

@yowl
Copy link
Contributor

yowl commented Oct 26, 2023

All good here, WasmImportLinkageAttribute is good for me.

@AaronRobinsonMSFT AaronRobinsonMSFT added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 26, 2023
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title [API Proposal]: WasmImportAttribute to control wasm module names [API Proposal]: WasmImportLinkageAttribute to control wasm module names Oct 26, 2023
@terrajobst
Copy link
Member

terrajobst commented Nov 7, 2023

Video

  • According to this comment it seems we decided that P/Invokes without this attribute would fail. Why don't we just make (1) behave like (3)? In other words, what does this attribute add?
  • If there is a good reason, we're OK with the proposed name and shape, but it would be nice if we didn't need it.
namespace System.Runtime.InteropServices;

[AttributeUsage(AttributeTargets.Method, Inherited = false)]
public sealed class WasmImportLinkageAttribute : Attribute
{
    public WasmImportLinkageAttribute();
}

@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 7, 2023
@SingleAccretion
Copy link
Contributor

SingleAccretion commented Nov 7, 2023

Why don't we just make (1) behave like (3)?

Unresolved WASM imports in a binary will lead to a fail fast on startup on some WASM hosts. It means that a dynamically unreachable but unresolvable (say, because it is for a different platform) PInvoke in some NuGet library would lead to a fail fast by default.

On the other hand, PNSEs are lazy, and so do not have this problem. That is the essential difference between 1 and 3.

@AaronRobinsonMSFT
Copy link
Member Author

Unresolved WASM imports in a binary will lead to a fail fast on some WASM hosts.

So this is dependent on what WASM host is being used? Can you share an example of each?

@SingleAccretion
Copy link
Contributor

So this is dependent on what WASM host is being used?

Correct.

Can you share an example of each?

  1. Web, aka Emscripten, is able to generate trapping stubs for unresolved imports. What they do cannot be controlled from managed code (e. g. they cannot throw managed exceptions), but they do avoid the fail fast.
  2. wasmtime: fail fast by default, has an option to generate unconditional traps similar to 1.
  3. wasmer: fail fast without the above option (to the best of my knowledge).

@jkotas
Copy link
Member

jkotas commented Nov 7, 2023

According to dotnet/runtimelab#2414 (comment) it seems we decided that P/Invokes without this attribute would fail.

Also, the P/Invokes can be ambiguous. We can have situations when there is both native library and wasm module of the same name. We would not be able to tell whether the PInvoke is trying to call the native library or the wasm module.

@AaronRobinsonMSFT AaronRobinsonMSFT added api-approved API was approved in API review, it can be implemented api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation api-approved API was approved in API review, it can be implemented labels Nov 7, 2023
@Perksey
Copy link
Member

Perksey commented Nov 8, 2023

On the actual API for a sec, concerns about future linkage types arising do seem pertinent and I wonder whether it's just better to have e.g. [ImportLinkage(ImportLinkageOptions.WasmModule)] to alleviate concerns of type explosions/accidentally invalid combinations (at least until those appear in the enum itself) where the absence of such attribute or the presence of the attribute with no flags set is interpreted as "use the runtime default behaviour (DirectPInvoke if present, or system default if such well-defined default exists for the given environment)".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices community-contribution Indicates that the PR has been added by a community member
Projects
Status: No status
Development

No branches or pull requests

9 participants