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

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

Merged
merged 7 commits into from
Sep 12, 2023

Conversation

Liamolucko
Copy link
Collaborator

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
Copy link
Collaborator

daxpedda commented Sep 6, 2023

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
Copy link
Collaborator Author

Liamolucko commented Sep 7, 2023

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
Copy link
Collaborator

daxpedda commented Sep 7, 2023

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
Copy link
Collaborator Author

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
Copy link
Collaborator

daxpedda commented Sep 7, 2023

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.

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

src/convert/impls.rs Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Liamolucko and others added 3 commits September 8, 2023 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants