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

WebAssembly ABI mismatch between clang and rust #71871

Open
tomaka opened this issue May 4, 2020 · 37 comments
Open

WebAssembly ABI mismatch between clang and rust #71871

tomaka opened this issue May 4, 2020 · 37 comments
Labels
A-abi Area: Concerning the application binary interface (ABI). A-ffi Area: Foreign Function Interface (FFI) C-bug Category: This is a bug. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@tomaka
Copy link
Contributor

tomaka commented May 4, 2020

When I compile this C code to wasm32-wasi:

typedef struct Vector {
    int a;
    int b;
} Vector;

extern int extract_a(Vector v) {
    return v.a;
}

And this Rust code to wasm32-wasi:

#[repr(C)]
struct Vector {
    a: i32,
    b: i32,
}

extern "C" {
    fn extract_a(v: Vector) -> i32;
}

fn main() {
    unsafe {
        extract_a(Vector { a: 5, b: 4 });
    }
}

And try link them together, I'm getting linking errors:

  = note: lld: error: function signature mismatch: extract_a
          >>> defined as (i32, i32) -> i32 in /home/pierre/Projets/wasm-abi-test/target/wasm32-wasi/debug/deps/wasm_abi_test.67xpw7l331r9o5x.rcgu.o
          >>> defined as (i32) -> i32 in /home/pierre/Projets/wasm-abi-test/target/wasm32-wasi/debug/build/wasm-abi-test-9c49ce7f6c5ca031/out/libfoo.a(test.o)

It seems that, according to clang, passing a struct by value should inline the fields, while for Rust it should be passed by pointer.

I uploaded an example project here: https://github.com/tomaka/wasm-abi-test
It can be built with something like:

AR_wasm32_wasi=/path/to/wasi-sdk-10.0/bin/ar CC_wasm32_wasi=/path/to/wasi-sdk-10.0/bin/clang cargo run --target=wasm32-wasi

Rust version: rustc 1.44.0-nightly (38114ff 2020-03-21)

@tomaka tomaka added the C-bug Category: This is a bug. label May 4, 2020
@jonas-schievink jonas-schievink added A-ffi Area: Foreign Function Interface (FFI) O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 4, 2020
@TheBlueMatt
Copy link

This is mostly tracked at rustwasm/team#291 and would be fixed by #79998.

@TheBlueMatt
Copy link

This should now work for wasm32-wasi (on current git head).

@workingjubilee workingjubilee added the A-abi Area: Concerning the application binary interface (ABI). label Jul 1, 2022
@programmerjake
Copy link
Member

programmerjake commented Aug 28, 2022

Note that #83763 changed the default, so only wasm32-unknown-unknown uses the mismatched ABI, all other wasm targets (including custom targets) now default to using the ABI that matches clang:

// This is a default for backwards-compatibility with the original
// definition of this target oh-so-long-ago. Once the "wasm" ABI is
// stable and the wasm-bindgen project has switched to using it then there's
// no need for this and it can be removed.
//
// Currently this is the reason that this target's ABI is mismatched with
// clang's ABI. This means that, in the limit, you can't merge C and Rust
// code on this target due to this ABI mismatch.
options.default_adjusted_cabi = Some(Abi::Wasm);

@temeddix
Copy link

temeddix commented Oct 26, 2023

There are discussions going on at wasm-bindgen to extend the support to wasm32-wasi in addition to the current wasm32-unknown-unknown.

In short, wasm-bindgen needs a consistent C ABI between wasm32-unknown-unknown and wasm32-wasi to support both of them.

Note that #83763 changed the default, so only wasm32-unknown-unknown uses the mismatched ABI, all other wasm targets (including custom targets) now default to using the ABI that matches clang:

// This is a default for backwards-compatibility with the original
// definition of this target oh-so-long-ago. Once the "wasm" ABI is
// stable and the wasm-bindgen project has switched to using it then there's
// no need for this and it can be removed.
//
// Currently this is the reason that this target's ABI is mismatched with
// clang's ABI. This means that, in the limit, you can't merge C and Rust
// code on this target due to this ABI mismatch.
options.default_adjusted_cabi = Some(Abi::Wasm);

Can we have ABI that matches clang on wasm32-unknown-unknown as well? I can help with this, but would it be a hard task?

@bjorn3
Copy link
Member

bjorn3 commented Oct 26, 2023

The change itself would be removing options.default_adjusted_cabi = Some(Abi::Wasm) from the target spec and that's it. Because it breaks older wasm-bindgen versions it will probably require waiting months to years before we can actually make this change. And preferably we would introduce a future compat warning for this change. Either against older wasm-bindgen versions or against any extern "C" function for which changing the abi actually has any effect.

@hamza1311
Copy link
Contributor

I don't see why breaking older versions wasm-bindgen is a blocker when there can be a compatibility warning and it'll take over 12 weeks until stable if the change were to be merged today. Updating wasm-bindgen is as simple as just running cargo update -p wasm-bindgen, we can tell users to run it. wasm-bindgen is also semver 0.x so it makes no stability guarantees

It is unfortunate that we have to break code (that we never promised stability for) but the longer we wait, the worse the problem gets.

@temeddix
Copy link

temeddix commented Oct 26, 2023

I think so too, since the users don't need to refactor their code, this change doesn't need to be warned and waited.

@bjorn3
Copy link
Member

bjorn3 commented Oct 26, 2023

We need to warn because it will breal everyone who uses an older wasm-bindgen version. We have done plenty of warnings and sometimes even crate version specific workaround in the past to avoid breakage even after a fixed crate version exists.

@temeddix
Copy link

temeddix commented Oct 26, 2023

We need to warn because it will breal everyone who uses an older wasm-bindgen version.

May I ask you what things would be broken and users should be aware of?

The reason this discussion started was basically major modules in std are not working in wasm32-unknown-unknown, making various crates be divided into wasm and non-wasm versions. Fixing this fragmentation in Rust ecosystem as soon as possible would be good, in my opinion. Maybe we can use a version-specific workaround as you mentioned.

One thing I'm concerned about is that this issue is being solved too slow. It's older than 3 years. I believe we need some momentum for this issue, or it will just be left unsolved even after 2026 or so..

@liamrosenfeld
Copy link

liamrosenfeld commented Oct 26, 2023

May I ask you what things would be broken and users should be aware of?

It'll break using a newer version of the rust compiler that has the fix alongside older versions of wasm-bindgen that expect the C abi to be different.

While this isn't a source breaking warning, it still requires user action to prevent breaking a build when updating one but not the other so it needs to be there.

@hamza1311
Copy link
Contributor

As someone who hasn't contributed to rustc, how hard would this be to fix? I would like to give it a shot but I don't know where to even begin with this

@temeddix
Copy link

temeddix commented Oct 26, 2023

When the wasm ABI was added for compatibility, the author @alexcrichton said like this:

it would be nice for this to get stabilized (if it works) in the near-ish
future to remove the wasm32-unknown-unknown incompatibility with the C

...and that was two and a half years ago.

I heard that in the old days when Rust wasn't so mature, there were breaking changes here and there, and that was possible because the user pool was much smaller than now.

I believe if we hesitate, with the increasing Rust population who are adopting wasm Rust to their server/web code, it would get harder and harder to fix this later. I would argue that wasm-bindgen should figure out how to warn its users(as written in its issue).

@hamza1311
Copy link
Contributor

wasm-bindgen can't warn because the issue would come up when using versions of wasm-bindgen that are already released. Newer versions would be fine.

We could back port the fix and have the crates.io team replace the published versions so every new build is unaffected. Not sure how feasible that is

@temeddix
Copy link

temeddix commented Oct 26, 2023

Or perhaps wasm-bindgen could possibly publish the final 0.2.x version that includes checking rustc version and warning the users, and go on to the 0.3.0-beta version that uses the wasm-compatible ABI with new MSRV.

Because wasm-bindgen is such a major crate, notifying the changes in README and docs would be effective for users out there.

@temeddix
Copy link

temeddix commented Oct 26, 2023

As someone who hasn't contributed to rustc, how hard would this be to fix? I would like to give it a shot but I don't know where to even begin with this

I'm not an expert either, but it looks like this part could be the starting point, @hamza1311 . This does look simple as mentioned in #71871 (comment)

// This is a default for backwards-compatibility with the original
// definition of this target oh-so-long-ago. Once the "wasm" ABI is
// stable and the wasm-bindgen project has switched to using it then there's
// no need for this and it can be removed.
//
// Currently this is the reason that this target's ABI is mismatched with
// clang's ABI. This means that, in the limit, you can't merge C and Rust
// code on this target due to this ABI mismatch.
options.default_adjusted_cabi = Some(Abi::Wasm);

@hamza1311
Copy link
Contributor

I'm not an expert either, but it looks like this part could be the starting point, @hamza1311 . This does look simple as mentioned in #71871 (comment)

// This is a default for backwards-compatibility with the original
// definition of this target oh-so-long-ago. Once the "wasm" ABI is
// stable and the wasm-bindgen project has switched to using it then there's
// no need for this and it can be removed.
//
// Currently this is the reason that this target's ABI is mismatched with
// clang's ABI. This means that, in the limit, you can't merge C and Rust
// code on this target due to this ABI mismatch.
options.default_adjusted_cabi = Some(Abi::Wasm);

If wasm-bindgen sets an environment variable in its build script, will it be available at that point in the process? If it is, that pretty much solves the breaking change issue as we can only use the better C ABI if wasm-bindgen tells us it's fine to use it. It has the unfortunate consequence of requiring wasm-bindgen to use that ABI at the moment but we can remove the check once the adoption of a later version of wasm-bindgen is good enough that this isn't an issue

@bjorn3
Copy link
Member

bjorn3 commented Oct 27, 2023

If wasm-bindgen sets an environment variable in its build script, will it be available at that point in the process?

No, it would only be available when compiling wasm-bindgen itself, but the extern "C" calls are generated by wasm-bindgen when compiling user code which wouldn't have those env vars set. It would also lead to issues when two crates interface using extern "C" but only one compiled with the env var set.

@hamza1311
Copy link
Contributor

Does the environment variable set during a build script get unset after the crate compilation finishes? I haven't tested it but I don't think it should. If wasm-bindgen sets an environment variable then when its dependents are compiled, they'll also have that set, no?

It doesn't have to be an environment variable, just something to indicate what ABI it wants

@bjorn3
Copy link
Member

bjorn3 commented Oct 27, 2023

Env vars set by a build script only apply to the package the build script is part of. So wasm-bindgen's build script would apply to libwasm_bindgen.rlib as well as all of it's examples and tests, but not to anything depending on wasm-bindgen.

@davidar
Copy link

davidar commented Oct 29, 2023

Are there any possible options that would allow users to opt into using the fixed ABI without needing to wait years for the stabilisation process to complete?

@temeddix
Copy link

temeddix commented Oct 29, 2023

There was a PR that I've made #117236 that would do what you said. Unfortunately, I just closed the PR because I had a feeling that wasm32-unknown-unknown is already too widespreaded to be fixed(And not enough people seemed to be interested). As far as I know, wasm32-wasi target is based on the fixed ABI.

If you're running compiled Rust on wasm runtimes, why not give wasm32-wasi a try?

If you're running compiled Rust on web browsers, you would still need to use wasm32-unknown-unknown because wasm-bindgen(which is necessary for JavaScript interaction) doesn't support wasm32-wasi. I'm trying to make PRs at wasm-bindgen to enable compiling Rust to wasm32-wasi with wasm-bindgen and using it inside web browsers with polyfills like wasmer-js or browser-wasi-shim, but honestly I'm not sure yet whether my idea would be accepted at wasm-bindgen repo.

@tritao
Copy link

tritao commented Oct 29, 2023

There was a PR that I've made #117236 that would do what you said. Unfortunately, I just closed the PR because I had a feeling that wasm32-unknown-unknown is already too widespreaded to be fixed(And not enough people seemed to be interested).

Would be great if you could re-open that PR. Re-aligning the wasm32-unknown-unknown ABI to be compatible with the official Clang / C ABI seems like the way to go IMHO. Anything else seems like a hack to me and very limiting, basically isolates Rust into its own world and makes it difficult to interop with other C libraries.

@temeddix
Copy link

temeddix commented Oct 30, 2023

Looks like you're running your Rust code in Deno/nodejs/Browser with wasm32-unknown-unknown. I made some related post on the Rust forum, have a look:

Some people were suggesting that wasm32-unknown-unknown should be deprecated, because what it means is that the architecture is really 'unknown'. There were also people from other places making the same claims, which does make sense. I personally think that wasm32-unknown-unknown is not the way to go for the Rust community, and we should gradually move onto wasm32-wasi, leaving broken wasm32-unknown-unknown behind.

The biggest problem that wasm32-unknown-unknown has is that it doesn't support certain std modules, such as std::time::Instant, std::thread, std::fs. This makes the Rust ecosystem seriously fractured, as it forces various crates to make separated wasm and non-wasm versions(or at best, write the code twice). Take a look at rayon, for example:

Of course, the misaligned C ABI is also a big problem of wasm32-unknown-unknown.

IMHO, wasm32-wasi could be the answer, even for JavaScript environment in Deno/nodejs/Browser. It has standard C ABI, and all of the std functionalities are enabled by default, just like native platforms. After Rust code is compiled to wasm32-wasi, browser shims like wasmer-js or browser_wasi_shim can handle WASI syscalls by interacting with JavaScript in Deno/nodejs/Browser. I might make a PR at wasm-pack that makes it use browser shims if I have time, but anyone can do it though.

wasm-bindgen was made compatible with wasm32-wasi on September, it's just not enabled yet, which also needs a PR:

@rpjohnst
Copy link
Contributor

wasm32-wasi is not a full replacement for wasm32-unknown-unknown, because some applications cannot take a dependency on WASI. And these scenarios often need to use the C ABI! Really what should have happened here is for wasm32-unknown-unknown to be #[no_std]-only, at least to begin with, like the other -unknown targets, but that's not really something we can change now.

On the other hand, fixing the wasm32-unknown-unknown C ABI was always the plan. If there needs to be a deprecation period, or some way to opt out of or into the fix, fine- but locking wasm32-unknown-unknown into its current behavior is not an option in the first place, because its current "C ABI" (equivalent to other targets' extern "wasm") relies on unstable implementation details and was never designed for anything beyond primitives: #115666

@temeddix
Copy link

temeddix commented Oct 30, 2023

I think I can I re-open #117236, but would there be any attention from reviewers soon enough so that I can implement warnings for wasm-bindgen 0.2.x, from the Rust side?

To be honest, I need to get confirmation from wasm-bindgen maintainers that with the new C ABI, its version will be bumped up to 0.3.0 or something. I could not really reach the consensus because not enough people were responding in the PR.

@Liamolucko
Copy link

To be honest, I need to get confirmation from wasm-bindgen maintainers that with the new C ABI, its version will be bumped up to 0.3.0 or something. I could not really reach the consensus because not enough people were responding in the PR.

We're not going to release a version 0.3.0 over this, it's not a breaking change and so there's no need to break compatibility with the ecosystem of crates using 0.2.x for no reason. wasm-bindgen is still fully compatible with the old C ABI, it just also supports the new one now. The warning can just be for versions of wasm-bindgen prior to 0.2.88, there's no need for it to be all 0.2.x versions.

@hamza1311
Copy link
Contributor

I agree. We should warn for versions <0.2.88, no need for 0.3.x just for this issue. Technically nothing in wasm-bindgen is broken so there's no need for a semver breaking version

@daxpedda
Copy link
Contributor

We need to warn because it will breal everyone who uses an older wasm-bindgen version. We have done plenty of warnings and sometimes even crate version specific workaround in the past to avoid breakage even after a fixed crate version exists.

I was looking into how to tackle this, but couldn't really find any past work except some specific proc-macro rental crate warnings. If there is anything else, could you point me into that direction @bjorn3?

Is there any accepted way to do something like that? I was thinking of just making a new lint and add it to the future-incompatible group.

@bjorn3
Copy link
Member

bjorn3 commented Nov 14, 2023

I was looking into how to tackle this, but couldn't really find any past work except some specific proc-macro rental crate warnings.

There have been a couple of that kind of hacks in the past. In any case this is what I meant.

Is there any accepted way to do something like that? I was thinking of just making a new lint and add it to the future-incompatible group.

I believe that is the intended way for adding this kind of lint.

@RalfJung
Copy link
Member

How does this issue relate to #115666? Here it seems rustc is passing things by-ref when it should be passed by-val, but in #115666 we have the issue that rustc passes things by-val in the wrong way. So that seems like it is a separate issue, but I would assume they are somehow entangled?

@Liamolucko
Copy link

How does this issue relate to #115666? Here it seems rustc is passing things by-ref when it should be passed by-val, but in #115666 we have the issue that rustc passes things by-val in the wrong way. So that seems like it is a separate issue, but I would assume they are somehow entangled?

The original comment has it backwards, Rust is passing things by-value when it should be passing them by-ref the same as #115666. You can see that the function with signature (i32, i32) -> i32 is coming from target/.../deps (i.e. Rust code) and the function with signature (i32) -> i32 is coming from target/.../build (i.e. C code).

@RalfJung
Copy link
Member

Ah I see. Someone please update that comment then. :)

@hamza1311
Copy link
Contributor

One idea I had is to move to C-ABI with an edition boundary (in 2024 edition). I would be happy to work on a PR for this, if we want it. This way we can avoid a forced breaking change and let users update wasm-bindgen as they migrate to 2024 edition

CC: @bjorn3 @daxpedda thought?

@daxpedda
Copy link
Contributor

daxpedda commented Jan 11, 2024

I'm not sure how that would work because we are talking about a compile feature and not a language one.
So it would depend on the edition of the "root crate"? I'm unfamiliar with the architecture here, but wouldn't that also require changes in Cargo to identify the "root crate"?

EDIT: But if it can be done it would be a very nice solution.

@Liamolucko
Copy link

I'm not sure how that would work because we are talking about a compile feature and not a language one. So it would depend on the edition of the "root crate"? I'm unfamiliar with the architecture here, but wouldn't that also require changes in Cargo to identify the "root crate"?

It feels a bit questionable to apply the change to all crates based on one crate's edition, since half the point of editions is that you can mix crates written against different editions without the older-edition crates being impacted. It'd also be possible for someone to update their root crate to the 2024 edition while still using a pre-0.2.88 version of wasm-bindgen and have it break.

Doing it on a crate-by-crate basis wouldn't work either though, since then an extern "C" function defined in a 2021-edition crate wouldn't work when called from a 2024-edition crate.

@RalfJung
Copy link
Member

Yeah this can't be done over an edition. We're going to have to do a breaking change at some point.

fmease added a commit to fmease/rust that referenced this issue Mar 16, 2024
…kingjubilee

Add `wasm_c_abi` `future-incompat` lint

This is a warning that will tell users to update to `wasm-bindgen` v0.2.88, which supports spec-compliant C ABI.

The idea is to prepare for a future where Rust will switch to the spec-compliant C ABI by default; so not to break everyone's world, this warning is introduced.

Addresses rust-lang#71871.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 17, 2024
Rollup merge of rust-lang#117918 - daxpedda:wasm-c-abi-warning, r=workingjubilee

Add `wasm_c_abi` `future-incompat` lint

This is a warning that will tell users to update to `wasm-bindgen` v0.2.88, which supports spec-compliant C ABI.

The idea is to prepare for a future where Rust will switch to the spec-compliant C ABI by default; so not to break everyone's world, this warning is introduced.

Addresses rust-lang#71871.
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) C-bug Category: This is a bug. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests