From 9f85f8aeea6c43c1a412bafc8fbcfb43aad0dd20 Mon Sep 17 00:00:00 2001 From: Ben Reeves Date: Fri, 20 Sep 2024 02:32:01 -0500 Subject: [PATCH] fix(wgpu): Raise validation error instead of panicking in get_bind_group_layout. (#6280) --- CHANGELOG.md | 14 ++- tests/tests/pipeline.rs | 205 +++++++++++++++++++++++++------ wgpu/src/api/compute_pipeline.rs | 2 + wgpu/src/api/render_pipeline.rs | 2 + wgpu/src/backend/wgpu_core.rs | 56 ++++++--- 5 files changed, 222 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aafc73b31..81dee037a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,11 +27,12 @@ Top level categories: Bottom level categories: +- Naga - General - DX12 - Vulkan - Metal -- GLES +- GLES / OpenGL - WebGPU - Emscripten - Hal @@ -82,6 +83,10 @@ By @bradwerth [#6216](https://github.com/gfx-rs/wgpu/pull/6216). - Add `first` and `either` sampling types for `@interpolate(flat, …)` in WGSL. By @ErichDonGubler in [#6181](https://github.com/gfx-rs/wgpu/pull/6181). - Support for more atomic ops in the SPIR-V frontend. By @schell in [#5824](https://github.com/gfx-rs/wgpu/pull/5824). +#### General + +- Add `VideoFrame` to `ExternalImageSource` enum. By @jprochazk in [#6170](https://github.com/gfx-rs/wgpu/pull/6170) + #### Vulkan - Allow using [VK_GOOGLE_display_timing](https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_GOOGLE_display_timing.html) unsafely with the `VULKAN_GOOGLE_DISPLAY_TIMING` feature. By @DJMcNab in [#6149](https://github.com/gfx-rs/wgpu/pull/6149) @@ -89,7 +94,6 @@ By @bradwerth [#6216](https://github.com/gfx-rs/wgpu/pull/6216). ### Bug Fixes - Fix incorrect hlsl image output type conversion. By @atlv24 in [#6123](https://github.com/gfx-rs/wgpu/pull/6123) -- Fix JS `TypeError` exception in `Instance::request_adapter` when browser doesn't support WebGPU but `wgpu` not compiled with `webgl` support. By @bgr360 in [#6197](https://github.com/gfx-rs/wgpu/pull/6197). #### Naga @@ -107,13 +111,17 @@ By @bradwerth [#6216](https://github.com/gfx-rs/wgpu/pull/6216). - Deduplicate bind group layouts that are created from pipelines with "auto" layouts. By @teoxoy [#6049](https://github.com/gfx-rs/wgpu/pull/6049) - Fix crash when dropping the surface after the device. By @wumpf in [#6052](https://github.com/gfx-rs/wgpu/pull/6052) - Fix error message that is thrown in create_render_pass to no longer say `compute_pass`. By @matthew-wong1 [#6041](https://github.com/gfx-rs/wgpu/pull/6041) -- Add `VideoFrame` to `ExternalImageSource` enum. By @jprochazk in [#6170](https://github.com/gfx-rs/wgpu/pull/6170) - Document `wgpu_hal` bounds-checking promises, and adapt `wgpu_core`'s lazy initialization logic to the slightly weaker-than-expected guarantees. By @jimblandy in [#6201](https://github.com/gfx-rs/wgpu/pull/6201) +- Raise validation error instead of panicking in `{Render,Compute}Pipeline::get_bind_group_layout` on native / WebGL. By @bgr360 in [#6280](https://github.com/gfx-rs/wgpu/pull/6280). #### GLES / OpenGL - Fix GL debug message callbacks not being properly cleaned up (causing UB). By @Imberflur in [#6114](https://github.com/gfx-rs/wgpu/pull/6114) +#### WebGPU + +- Fix JS `TypeError` exception in `Instance::request_adapter` when browser doesn't support WebGPU but `wgpu` not compiled with `webgl` support. By @bgr360 in [#6197](https://github.com/gfx-rs/wgpu/pull/6197). + #### Vulkan - Vulkan debug labels assumed no interior nul byte. By @DJMcNab in [#6257](https://github.com/gfx-rs/wgpu/pull/6257) diff --git a/tests/tests/pipeline.rs b/tests/tests/pipeline.rs index f7d8d1ec7..8e9c91e52 100644 --- a/tests/tests/pipeline.rs +++ b/tests/tests/pipeline.rs @@ -1,44 +1,16 @@ -use wgpu_test::{fail, gpu_test, FailureCase, GpuTestConfiguration, TestParameters}; +use wgpu_test::{fail, gpu_test, GpuTestConfiguration, TestParameters}; -// Create an invalid shader and a compute pipeline that uses it -// with a default bindgroup layout, and then ask for that layout. -// Validation should fail, but wgpu should not panic. -#[gpu_test] -static PIPELINE_DEFAULT_LAYOUT_BAD_MODULE: GpuTestConfiguration = GpuTestConfiguration::new() - .parameters( - TestParameters::default() - // https://github.com/gfx-rs/wgpu/issues/4167 - .expect_fail(FailureCase::always().panic("Error reflecting bind group")), - ) - .run_sync(|ctx| { - ctx.device.push_error_scope(wgpu::ErrorFilter::Validation); +const INVALID_SHADER_DESC: wgpu::ShaderModuleDescriptor = wgpu::ShaderModuleDescriptor { + label: Some("invalid shader"), + source: wgpu::ShaderSource::Wgsl(std::borrow::Cow::Borrowed("not valid wgsl")), +}; - fail( - &ctx.device, - || { - let module = ctx - .device - .create_shader_module(wgpu::ShaderModuleDescriptor { - label: None, - source: wgpu::ShaderSource::Wgsl("not valid wgsl".into()), - }); - - let pipeline = - ctx.device - .create_compute_pipeline(&wgpu::ComputePipelineDescriptor { - label: Some("mandelbrot compute pipeline"), - layout: None, - module: &module, - entry_point: Some("doesn't exist"), - compilation_options: Default::default(), - cache: None, - }); - - pipeline.get_bind_group_layout(0); - }, - None, - ); - }); +const TRIVIAL_COMPUTE_SHADER_DESC: wgpu::ShaderModuleDescriptor = wgpu::ShaderModuleDescriptor { + label: Some("trivial compute shader"), + source: wgpu::ShaderSource::Wgsl(std::borrow::Cow::Borrowed( + "@compute @workgroup_size(1) fn main() {}", + )), +}; const TRIVIAL_VERTEX_SHADER_DESC: wgpu::ShaderModuleDescriptor = wgpu::ShaderModuleDescriptor { label: Some("trivial vertex shader"), @@ -47,6 +19,161 @@ const TRIVIAL_VERTEX_SHADER_DESC: wgpu::ShaderModuleDescriptor = wgpu::ShaderMod )), }; +const TRIVIAL_FRAGMENT_SHADER_DESC: wgpu::ShaderModuleDescriptor = wgpu::ShaderModuleDescriptor { + label: Some("trivial fragment shader"), + source: wgpu::ShaderSource::Wgsl(std::borrow::Cow::Borrowed( + "@fragment fn main() -> @location(0) vec4 { return vec4(0); }", + )), +}; + +// Create an invalid shader and a compute pipeline that uses it +// with a default bindgroup layout, and then ask for that layout. +// Validation should fail, but wgpu should not panic. +#[gpu_test] +static COMPUTE_PIPELINE_DEFAULT_LAYOUT_BAD_MODULE: GpuTestConfiguration = + GpuTestConfiguration::new() + .parameters(TestParameters::default()) + .run_sync(|ctx| { + ctx.device.push_error_scope(wgpu::ErrorFilter::Validation); + + fail( + &ctx.device, + || { + let module = ctx.device.create_shader_module(INVALID_SHADER_DESC); + + let pipeline = + ctx.device + .create_compute_pipeline(&wgpu::ComputePipelineDescriptor { + label: Some("compute pipeline"), + layout: None, + module: &module, + entry_point: Some("doesn't exist"), + compilation_options: Default::default(), + cache: None, + }); + + // https://github.com/gfx-rs/wgpu/issues/4167 this used to panic + pipeline.get_bind_group_layout(0); + }, + Some("Shader 'invalid shader' parsing error"), + ); + }); + +#[gpu_test] +static COMPUTE_PIPELINE_DEFAULT_LAYOUT_BAD_BGL_INDEX: GpuTestConfiguration = + GpuTestConfiguration::new() + .parameters(TestParameters::default().test_features_limits()) + .run_sync(|ctx| { + ctx.device.push_error_scope(wgpu::ErrorFilter::Validation); + + fail( + &ctx.device, + || { + let module = ctx.device.create_shader_module(TRIVIAL_COMPUTE_SHADER_DESC); + + let pipeline = + ctx.device + .create_compute_pipeline(&wgpu::ComputePipelineDescriptor { + label: Some("compute pipeline"), + layout: None, + module: &module, + entry_point: Some("main"), + compilation_options: Default::default(), + cache: None, + }); + + pipeline.get_bind_group_layout(0); + }, + Some("Invalid group index 0"), + ); + }); + +#[gpu_test] +static RENDER_PIPELINE_DEFAULT_LAYOUT_BAD_MODULE: GpuTestConfiguration = + GpuTestConfiguration::new() + .parameters(TestParameters::default()) + .run_sync(|ctx| { + ctx.device.push_error_scope(wgpu::ErrorFilter::Validation); + + fail( + &ctx.device, + || { + let module = ctx.device.create_shader_module(INVALID_SHADER_DESC); + + let pipeline = + ctx.device + .create_render_pipeline(&wgpu::RenderPipelineDescriptor { + label: Some("render pipeline"), + layout: None, + vertex: wgpu::VertexState { + module: &module, + entry_point: Some("doesn't exist"), + compilation_options: Default::default(), + buffers: &[], + }, + primitive: Default::default(), + depth_stencil: None, + multisample: Default::default(), + fragment: None, + multiview: None, + cache: None, + }); + + pipeline.get_bind_group_layout(0); + }, + Some("Shader 'invalid shader' parsing error"), + ); + }); + +#[gpu_test] +static RENDER_PIPELINE_DEFAULT_LAYOUT_BAD_BGL_INDEX: GpuTestConfiguration = + GpuTestConfiguration::new() + .parameters(TestParameters::default().test_features_limits()) + .run_sync(|ctx| { + ctx.device.push_error_scope(wgpu::ErrorFilter::Validation); + + fail( + &ctx.device, + || { + let vs_module = ctx.device.create_shader_module(TRIVIAL_VERTEX_SHADER_DESC); + let fs_module = ctx + .device + .create_shader_module(TRIVIAL_FRAGMENT_SHADER_DESC); + + let pipeline = + ctx.device + .create_render_pipeline(&wgpu::RenderPipelineDescriptor { + label: Some("render pipeline"), + layout: None, + vertex: wgpu::VertexState { + module: &vs_module, + entry_point: Some("main"), + compilation_options: Default::default(), + buffers: &[], + }, + primitive: Default::default(), + depth_stencil: None, + multisample: Default::default(), + fragment: Some(wgpu::FragmentState { + module: &fs_module, + entry_point: Some("main"), + compilation_options: Default::default(), + targets: &[Some(wgpu::ColorTargetState { + format: wgpu::TextureFormat::Rgba8Unorm, + blend: None, + write_mask: wgpu::ColorWrites::ALL, + })], + }), + multiview: None, + cache: None, + }); + + pipeline.get_bind_group_layout(0); + }, + Some("Invalid group index 0"), + ); + }); + #[gpu_test] static NO_TARGETLESS_RENDER: GpuTestConfiguration = GpuTestConfiguration::new() .parameters(TestParameters::default()) diff --git a/wgpu/src/api/compute_pipeline.rs b/wgpu/src/api/compute_pipeline.rs index 18d4e904e..16885ac96 100644 --- a/wgpu/src/api/compute_pipeline.rs +++ b/wgpu/src/api/compute_pipeline.rs @@ -24,6 +24,8 @@ impl ComputePipeline { /// If this pipeline was created with a [default layout][ComputePipelineDescriptor::layout], /// then bind groups created with the returned `BindGroupLayout` can only be used with this /// pipeline. + /// + /// This method will raise a validation error if there is no bind group layout at `index`. pub fn get_bind_group_layout(&self, index: u32) -> BindGroupLayout { let context = Arc::clone(&self.context); let data = self diff --git a/wgpu/src/api/render_pipeline.rs b/wgpu/src/api/render_pipeline.rs index 3009cde1d..dd1c1cefe 100644 --- a/wgpu/src/api/render_pipeline.rs +++ b/wgpu/src/api/render_pipeline.rs @@ -31,6 +31,8 @@ impl RenderPipeline { /// /// If this pipeline was created with a [default layout][RenderPipelineDescriptor::layout], then /// bind groups created with the returned `BindGroupLayout` can only be used with this pipeline. + /// + /// This method will raise a validation error if there is no bind group layout at `index`. pub fn get_bind_group_layout(&self, index: u32) -> BindGroupLayout { let context = Arc::clone(&self.context); let data = self diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index 37850c770..3aac20e21 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -473,6 +473,18 @@ pub struct Queue { error_sink: ErrorSink, } +#[derive(Debug)] +pub struct ComputePipeline { + id: wgc::id::ComputePipelineId, + error_sink: ErrorSink, +} + +#[derive(Debug)] +pub struct RenderPipeline { + id: wgc::id::RenderPipelineId, + error_sink: ErrorSink, +} + #[derive(Debug)] pub struct ComputePass { pass: wgc::command::ComputePass, @@ -505,8 +517,8 @@ impl crate::Context for ContextWgpuCore { type TextureData = Texture; type QuerySetData = wgc::id::QuerySetId; type PipelineLayoutData = wgc::id::PipelineLayoutId; - type RenderPipelineData = wgc::id::RenderPipelineId; - type ComputePipelineData = wgc::id::ComputePipelineId; + type RenderPipelineData = RenderPipeline; + type ComputePipelineData = ComputePipeline; type PipelineCacheData = wgc::id::PipelineCacheId; type CommandEncoderData = CommandEncoder; type ComputePassData = ComputePass; @@ -1097,7 +1109,10 @@ impl crate::Context for ContextWgpuCore { "Device::create_render_pipeline", ); } - id + RenderPipeline { + id, + error_sink: Arc::clone(&device_data.error_sink), + } } fn device_create_compute_pipeline( &self, @@ -1139,7 +1154,10 @@ impl crate::Context for ContextWgpuCore { "Device::create_compute_pipeline", ); } - id + ComputePipeline { + id, + error_sink: Arc::clone(&device_data.error_sink), + } } unsafe fn device_create_pipeline_cache( @@ -1531,11 +1549,11 @@ impl crate::Context for ContextWgpuCore { } fn compute_pipeline_drop(&self, pipeline_data: &Self::ComputePipelineData) { - self.0.compute_pipeline_drop(*pipeline_data) + self.0.compute_pipeline_drop(pipeline_data.id) } fn render_pipeline_drop(&self, pipeline_data: &Self::RenderPipelineData) { - self.0.render_pipeline_drop(*pipeline_data) + self.0.render_pipeline_drop(pipeline_data.id) } fn pipeline_cache_drop(&self, cache_data: &Self::PipelineCacheData) { @@ -1549,9 +1567,13 @@ impl crate::Context for ContextWgpuCore { ) -> Self::BindGroupLayoutData { let (id, error) = self.0 - .compute_pipeline_get_bind_group_layout(*pipeline_data, index, None); + .compute_pipeline_get_bind_group_layout(pipeline_data.id, index, None); if let Some(err) = error { - panic!("Error reflecting bind group {index}: {err}"); + self.handle_error_nolabel( + &pipeline_data.error_sink, + err, + "ComputePipeline::get_bind_group_layout", + ) } id } @@ -1561,11 +1583,15 @@ impl crate::Context for ContextWgpuCore { pipeline_data: &Self::RenderPipelineData, index: u32, ) -> Self::BindGroupLayoutData { - let (id, error) = self - .0 - .render_pipeline_get_bind_group_layout(*pipeline_data, index, None); + let (id, error) = + self.0 + .render_pipeline_get_bind_group_layout(pipeline_data.id, index, None); if let Some(err) = error { - panic!("Error reflecting bind group {index}: {err}"); + self.handle_error_nolabel( + &pipeline_data.error_sink, + err, + "RenderPipeline::get_bind_group_layout", + ) } id } @@ -2108,7 +2134,7 @@ impl crate::Context for ContextWgpuCore { ) { if let Err(cause) = self .0 - .compute_pass_set_pipeline(&mut pass_data.pass, *pipeline_data) + .compute_pass_set_pipeline(&mut pass_data.pass, pipeline_data.id) { self.handle_error( &pass_data.error_sink, @@ -2311,7 +2337,7 @@ impl crate::Context for ContextWgpuCore { encoder_data: &mut Self::RenderBundleEncoderData, pipeline_data: &Self::RenderPipelineData, ) { - wgpu_render_bundle_set_pipeline(encoder_data, *pipeline_data) + wgpu_render_bundle_set_pipeline(encoder_data, pipeline_data.id) } fn render_bundle_encoder_set_bind_group( @@ -2434,7 +2460,7 @@ impl crate::Context for ContextWgpuCore { ) { if let Err(cause) = self .0 - .render_pass_set_pipeline(&mut pass_data.pass, *pipeline_data) + .render_pass_set_pipeline(&mut pass_data.pass, pipeline_data.id) { self.handle_error( &pass_data.error_sink,