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

[Migrated] core::mem::swap used to work, then broke (and later accidentally got unbroken again). #117

Open
rust-gpu-bot opened this issue Nov 13, 2024 · 0 comments

Comments

@rust-gpu-bot
Copy link

Issue automatically imported from old repo: EmbarkStudios/rust-gpu#804
Old labels: t: bug,a: rust-lang,t: external,s: qptr may fix
Originally creatd by hatoo on 2021-11-21T13:05:53Z


Expected Behaviour

Codes that uses core::mem::swap

#![cfg_attr(
    target_arch = "spirv",
    no_std,
    feature(register_attr),
    register_attr(spirv)
)]

#[cfg(not(target_arch = "spirv"))]
use spirv_std::macros::spirv;

use spirv_std as _;

#[spirv(vertex)]
pub fn test_vs() {
    let mut x1 = 0.0f32;
    let mut x2 = 1.0f32;

    core::mem::swap(&mut x1, &mut x2);
}

will compile. it's OK in previous version at least b9867d0.

Example & Steps To Reproduce

  1. Clone https://github.com/hatoo/rust-gpu-issue/tree/swap-fail (note: swap-fail branch)
  2. cargo build and error
> cargo build
   Compiling builder v0.1.0 (C:\Users\hato2\Desktop\rust-gpu-issue\builder)
error: failed to run custom build command for `builder v0.1.0 (C:\Users\hato2\Desktop\rust-gpu-issue\builder)`

Caused by:
  process didn't exit successfully: `C:\Users\hato2\Desktop\rust-gpu-issue\target\debug\build\builder-caf084bc583adeed\build-script-build` (exit code: 1)
  --- stderr
     Compiling shader v0.1.0 (C:\Users\hato2\Desktop\rust-gpu-issue\shader)
  error: Cannot memcpy dynamically sized data
      --> C:\Users\hato2\.rustup\toolchains\nightly-2021-10-26-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\intrinsics.rs:2057:14
       |
  2057 |     unsafe { copy_nonoverlapping(src, dst, count) }
       |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

  error: could not compile `shader` due to previous error
  Error: BuildFailed

It's OK in the previous version. Please see https://github.com/hatoo/rust-gpu-issue/tree/swap-succ.

System Info

  • Rust: rustc 1.58.0-nightly (29b124802 2021-10-25)
  • OS: Windows 11
  • GPU: RTX2080ti
  • SPIR-V: SPIRV-Tools v2021.3 v2021.3

Backtrace

Backtrace

<backtrace>

@rust-gpu-bot
Copy link
Author

Comment from eddyb (CONTRIBUTOR) on 2021-11-22T14:33:16Z 👍: 1


In #716 we updated to a version that supports mem::replace again, thanks to rust-lang/rust#87827, but mem::swap indeed did not get fixed the same way - if someone wants to try and land upstream something like that PR but for mem::swap, feel free to.

I gave up on it because of how rare mem::swap is compared to mem::replace (which gets used everywhere in core, e.g. Option::take(self) is mem::replace(self, None) and range iterators also depend on it).

There's a more general fix at rust-lang/rust#86699 but it requires a bunch of unstable feature-gating stuff I haven't gone back to for.

@rust-gpu-bot
Copy link
Author

Comment from oisyn (CONTRIBUTOR) on 2023-03-29T13:32:12Z


Closing as won't fix, it's very unlikely we will address this in the forseeable future.

@rust-gpu-bot
Copy link
Author

Comment from eddyb (CONTRIBUTOR) on 2023-10-01T17:15:11Z


This actually works in Rust-GPU 0.4, and more specifically since nightly-2022-02-28, thanks to:

But at the same time, I wanted to propose to upstream that they can remove the target_arch = "spirv" hack, as it got brought up in this issue:

Now I'm worried that people may be relying on it, since it works, but I'm not sure how to check.
core itself seems to mostly use for sorting/partitioning-like tasks.

Oh, according to sourcegraph, it's definitely gotten used, oh well.

So we might want to do nothing for now, and only remove the upstream hack if qptr can supersede it.

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

No branches or pull requests

1 participant