fix(wgpu): Raise validation error instead of panicking in get_bind_gr…

…oup_layout. (#6280)
gfx-rs · Sep 20, 2024 · 9f85f8a · 9f85f8a
1 parent b54fb72
commit 9f85f8a
Show file tree

Hide file tree

Showing 5 changed files with 222 additions and 57 deletions.
diff --git 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,14 +83,17 @@ 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)
 
 ### 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
@@ -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);
-
-        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,
-                        });
+const INVALID_SHADER_DESC: wgpu::ShaderModuleDescriptor = wgpu::ShaderModuleDescriptor {
+    label: Some("invalid shader"),
+    source: wgpu::ShaderSource::Wgsl(std::borrow::Cow::Borrowed("not valid wgsl")),
+};
 
-                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<f32> { return vec4<f32>(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
@@ -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
@@ -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