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

fix(wgpu): Raise validation error instead of panicking in get_bind_group_layout. #6280

Merged
merged 3 commits into from
Sep 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@ Top level categories:

Bottom level categories:

- Naga
- General
- DX12
- Vulkan
- Metal
- GLES
- GLES / OpenGL
- WebGPU
- Emscripten
- Hal
Expand Down Expand Up @@ -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

Expand All @@ -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)
Expand Down
205 changes: 166 additions & 39 deletions tests/tests/pipeline.rs
Original file line number Diff line number Diff line change
@@ -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"),
Expand All @@ -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())
Expand Down
2 changes: 2 additions & 0 deletions wgpu/src/api/compute_pipeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions wgpu/src/api/render_pipeline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading