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

Tracking issue for the unstable "wasm" ABI #83788

Closed
alexcrichton opened this issue Apr 2, 2021 · 32 comments
Closed

Tracking issue for the unstable "wasm" ABI #83788

alexcrichton opened this issue Apr 2, 2021 · 32 comments
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-FFI Area: Foreign function interface (FFI) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

This issue is intended to track the stabilization of the "wasm" ABI added in #83763. This new ABI is notably different from the "C" ABI where the "C" ABI's purpose is to match whatever the C ABI for the platform is (in this case whatever clang does). The "wasm" ABI, however, is intended to match the WebAssembly specification and should be used for functions that need to be exported/imported from a wasm module to precisely match a desired WebAssembly signature.

One of the main features of the "wasm" ABI is that it automatically enables the multi-value feature in LLVM which means that functions can return more than one value. Additionally parameters are never passed indirectly and are always passed by-value when possible to ensure that what was written is what shows up in the ABI on the other end.

It's expected that one day the "C" ABI will be updated within clang to use multi-value, but even if that happens the ABI is likely to optimize for the performance of C itself rather than for binding the WebAssembly platform. The presence of a "wasm" ABI in Rust guarantees that there will always be an ABI to match the exact WebAssembly signature desired.

Another nice effect of stabilizing this ABI is that the wasm32-unknown-unknown target's default C ABI can be switched back to matching clang. This longstanding mismatch is due to my own personal mistake with the original definition and the reliance on the mistake in wasm-bindgen. Once the "wasm" ABI is stable, however, I can switch wasm-bindgen to using the "wasm" ABI everywhere and then the compiler can switch the default ABI of the wasm32-unknown-unknown target to match the C ABI of clang.

@alexcrichton alexcrichton added B-unstable Blocker: Implemented in the nightly compiler and unstable. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Apr 2, 2021
@jonas-schievink jonas-schievink added the A-FFI Area: Foreign function interface (FFI) label Apr 2, 2021
@joshtriplett joshtriplett added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Apr 2, 2021
@wleslie
Copy link

wleslie commented Sep 30, 2021

For folks wanting to give this a try, is there an experimental branch of wasm-bindgen that uses this "wasm" ABI?

@alexcrichton
Copy link
Member Author

There is not a branch at this time, no.

@workingjubilee workingjubilee added the A-ABI Area: Concerning the application binary interface (ABI) label Jul 1, 2022
@joshtriplett joshtriplett added the S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. label Jul 20, 2022
@joshtriplett
Copy link
Member

joshtriplett commented Jul 20, 2022

What is the current state of this? Is there wasm-bindgen support or tooling support for this? Is anything using this?

cc @alexcrichton

@memoryruins
Copy link
Contributor

@joshtriplett

Is anything using this?

Searching sourcegraph and github, I've so far only found one project previously using it then switched to "C" ABI due to a couple of issues oasisprotocol/oasis-sdk#561

@alexcrichton
Copy link
Member Author

The wasm-bindgen crate hasn't updated to using this since it requires nightly Rust and it otherwise wants to compile on stable. I'm otherwise unaware of any other tooling using this myself.

@joshtriplett
Copy link
Member

@alexcrichton Is there a chicken and egg problem there? I think we were waiting to consider stabilization until it was clear this was being used. :)

@alexcrichton
Copy link
Member Author

Personally I think this is more of a "what is C going to do" problem for stabilization. I don't think Rust is in the position to blaze a trail here when C/C++ otherwise can't access it. One of the main purposes of this ABI is to give access to the ability to define a multi-return function in WebAssembly, but C/C++ have no corresponding mechanism to do that. Until C/C++ figure out how they're going to return multiple values then multi-return is unlikely to be used in the wasm ecosystem so stabilizing this wouldn't provide much benefit. Additionally if this were stabilized and then C/C++ picked a different way to use multi-return then that would be yet-another ABI that Rust would have to implement.

@soqb
Copy link
Contributor

soqb commented Aug 29, 2022

Rust isn’t C. Rust is Rust and Rust shouldn’t care what C (clang) does for anything not explicitly using the term C (repr(C) and extern "C").

Rust on wasm’s first priority will always be compatibility with wasm and web standards not with C/C++ on wasm, especially considering wasm doesn’t even have a good way of interoperating betweeen Rust and C that isn’t already mangled through javascript. multiple returns are natively supported by wasm, and Rust should take advantage of this whether clang likes it or not.

perhaps we should maintain extern "C" as “C/C++ compatible wasm bindings” but we most definitely should not block the addition of a feature explicitly designed for decoupling Rust on wasm from C because it might not match what C is going to do.

i also see absolutely no reason that Rust shouldn’t be a trailblazer in this field, as it’s at least as favourable as C/C++ in the wasm community.

i don’t think C or C++ even belong in this discussion.

@coderedart
Copy link

just wanted to know if this affects rustwasm/team#291 where we can't compile c/cpp binding crates to wasm32-unknown-unknown yet. eg: sdl2-sys or zstd-sys etc..

@Liamolucko
Copy link

Yes - adding the "wasm" ABI would allow wasm-bindgen to switch to it, and then the "C" ABI should be able to be fixed to match clang since there wouldn't be anything relying on it. That would fix the ABI-incompatibility issue.

A different way to fix it would be to change wasm-bindgen to generate code which works the same no matter whether the current, incorrect "C" ABI or the standard one is being used (i.e. explicitly passing structs by pointer). That could be a performance hit, but I doubt it'd be huge.

@paulyoung
Copy link

@alexcrichton

Another nice effect of stabilizing this ABI is that the wasm32-unknown-unknown target's default C ABI can be switched back to matching clang.

…and then the compiler can switch the default ABI of the wasm32-unknown-unknown target to match the C ABI of clang.

This suggests that maybe there’s a way to use something other than the default C ABI.

Is there a way to opt-in to using a C ABI that isn’t the default but does match clang?

I’m targeting wasm32-unknown-unknown but not using wasm-bindgen in case that’s relevant.

Please forgive my ignorance on this. I’m in way over my head here 😄

@alexcrichton
Copy link
Member Author

I'm not aware of any mechanism at this time to use something else to match clang. That's the purpose of the "C" ABI so I've at least personally been reluctant to add another name for that.

@coderedart
Copy link

coderedart commented Dec 28, 2022

is there a place i can track the progress of this clang's C multi-return ABI thing?
also, any idea as to how long it will take? 2024? or even longer?
#73755 ?

@NicholasLYang
Copy link

Have there been any attempts to form a committee around this? Ideally this would be a W3C subgroup.

@bjorn3
Copy link
Member

bjorn3 commented Mar 20, 2023

The "wasm" ABI, however, is intended to match the WebAssembly specification and should be used for functions that need to be exported/imported from a wasm module to precisely match a desired WebAssembly signature.

This is not how extern "wasm" is implemented. As it is currently implemented it is effectively extern "unadjusted" + integer promotion. extern "unadjusted" is perma-unstable as it depends ob the exact LLVM types the backend uses to represent Rust types and the exact rules LLVM uses to legalize non-primitive and non-byref arguments. For example unions like #[repr(C)] union Foo { a: [u8; 9] } are currently represented in LLVM ir using type_padding_filler whose return value is for example [9 x i8] for Foo (as this is is an array with the exact same size as the union and using bigger element types will leave a rest) and this is lowered by current LLVM versions as 9 32bit integer arguments for extern "unadjusted" and thus extern "wasm". I believe LLVM in case of nested types even recursively picks out primitive fields. The only types that can match the Webassembly specification are primitives. Anything else and you are inventing your own ABI and in this case it is an ABI which depends on implementation details.

$ echo '#![feature(wasm_abi)] #[repr(C)] pub union Foo { b: [u8; 9] } #[no_mangle] pub extern "wasm" fn foo(a: Foo) {}' | rustc +nightly --target wasm32-unknown-unknown --crate-type lib --emit asm=/dev/stdout,llvm-ir=/dev/stdout -
[...]
.globl  foo
        .type   foo,@function
foo:
        .functype       foo (i32, i32, i32, i32, i32, i32, i32, i32, i32) -> ()
        end_function
[...]
%Foo = type { [9 x i8] }

; Function Attrs: mustprogress nofree norecurse nosync nounwind readnone willreturn
define dso_local void @foo(%Foo %0) unnamed_addr #0 {
start:
  ret void
}
[...]

#76916 is likely caused by this.

@daxpedda
Copy link
Contributor

@alexcrichton I was going down the rabbit hole of why wasm32-unknown-unknown is currently incompatible with C and eventually landed here. I didn't fully understand why extern "wasm" is blocked on "what is C going to do" about multivalue: #83788 (comment).

Until C/C++ figure out how they're going to return multiple values then multi-return is unlikely to be used in the wasm ecosystem so stabilizing this wouldn't provide much benefit.

But stabilizing this has a huge benefit even without any multivalue support: be compatible with C!

Additionally if this were stabilized and then C/C++ picked a different way to use multi-return then that would be yet-another ABI that Rust would have to implement.

So that's the part I'm confused about, I thought the whole point of this new ABI is to separate C and Wasm ABI concerns. extern "wasm" will always correspond to the Wasm ABI, the one we are using in wasm-bindgen, and extern "C" will always follow the C ABI. So whenever C figures out multivalue, we can apply it to the C ABI. Why would that require another ABI?

If the concern is to support both a C ABI with and without multivalue support, then that should probably be gated by -Ctarget-feature=+multivalue. In any case, I don't see the conflict with extern "wasm" here.

@bjorn3
Copy link
Member

bjorn3 commented May 25, 2023

There is no "Wasm ABI". extern "wasm" implements some weird ABI that is decided by the interaction between LLVM (maybe stable?) and the exact way rustc_codegen_llvm uses struct types in arguments (definitively not stable!). extern "wasm" only makes sense to me if it is restricted to i32, i64, f32, f64 and (when enabling the simd feature) core::arch::wasm::v128 for arguments and those types + (when enabling the multivalue feature) tuples of those types. Anything else is not representable by wasm and as such needs an ABI to decide how to lower it. The only stable ABI for wasm is the C ABI.

@daxpedda
Copy link
Contributor

daxpedda commented May 25, 2023

Thanks for the clarification! I went back and read your last comment again too: #83788 (comment).

So does this effectively mean that we can never stabilize extern "wasm"? Or are we happy to say that extern "wasm" is an unofficial ABI that Rust made to support multivalue and wasm-bindgen?

Honestly I'm not too sure how else to proceed on the issue of Wasm and C incompatibility. The other alternative is to just drop all enhancements wasm-bindgen did and go back to only following the C ABI.

@bjorn3
Copy link
Member

bjorn3 commented May 25, 2023

In my opinion extern "wasm" as currently implemented should not be stabilized. If it is restricted to primitive types representable directly in wasm I did be fine with stabilizing it. I'm not on the language team though, so it is not up to me to decide on stabilizing it or not.

Wasm-bindgen currently attempts to pass struct WasmSlice { ptr: u32, len: u32 } as a single argument and expects it to be decomposed into two arguments for ptr and len respectively at the ABI boundary while the wasm C ABI says that it must be passed by-reference. There is however no guarantee that future LLVM or rustc versions will keep decomposing WasmSlice into ptr and len. Especially for enums and unions the exact way we represent them in LLVM ir has changed over time. This shouldn't normally matter, but because both the current extern "C" abi for wasm32-unknown-unknown and the `extern "wasm" abi lack the abi adjustments that normally determine how to pass arguments and return types, the internal representation of types in LLVM ir determines the abi, thus breaking the abi if we change this representation.

@bjorn3
Copy link
Member

bjorn3 commented May 25, 2023

The way to make wasm-bindgen work even if we start using the official C abi for webassembly on wasm32-unknown-unknown would be to either do the decomposition of WasmSlice in wasm-bindgen itself or to explicitly pass it by-ref in wasm-bindgen. In either case no struct types would cross the abi and for primitive types the official C abi and the currently used abi agree with each other.

@daxpedda
Copy link
Contributor

There is no "Wasm ABI".

I believe there is: https://github.com/WebAssembly/tool-conventions/blob/main/BasicCABI.md, I'm not sure if it differs from the C ABI, but presumably this is v1 and they planned to add multivalue support to it. In any case, the ABI wasm-bindgen currently uses doesn't follow it either. See WebAssembly/tool-conventions#88.

The way to make wasm-bindgen work even if we start using the official C abi for webassembly on wasm32-unknown-unknown would be to either do the decomposition of WasmSlice in wasm-bindgen itself or to explicitly pass it by-ref in wasm-bindgen.

If this really is the whole issue, then I would argue this is fixable. I will proceed to figure out what needs to be done on wasm-bindgens side and hopefully work on it.

@bjorn3
Copy link
Member

bjorn3 commented May 25, 2023

I believe there is: https://github.com/WebAssembly/tool-conventions/blob/main/BasicCABI.md, I'm not sure if it differs from the C ABI

That is the C abi I was talking about. extern "wasm" does not implement that abi however. extern "C" on all wasm targets except wasm32-unknown-unknown does.

@daxpedda
Copy link
Contributor

So after finding and reading https://github.com/rust-lang/lang-team/blob/master/design-meeting-minutes/2021-04-21-wasm-abi.md, which basically answers and explains all questions I had, I'm coming to the following conclusions:

I think the path forward is to drop splatting in wasm-bindgen and fix the C ABI in wasm32-unknown-unknown (as bjorn3 was suggesting earlier).

@alexcrichton
Copy link
Member Author

I don't have much more to add over what's been said already. I would concur that the ideal course of action would be to get wasm-bindgen not passing structs-by-value.

As for the "wasm" ABI itself, I think the future story heavily depends on what happens with C and how it integrates multi-value. If it's done by "simply" changing the default, then there's no need for "wasm". If it does it by allowing an opt-in, then "wasm" makes sense to keep (perhaps renaming of course to match C and also changing the semantics to whatever C requires).

Changing the C ABI or adding a new one is not trivial due to the number of people using it. In that sense by far the easiest thing to do is to update wasm-bindgen, delete "wasm", and then wait for C/Clang/upstream to do something. That does, however, place Rust at a bit of a disadvantage where it's no longer attempting to gather feedback on a possible design, it's simply stepping out hoping someone else will fill the gap (but arguably that's the position everyone's already in, waiting for someone else to come fix issues like this)

@daxpedda
Copy link
Contributor

That does, however, place Rust at a bit of a disadvantage where it's no longer attempting to gather feedback on a possible design, [..]

We could leave "wasm" in place and add a flag in wasm-bindgen that makes use of it, which would require nightly. I guess that's better then nothing.

@majaha
Copy link
Contributor

majaha commented Aug 21, 2023

I want to point out that the "wasm_abi" feature is currently buggy. An extern "wasm" function affects the ABI of other functions in the same compilation unit that are not tagged as extern "wasm". Example Godbolt.

This is because LLVM target-features like +multivalue apply to the whole module, not just a single function: Godbolt.

I had a discussion on the LLVM forums, and for the time being this seems to be the intended behaviour of target-features.

The tests/run-make/wasm-abi test uses the extern "wasm" feature, and mixes it with extern "C" as well as the default ABI in all the imported panicking code. This likely makes the test fragile. We should consider disabling it, or at least adding a note to it, in case it randomly breaks for someone else.

@majaha
Copy link
Contributor

majaha commented Sep 3, 2023

In case anyone is following the breadcrumbs, the LLVM code that coalesces Wasm target-features is here: https://github.com/llvm/llvm-project/blob/fa8e74073762300d07b02adec42c629daf82c44b/llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp#L181

@RalfJung
Copy link
Member

RalfJung commented Sep 7, 2023

I agree with @bjorn3 that the wasm ABI needs to be fixed before it can be stabilized.

Currently it is (as far as I know) the only ABI that uses PassMode::Direct for Aggregate types. I'd like to add assertions that we never do this since it makes reasoning about and ensuring ABI compatibility so much harder. So the ABI should be changed to no longer do that while there still is a chance of doing ABI-breaking changes, i.e., before stabilization.

@RalfJung
Copy link
Member

RalfJung commented Sep 8, 2023

IOW, I think stabilizing this should be blocked on resolving #115666 (at least for the "wasm" ABI).

nikic added a commit to nikic/rust that referenced this issue Jul 11, 2024
Remove the unstable `extern "wasm"` ABI (`wasm_abi` feature tracked
in rust-lang#83788).

As discussed in rust-lang#127513 (comment)
and following, this ABI is a failed experiment that did not end
up being used for anything. Keeping support for this ABI in LLVM 19
would require us to switch wasm targets to the `experimental-mv`
ABI, which we do not want to do.

It should be noted that `Abi::Wasm` was internally used for two
things: The `-Z wasm-c-abi=legacy` ABI that is still used by
default on some wasm targets, and the `extern "wasm"` ABI. Despite
both being `Abi::Wasm` internally, they were not the same. An
explicit `extern "wasm"` additionally enabled the `+multivalue`
feature.

I've opted to remove `Abi::Wasm` in this patch entirely, instead
of keeping it as an ABI with only internal usage. Both
`-Z wasm-c-abi` variants are now treated as part of the normal
C ABI, just with different different treatment in
adjust_for_foreign_abi.
nikic added a commit to nikic/rust that referenced this issue Jul 11, 2024
Remove the unstable `extern "wasm"` ABI (`wasm_abi` feature tracked
in rust-lang#83788).

As discussed in rust-lang#127513 (comment)
and following, this ABI is a failed experiment that did not end
up being used for anything. Keeping support for this ABI in LLVM 19
would require us to switch wasm targets to the `experimental-mv`
ABI, which we do not want to do.

It should be noted that `Abi::Wasm` was internally used for two
things: The `-Z wasm-c-abi=legacy` ABI that is still used by
default on some wasm targets, and the `extern "wasm"` ABI. Despite
both being `Abi::Wasm` internally, they were not the same. An
explicit `extern "wasm"` additionally enabled the `+multivalue`
feature.

I've opted to remove `Abi::Wasm` in this patch entirely, instead
of keeping it as an ABI with only internal usage. Both
`-Z wasm-c-abi` variants are now treated as part of the normal
C ABI, just with different different treatment in
adjust_for_foreign_abi.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 11, 2024
Remove extern "wasm" ABI

Remove the unstable `extern "wasm"` ABI (`wasm_abi` feature tracked in rust-lang#83788).

As discussed in rust-lang#127513 (comment) and following, this ABI is a failed experiment that did not end up being used for anything. Keeping support for this ABI in LLVM 19 would require us to switch wasm targets to the `experimental-mv` ABI, which we do not want to do.

It should be noted that `Abi::Wasm` was internally used for two things: The `-Z wasm-c-abi=legacy` ABI that is still used by default on some wasm targets, and the `extern "wasm"` ABI. Despite both being `Abi::Wasm` internally, they were not the same. An explicit `extern "wasm"` additionally enabled the `+multivalue` feature.

I've opted to remove `Abi::Wasm` in this patch entirely, instead of keeping it as an ABI with only internal usage. Both `-Z wasm-c-abi` variants are now treated as part of the normal C ABI, just with different different treatment in
adjust_for_foreign_abi.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jul 12, 2024
Rollup merge of rust-lang#127605 - nikic:remove-extern-wasm, r=oli-obk

Remove extern "wasm" ABI

Remove the unstable `extern "wasm"` ABI (`wasm_abi` feature tracked in rust-lang#83788).

As discussed in rust-lang#127513 (comment) and following, this ABI is a failed experiment that did not end up being used for anything. Keeping support for this ABI in LLVM 19 would require us to switch wasm targets to the `experimental-mv` ABI, which we do not want to do.

It should be noted that `Abi::Wasm` was internally used for two things: The `-Z wasm-c-abi=legacy` ABI that is still used by default on some wasm targets, and the `extern "wasm"` ABI. Despite both being `Abi::Wasm` internally, they were not the same. An explicit `extern "wasm"` additionally enabled the `+multivalue` feature.

I've opted to remove `Abi::Wasm` in this patch entirely, instead of keeping it as an ABI with only internal usage. Both `-Z wasm-c-abi` variants are now treated as part of the normal C ABI, just with different different treatment in
adjust_for_foreign_abi.
@RalfJung
Copy link
Member

@nikic should this be closed, since everything tracked by this feature was removed in #127605? Or are there things left?

@nikic
Copy link
Contributor

nikic commented Aug 24, 2024

Yes, this one should be closed. We have a separate tracking issue for wasm-c-abi (#122532).

@nikic nikic closed this as completed Aug 24, 2024
@RalfJung
Copy link
Member

RalfJung commented Aug 24, 2024

Great, so #115666 only affects wasm32-u-u now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-FFI Area: Foreign function interface (FFI) B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ S-tracking-needs-summary Status: It's hard to tell what's been done and what hasn't! Someone should do some investigation. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests