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

C ABI Changes for wasm32-unknown-unknown #1531

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Mar 21, 2025

This adds a post for changes discussed in
rust-lang/rust#122532 and rust-lang/rust#138601, notably as a result of this decision:
rust-lang/rust#122532 (comment)

This adds a post for changes discussed in
rust-lang/rust#122532 and
rust-lang/rust#138601, notably as a result of
this decision:
rust-lang/rust#122532 (comment)
@alexcrichton
Copy link
Member Author

I'll note that at this time I'm opening this up to share with folks and get feedback, there's no concrete timeline for when to publish this. My hope for now is "soon after rust-lang/rust#138601"

@alexcrichton
Copy link
Member Author

I'll also note, wow, this issue has a huge and sprawling history. I've surely missed something and wanted to clarify that it's not intentional. Let me know!

with `-Zwasm-c-abi=spec`. If all that passes then you're good to go and the
upcoming change to the C ABI will not affect your project.

## I'm affected, now what?
Copy link
Member

Choose a reason for hiding this comment

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

this looks great, thanks!

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

That's a lot of detail, thanks!

I think what would help is to have a short sentence, before the history lessons, telling people where to jump to in case they just want to get the gist of it and read up on the history later.

alexcrichton and others added 4 commits March 21, 2025 10:28

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Ralf Jung <[email protected]>
Co-authored-by: Eric Huss <[email protected]>
@alexcrichton alexcrichton marked this pull request as ready for review March 21, 2025 15:38
@alexcrichton
Copy link
Member Author

alexcrichton commented Mar 21, 2025

I believe I've addressed all feedback here and I've also marked this as ready-to-go, but I'll drop a comment here after a nightly is available since I think it would be best to wait for a compiler first before publishing this.

tgross35 added a commit to tgross35/rust that referenced this pull request Mar 22, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 24, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2025
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 added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2025
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 added a commit to rust-lang-ci/rust that referenced this pull request Mar 25, 2025
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 added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2025
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
@alexcrichton
Copy link
Member Author

rust-lang/rust#138601 has now landed, but it didn't make today's nightly, so best to wait one more day for this

github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Mar 27, 2025
add FCW to warn about wasm ABI transition

See rust-lang/rust#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/rust#138762
Transition plan blog post: rust-lang/blog.rust-lang.org#1531

try-job: dist-various-2
@alexcrichton
Copy link
Member Author

Ok I've added the date of a rustc that has this change now, and this should be good to go

@@ -0,0 +1,273 @@
+++
layout = "post"
date = 2024-03-20
Copy link
Contributor

Choose a reason for hiding this comment

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

The date should be updated to whatever date it goes out.

Copy link
Contributor

Choose a reason for hiding this comment

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

And make sure it is the correct year. 😜

Copy link
Member Author

Choose a reason for hiding this comment

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

Oop yes thanks! Updated to tomorrow, but happy to update this again if needed

@ehuss
Copy link
Contributor

ehuss commented Mar 28, 2025

Just to ensure this doesn't block without a clear owner, I assume this is on behalf of the compiler team, so @davidtwco or @wesleywiser I assume this would be yours to approve?

@@ -194,7 +194,7 @@ To determine the impact to your project there are a few tools at your disposal:
Code can use `-Zwasm-c-abi=spec` to use the standard definition of the C ABI
for a crate to test out if changes work.

The best way to test your crate is to compile with the most recent Nightly
The best way to test your crate is to compile with a `nightly-2025-03-27`
Copy link
Member

Choose a reason for hiding this comment

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

or more recent, right?

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.

None yet

5 participants