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

try-job: dist-various-2

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.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#138601 (add FCW to warn about wasm ABI transition) - rust-lang#138662 (Implement some basics in UEFI fs) - rust-lang#138800 (remove remnants of const_box feature) - rust-lang#138821 (match lowering cleanup: remove unused unsizing logic from `non_scalar_compare`) - rust-lang#138864 (Rework `--print` options documentation) - rust-lang#138868 (Add do_not_recommend typo help) - rust-lang#138882 (`with_scope` is only ever used for ast modules) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2025-03-24T19:39:12Z

@bors r- rollup=iffy
#138890 (comment)

RalfJung · 2025-03-25T07:16:31Z

Eh, but I have allowed the lint for the entire module, why does it still trigger?

RalfJung · 2025-03-25T07:22:59Z

Locally a wasm32-unknown-unknown build of proc_macro works just fine... wtf.

@bors try

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 try-job: dist-various-2

bors · 2025-03-25T07:24:12Z

⌛ Trying commit 61e24e6 with merge cb87fb8ade9ebde8ee368f236e61795631f523a2...

bors · 2025-03-25T08:17:18Z

💔 Test failed - checks-actions

RalfJung · 2025-03-25T10:04:43Z

Yeah same error. No idea what is happening.

library/proc_macro/src/bridge/mod.rs

RalfJung · 2025-03-25T10:29:48Z

@bors try

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 try-job: dist-various-2

bors · 2025-03-25T10:31:04Z

⌛ Trying commit 782a797 with merge fde8c7793c0fdf859d230a4b86205a9495d2965e...

RalfJung · 2025-03-25T10:32:10Z

@bors try

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 try-job: dist-various-2

bors · 2025-03-25T10:33:23Z

⌛ Trying commit e88c49c with merge c19c3fbac0c3230987fe8ccfcb40573cd8ef9b14...

RalfJung · 2025-03-25T11:18:25Z

r? @alexcrichton
Hopefully CI will pass this time... but there's a new commit someone should look at :)

bors · 2025-03-25T11:43:56Z

☀️ Try build successful - checks-actions
Build commit: c19c3fb (c19c3fbac0c3230987fe8ccfcb40573cd8ef9b14)

rustbot assigned wesleywiser Mar 17, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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 Target: WASM (WebAssembly), http://webassembly.org/ 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 force-pushed the wasm-abi-fcw branch from 609809f to 3301e0e Compare March 21, 2025 07:08

matthiaskrgr mentioned this pull request Mar 24, 2025

Rollup of 7 pull requests #138890

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 24, 2025

RalfJung force-pushed the wasm-abi-fcw branch from a75a841 to 3895729 Compare March 25, 2025 07:18

RalfJung added 3 commits March 25, 2025 08:22

add FCW to warn about wasm ABI transition

c961d12

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

072ccce

allow wasm_c_abi in proc_macro bridge

61e24e6

RalfJung force-pushed the wasm-abi-fcw branch from 3895729 to 61e24e6 Compare March 25, 2025 07:22

This comment has been minimized.

Sign in to view

bjorn3 reviewed Mar 25, 2025

View reviewed changes

library/proc_macro/src/bridge/mod.rs Show resolved Hide resolved

acquire more accurate HirId for ABI check lints

e88c49c

RalfJung force-pushed the wasm-abi-fcw branch from 782a797 to e88c49c Compare March 25, 2025 10:31

rustbot assigned alexcrichton and unassigned wesleywiser Mar 25, 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

matthiaskrgr commented Mar 24, 2025

RalfJung commented Mar 25, 2025

RalfJung commented Mar 25, 2025

bors commented Mar 25, 2025

This comment has been minimized.

bors commented Mar 25, 2025

RalfJung commented Mar 25, 2025 via email

RalfJung commented Mar 25, 2025

bors commented Mar 25, 2025

RalfJung commented Mar 25, 2025

bors commented Mar 25, 2025

RalfJung commented Mar 25, 2025

bors commented Mar 25, 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

matthiaskrgr commented Mar 24, 2025

RalfJung commented Mar 25, 2025

RalfJung commented Mar 25, 2025

bors commented Mar 25, 2025

This comment has been minimized.

bors commented Mar 25, 2025

RalfJung commented Mar 25, 2025 via email

RalfJung commented Mar 25, 2025

bors commented Mar 25, 2025

RalfJung commented Mar 25, 2025

bors commented Mar 25, 2025

RalfJung commented Mar 25, 2025

bors commented Mar 25, 2025

RalfJung commented Mar 17, 2025 •

edited

Loading

RalfJung commented Mar 17, 2025 •

edited

Loading