From f553f04db3274541245ec649476fc0775c3f0133 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Thu, 21 Mar 2024 16:09:29 -0400 Subject: [PATCH 1/5] fix(dx12): don't depend on BG{,L} entry order This isn't guaranteed by `wgpu-core`; we should try to match by binding slot index instead. --- CHANGELOG.md | 5 +++++ wgpu-hal/src/dx12/device.rs | 11 ++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b78f5c9ba..d5f3aa0249 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -60,6 +60,11 @@ Bottom level categories: - `step` - `tan` - `tanh` +### Bug Fixes + +#### DX12 + +- Don't depend on bind group and bind group layout entry order in HAL. This caused incorrect severely incorrect command execution and, in some cases, crashes. By @ErichDonGubler in [#5421](https://github.com/gfx-rs/wgpu/pull/5421). ## v0.19.3 (2024-03-01) diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs index f6281d2b8a..98fca8a29e 100644 --- a/wgpu-hal/src/dx12/device.rs +++ b/wgpu-hal/src/dx12/device.rs @@ -1098,7 +1098,16 @@ impl crate::Device for super::Device { } let mut dynamic_buffers = Vec::new(); - for (layout, entry) in desc.layout.entries.iter().zip(desc.entries.iter()) { + let layout_and_entry_iter = desc.entries.iter().map(|entry| { + let layout = desc + .layout + .entries + .iter() + .find(|layout_entry| layout_entry.binding == entry.binding) + .expect("internal error: no layout entry found with binding slot"); + (layout, entry) + }); + for (layout, entry) in layout_and_entry_iter { match layout.ty { wgt::BindingType::Buffer { has_dynamic_offset: true, From 7bdb145cc6f035b827a774f71e8bee5919437a37 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Thu, 21 Mar 2024 16:09:29 -0400 Subject: [PATCH 2/5] fix(gles): don't depend on BG{,L} entry order This isn't guaranteed by `wgpu-core`; we should try to match by binding slot index instead. --- CHANGELOG.md | 4 ++++ wgpu-hal/src/gles/device.rs | 17 ++++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5f3aa0249..0e7086f3c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,10 @@ Bottom level categories: - `tanh` ### Bug Fixes +#### GLES + +- Don't depend on bind group and bind group layout entry order in HAL. This caused incorrect severely incorrect command execution and, in some cases, crashes. By @ErichDonGubler in [#5421](https://github.com/gfx-rs/wgpu/pull/5421). + #### DX12 - Don't depend on bind group and bind group layout entry order in HAL. This caused incorrect severely incorrect command execution and, in some cases, crashes. By @ErichDonGubler in [#5421](https://github.com/gfx-rs/wgpu/pull/5421). diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs index 2678488cf8..48719b97fe 100644 --- a/wgpu-hal/src/gles/device.rs +++ b/wgpu-hal/src/gles/device.rs @@ -1123,8 +1123,10 @@ impl crate::Device for super::Device { !0; bg_layout .entries - .last() - .map_or(0, |b| b.binding as usize + 1) + .iter() + .map(|b| b.binding) + .max() + .map_or(0, |idx| idx as usize + 1) ] .into_boxed_slice(); @@ -1177,7 +1179,16 @@ impl crate::Device for super::Device { ) -> Result { let mut contents = Vec::new(); - for (entry, layout) in desc.entries.iter().zip(desc.layout.entries.iter()) { + let layout_and_entry_iter = desc.entries.iter().map(|entry| { + let layout = desc + .layout + .entries + .iter() + .find(|layout_entry| layout_entry.binding == entry.binding) + .expect("internal error: no layout entry found with binding slot"); + (entry, layout) + }); + for (entry, layout) in layout_and_entry_iter { let binding = match layout.ty { wgt::BindingType::Buffer { .. } => { let bb = &desc.buffers[entry.resource_index as usize]; From 990891a281652e3982f24f80614c9f91b6fa6ba9 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Thu, 21 Mar 2024 16:09:29 -0400 Subject: [PATCH 3/5] fix(metal): don't depend on BG{,L} entry order This isn't guaranteed by `wgpu-core`; we should try to match by binding slot index instead. --- CHANGELOG.md | 4 ++++ wgpu-hal/src/metal/device.rs | 11 ++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e7086f3c7..7331430e34 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,10 @@ Bottom level categories: - Don't depend on bind group and bind group layout entry order in HAL. This caused incorrect severely incorrect command execution and, in some cases, crashes. By @ErichDonGubler in [#5421](https://github.com/gfx-rs/wgpu/pull/5421). +#### Metal + +- Don't depend on bind group and bind group layout entry order in HAL. This caused incorrect severely incorrect command execution and, in some cases, crashes. By @ErichDonGubler in [#5421](https://github.com/gfx-rs/wgpu/pull/5421). + #### DX12 - Don't depend on bind group and bind group layout entry order in HAL. This caused incorrect severely incorrect command execution and, in some cases, crashes. By @ErichDonGubler in [#5421](https://github.com/gfx-rs/wgpu/pull/5421). diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs index d7fd06c8f3..216afab9ef 100644 --- a/wgpu-hal/src/metal/device.rs +++ b/wgpu-hal/src/metal/device.rs @@ -706,7 +706,16 @@ impl crate::Device for super::Device { for (&stage, counter) in super::NAGA_STAGES.iter().zip(bg.counters.iter_mut()) { let stage_bit = map_naga_stage(stage); let mut dynamic_offsets_count = 0u32; - for (entry, layout) in desc.entries.iter().zip(desc.layout.entries.iter()) { + let layout_and_entry_iter = desc.entries.iter().map(|entry| { + let layout = desc + .layout + .entries + .iter() + .find(|layout_entry| layout_entry.binding == entry.binding) + .expect("internal error: no layout entry found with binding slot"); + (entry, layout) + }); + for (entry, layout) in layout_and_entry_iter { let size = layout.count.map_or(1, |c| c.get()); if let wgt::BindingType::Buffer { has_dynamic_offset: true, From b15a6c191b677746ba12a4cc4864385a90c9db01 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Thu, 21 Mar 2024 16:09:29 -0400 Subject: [PATCH 4/5] test: don't depend on BG{,L} entry order in HAL --- tests/tests/device.rs | 122 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/tests/tests/device.rs b/tests/tests/device.rs index 657af4f248..323a20edad 100644 --- a/tests/tests/device.rs +++ b/tests/tests/device.rs @@ -615,3 +615,125 @@ static DROPPED_GLOBAL_THEN_DEVICE_LOST: GpuTestConfiguration = GpuTestConfigurat "Device lost callback should have been called." ); }); + +#[gpu_test] +static DIFFERENT_BGL_ORDER_BW_SHADER_AND_API: GpuTestConfiguration = GpuTestConfiguration::new() + .parameters(TestParameters::default()) + .run_sync(|ctx| { + // This test addresses a bug found in multiple backends where `wgpu_core` and `wgpu_hal` + // backends made different assumptions about the element order of vectors of bind group + // layout entries and bind group resource bindings. + // + // Said bug was exposed originally by: + // + // 1. Shader-declared bindings having a different order than resource bindings provided to + // `Device::create_bind_group`. + // 2. Having more of one type of resource in the bind group than another. + // + // …such that internals would accidentally attempt to use an out-of-bounds index (of one + // resource type) in the wrong list of a different resource type. Let's reproduce that + // here. + + let trivial_shaders_with_some_reversed_bindings = "\ +@group(0) @binding(3) var myTexture2: texture_2d; +@group(0) @binding(2) var myTexture1: texture_2d; +@group(0) @binding(1) var mySampler: sampler; + +@fragment +fn fs_main(@builtin(position) pos: vec4) -> @location(0) vec4f { + return textureSample(myTexture1, mySampler, pos.xy) + textureSample(myTexture2, mySampler, pos.xy); +} + +@vertex +fn vs_main() -> @builtin(position) vec4 { + return vec4(0.0, 0.0, 0.0, 1.0); +} +"; + + let trivial_shaders_with_some_reversed_bindings = + ctx.device + .create_shader_module(wgpu::ShaderModuleDescriptor { + label: None, + source: wgpu::ShaderSource::Wgsl( + trivial_shaders_with_some_reversed_bindings.into(), + ), + }); + + let my_texture = ctx.device.create_texture(&wgt::TextureDescriptor { + label: None, + size: wgt::Extent3d { + width: 1024, + height: 512, + depth_or_array_layers: 1, + }, + mip_level_count: 1, + sample_count: 1, + dimension: wgt::TextureDimension::D2, + format: wgt::TextureFormat::Rgba8Unorm, + usage: wgt::TextureUsages::RENDER_ATTACHMENT | wgt::TextureUsages::TEXTURE_BINDING, + view_formats: &[], + }); + + let my_texture_view = my_texture.create_view(&wgpu::TextureViewDescriptor { + label: None, + format: None, + dimension: None, + aspect: wgt::TextureAspect::All, + base_mip_level: 0, + mip_level_count: None, + base_array_layer: 0, + array_layer_count: None, + }); + + let my_sampler = ctx + .device + .create_sampler(&wgpu::SamplerDescriptor::default()); + + let render_pipeline = ctx + .device + .create_render_pipeline(&wgpu::RenderPipelineDescriptor { + fragment: Some(wgpu::FragmentState { + module: &trivial_shaders_with_some_reversed_bindings, + entry_point: "fs_main", + targets: &[Some(wgt::ColorTargetState { + format: wgt::TextureFormat::Bgra8Unorm, + blend: None, + write_mask: wgt::ColorWrites::ALL, + })], + }), + layout: None, + + // Other fields below aren't interesting for this text. + label: None, + vertex: wgpu::VertexState { + module: &trivial_shaders_with_some_reversed_bindings, + entry_point: "vs_main", + buffers: &[], + }, + primitive: wgt::PrimitiveState::default(), + depth_stencil: None, + multisample: wgt::MultisampleState::default(), + multiview: None, + }); + + // fail(&ctx.device, || { + // }, ""); + ctx.device.create_bind_group(&wgpu::BindGroupDescriptor { + label: None, + layout: &render_pipeline.get_bind_group_layout(0), + entries: &[ + wgpu::BindGroupEntry { + binding: 1, + resource: wgpu::BindingResource::Sampler(&my_sampler), + }, + wgpu::BindGroupEntry { + binding: 2, + resource: wgpu::BindingResource::TextureView(&my_texture_view), + }, + wgpu::BindGroupEntry { + binding: 3, + resource: wgpu::BindingResource::TextureView(&my_texture_view), + }, + ], + }); + }); From 2347c13b5326ac980bb5d797811cc1ba7dda7d21 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 4 Mar 2024 21:32:41 +0000 Subject: [PATCH 5/5] build(deps): bump mio from 0.8.10 to 0.8.11 Bumps [mio](https://github.com/tokio-rs/mio) from 0.8.10 to 0.8.11. - [Release notes](https://github.com/tokio-rs/mio/releases) - [Changelog](https://github.com/tokio-rs/mio/blob/master/CHANGELOG.md) - [Commits](https://github.com/tokio-rs/mio/compare/v0.8.10...v0.8.11) --- updated-dependencies: - dependency-name: mio dependency-type: indirect ... Signed-off-by: dependabot[bot] --- Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a2de51b4fb..a6229bd9d7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2040,9 +2040,9 @@ dependencies = [ [[package]] name = "mio" -version = "0.8.10" +version = "0.8.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8f3d0b296e374a4e6f3c7b0a1f5a51d748a0d34c85e7dc48fc3fa9a87657fe09" +checksum = "a4a650543ca06a924e8b371db273b2756685faae30f8487da1b56505a8f78b0c" dependencies = [ "libc", "log",