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

wasm32-unknown-unknown "C" ABI doesn't use proper ABI adjustments #115666

Open
RalfJung opened this issue Sep 8, 2023 · 5 comments · May be fixed by #133952
Open

wasm32-unknown-unknown "C" ABI doesn't use proper ABI adjustments #115666

RalfJung opened this issue Sep 8, 2023 · 5 comments · May be fixed by #133952
Labels
A-ABI Area: Concerning the application binary interface (ABI) O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Sep 8, 2023

rustc has a PassMode enum to declare how arguments should be passed between functions. PassMode::Direct is used for the simple case where we can just generate LLVM IR with an argument type that matches the usual LLVM type for this Rust type. It should only be used for types with Scalar or Vector ABI; for larger types, Rust will generate LLVM struct/union types and those

  • are sometimes adjusted to obtain better code generation
  • make it very hard to reason about our effective ABI, since it depends on how LLVM lowers those complex struct types into an effective ABI. In particular, this causes a huge risk of violating our ABI compatibility guarantees, e.g. for repr(transparent) types.

Unfortunately the compiler never had an assertion ensuring that PassMode::Direct is only used with Scalar/Vector types, and so one target accidentally broke this rule: wasm32-unknown-unknown. Later all wasm targets got an extern "wasm" ABI that has the same issue. We should find some way to fix this target, as the current situation makes life harder for anyone wanting to tweak our Rust-to-LLVM-type-mapping, and for alternative backends that have to match our effective ABI.

The issue is that last time @bjorn3 tried to fix this, it lead to #81386 and had to be reverted. It will be basically impossible to do this change without breaking the ABI at least for some cases. I'm not sure how much ABI breakage we can get away with. This might have to be blocked on rust-lang/compiler-team#672 which should help to implement the current effective ABI at least for types that are ABI-stable (i.e., repr(C) and repr(transparent)).

Cc #122532

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 8, 2023
@RalfJung RalfJung added O-wasm Target: WASM (WebAssembly), http://webassembly.org/ A-ABI Area: Concerning the application binary interface (ABI) labels Sep 8, 2023
@saethlin saethlin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 10, 2023
@kjetilkjeka

This comment was marked as off-topic.

@RalfJung

This comment was marked as off-topic.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 27, 2023
…exception, r=workingjubilee,RalfJung

NVPTX: Allow PassMode::Direct for ptx kernels for now

Upgrading the nvptx toolchain to the newest nightly makes it hit the assert that links to rust-lang#115666

It seems like most targets get around this by using `PassMode::Indirect`. That is impossible for the kernel as it's not a normal call, but instead the arguments are copied from CPU to GPU and the passed pointer would be invalid when it reached the GPU.

I also made an experiment with `PassMode::Cast` but at least the most simple version of this broke the assembly API tests.

I added  fixing the pass mode in my unofficial tracking issue list (I do not have the necessary permissions to update to official one). rust-lang#38788 (comment)

Since the ptx_abi is currently unstable and have been working with `PassMode::Direct` for more than a year now, the steps above is hopefully sufficient to enable it as an exception until I can prioritize to fix it. I'm currently looking at steps to enable the CI for nvptx64 again and would prefer to finish that first.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 27, 2023
Rollup merge of rust-lang#117247 - kjetilkjeka:nvptx_direct_passmode_exception, r=workingjubilee,RalfJung

NVPTX: Allow PassMode::Direct for ptx kernels for now

Upgrading the nvptx toolchain to the newest nightly makes it hit the assert that links to rust-lang#115666

It seems like most targets get around this by using `PassMode::Indirect`. That is impossible for the kernel as it's not a normal call, but instead the arguments are copied from CPU to GPU and the passed pointer would be invalid when it reached the GPU.

I also made an experiment with `PassMode::Cast` but at least the most simple version of this broke the assembly API tests.

I added  fixing the pass mode in my unofficial tracking issue list (I do not have the necessary permissions to update to official one). rust-lang#38788 (comment)

Since the ptx_abi is currently unstable and have been working with `PassMode::Direct` for more than a year now, the steps above is hopefully sufficient to enable it as an exception until I can prioritize to fix it. I'm currently looking at steps to enable the CI for nvptx64 again and would prefer to finish that first.
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Oct 28, 2023
…, r=workingjubilee,RalfJung

NVPTX: Allow PassMode::Direct for ptx kernels for now

Upgrading the nvptx toolchain to the newest nightly makes it hit the assert that links to rust-lang/rust#115666

It seems like most targets get around this by using `PassMode::Indirect`. That is impossible for the kernel as it's not a normal call, but instead the arguments are copied from CPU to GPU and the passed pointer would be invalid when it reached the GPU.

I also made an experiment with `PassMode::Cast` but at least the most simple version of this broke the assembly API tests.

I added  fixing the pass mode in my unofficial tracking issue list (I do not have the necessary permissions to update to official one). rust-lang/rust#38788 (comment)

Since the ptx_abi is currently unstable and have been working with `PassMode::Direct` for more than a year now, the steps above is hopefully sufficient to enable it as an exception until I can prioritize to fix it. I'm currently looking at steps to enable the CI for nvptx64 again and would prefer to finish that first.
@LegNeato
Copy link
Contributor

FWIW, rust-gpu had to use the new make_direct_deprecated(): https://github.com/EmbarkStudios/rust-gpu/pull/1109/files#diff-9ff3a35aa9e19911b0f93cebdc627768b3ca62ee19cb2c233c787606df7a4426R67. I don't know enough about either project to know if it is the same issue as this or if it should be fixed on the rust-gpu side, but I wanted to add a link here for posterity.

@RalfJung
Copy link
Member Author

Hm yeah that's not great, though it might be related to #117271 given both are about GPUs.

@eddyb
Copy link
Member

eddyb commented Aug 2, 2024

Hm yeah that's not great, though it might be related to #117271 given both are about GPUs.

I'm reviewing the change @LegNeato mentioned and ended up here semi-randomly, so I might as well answer this:

Rust-GPU only started using PassMode::Direct:

  • mostly to avoid PassMode::Cast, as it's transmute-like
    (as it gets into "antiquated GPU typed memory lacking unions" territory, or at least ptr<->int transmutes - and even with those being theoretically solvable, in the short term it helps to not even have the problem)
  • skipping PassMode::Indirect as well avoids having to eliminate memory accesses (i.e. via equivalents of LLVM mem2reg+SROA)
    (not strictly necessary: calls or even just moving values around already likely introduces this problem intra-function, and any calls with problematic indirection would get inlined anyway)

So it probably makes most sense to turn PassMode::Cast into PassMode::Indirect instead, anyway (which AFAICT is equivalent to using ArgAbi::new and just never doing any adjustment logic beyond "ignore ZSTs").

And while writing this comment I went ahead and tried it and the only issues stem from lacking extra logic to handle the indirect case in a few places (mainly the format_args! "decompiler"), so Rust-GPU wouldn't get impacted by make_direct_deprecated being removed, beyond a bit of busywork.
(which I'm only punting on in the very short term because of similar-yet-different changes needed to update to newer nightlies etc., but after that I'll be happy to get rid of another hack from Rust-GPU)

@RalfJung RalfJung changed the title wasm32-unknown-unknown "C" ABI, and "wasm" ABI on all wasm targets, don't use proper ABI adjustments wasm32-unknown-unknown "C" ABI doesn't use proper ABI adjustments Aug 24, 2024
newpavlov pushed a commit to rust-random/getrandom that referenced this issue Aug 26, 2024
wasm-bindgen 0.2.62 is not compatible with a wasm ABI change that rustc
wishes to enable by default for wasm32-unknown-unknown, currently gated
behind passing the -Zwasm-c-abi flag to rustc.

wasm-bindgen 0.2.89 should exhibit seamless behavior before and after
the ABI change to match the C ABI, so depend on that.

For more information, see
- rust-lang/rust#115666
- rust-lang/rust#117918
- rust-lang/rust#122532
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants