EmbarkStudios / rust-gpu

🐉 Making Rust a first-class language and ecosystem for GPU shaders 🚧
https://shader.rs
Apache License 2.0
7.36k stars 244 forks source link

`cargo clippy` breaks the nested `cargo build` from `spirv-builder`. #864

Open hatoo opened 2 years ago

hatoo commented 2 years ago

Expected Behaviour

cargo clippy works without a crash in this project.

It worked fine in older rust-gpu.

Example & Steps To Reproduce

  1. Clone this
  2. cargo clippy
  3. Got error
PS C:\Users\hato2\Desktop\zenn-content\rasterization-example> cargo clippy
   Compiling rasterization-example v0.1.0 (C:\Users\hato2\Desktop\zenn-content\rasterization-example)
error: failed to run custom build command for `rasterization-example v0.1.0 (C:\Users\hato2\Desktop\zenn-content\rasterization-example)`

Caused by:
  process didn't exit successfully: `C:\Users\hato2\Desktop\zenn-content\rasterization-example\target\debug\build\rasterization-example-bf4f4e1cfd480af7\build-script-build` (exit code: 1)
  --- stderr
     Compiling core v0.0.0 (C:\Users\hato2\.rustup\toolchains\nightly-2022-04-11-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core)
     Compiling rustc-std-workspace-core v1.99.0 (C:\Users\hato2\.rustup\toolchains\nightly-2022-04-11-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\rustc-std-workspace-core)
     Compiling compiler_builtins v0.1.70
     Compiling libm v0.2.1
     Compiling bitflags v1.3.2
     Compiling spirv-types v0.4.0-alpha.12 (https://github.com/EmbarkStudios/rust-gpu.git#11e21c64)
     Compiling num-traits v0.2.14
     Compiling glam v0.20.2
     Compiling spirv-std v0.4.0-alpha.12 (https://github.com/EmbarkStudios/rust-gpu.git#11e21c64)
     Compiling shader v0.1.0 (C:\Users\hato2\Desktop\zenn-content\rasterization-example\shader)
  thread 'rustc' panicked at 'not yet implemented', C:\Users\hato2\.cargo\git\checkouts\rust-gpu-e0a37a82a46176e6\116bf9c\crates\rustc_codegen_spirv\src\codegen_cx\mod.rs:564:9
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
  error: internal compiler error: unexpected panic

  note: the compiler unexpectedly panicked. this is a bug.

  note: we would appreciate a bug report: https://github.com/EmbarkStudios/rust-gpu/issues/new

  note: rustc 1.62.0-nightly (1f7fb6413 2022-04-10) running on x86_64-pc-windows-msvc

  note: compiler flags: --crate-type lib --crate-type dylib -C opt-level=3 -C embed-bitcode=no -Z unstable-options -Z codegen-backend=C:\Users\hato2\Desktop\zenn-content\rasterization-example\target\debug\rustc_codegen_spirv.dll -C symbol-mangling-version=v0

  note: some of the compiler flags provided by cargo are hidden

  query stack during panic:
  end of query stack
  note: `rust-gpu` version 0.4.0-alpha.12

  error: could not compile `shader`
  Error: BuildFailed

System Info

Backtrace

Backtrace

``` ```

repi commented 2 years ago

Looks like that rust-gpu panic was on this:

    fn eh_personality(&self) -> Self::Value {
        todo!()
    }

Which for this type of no-std builds one has to implement oneself (docs).

We do this in spirv-std but looks like your dependency is not including that so in that case you'll have to do some of that set up manually. https://github.com/EmbarkStudios/rust-gpu/blob/132b960e8320d7e607fd912142cff7da05d49d92/crates/spirv-std/src/lib.rs#L128-L129

We should improve the rust-gpu todo panic though to be clear about this rather than crashing

eddyb commented 2 years ago

We can probably return a zombie (or even a non-function instead of panicking, since we don't actually do any kind of exception handling, so rust_eh_personality shouldn't actually be necessary.

Wait, no, any use of .eh_personality()'s result is itself a todo!(), and I'm not sure we even need #[lang = "eh_personality"] ourselves...

We force -C panic=abort in the target spec: https://github.com/EmbarkStudios/rust-gpu/blob/132b960e8320d7e607fd912142cff7da05d49d92/crates/rustc_codegen_spirv/src/target.rs#L93

The relevance of cargo clippy is also suspect, since it's invoking a build script that should be using spirv-builder (and seems to, by all accounts), maybe it's caused by Clippy inserting itself as a rustc proxy?


A quick look around found that clippy indeed turns off MIR optimizations (which are likely needed for panic=abort to remove unwinding logic from MIR).

The decision logic for whether or not to run in "clippy mode" doesn't seem to consider build scripts?

We may need to unset the RUSTC_WRAPPER env var in spirv-builder to avoid clippy interference.

Though ideally clippy should check for --emit=rmeta to tell whether the cargo check invocation (started by cargo clippy) is actually doing a "check" build, or something else is going on (like a nested build script build).


We can probably start by adding this assert to rustc_codegen_spirv:

assert_ne!(
    sess.opts.debugging_opts.mir_opt_level, Some(0),
    "`-Z mir-opt-level=0` not supported \
     (if using `cargo clippy`, it may have added the flag automatically)"
);
eddyb commented 2 years ago

Was able to reproduce in-tree like this:

rustup target add wasm32-unknown-unknown
cargo clippy -p example-runner-wgpu --target=wasm32-unknown-unknown

The reason it doesn't repro for the host target is the hotloading support.

Alternatively, commenting out these lines should also repro: https://github.com/EmbarkStudios/rust-gpu/blob/685c79a9725f9fb68ce17dd00e742b98a03f3870/examples/runners/wgpu/build.rs#L15-L17

oisyn commented 1 year ago

Can't really repro on 0.6.1 with above instructions. This needs further investigation whether this is indeed fixed now, and why.

eddyb commented 1 year ago

Adding the assert from https://github.com/EmbarkStudios/rust-gpu/issues/864#issuecomment-1102593603 does get tripped with the repro instructions (the ones about commenting out the android/wasm check), but I don't understand why that doesn't break anything.

Probably requires going back to see which nightly made -C panic=abort not require MIR optimizations.

What clippy is doing here is still sketchy (AFAICT the relevant parts of its code haven't changed since I investigated a year ago above), but it sounds like it maybe no longer breaks Rust-GPU?