-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Comments
This is mostly tracked at rustwasm/team#291 and would be fixed by #79998. |
This should now work for wasm32-wasi (on current git head). |
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: rust/compiler/rustc_target/src/spec/wasm32_unknown_unknown.rs Lines 22 to 30 in 482a3d0
|
There are discussions going on at In short,
Can we have ABI that matches clang on |
The change itself would be removing |
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 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. |
I think so too, since the users don't need to refactor their code, this change doesn't need to be warned and waited. |
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. |
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 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.. |
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. |
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 |
When the
...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 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 |
Or perhaps Because |
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) rust/compiler/rustc_target/src/spec/wasm32_unknown_unknown.rs Lines 20 to 28 in 9ab0749
|
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 |
No, it would only be available when compiling wasm-bindgen itself, but the |
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 |
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. |
Looks like you're running your Rust code in Deno/nodejs/Browser with Some people were suggesting that The biggest problem that
Of course, the misaligned C ABI is also a big problem of IMHO,
|
On the other hand, fixing the |
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 To be honest, I need to get confirmation from |
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. |
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 |
I was looking into how to tackle this, but couldn't really find any past work except some specific proc-macro Is there any accepted way to do something like that? I was thinking of just making a new lint and add it to the |
There have been a couple of that kind of hacks in the past. In any case this is what I meant.
I believe that is the intended way for adding this kind of lint. |
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 |
Ah I see. Someone please update that comment then. :) |
I'm not sure how that would work because we are talking about a compile feature and not a language one. EDIT: But if it can be done it would be a very nice solution. |
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 Doing it on a crate-by-crate basis wouldn't work either though, since then an |
Yeah this can't be done over an edition. We're going to have to do a breaking change at some point. |
…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.
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.
The rustc warns ``` The package `wasm-bindgen v0.2.87` currently triggers the following future incompatibility lints: > warning: older versions of the `wasm-bindgen` crate will be incompatible with future versions of Rust; please update to `wasm-bindgen` v0.2.88 > | > = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! > = note: for more information, see issue #71871 <rust-lang/rust#71871> > ```
The rustc warns ``` The package `wasm-bindgen v0.2.87` currently triggers the following future incompatibility lints: > warning: older versions of the `wasm-bindgen` crate will be incompatible with future versions of Rust; please update to `wasm-bindgen` v0.2.88 > | > = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! > = note: for more information, see issue #71871 <rust-lang/rust#71871> > ```
The rustc warns ``` The package `wasm-bindgen v0.2.87` currently triggers the following future incompatibility lints: > warning: older versions of the `wasm-bindgen` crate will be incompatible with future versions of Rust; please update to `wasm-bindgen` v0.2.88 > | > = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! > = note: for more information, see issue #71871 <rust-lang/rust#71871> > ```
When I compile this C code to
wasm32-wasi
:And this Rust code to
wasm32-wasi
:And try link them together, I'm getting linking errors:
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:
Rust version: rustc 1.44.0-nightly (38114ff 2020-03-21)
The text was updated successfully, but these errors were encountered: