Skip to content

Introduce perma-unstable wasm-c-abi flag #117919

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Apr 19, 2024
Merged

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Nov 14, 2023

Now that wasm-bindgen v0.2.88 supports the spec-compliant C ABI, the idea is to switch to that in a future version of Rust. In the meantime it would be good to let people test and play around with it.

This PR introduces a new perma-unstable -Zwasm-c-abi compiler flag, which switches to the new spec-compliant C ABI when targeting wasm32-unknown-unknown.

Alternatively, we could also stabilize this and then deprecate it when we switch. I will leave this to the Rust maintainers to decide.

This is a companion PR to #117918, but they could be merged independently.
MCP: rust-lang/compiler-team#703
Tracking issue: #122532

@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2023

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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 Nov 14, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2023

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

Comment on lines 1833 to 1821
pub trait HasWasmCAbiOpt {
fn wasm_c_abi_opt(&self) -> bool;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the best I could come up with to keep the logic inside of Target::adjust_abi(), I believe moving this logic outside of it would introduce some very hacky code that could potentially become a trap for other contributors.

I'm pretty unfamiliar with the compiler though, so any advice would be appreciated.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2023

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

@rust-log-analyzer

This comment has been minimized.

@daxpedda daxpedda force-pushed the wasm-c-abi branch 2 times, most recently from 571d6df to b5d3f7b Compare November 16, 2023 08:36
@rustbot
Copy link
Collaborator

rustbot commented Nov 16, 2023

Some changes occurred in src/tools/cargo

cc @ehuss

@daxpedda
Copy link
Contributor Author

Apologies @ehuss, I messed up during rebasing onto master and mistakenly changed the Cargo submodule!

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Dec 1, 2023

☔ The latest upstream changes (presumably #116892) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member

Sorry, I'm extremely busy

r? compiler

@petrochenkov
Copy link
Contributor

r? compiler
I'm overloaded with reviews.

@rustbot rustbot assigned b-naber and unassigned petrochenkov Dec 5, 2023
@b-naber
Copy link
Contributor

b-naber commented Dec 11, 2023

The companion PR is lang-nominated, if the lang team approves that change this one could be merged too, but if this PR would be merged independently it would need at least an MCP, I think, since it introduces a new flag.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 18, 2024
Introduce perma-unstable `wasm-c-abi` flag

Now that `wasm-bindgen` v0.2.88 supports the spec-compliant C ABI, the idea is to switch to that in a future version of Rust. In the meantime it would be good to let people test and play around with it.

This PR introduces a new perma-unstable `-Zwasm-c-abi` compiler flag, which switches to the new spec-compliant C ABI when targeting `wasm32-unknown-unknown`.

Alternatively, we could also stabilize this and then deprecate it when we switch. I will leave this to the Rust maintainers to decide.

This is a companion PR to rust-lang#117918, but they could be merged independently.
MCP: rust-lang/compiler-team#703
Tracking issue: rust-lang#122532
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Apr 18, 2024
Introduce perma-unstable `wasm-c-abi` flag

Now that `wasm-bindgen` v0.2.88 supports the spec-compliant C ABI, the idea is to switch to that in a future version of Rust. In the meantime it would be good to let people test and play around with it.

This PR introduces a new perma-unstable `-Zwasm-c-abi` compiler flag, which switches to the new spec-compliant C ABI when targeting `wasm32-unknown-unknown`.

Alternatively, we could also stabilize this and then deprecate it when we switch. I will leave this to the Rust maintainers to decide.

This is a companion PR to rust-lang#117918, but they could be merged independently.
MCP: rust-lang/compiler-team#703
Tracking issue: rust-lang#122532
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 18, 2024
…kingjubilee

Rollup of 6 pull requests

Successful merges:

 - rust-lang#117919 (Introduce perma-unstable `wasm-c-abi` flag)
 - rust-lang#123571 (Correctly change type when adding adjustments on top of `NeverToAny`)
 - rust-lang#123752 (Properly handle emojis as literal prefix in macros)
 - rust-lang#123980 ( Add an opt-in to store incoming edges in `VecGraph` + misc)
 - rust-lang#124110 (Fix negating `f16` and `f128` constants)
 - rust-lang#124116 (when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Apr 18, 2024
Introduce perma-unstable `wasm-c-abi` flag

Now that `wasm-bindgen` v0.2.88 supports the spec-compliant C ABI, the idea is to switch to that in a future version of Rust. In the meantime it would be good to let people test and play around with it.

This PR introduces a new perma-unstable `-Zwasm-c-abi` compiler flag, which switches to the new spec-compliant C ABI when targeting `wasm32-unknown-unknown`.

Alternatively, we could also stabilize this and then deprecate it when we switch. I will leave this to the Rust maintainers to decide.

This is a companion PR to rust-lang#117918, but they could be merged independently.
MCP: rust-lang/compiler-team#703
Tracking issue: rust-lang#122532
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 19, 2024
…kingjubilee

Rollup of 9 pull requests

Successful merges:

 - rust-lang#117919 (Introduce perma-unstable `wasm-c-abi` flag)
 - rust-lang#123406 (Force exhaustion in iter::ArrayChunks::into_remainder)
 - rust-lang#123752 (Properly handle emojis as literal prefix in macros)
 - rust-lang#123935 (Don't inline integer literals when they overflow - new attempt)
 - rust-lang#123980 ( Add an opt-in to store incoming edges in `VecGraph` + misc)
 - rust-lang#124019 (Use raw-dylib for Windows synchronization functions)
 - rust-lang#124110 (Fix negating `f16` and `f128` constants)
 - rust-lang#124112 (Fix ICE when there is a non-Unicode entry in the incremental crate directory)
 - rust-lang#124116 (when suggesting RUST_BACKTRACE=1, add a special note for Miri's env var isolation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Collaborator

bors commented Apr 19, 2024

⌛ Testing commit 9e2c658 with merge 13e63f7...

@bors
Copy link
Collaborator

bors commented Apr 19, 2024

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 13e63f7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 19, 2024
@bors bors merged commit 13e63f7 into rust-lang:master Apr 19, 2024
@rustbot rustbot added this to the 1.79.0 milestone Apr 19, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (13e63f7): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.2% [0.2%, 0.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.2%, 0.2%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.1% [2.1%, 4.4%] 8
Regressions ❌
(secondary)
2.7% [2.1%, 3.5%] 11
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.1% [2.1%, 4.4%] 8

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 673.311s -> 673.499s (0.03%)
Artifact size: 315.21 MiB -> 315.24 MiB (0.01%)

GuillaumeGomez pushed a commit to GuillaumeGomez/rust that referenced this pull request Jul 10, 2024
Introduce perma-unstable `wasm-c-abi` flag

Now that `wasm-bindgen` v0.2.88 supports the spec-compliant C ABI, the idea is to switch to that in a future version of Rust. In the meantime it would be good to let people test and play around with it.

This PR introduces a new perma-unstable `-Zwasm-c-abi` compiler flag, which switches to the new spec-compliant C ABI when targeting `wasm32-unknown-unknown`.

Alternatively, we could also stabilize this and then deprecate it when we switch. I will leave this to the Rust maintainers to decide.

This is a companion PR to rust-lang#117918, but they could be merged independently.
MCP: rust-lang/compiler-team#703
Tracking issue: rust-lang#122532
@nikic nikic mentioned this pull request Jul 10, 2024
@Manishearth
Copy link
Member

Should this have been a -Cwasm-c-abi flag? I was under the impression -Z was for debugging and -C was for codegen

@teohhanhui
Copy link
Contributor

@Manishearth
Copy link
Member

ah, I see!

warpfork added a commit to warpfork/datamaps that referenced this pull request Dec 7, 2024
This... finally breaks down and requires rust nightly.
I've been going to considerable lengths to try to avoid this, but...
I earnestly don't think shipping C+Rust to wasm is possible without it.

The main new thing here is the "-Zwasm-c-abi=spec" flag.
As described in the README diff, it's essential for rust to call
into C correctly when both are wasm'd.

(It's beyond me why any other mode exists, but I'll... attempt to bite
my tongue.)

This critical flag was introduced in
rust-lang/rust#117919 .

Which describes it as "perma-unstable".  And gosh I hope that's not
actually going to be true... because again, this seems to be flat out
*required* for Rust and C to play together in wasm.  And also because
"perma-unstable" is just a deeply silly concept, period, context-free.

I do not understand any possible reason to want to push people
*permanently* towards using "unstable" "nightly" compilers.
I don't understand how this needs to be said, but creating a situation
where the words "permanently unstable" go together is a Bad Idea.

Anyway.

The "target-feature=+bulk-memory" flag also comes along because without
that, we get a "Uncaught RangeError: Maximum call stack size exceeded"
error at runtime.  It's attributed to
"core::intrinsics::copy::precondition_check" called from "memmove".
(That doesn't make a ton of sense to me, looking at the code, but...
a lot of things don't make sense to me, looking at any of this; add
it to the pile.)  Whatever the reason, this flag makes it go.

And now it's alive.

It's taken days.  But hello world has landed.  Hooray.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.