Make `wasm-bindgen` compatible with the standard C ABI by Liamolucko · Pull Request #3595 · wasm-bindgen/wasm-bindgen

Liamolucko · 2023-09-06T09:16:00Z

This is the first step of #3454: making wasm-bindgen compatible with the standard(ish) Wasm C ABI, which clang uses.

This PR more or less implements @alexcrichton's suggestion in #3454 (comment), to allow {From,Into}WasmAbi impls to manually specify multiple types that Self should get splatted into when passed as an argument. I implemented it a bit differently though - instead of actually adding multiple associated types to {From,Into}WasmAbi, I changed the meaning of WasmAbi:

pub trait WasmAbi {
    type Prim1: WasmPrimitive;
    type Prim2: WasmPrimitive;
    type Prim3: WasmPrimitive;
    type Prim4: WasmPrimitive;

    fn split(self) -> (Self::Prim1, Self::Prim2, Self::Prim3, Self::Prim4);
    fn join(prim1: Self::Prim1, prim2: Self::Prim2, prim3: Self::Prim3, prim4: Self::Prim4)
        -> Self;
}

This means that {From,Into}WasmAbi impls can still return single, typed values like before, and then it's up to those types to implement WasmAbi and specify how they should be splatted. Also, WasmAbi types no longer actually have to be FFI-safe themselves - even when they're passed as return values, split is still called, and those values are then put into a #[repr(C)] struct (WasmRet<T>).

The new trait WasmPrimitive now has the old meaning of WasmAbi, minus allowing #[repr(C)] types - any Wasm primitive type.

This PR doesn't actually change wasm-bindgen's ABI at all, just how it's implemented. This meant that I didn't have to touch the CLI.

One thing that this PR doesn't fix is #3552. extern "C" functions still return #[repr(C)] structs the same as before, since the retptr handling happens to be the same between the current and standard C ABI. That is, unless you turn on multivalue, in which case the return value will get splatted with the current ABI. (I'm referring to rustc's multivalue target feature here, not wasm-bindgen's own multivalue post processing step.)

It's not a blocker for migrating the ABI, since wasm-bindgen's already broken when multivalue is enabled, but it's still be nice to fix it.

I'm not sure how it could be fixed without always using a retptr though, which I'd rather avoid. I've experimented a little with type system shenanigans like this:

trait WasmRetCfg: WasmAbi {
    type Retptr: Retptr;
    type Ret: WasmPrimitive;

    fn ret(self, retptr: Self::Retptr) -> Self::Ret;
    fn recover(retptr: Self::Retptr, ret: Self::Ret) -> Self;
}

There'd be one impl where Retptr and Ret get set to () and Self::Prim1 respectively, if Self's last 3 primitives are all (), and another one where Retptr and Ret get set to *mut WasmRet<Self> and () respectively, if Self::Prim2 (and potentially Prim3, Prim4) isn't (). Then a generated import foo() -> T would look something like this:

fn foo() -> T {
    type Abi = <T as FromWasmAbi>::Abi;
    extern "C" foo_extern(retptr: Abi::Retptr) -> Abi::Ret;

    // `Target` is `WasmRet<Abi>` if `Retptr` is `*mut WasmRet<Abi>`, otherwise `()`.
    let ret_space: MaybeUninit<<Abi::Retptr as Retptr>::Target> = MaybeUninit::uninit();
    let retptr = <Abi::Retptr as Retptr>::from_ptr(ret_space.as_mut_ptr());
    let ret = foo_extern(retptr);
    let real_ret = Abi::recover(retptr, ret);
    real_ret.join()
}

Unfortunately, I couldn't convince rustc that Prim2 not being () meant that those two impls didn't conflict. Maybe someone who's more of a type system guru than me can figure it out, but it might be impossible without specialisation. It'll become unnecessary if/when the C ABI gets fixed anyway.

For some reason a couple of functions got switched around; no actual meaningful changes.

daxpedda · 2023-09-06T20:53:06Z

I've given it a quick glance, really nice solution! I imagined this to be way messier, but it's pretty neat!
Definitely will try to review it over the weekend.

Even though this only touched unstable ABI would you mind adding an entry in the changelog?

Thinking of deployment here, we could expose an unstable cfg flag, like web_sys_unstable_apis, that switches all generated APIs to this new ABI. But this is pointless right now, we have to fix Rust's side first, which I assume will need some nightly feature, then we can try it in wasm-bindgen as well.

Liamolucko · 2023-09-07T00:36:51Z

Even though this only touched unstable ABI would you mind adding an entry in the changelog?

Done. I've also just realised that I forgot to add a changelog entry in #3554, should I open a PR to add one? (edit: opened #3599)

Thinking of deployment here, we could expose an unstable cfg flag, like web_sys_unstable_apis, that switches all generated APIs to this new ABI. But this is pointless right now, we have to fix Rust's side first, which I assume will need some nightly feature, then we can try it in wasm-bindgen as well.

I'm not sure what you mean. The new generated bindings still work fine with the existing C ABI, the whole point is that they now work with both that and the standard one. So there's no need for this to be unstable.

I'm now realising that I've been using the word 'ABI' to mean two different things interchangeably: actual Rust ABIs like extern "C", and the format of the bindings that wasm-bindgen generates. Sorry about that, I'll try and only use it for the first meaning from here on.

daxpedda · 2023-09-07T08:16:30Z

I'm not sure what you mean. The new generated bindings still work fine with the existing C ABI, the whole point is that they now work with both that and the standard one. So there's no need for this to be unstable.

I mean specifically to switch to the new ABI, that doesn't work until Rust fixes it's C ABI, right?

Liamolucko · 2023-09-07T09:02:01Z

I mean specifically to switch to the new ABI, that doesn't work until Rust fixes it's C ABI, right?

I've been assuming that Rust will just directly cut over to the fixed ABI, so that all of wasm-bindgen's existing extern "C" declarations will automatically start referring to the new ABI without wasm-bindgen having to do anything.

Are you suggesting that there should be some kind of testing period, where there's a perma-nightly "C-fixed" ABI, and then wasm-bindgen gets an unstable feature to use it and make sure everything's working?

daxpedda · 2023-09-07T09:41:32Z

Are you suggesting that there should be some kind of testing period, where there's a perma-nightly "C-fixed" ABI, and then wasm-bindgen gets an unstable feature to use it and make sure everything's working?

I was assuming that this would be the case, but I'm not advocating for it. If we can skip it and move directly to the fixed C ABI I'm fine with that, but that's not up to us.

In any case, there's nothing for us to do here right now until we know what will happen on Rusts side.

daxpedda

LGTM!

Co-authored-by: daxpedda <daxpedda@gmail.com>

Liamolucko added 3 commits September 5, 2023 19:00

Make wasm-bindgen compatible with the standard C ABI

4b73e0f

Update the guide

9460acc

Fix reference tests

fea9ca9

For some reason a couple of functions got switched around; no actual meaningful changes.

Add changelog entry

4b8aecf

daxpedda approved these changes Sep 7, 2023

View reviewed changes

Comment thread src/convert/impls.rs

Comment thread CHANGELOG.md

daxpedda mentioned this pull request Sep 7, 2023

Fix wasm_bindgen_futures not using custom path #3602

Merged

Liamolucko and others added 3 commits September 8, 2023 07:47

Add spacing

600c40c

Co-authored-by: daxpedda <daxpedda@gmail.com>

Mention WasmAbi in the changelog

c83a290

fix a tiny typo

6078cb3

Liamolucko merged commit 51e89eb into wasm-bindgen:main Sep 12, 2023

Liamolucko deleted the abi-fix branch September 12, 2023 12:24

This was referenced Oct 25, 2023

Align with the "C" ABI #3454

Closed

WebAssembly ABI mismatch between clang and rust rust-lang/rust#71871

Closed

Conditionally enable bindings for wasm32-wasi with --wasi flag #3673

Closed

Liamolucko mentioned this pull request Dec 8, 2023

Support for multivalue #3552

Closed

mockersf mentioned this pull request Jan 25, 2024

emscripten: directly return Option instead of WasmOption RustAudio/cpal#823

Merged

RalfJung mentioned this pull request Mar 14, 2025

Tracking Issue for perma-unstable wasm-c-abi flag rust-lang/rust#122532

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make `wasm-bindgen` compatible with the standard C ABI#3595

Make `wasm-bindgen` compatible with the standard C ABI#3595
Liamolucko merged 7 commits intowasm-bindgen:mainfrom
Liamolucko:abi-fix

Liamolucko commented Sep 6, 2023

Uh oh!

daxpedda commented Sep 6, 2023 •

edited

Loading

Uh oh!

Liamolucko commented Sep 7, 2023 •

edited

Loading

Uh oh!

daxpedda commented Sep 7, 2023

Uh oh!

Liamolucko commented Sep 7, 2023

Uh oh!

daxpedda commented Sep 7, 2023

Uh oh!

daxpedda left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Liamolucko commented Sep 6, 2023

Uh oh!

daxpedda commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Liamolucko commented Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxpedda commented Sep 7, 2023

Uh oh!

Liamolucko commented Sep 7, 2023

Uh oh!

daxpedda commented Sep 7, 2023

Uh oh!

daxpedda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

daxpedda commented Sep 6, 2023 •

edited

Loading

Liamolucko commented Sep 7, 2023 •

edited

Loading