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

support wasm inline assembly in naked_asm! #135648

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

Conversation

folkertdev
Copy link
Contributor

fixes #135518

Webassembly was overlooked previously, but now naked_asm! and #[naked] functions work on the webassembly targets.

Or, they almost do right now. I guess this is no surprise, but the wasm32-unknown-unknown target causes me some trouble. I'll add some inline comments with more details.

r? @bjorn3

cc @daxpedda, @tgross35

@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 Jan 17, 2025
@rust-log-analyzer

This comment has been minimized.

@tgross35
Copy link
Contributor

tgross35 commented Jan 17, 2025

@rustbot ping wasm

@rustbot

This comment was marked as outdated.

@rustbot rustbot added the O-wasm Target: WASM (WebAssembly), http://webassembly.org/ label Jan 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2025

Hey WASM notification group! This issue or PR could use some WebAssembly-specific
guidance. Could one of you weigh in? Thanks <3

(In case it's useful, here are some instructions for tackling these sorts of
issues).

cc @alexcrichton @burakemir @hoodmane @juntyr

@bjorn3
Copy link
Member

bjorn3 commented Jan 17, 2025

To clarify how the wasm32-unknown-unknown ABI is broken: For each ABI we implement there is supposed to be some code adjusting the PassMode from something guessed based on the BackendRepr to the actual ABI for the target. When wasm32-unknown-unknown we unfortunately forgot to implement this code causing us to pass everything as PassMode::Direct or PassMode::Pair. In doing so the internal representation of rust types at the LLVM IR level leaked out as well as causing LLVM to make up some random ABI for struct types. While we discovered this years ago, unfortunately by that point wasm-bindgen depended on the broken ABI. As such newer wasm targets like wasm32-unknown-emscripten and wasm32-wasip1 did get the correct ABI implemented, but wasm32-unknown-unknown was forced to keep the broken ABI. Only somewhat recently did wasm-bindgen add support for the correct ABI allowing us to move to the correct ABI on wasm32-unknown-unknown too in the hopefully near future. #133951 will make using an old wasm-bindgen from before the fix a hard error, while #133952 will move wasm32-unknown-unknown to the correct ABI.

Comment on lines 356 to 378
// FIXME: remove this branch once the wasm32-unknown-unknown ABI is fixed
bug!(
"cannot use memory args (the wasm32-unknown-unknown ABI is broken, see https://github.com/rust-lang/rust/issues/115666"
);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to refer to WasmCAbi::Legacy whenever these bug! statements are emitted? That should help ensure that when #133952 lands this'll all get naturally updated.

Additionally, would it be possible to attach a span of some kind to this bug! or the unreachable! statements (or asserts)? In case this triggers and ICE that might make it much easier to track down what the cause is

Copy link
Contributor Author

@folkertdev folkertdev Jan 17, 2025

Choose a reason for hiding this comment

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

sure, I added some let _ = WasmCAbi::Legacy; statements that will make noise when the WasmCAbi enum gets removed, and the bug!s are now tied to the span of the surrounding naked function.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks great to me! (although someone else should probably also sign off on this how knows more about naked functions and naked_asm than I)

@bjorn3
Copy link
Member

bjorn3 commented Jan 20, 2025

r=me with the above changes

@tgross35
Copy link
Contributor

@bors r=bjorn3

@bors
Copy link
Contributor

bors commented Jan 20, 2025

📌 Commit bcf478b has been approved by bjorn3

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 20, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Jan 21, 2025
support wasm inline assembly in `naked_asm!`

fixes rust-lang#135518

Webassembly was overlooked previously, but now `naked_asm!` and `#[naked]` functions work on the webassembly targets.

Or, they almost do right now. I guess this is no surprise, but the `wasm32-unknown-unknown` target causes me some trouble. I'll add some inline comments with more details.

r? `@bjorn3`

cc `@daxpedda,` `@tgross35`
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jan 21, 2025
support wasm inline assembly in `naked_asm!`

fixes rust-lang#135518

Webassembly was overlooked previously, but now `naked_asm!` and `#[naked]` functions work on the webassembly targets.

Or, they almost do right now. I guess this is no surprise, but the `wasm32-unknown-unknown` target causes me some trouble. I'll add some inline comments with more details.

r? ``@bjorn3``

cc ``@daxpedda,`` ``@tgross35``
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jan 21, 2025
support wasm inline assembly in `naked_asm!`

fixes rust-lang#135518

Webassembly was overlooked previously, but now `naked_asm!` and `#[naked]` functions work on the webassembly targets.

Or, they almost do right now. I guess this is no surprise, but the `wasm32-unknown-unknown` target causes me some trouble. I'll add some inline comments with more details.

r? ```@bjorn3```

cc ```@daxpedda,``` ```@tgross35```
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2025
Rollup of 10 pull requests

Successful merges:

 - rust-lang#132232 (CI: build FreeBSD artifacts on FreeBSD 13.4)
 - rust-lang#135625 ([cfg_match] Document the use of expressions.)
 - rust-lang#135638 (Make it possible to build GCC on CI)
 - rust-lang#135648 (support wasm inline assembly in `naked_asm!`)
 - rust-lang#135707 (Shorten linker output even more when `--verbose` is not present)
 - rust-lang#135750 (Add an example of using `carrying_mul_add` to write wider multiplication)
 - rust-lang#135779 (CI: free disk on linux arm runner)
 - rust-lang#135793 (Ignore `mermaid.min.js`)
 - rust-lang#135810 (Add Kobzol on vacation)
 - rust-lang#135814 (ci: use ghcr buildkit image)

r? `@ghost`
`@rustbot` modify labels: rollup
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Jan 22, 2025
support wasm inline assembly in `naked_asm!`

fixes rust-lang#135518

Webassembly was overlooked previously, but now `naked_asm!` and `#[naked]` functions work on the webassembly targets.

Or, they almost do right now. I guess this is no surprise, but the `wasm32-unknown-unknown` target causes me some trouble. I'll add some inline comments with more details.

r? ````@bjorn3````

cc ````@daxpedda,```` ````@tgross35````
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2025
Rollup of 8 pull requests

Successful merges:

 - rust-lang#135557 (Point at invalid utf-8 span on user's source code)
 - rust-lang#135596 (Properly note when query stack is being cut off)
 - rust-lang#135638 (Make it possible to build GCC on CI)
 - rust-lang#135648 (support wasm inline assembly in `naked_asm!`)
 - rust-lang#135826 (Misc. `rustc_resolve` cleanups)
 - rust-lang#135827 (CI: free disk with in-tree script instead of GitHub Action)
 - rust-lang#135850 (Update the `wasm-component-ld` tool)
 - rust-lang#135855 (Only assert the `Parser` size on specific arches)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-wasm Target: WASM (WebAssembly), http://webassembly.org/ 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.

naked_asm! on wasm emits invalid assembly
7 participants