add FCW to warn about wasm ABI transition #138601

RalfJung · 2025-03-17T11:18:27Z

See #122532 for context: the "C" ABI on wasm32-unk-unk will change. The goal of this lint is to warn about any function definition and calls whose behavior will be affected by the change. My understanding is the following:

scalar arguments are fine
- including 128 bit types, they get passed as two i64 arguments in both ABIs
repr(C) structs (recursively) wrapping a single scalar argument are fine (unless they have extra padding due to over-alignment attributes)
all return values are fine

@bjorn3 @alexcrichton @Manishearth is that correct?

I am making this a "show up in future compat reports" lint to maximize the chances people become aware of this. OTOH this likely means warnings for most users of Diplomat so maybe we shouldn't do this?

IIUC, wasm-bindgen should be unaffected by this lint as they only pass scalar types as arguments.

Tracking issue: #138762
Transition plan blog post: rust-lang/blog.rust-lang.org#1531

rustbot · 2025-03-17T11:18:32Z

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

RalfJung · 2025-03-17T11:22:19Z

tests/ui/lint/wasm_c_abi_transition.stderr

+LL | pub extern "C" fn my_fun(_x: MyType) {}
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!


It's not technically correct that this will become a hard error (we'll just do the ABI switch), but I don't know how to make an FCW that does not have this message...

bjorn3 · 2025-03-17T11:29:20Z

I am a bit concerned about 128bit scalar types as wasm doesn't have those. Do we have to warn about those as well?

That doesn't seem to be necessary. Both the old abi and new abi pass it as two i64 arguments.

compiler/rustc_monomorphize/src/mono_checks/abi_check.rs

hanna-kruppe · 2025-03-17T12:10:12Z

Re: i128, we just had an ABI break (independent of -Zwasm-c-abi) in 1.85 but it only changed alignment not the by-value ABI I believe #134165

alexcrichton · 2025-03-17T15:48:08Z

... @alexcrichton ... is that correct?

Honestly I'm not actually sure. The ABI bits have always been a black box to me and I don't feel confident that I would be able to enumerate the list of differences between -Zwasm-c-abi={spec,legacy}. I think the problem is that the before state is "whatever rustc/llvm together do, pending private details".

That being said what you listed matches my rough understanding. I mostly want to clarify in that I do not have certainty on the exhaustiveness of my understanding.

Would it be possible to "run both ABIs and look for a difference"? That'd help make me more confident in the exhaustiveness of the check, but I'm not sure if that's possible given the structure of the code.

What I can perhaps clarify is that the "official definition" is this for params/results. I'm not knowledgable enough of rustc ABI internals to diff that with what the previous behavior was myself though.

alexcrichton · 2025-03-17T15:51:16Z

compiler/rustc_lint_defs/src/builtin.rs

+    "detects code relying on rustc's non-spec-compliant wasm C ABI",
+    @future_incompatible = FutureIncompatibleInfo {
+        reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps,
+        reference: "issue #122532 <https://github.com/rust-lang/rust/issues/122532>",


Before landing this I think it might be best to open a fresh issue here. This one's got ~100 comments and is quite old. I'm happy to open a fresh issue to fill in here once we're more settled on a final transition plan.

I don't think it's 100 comments, and it is from May last year. But sure happy to update this to another issue number if you tell me the number :)

Yeah I think we should have a separate issue that helps talk about mitigation strategies.

RalfJung · 2025-03-17T16:10:38Z

Would it be possible to "run both ABIs and look for a difference"? That'd help make me more confident in the exhaustiveness of the check, but I'm not sure if that's possible given the structure of the code.

No, it's not possible unfortunately. The entire issue is that the legacy ABI relies on LLVM details rustc has no knowledge about, and on rustc_codegen_llvm details the rest of rustc has no knowledge about.

alexcrichton · 2025-03-17T16:13:11Z

Ok thanks for clarifying, in that case I think we can mitigate that by (a) adding appropriate wording to the blog post about this change and perhaps (b) calling out in the release notes for the next release of Rust "hey things are changing in wasm go read this blog post for more info". Basically just doing our best to raise awareness and get folks testing out -Zwasm-c-abi=spec. If things come up we can continue to refine the lint.

compiler/rustc_monomorphize/src/mono_checks/abi_check.rs

rustbot · 2025-03-17T22:00:12Z

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

These commits modify compiler targets.
(See the Target Tier Policy.)

alexcrichton · 2025-03-21T00:27:23Z

I've filed #138762 as a landing page for this lint

RalfJung · 2025-03-21T07:08:31Z

Thanks, I have updated the PR to link to that issue. The PR should be good to go then?

Nowhere in the blog post or the tracking issue do we mention that -Zwasm-c-abi=legacy suppresses the lint. That seemed to be a key part of Diplomat's transition plan, or is that not the case any more? This PR becomes simpler if we don't have that flag suppress the lint.

alexcrichton · 2025-03-21T15:25:49Z

Oh that's just an oversight on my part. @Manishearth I know you said it would be nice for -Zwasm-c-abi=legacy to suppress the lint, but can you confirm that it's desirable enough for the extra complexity in this PR?

Manishearth · 2025-03-21T15:27:55Z

Having spec turn off the lint is desirable, having legacy turn off the lint is ... only somewhat a benefit, so it's fine if you don't.

Ultimately it's just a warning

RalfJung · 2025-03-21T15:29:41Z

Ultimately I already wrote the code so it doesn't need to be a huge benefit. ;) But it if provides a benefit, it should probably occur somewhere in our recommended transition plan.

alexcrichton · 2025-03-21T15:37:52Z

Ok I've updated both #138762 and rust-lang/blog.rust-lang.org#1531 to mention the lint-silencing nature of -Zwasm-c-abi=legacy, and in that case...

@bors: r+

bors · 2025-03-21T15:37:55Z

📌 Commit 3301e0e has been approved by alexcrichton

It is now in the queue for this repository.

add FCW to warn about wasm ABI transition See rust-lang#122532 for context: the "C" ABI on wasm32-unk-unk will change. The goal of this lint is to warn about any function definition and calls whose behavior will be affected by the change. My understanding is the following: - scalar arguments are fine - including 128 bit types, they get passed as two `i64` arguments in both ABIs - `repr(C)` structs (recursively) wrapping a single scalar argument are fine (unless they have extra padding due to over-alignment attributes) - all return values are fine `@bjorn3` `@alexcrichton` `@Manishearth` is that correct? I am making this a "show up in future compat reports" lint to maximize the chances people become aware of this. OTOH this likely means warnings for most users of Diplomat so maybe we shouldn't do this? IIUC, wasm-bindgen should be unaffected by this lint as they only pass scalar types as arguments. Tracking issue: rust-lang#138762 Transition plan blog post: rust-lang/blog.rust-lang.org#1531

Rollup of 10 pull requests Successful merges: - rust-lang#137736 (Don't attempt to export compiler-builtins symbols from rust dylibs) - rust-lang#138236 (uefi: Add OwnedEvent abstraction) - rust-lang#138321 ([bootstrap] Distribute split debuginfo if present) - rust-lang#138410 (Couple mir building cleanups) - rust-lang#138490 (Forward `stream_position` in `Arc<File>` as well) - rust-lang#138535 (Cleanup `LangString::parse`) - rust-lang#138536 (stable_mir: Add `MutMirVisitor`) - rust-lang#138580 (resolve: Avoid some unstable iteration 2) - rust-lang#138601 (add FCW to warn about wasm ABI transition) - rust-lang#138631 (Update test for SGX now implementing `read_buf`) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2025-03-22T10:58:40Z

@bors r-
#138827 (comment)

RalfJung · 2025-03-22T13:50:56Z

Seems our own internal proc macro ABI is affected. That should be fine though as long as both sides of this are compiled with the same rustc.

RalfJung · 2025-03-22T17:42:58Z

I just allowed the lint in the proc_macro bridge. I hope that makes sense?
Not sure whom to ping for that bridge... @eddyb and @petrochenkov maybe?

hanna-kruppe · 2025-03-22T17:49:47Z

Why’s the bridge being compiled for wasm32-unknown-unknown anyway?

alexcrichton · 2025-03-22T17:51:11Z

@bors: r+

I believe the comment Ralf left is right, both sides of the bridge are rustc and JS isn't involved, so it doesn't matter what the exact ABI is. I believe this is built for wasm32-unknown-unknown as a standard part of any std-containing sysroot for a target. For example extern crate proc_macro; is valid on wasm32-unknown-unknown

bors · 2025-03-22T17:51:13Z

📌 Commit a75a841 has been approved by alexcrichton

It is now in the queue for this repository.

bjorn3 · 2025-03-22T18:17:22Z

I believe the comment Ralf left is right, both sides of the bridge are rustc and JS isn't involved, so it doesn't matter what the exact ABI is.

While both sides of the bridge are rustc, they can be different versions of rustc. That is why the bridge exists in the first place and uses the C ABI rather than the Rust ABI. That said as rustc doesn't even support loading proc-macros when it is itself compiled to wasm, it doesn't really matter that the ABI changes just on wasm.

rustbot assigned wesleywiser Mar 17, 2025

rustbot added S-waiting-on-review T-compiler labels Mar 17, 2025

RalfJung mentioned this pull request Mar 17, 2025

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

Open

2 tasks

RalfJung commented Mar 17, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

bjorn3 reviewed Mar 17, 2025

View reviewed changes

compiler/rustc_monomorphize/src/mono_checks/abi_check.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Mar 17, 2025

View reviewed changes

compiler/rustc_monomorphize/src/mono_checks/abi_check.rs Show resolved Hide resolved

RalfJung force-pushed the wasm-abi-fcw branch 2 times, most recently from aed14e4 to fe72062 Compare March 17, 2025 12:32

This comment has been minimized.

Sign in to view

RalfJung force-pushed the wasm-abi-fcw branch from fe72062 to a64f808 Compare March 17, 2025 12:46

alexcrichton reviewed Mar 17, 2025

View reviewed changes

RalfJung force-pushed the wasm-abi-fcw branch from a64f808 to 7a753bc Compare March 17, 2025 16:16

jieyouxu added the O-wasm label Mar 17, 2025

RalfJung force-pushed the wasm-abi-fcw branch from 7a753bc to 7c01ac5 Compare March 17, 2025 16:27

Manishearth reviewed Mar 17, 2025

View reviewed changes

compiler/rustc_monomorphize/src/mono_checks/abi_check.rs Show resolved Hide resolved

RalfJung force-pushed the wasm-abi-fcw branch from d0bcd14 to 609809f Compare March 17, 2025 22:01

This was referenced Mar 21, 2025

Tracking issue for wasm32-unknown-unknown's future-incompat warning for C ABI changes #138762

Open

C ABI Changes for wasm32-unknown-unknown rust-lang/blog.rust-lang.org#1531

Open

RalfJung added 2 commits March 21, 2025 08:08

add FCW to warn about wasm ABI transition

a408043

make -Zwasm-c-abi=legacy suppress the lint

3301e0e

RalfJung force-pushed the wasm-abi-fcw branch from 609809f to 3301e0e Compare March 21, 2025 07:08

bors added S-waiting-on-bors and removed S-waiting-on-review labels Mar 21, 2025

tgross35 mentioned this pull request Mar 22, 2025

Rollup of 10 pull requests #138827

Closed

bors added S-waiting-on-author and removed S-waiting-on-bors labels Mar 22, 2025

allow wasm_c_abi in proc_macro bridge

a75a841

bors added S-waiting-on-bors and removed S-waiting-on-author labels Mar 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add FCW to warn about wasm ABI transition #138601

add FCW to warn about wasm ABI transition #138601

RalfJung commented Mar 17, 2025 •

edited

Loading

rustbot commented Mar 17, 2025

RalfJung Mar 17, 2025

This comment has been minimized.

bjorn3 commented Mar 17, 2025

hanna-kruppe commented Mar 17, 2025

This comment has been minimized.

alexcrichton commented Mar 17, 2025

alexcrichton Mar 17, 2025

RalfJung Mar 17, 2025

Manishearth Mar 17, 2025

RalfJung commented Mar 17, 2025 •

edited

Loading

alexcrichton commented Mar 17, 2025

rustbot commented Mar 17, 2025

alexcrichton commented Mar 21, 2025

RalfJung commented Mar 21, 2025

alexcrichton commented Mar 21, 2025

Manishearth commented Mar 21, 2025

RalfJung commented Mar 21, 2025

alexcrichton commented Mar 21, 2025

bors commented Mar 21, 2025

matthiaskrgr commented Mar 22, 2025

RalfJung commented Mar 22, 2025 via email

RalfJung commented Mar 22, 2025

hanna-kruppe commented Mar 22, 2025

alexcrichton commented Mar 22, 2025

bors commented Mar 22, 2025

bjorn3 commented Mar 22, 2025

add FCW to warn about wasm ABI transition #138601

Are you sure you want to change the base?

add FCW to warn about wasm ABI transition #138601

Conversation

RalfJung commented Mar 17, 2025 • edited Loading

rustbot commented Mar 17, 2025

RalfJung Mar 17, 2025

Choose a reason for hiding this comment

This comment has been minimized.

bjorn3 commented Mar 17, 2025

hanna-kruppe commented Mar 17, 2025

This comment has been minimized.

alexcrichton commented Mar 17, 2025

alexcrichton Mar 17, 2025

Choose a reason for hiding this comment

RalfJung Mar 17, 2025

Choose a reason for hiding this comment

Manishearth Mar 17, 2025

Choose a reason for hiding this comment

RalfJung commented Mar 17, 2025 • edited Loading

alexcrichton commented Mar 17, 2025

rustbot commented Mar 17, 2025

alexcrichton commented Mar 21, 2025

RalfJung commented Mar 21, 2025

alexcrichton commented Mar 21, 2025

Manishearth commented Mar 21, 2025

RalfJung commented Mar 21, 2025

alexcrichton commented Mar 21, 2025

bors commented Mar 21, 2025

matthiaskrgr commented Mar 22, 2025

RalfJung commented Mar 22, 2025 via email

RalfJung commented Mar 22, 2025

hanna-kruppe commented Mar 22, 2025

alexcrichton commented Mar 22, 2025

bors commented Mar 22, 2025

bjorn3 commented Mar 22, 2025

RalfJung commented Mar 17, 2025 •

edited

Loading

RalfJung commented Mar 17, 2025 •

edited

Loading