wgpu icon indicating copy to clipboard operation
wgpu copied to clipboard

ShaderSource::Glsl panics if shader has syntax error

Open kpreid opened this issue 2 years ago • 4 comments

Description I'm trying to port an existing application using GLSL to wgpu, so I have existing complex shaders I want to modify to be compatible. While trying to get them working, I have found that if the shader has (something which naga considers) a syntax error, wgpu will panic via .unwrap():

https://github.com/gfx-rs/wgpu/blob/326af60df8623e93b47a0de090e6cb449c8507f5/wgpu/src/backend/direct.rs#L1054

This has the following problems:

  1. The application cannot recover from the error (e.g. in case it is allowing the shaders to be edited and reloaded) without using catch_unwind().
  2. The error message is presented in Debug formatting, which is not ideal for readability.
  3. The error message provides only character (byte?), not line, positions (let alone line positions respecting GLSL #line directives), so it is difficult to correlate with a line in one's editor.

I propose that wgpu should report shader parsing errors in a Result::Err() instead of a panic, and provide a means to format the reported error nicely with line number (I see that naga-cli has this, so presumably it shouldn't be too hard).

(The ShaderSource::SpirV path also has an .unwrap() which could be improved the same way, but presumably that's less of a routine occurrence.)

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: [Error { kind: NotImplemented("variable qualifier"), meta: Span { start: 571, end: 580 } }]', /Users/kpreid/.cargo/registry/src/github.com-1ecc6299db9ec823/wgpu-0.12.0/src/backend/direct.rs:1037:61
stack backtrace:
   0: rust_begin_unwind
             at /rustc/3a06854129d1ff7cb1e8a7fb62671782fe2513ba/library/std/src/panicking.rs:498:5
   1: core::panicking::panic_fmt
             at /rustc/3a06854129d1ff7cb1e8a7fb62671782fe2513ba/library/core/src/panicking.rs:116:14
   2: core::result::unwrap_failed
             at /rustc/3a06854129d1ff7cb1e8a7fb62671782fe2513ba/library/core/src/result.rs:1690:5
   3: core::result::Result<T,E>::unwrap
             at /rustc/3a06854129d1ff7cb1e8a7fb62671782fe2513ba/library/core/src/result.rs:1018:23
   4: <wgpu::backend::direct::Context as wgpu::Context>::device_create_shader_module
             at /Users/kpreid/.cargo/registry/src/github.com-1ecc6299db9ec823/wgpu-0.12.0/src/backend/direct.rs:1037:30
   5: wgpu::Device::create_shader_module
             at /Users/kpreid/.cargo/registry/src/github.com-1ecc6299db9ec823/wgpu-0.12.0/src/lib.rs:1705:17
   6: all_is_cubes_gpu::in_wgpu::EverythingRenderer::new
             at ./all-is-cubes-gpu/src/in_wgpu.rs:144:29

Platform macOS 12.3, wgpu 0.12.0

kpreid avatar Mar 21 '22 01:03 kpreid

Workaround for now is manually pre-parse shader via naga lib to handle errors.

lebedec avatar May 04 '22 11:05 lebedec

Removing 'good first issue' label; see the discussion in #2757 for why.

jimblandy avatar Jun 21 '22 05:06 jimblandy

Removing from 0.13 for same reason.

cwfitzgerald avatar Jun 23 '22 01:06 cwfitzgerald

e-parse shader via naga lib to handle error

Just a comment RE: @lebedec, naga::front::wgsl.parse_str() will trap some errors e.g. "uv.u" instead of "uv.x" but wgpu::device.create_shader_module() will catch a further set of errors e.g. variable type f32 where i32 was expected.

Until/unless create_shader_module() returns a Result (please!) then the messy on_uncaptured_error() seems the only reliable approach.

brent-williams avatar Aug 14 '22 06:08 brent-williams

The app I'm working on involves a shader editor, and being able to capture these compilation errors separate from other potential unhandled errors, especially with source location information, would be extremely valuable to me.

akhudek avatar May 02 '23 18:05 akhudek