From f40e01db7949d4db4f5380ba4114bfe271080b21 Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Wed, 9 Jul 2025 13:05:55 -0700 Subject: [PATCH 1/2] Revert "Restore unintentional support for zero-size buffers" This reverts commit c0a580d6f0343a725b3defa8be4fdf0a9691eaad. --- wgpu-core/src/binding_model.rs | 2 +- wgpu-core/src/command/bundle.rs | 30 ++++++++------ wgpu-core/src/command/render.rs | 19 ++++----- wgpu-core/src/command/render_command.rs | 21 +++++----- wgpu-core/src/device/resource.rs | 5 +-- wgpu-core/src/resource.rs | 51 +++++++++++------------- wgpu-hal/examples/halmark/main.rs | 6 ++- wgpu-hal/src/dx12/command.rs | 4 +- wgpu-hal/src/dx12/device.rs | 2 +- wgpu-hal/src/dx12/mod.rs | 7 ++++ wgpu-hal/src/gles/device.rs | 3 +- wgpu-hal/src/lib.rs | 53 +++++++------------------ wgpu-hal/src/metal/command.rs | 10 ++--- wgpu-hal/src/metal/device.rs | 7 +--- wgpu-hal/src/vulkan/device.rs | 4 +- wgpu-types/src/lib.rs | 6 --- 16 files changed, 101 insertions(+), 129 deletions(-) diff --git a/wgpu-core/src/binding_model.rs b/wgpu-core/src/binding_model.rs index b049ceb108b..9b1c12fada4 100644 --- a/wgpu-core/src/binding_model.rs +++ b/wgpu-core/src/binding_model.rs @@ -106,7 +106,7 @@ pub enum BindingError { binding_size: u64, buffer_size: u64, }, - #[error("Buffer {buffer}: Binding offset {offset} is greater than buffer size {buffer_size}")] + #[error("Buffer {buffer}: Binding offset {offset} is greater than or equal to buffer size {buffer_size}")] BindingOffsetTooLarge { buffer: ResourceErrorIdent, offset: wgt::BufferAddress, diff --git a/wgpu-core/src/command/bundle.rs b/wgpu-core/src/command/bundle.rs index 357ebf2bc18..d924190db26 100644 --- a/wgpu-core/src/command/bundle.rs +++ b/wgpu-core/src/command/bundle.rs @@ -93,7 +93,6 @@ use core::{ use arrayvec::ArrayVec; use thiserror::Error; -use wgpu_hal::ShouldBeNonZeroExt; use wgt::error::{ErrorType, WebGpuError}; use crate::{ @@ -505,7 +504,7 @@ impl RenderBundleEncoder { buffer_id, index_format, offset, - size: size.map(NonZeroU64::get), + size, }); } } @@ -610,7 +609,7 @@ fn set_index_buffer( buffer_id: id::Id, index_format: wgt::IndexFormat, offset: u64, - size: Option, + size: Option, ) -> Result<(), RenderBundleErrorInner> { let buffer = buffer_guard.get(buffer_id).get()?; @@ -642,7 +641,7 @@ fn set_vertex_buffer( slot: u32, buffer_id: id::Id, offset: u64, - size: Option, + size: Option, ) -> Result<(), RenderBundleErrorInner> { let max_vertex_buffers = state.device.limits.max_vertex_buffers; if slot >= max_vertex_buffers { @@ -1167,8 +1166,11 @@ impl IndexState { .range .end .checked_sub(self.range.start) - .filter(|_| self.range.end <= self.buffer.size) - .expect("index range must be contained in buffer"); + .and_then(wgt::BufferSize::new); + assert!( + self.range.end <= self.buffer.size && binding_size.is_some(), + "index buffer range must have non-zero size and be contained in buffer", + ); if self.is_dirty { self.is_dirty = false; @@ -1176,7 +1178,7 @@ impl IndexState { buffer: self.buffer.clone(), index_format: self.format, offset: self.range.start, - size: Some(binding_size), + size: binding_size, }) } else { None @@ -1219,12 +1221,16 @@ impl VertexState { /// /// `slot` is the index of the vertex buffer slot that `self` tracks. fn flush(&mut self, slot: u32) -> Option { + // This was all checked before, but let's check again just in case. let binding_size = self .range .end .checked_sub(self.range.start) - .filter(|_| self.range.end <= self.buffer.size) - .expect("vertex range must be contained in buffer"); + .and_then(wgt::BufferSize::new); + assert!( + self.range.end <= self.buffer.size && binding_size.is_some(), + "vertex buffer range must have non-zero size and be contained in buffer", + ); if self.is_dirty { self.is_dirty = false; @@ -1232,7 +1238,7 @@ impl VertexState { slot, buffer: self.buffer.clone(), offset: self.range.start, - size: Some(binding_size), + size: binding_size, }) } else { None @@ -1596,7 +1602,7 @@ where pub mod bundle_ffi { use super::{RenderBundleEncoder, RenderCommand}; use crate::{id, RawString}; - use core::{convert::TryInto, num::NonZeroU64, slice}; + use core::{convert::TryInto, slice}; use wgt::{BufferAddress, BufferSize, DynamicOffset, IndexFormat}; /// # Safety @@ -1655,7 +1661,7 @@ pub mod bundle_ffi { slot, buffer_id, offset, - size: size.map(NonZeroU64::get), + size, }); } diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index db533329014..d1596a5c474 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -1,17 +1,12 @@ use alloc::{borrow::Cow, sync::Arc, vec::Vec}; -use core::{ - fmt, - num::{NonZeroU32, NonZeroU64}, - str, -}; -use hal::ShouldBeNonZeroExt; +use core::{fmt, num::NonZeroU32, str}; use arrayvec::ArrayVec; use thiserror::Error; use wgt::{ error::{ErrorType, WebGpuError}, - BufferAddress, BufferSize, BufferSizeOrZero, BufferUsages, Color, DynamicOffset, IndexFormat, - ShaderStages, TextureSelector, TextureUsages, TextureViewDimension, VertexStepMode, + BufferAddress, BufferSize, BufferUsages, Color, DynamicOffset, IndexFormat, ShaderStages, + TextureSelector, TextureUsages, TextureViewDimension, VertexStepMode, }; use crate::command::{ @@ -2338,7 +2333,7 @@ fn set_index_buffer( buffer: Arc, index_format: IndexFormat, offset: u64, - size: Option, + size: Option, ) -> Result<(), RenderPassErrorInner> { api_log!("RenderPass::set_index_buffer {}", buffer.error_ident()); @@ -2378,7 +2373,7 @@ fn set_vertex_buffer( slot: u32, buffer: Arc, offset: u64, - size: Option, + size: Option, ) -> Result<(), RenderPassErrorInner> { api_log!( "RenderPass::set_vertex_buffer {slot} {}", @@ -3089,7 +3084,7 @@ impl Global { buffer: pass_try!(base, scope, self.resolve_render_pass_buffer_id(buffer_id)), index_format, offset, - size: size.map(NonZeroU64::get), + size, }); Ok(()) @@ -3110,7 +3105,7 @@ impl Global { slot, buffer: pass_try!(base, scope, self.resolve_render_pass_buffer_id(buffer_id)), offset, - size: size.map(NonZeroU64::get), + size, }); Ok(()) diff --git a/wgpu-core/src/command/render_command.rs b/wgpu-core/src/command/render_command.rs index f57ec026e24..606d3fe9498 100644 --- a/wgpu-core/src/command/render_command.rs +++ b/wgpu-core/src/command/render_command.rs @@ -1,6 +1,6 @@ use alloc::sync::Arc; -use wgt::{BufferAddress, BufferSizeOrZero, Color}; +use wgt::{BufferAddress, BufferSize, Color}; use super::{Rect, RenderBundle}; use crate::{ @@ -24,13 +24,13 @@ pub enum RenderCommand { buffer_id: id::BufferId, index_format: wgt::IndexFormat, offset: BufferAddress, - size: Option, + size: Option, }, SetVertexBuffer { slot: u32, buffer_id: id::BufferId, offset: BufferAddress, - size: Option, + size: Option, }, SetBlendConstant(Color), SetStencilReference(u32), @@ -418,18 +418,21 @@ pub enum ArcRenderCommand { offset: BufferAddress, // For a render pass, this reflects the argument passed by the - // application, which may be `None`. For a finished render bundle, this - // reflects the validated size of the binding, and will be populated - // even in the case that the application omitted the size. - size: Option, + // application, which may be `None`. For a render bundle, this reflects + // the validated size of the binding, and will be populated even in the + // case that the application omitted the size. + size: Option, }, SetVertexBuffer { slot: u32, buffer: Arc, offset: BufferAddress, - // See comment in `SetIndexBuffer`. - size: Option, + // For a render pass, this reflects the argument passed by the + // application, which may be `None`. For a render bundle, this reflects + // the validated size of the binding, and will be populated even in the + // case that the application omitted the size. + size: Option, }, SetBlendConstant(Color), SetStencilReference(u32), diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index e974f39f162..a668f270a89 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -8,10 +8,9 @@ use alloc::{ use core::{ fmt, mem::{self, ManuallyDrop}, - num::{NonZeroU32, NonZeroU64}, + num::NonZeroU32, sync::atomic::{AtomicBool, Ordering}, }; -use hal::ShouldBeNonZeroExt; use arrayvec::ArrayVec; use bitflags::Flags; @@ -2198,7 +2197,7 @@ impl Device { buffer.check_usage(pub_usage)?; - let bb = buffer.binding(bb.offset, bb.size.map(NonZeroU64::get), snatch_guard)?; + let bb = buffer.binding(bb.offset, bb.size, snatch_guard)?; let bind_size = bb.size.get(); if bind_size > range_limit as u64 { diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index 326a30bcfd8..df18ae83e2e 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -490,32 +490,35 @@ impl Buffer { /// If `size` is `None`, then the remainder of the buffer starting from /// `offset` is used. /// - /// If the binding would overflow the buffer, then an error is returned. - /// - /// Zero-size bindings are permitted here for historical reasons. Although - /// zero-size bindings are permitted by WebGPU, they are not permitted by - /// some backends. See [`Buffer::binding`] and - /// [#3170](https://github.com/gfx-rs/wgpu/issues/3170). + /// If the binding would overflow the buffer or is empty (see + /// [`hal::BufferBinding`]), then an error is returned. pub fn resolve_binding_size( &self, offset: wgt::BufferAddress, - binding_size: Option, - ) -> Result { + binding_size: Option, + ) -> Result { let buffer_size = self.size; match binding_size { - Some(binding_size) => match offset.checked_add(binding_size) { - Some(end) if end <= buffer_size => Ok(binding_size), - _ => Err(BindingError::BindingRangeTooLarge { - buffer: self.error_ident(), - offset, - binding_size, - buffer_size, - }), - }, + Some(binding_size) => { + match offset.checked_add(binding_size.get()) { + // `binding_size` is not zero which means `end == buffer_size` is ok. + Some(end) if end <= buffer_size => Ok(binding_size), + _ => Err(BindingError::BindingRangeTooLarge { + buffer: self.error_ident(), + offset, + binding_size: binding_size.get(), + buffer_size, + }), + } + } None => { + // We require that `buffer_size - offset` converts to + // `BufferSize` (`NonZeroU64`) because bindings must not be + // empty. buffer_size .checked_sub(offset) + .and_then(wgt::BufferSize::new) .ok_or_else(|| BindingError::BindingOffsetTooLarge { buffer: self.error_ident(), offset, @@ -531,20 +534,12 @@ impl Buffer { /// If `size` is `None`, then the remainder of the buffer starting from /// `offset` is used. /// - /// If the binding would overflow the buffer, then an error is returned. - /// - /// Zero-size bindings are permitted here for historical reasons. Although - /// zero-size bindings are permitted by WebGPU, they are not permitted by - /// some backends. Previous documentation for `hal::BufferBinding` - /// disallowed zero-size bindings, but this restriction was not honored - /// elsewhere in the code. Zero-size bindings need to be quashed or remapped - /// to a non-zero size, either universally in wgpu-core, or in specific - /// backends that do not support them. See - /// [#3170](https://github.com/gfx-rs/wgpu/issues/3170). + /// If the binding would overflow the buffer or is empty (see + /// [`hal::BufferBinding`]), then an error is returned. pub fn binding<'a>( &'a self, offset: wgt::BufferAddress, - binding_size: Option, + binding_size: Option, snatch_guard: &'a SnatchGuard, ) -> Result, BindingError> { let buf_raw = self.try_raw(snatch_guard)?; diff --git a/wgpu-hal/examples/halmark/main.rs b/wgpu-hal/examples/halmark/main.rs index 60eb3d59b82..5641eb4de2f 100644 --- a/wgpu-hal/examples/halmark/main.rs +++ b/wgpu-hal/examples/halmark/main.rs @@ -447,7 +447,11 @@ impl Example { let global_group = { let global_buffer_binding = unsafe { // SAFETY: This is the same size that was specified for buffer creation. - hal::BufferBinding::new_unchecked(&global_buffer, 0, global_buffer_desc.size) + hal::BufferBinding::new_unchecked( + &global_buffer, + 0, + global_buffer_desc.size.try_into().unwrap(), + ) }; let texture_binding = hal::TextureBinding { view: &texture_view, diff --git a/wgpu-hal/src/dx12/command.rs b/wgpu-hal/src/dx12/command.rs index 39e18af13b0..f57e6b9238a 100644 --- a/wgpu-hal/src/dx12/command.rs +++ b/wgpu-hal/src/dx12/command.rs @@ -1136,7 +1136,7 @@ impl crate::CommandEncoder for super::CommandEncoder { ) { let ibv = Direct3D12::D3D12_INDEX_BUFFER_VIEW { BufferLocation: binding.resolve_address(), - SizeInBytes: binding.size.try_into().unwrap(), + SizeInBytes: binding.resolve_size() as u32, Format: auxil::dxgi::conv::map_index_format(format), }; @@ -1149,7 +1149,7 @@ impl crate::CommandEncoder for super::CommandEncoder { ) { let vb = &mut self.pass.vertex_buffers[index as usize]; vb.BufferLocation = binding.resolve_address(); - vb.SizeInBytes = binding.size.try_into().unwrap(); + vb.SizeInBytes = binding.resolve_size() as u32; self.pass.dirty_vertex_buffers |= 1 << index; } diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs index e4b8e449e54..24cd3826d4b 100644 --- a/wgpu-hal/src/dx12/device.rs +++ b/wgpu-hal/src/dx12/device.rs @@ -1442,7 +1442,7 @@ impl crate::Device for super::Device { let end = start + entry.count as usize; for data in &desc.buffers[start..end] { let gpu_address = data.resolve_address(); - let mut size = data.size.try_into().unwrap(); + let mut size = data.resolve_size() as u32; if has_dynamic_offset { match ty { diff --git a/wgpu-hal/src/dx12/mod.rs b/wgpu-hal/src/dx12/mod.rs index cb5b1974f49..c8e6c3e0cfb 100644 --- a/wgpu-hal/src/dx12/mod.rs +++ b/wgpu-hal/src/dx12/mod.rs @@ -865,6 +865,13 @@ unsafe impl Sync for Buffer {} impl crate::DynBuffer for Buffer {} impl crate::BufferBinding<'_, Buffer> { + fn resolve_size(&self) -> wgt::BufferAddress { + match self.size { + Some(size) => size.get(), + None => self.buffer.size - self.offset, + } + } + // TODO: Return GPU handle directly? fn resolve_address(&self) -> wgt::BufferAddress { (unsafe { self.buffer.resource.GetGPUVirtualAddress() }) + self.offset diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs index 17a2fe72216..0b5718cf0e9 100644 --- a/wgpu-hal/src/gles/device.rs +++ b/wgpu-hal/src/gles/device.rs @@ -1261,11 +1261,10 @@ impl crate::Device for super::Device { let binding = match layout.ty { wgt::BindingType::Buffer { .. } => { let bb = &desc.buffers[entry.resource_index as usize]; - assert!(bb.size != 0, "zero-size bindings are not supported"); super::RawBinding::Buffer { raw: bb.buffer.raw.unwrap(), offset: bb.offset.try_into().unwrap(), - size: bb.size.try_into().unwrap(), + size: bb.size.get().try_into().unwrap(), } } wgt::BindingType::Sampler { .. } => { diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index 0ffdac35e7c..65e42180d01 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -297,7 +297,7 @@ use core::{ borrow::Borrow, error::Error, fmt, - num::{NonZeroU32, NonZeroU64}, + num::NonZeroU32, ops::{Range, RangeInclusive}, ptr::NonNull, }; @@ -1979,7 +1979,7 @@ pub struct PipelineLayoutDescriptor<'a, B: DynBindGroupLayout + ?Sized> { /// /// `wgpu_hal` guarantees that shaders compiled with /// [`ShaderModuleDescriptor::runtime_checks`] set to `true` cannot read or -/// write data via this binding outside the *accessible region* of a buffer: +/// write data via this binding outside the *accessible region* of [`buffer`]: /// /// - The accessible region starts at [`offset`]. /// @@ -2004,14 +2004,14 @@ pub struct PipelineLayoutDescriptor<'a, B: DynBindGroupLayout + ?Sized> { /// Some back ends cannot tolerate zero-length regions; for example, see /// [VUID-VkDescriptorBufferInfo-offset-00340][340] and /// [VUID-VkDescriptorBufferInfo-range-00341][341], or the -/// documentation for GLES's [glBindBufferRange][bbr]. This documentation -/// previously stated that a `BufferBinding` must have `offset` strictly less -/// than the size of the buffer, but this restriction was not honored elsewhere -/// in the code, so has been removed. However, it remains the case that -/// some backends do not support zero-length bindings, so additional -/// logic is needed somewhere to handle this properly. See -/// [#3170](https://github.com/gfx-rs/wgpu/issues/3170). +/// documentation for GLES's [glBindBufferRange][bbr]. For this reason, a valid +/// `BufferBinding` must have `offset` strictly less than the size of the +/// buffer. /// +/// WebGPU allows zero-length bindings, and there is not currently a mechanism +/// in place +/// +/// [`buffer`]: BufferBinding::buffer /// [`offset`]: BufferBinding::offset /// [`size`]: BufferBinding::size /// [`Storage`]: wgt::BufferBindingType::Storage @@ -2031,11 +2031,12 @@ pub struct BufferBinding<'a, B: DynBuffer + ?Sized> { /// The offset at which the bound region starts. /// - /// This must be less or equal to the size of the buffer. + /// Because zero-length bindings are not permitted (see above), this must be + /// strictly less than the size of the buffer. pub offset: wgt::BufferAddress, /// The size of the region bound, in bytes. - pub size: wgt::BufferSizeOrZero, + pub size: wgt::BufferSize, } // We must implement this manually because `B` is not necessarily `Clone`. @@ -2049,25 +2050,6 @@ impl Clone for BufferBinding<'_, B> { } } -/// Temporary convenience trait to let us call `.get()` on `u64`s in code that -/// really wants to be using `NonZeroU64`. -/// TODO(): remove this -pub trait ShouldBeNonZeroExt { - fn get(&self) -> u64; -} - -impl ShouldBeNonZeroExt for NonZeroU64 { - fn get(&self) -> u64 { - NonZeroU64::get(*self) - } -} - -impl ShouldBeNonZeroExt for u64 { - fn get(&self) -> u64 { - *self - } -} - impl<'a, B: DynBuffer + ?Sized> BufferBinding<'a, B> { /// Construct a `BufferBinding` with the given contents. /// @@ -2080,20 +2062,15 @@ impl<'a, B: DynBuffer + ?Sized> BufferBinding<'a, B> { /// /// SAFETY: The caller is responsible for ensuring that a binding of `size` /// bytes starting at `offset` is contained within the buffer. - /// - /// The `S` type parameter is a temporary convenience to allow callers to - /// pass either a `u64` or a `NonZeroU64`. When the zero-size binding issue - /// is resolved, the argument should just match the type of the member. - /// TODO(): remove the parameter - pub unsafe fn new_unchecked>( + pub unsafe fn new_unchecked( buffer: &'a B, offset: wgt::BufferAddress, - size: S, + size: wgt::BufferSize, ) -> Self { Self { buffer, offset, - size: size.into(), + size, } } } diff --git a/wgpu-hal/src/metal/command.rs b/wgpu-hal/src/metal/command.rs index 9f331fd4685..4fc1987ce99 100644 --- a/wgpu-hal/src/metal/command.rs +++ b/wgpu-hal/src/metal/command.rs @@ -4,7 +4,7 @@ use alloc::{ borrow::{Cow, ToOwned as _}, vec::Vec, }; -use core::{num::NonZeroU64, ops::Range}; +use core::ops::Range; use metal::{ MTLIndexType, MTLLoadAction, MTLPrimitiveType, MTLScissorRect, MTLSize, MTLStoreAction, MTLViewport, MTLVisibilityResultMode, NSRange, @@ -977,11 +977,9 @@ impl crate::CommandEncoder for super::CommandEncoder { let encoder = self.state.render.as_ref().unwrap(); encoder.set_vertex_buffer(buffer_index, Some(&binding.buffer.raw), binding.offset); - // https://github.com/gfx-rs/wgpu/issues/3170 - let size = - NonZeroU64::new(binding.size).expect("zero-size vertex buffers are not supported"); - - self.state.vertex_buffer_size_map.insert(buffer_index, size); + self.state + .vertex_buffer_size_map + .insert(buffer_index, binding.size); if let Some((index, sizes)) = self .state diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs index ba40854dd43..3835fd022b8 100644 --- a/wgpu-hal/src/metal/device.rs +++ b/wgpu-hal/src/metal/device.rs @@ -1,5 +1,5 @@ use alloc::{borrow::ToOwned as _, sync::Arc, vec::Vec}; -use core::{num::NonZeroU64, ptr::NonNull, sync::atomic}; +use core::{ptr::NonNull, sync::atomic}; use std::{thread, time}; use parking_lot::Mutex; @@ -928,12 +928,9 @@ impl crate::Device for super::Device { let end = start + 1; bg.buffers .extend(desc.buffers[start..end].iter().map(|source| { - // https://github.com/gfx-rs/wgpu/issues/3170 - let source_size = NonZeroU64::new(source.size) - .expect("zero-size bindings are not supported"); let binding_size = match ty { wgt::BufferBindingType::Storage { .. } => { - Some(source_size) + Some(source.size) } _ => None, }; diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs index 5251d0e05d6..ee0a88a469c 100644 --- a/wgpu-hal/src/vulkan/device.rs +++ b/wgpu-hal/src/vulkan/device.rs @@ -1804,12 +1804,10 @@ impl crate::Device for super::Device { (buffer_infos, local_buffer_infos) = buffer_infos.extend(desc.buffers[start as usize..end as usize].iter().map( |binding| { - // https://github.com/gfx-rs/wgpu/issues/3170 - assert!(binding.size != 0, "zero-size bindings are not supported"); vk::DescriptorBufferInfo::default() .buffer(binding.buffer.raw) .offset(binding.offset) - .range(binding.size) + .range(binding.size.get()) }, )); write.buffer_info(local_buffer_infos) diff --git a/wgpu-types/src/lib.rs b/wgpu-types/src/lib.rs index 0b676dd8cfc..ea821512eb5 100644 --- a/wgpu-types/src/lib.rs +++ b/wgpu-types/src/lib.rs @@ -61,12 +61,6 @@ pub type BufferAddress = u64; /// [`BufferSlice`]: ../wgpu/struct.BufferSlice.html pub type BufferSize = core::num::NonZeroU64; -/// Integral type used for buffer sizes that may be zero. -/// -/// Although the wgpu Rust API disallows zero-size `BufferSlice` and wgpu-hal -/// disallows zero-size bindings, WebGPU permits zero-size buffers and bindings. -pub type BufferSizeOrZero = u64; - /// Integral type used for binding locations in shaders. /// /// Used in [`VertexAttribute`]s and errors. From cbb166a8d9f784ce788fc8ee3869991cc1a21e4d Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Wed, 9 Jul 2025 13:05:59 -0700 Subject: [PATCH 2/2] Revert "Validate binding ranges against buffer size" This reverts commit ef428fcab8059e898b42542b6445bd94a9683e69. --- wgpu-core/src/binding_model.rs | 43 ++------- wgpu-core/src/command/bundle.rs | 81 +++++------------ wgpu-core/src/command/draw.rs | 5 +- wgpu-core/src/command/render.rs | 51 ++++++----- wgpu-core/src/command/render_command.rs | 21 ----- wgpu-core/src/device/global.rs | 3 +- wgpu-core/src/device/resource.rs | 91 +++++++++++-------- wgpu-core/src/indirect_validation/dispatch.rs | 14 +-- wgpu-core/src/indirect_validation/draw.rs | 14 +-- wgpu-core/src/resource.rs | 72 +-------------- wgpu-core/src/timestamp_normalization/mod.rs | 16 ++-- wgpu-hal/examples/halmark/main.rs | 22 ++--- wgpu-hal/examples/ray-traced-triangle/main.rs | 11 +-- wgpu-hal/src/gles/device.rs | 9 +- wgpu-hal/src/gles/mod.rs | 1 + wgpu-hal/src/lib.rs | 73 ++++----------- wgpu-hal/src/metal/command.rs | 12 ++- wgpu-hal/src/metal/device.rs | 16 +++- wgpu-hal/src/metal/mod.rs | 10 ++ wgpu-hal/src/vulkan/device.rs | 4 +- wgpu/src/api/device.rs | 1 - wgpu/src/backend/wgpu_core.rs | 6 -- 22 files changed, 213 insertions(+), 363 deletions(-) diff --git a/wgpu-core/src/binding_model.rs b/wgpu-core/src/binding_model.rs index 9b1c12fada4..8075887ed98 100644 --- a/wgpu-core/src/binding_model.rs +++ b/wgpu-core/src/binding_model.rs @@ -94,39 +94,8 @@ impl WebGpuError for CreateBindGroupLayoutError { } } -#[derive(Clone, Debug, Error)] -#[non_exhaustive] -pub enum BindingError { - #[error(transparent)] - DestroyedResource(#[from] DestroyedResourceError), - #[error("Buffer {buffer}: Binding with size {binding_size} at offset {offset} would overflow buffer size of {buffer_size}")] - BindingRangeTooLarge { - buffer: ResourceErrorIdent, - offset: wgt::BufferAddress, - binding_size: u64, - buffer_size: u64, - }, - #[error("Buffer {buffer}: Binding offset {offset} is greater than or equal to buffer size {buffer_size}")] - BindingOffsetTooLarge { - buffer: ResourceErrorIdent, - offset: wgt::BufferAddress, - buffer_size: u64, - }, -} +//TODO: refactor this to move out `enum BindingError`. -impl WebGpuError for BindingError { - fn webgpu_error_type(&self) -> ErrorType { - match self { - Self::DestroyedResource(e) => e.webgpu_error_type(), - Self::BindingRangeTooLarge { .. } | Self::BindingOffsetTooLarge { .. } => { - ErrorType::Validation - } - } - } -} - -// TODO: there may be additional variants here that can be extracted into -// `BindingError`. #[derive(Clone, Debug, Error)] #[non_exhaustive] pub enum CreateBindGroupError { @@ -134,8 +103,6 @@ pub enum CreateBindGroupError { Device(#[from] DeviceError), #[error(transparent)] DestroyedResource(#[from] DestroyedResourceError), - #[error(transparent)] - BindingError(#[from] BindingError), #[error( "Binding count declared with at most {expected} items, but {actual} items were provided" )] @@ -146,6 +113,12 @@ pub enum CreateBindGroupError { BindingArrayLengthMismatch { actual: usize, expected: usize }, #[error("Array binding provided zero elements")] BindingArrayZeroLength, + #[error("The bound range {range:?} of {buffer} overflows its size ({size})")] + BindingRangeTooLarge { + buffer: ResourceErrorIdent, + range: Range, + size: u64, + }, #[error("Binding size {actual} of {buffer} is less than minimum {min}")] BindingSizeTooSmall { buffer: ResourceErrorIdent, @@ -260,7 +233,6 @@ impl WebGpuError for CreateBindGroupError { let e: &dyn WebGpuError = match self { Self::Device(e) => e, Self::DestroyedResource(e) => e, - Self::BindingError(e) => e, Self::MissingBufferUsage(e) => e, Self::MissingTextureUsage(e) => e, Self::ResourceUsageCompatibility(e) => e, @@ -268,6 +240,7 @@ impl WebGpuError for CreateBindGroupError { Self::BindingArrayPartialLengthMismatch { .. } | Self::BindingArrayLengthMismatch { .. } | Self::BindingArrayZeroLength + | Self::BindingRangeTooLarge { .. } | Self::BindingSizeTooSmall { .. } | Self::BindingsNumMismatch { .. } | Self::BindingZeroSize(_) diff --git a/wgpu-core/src/command/bundle.rs b/wgpu-core/src/command/bundle.rs index d924190db26..7a64502b14d 100644 --- a/wgpu-core/src/command/bundle.rs +++ b/wgpu-core/src/command/bundle.rs @@ -602,7 +602,6 @@ fn set_pipeline( Ok(()) } -// This function is duplicative of `render::set_index_buffer`. fn set_index_buffer( state: &mut State, buffer_guard: &crate::storage::Storage>, @@ -621,20 +620,21 @@ fn set_index_buffer( buffer.same_device(&state.device)?; buffer.check_usage(wgt::BufferUsages::INDEX)?; - let end = buffer.resolve_binding_size(offset, size)?; - + let end = match size { + Some(s) => offset + s.get(), + None => buffer.size, + }; state .buffer_memory_init_actions .extend(buffer.initialization_status.read().create_action( &buffer, - offset..end.get(), + offset..end, MemoryInitKind::NeedsInitializedMemory, )); - state.set_index_buffer(buffer, index_format, offset..end.get()); + state.set_index_buffer(buffer, index_format, offset..end); Ok(()) } -// This function is duplicative of `render::set_vertex_buffer`. fn set_vertex_buffer( state: &mut State, buffer_guard: &crate::storage::Storage>, @@ -662,16 +662,18 @@ fn set_vertex_buffer( buffer.same_device(&state.device)?; buffer.check_usage(wgt::BufferUsages::VERTEX)?; - let end = buffer.resolve_binding_size(offset, size)?; - + let end = match size { + Some(s) => offset + s.get(), + None => buffer.size, + }; state .buffer_memory_init_actions .extend(buffer.initialization_status.read().create_action( &buffer, - offset..end.get(), + offset..end, MemoryInitKind::NeedsInitializedMemory, )); - state.vertex[slot as usize] = Some(VertexState::new(buffer, offset..end.get())); + state.vertex[slot as usize] = Some(VertexState::new(buffer, offset..end)); Ok(()) } @@ -963,14 +965,10 @@ impl RenderBundle { size, } => { let buffer = buffer.try_raw(snatch_guard)?; - let bb = unsafe { - // SAFETY: The binding size was checked against the buffer size - // in `set_index_buffer` and again in `IndexState::flush`. - hal::BufferBinding::new_unchecked( - buffer, - *offset, - size.expect("size was resolved in `RenderBundleEncoder::finish`"), - ) + let bb = hal::BufferBinding { + buffer, + offset: *offset, + size: *size, }; unsafe { raw.set_index_buffer(bb, *index_format) }; } @@ -981,14 +979,10 @@ impl RenderBundle { size, } => { let buffer = buffer.try_raw(snatch_guard)?; - let bb = unsafe { - // SAFETY: The binding size was checked against the buffer size - // in `set_vertex_buffer` and again in `VertexState::flush`. - hal::BufferBinding::new_unchecked( - buffer, - *offset, - size.expect("size was resolved in `RenderBundleEncoder::finish`"), - ) + let bb = hal::BufferBinding { + buffer, + offset: *offset, + size: *size, }; unsafe { raw.set_vertex_buffer(*slot, bb) }; } @@ -1137,9 +1131,6 @@ crate::impl_trackable!(RenderBundle); /// [`RenderBundleEncoder::finish`] records the currently set index buffer here, /// and calls [`State::flush_index`] before any indexed draw command to produce /// a `SetIndexBuffer` command if one is necessary. -/// -/// Binding ranges must be validated against the size of the buffer before -/// being stored in `IndexState`. #[derive(Debug)] struct IndexState { buffer: Arc, @@ -1161,24 +1152,13 @@ impl IndexState { /// Generate a `SetIndexBuffer` command to prepare for an indexed draw /// command, if needed. fn flush(&mut self) -> Option { - // This was all checked before, but let's check again just in case. - let binding_size = self - .range - .end - .checked_sub(self.range.start) - .and_then(wgt::BufferSize::new); - assert!( - self.range.end <= self.buffer.size && binding_size.is_some(), - "index buffer range must have non-zero size and be contained in buffer", - ); - if self.is_dirty { self.is_dirty = false; Some(ArcRenderCommand::SetIndexBuffer { buffer: self.buffer.clone(), index_format: self.format, offset: self.range.start, - size: binding_size, + size: wgt::BufferSize::new(self.range.end - self.range.start), }) } else { None @@ -1194,9 +1174,6 @@ impl IndexState { /// calls this type's [`flush`] method just before any draw command to /// produce a `SetVertexBuffer` commands if one is necessary. /// -/// Binding ranges must be validated against the size of the buffer before -/// being stored in `VertexState`. -/// /// [`flush`]: IndexState::flush #[derive(Debug)] struct VertexState { @@ -1206,9 +1183,6 @@ struct VertexState { } impl VertexState { - /// Create a new `VertexState`. - /// - /// The `range` must be contained within `buffer`. fn new(buffer: Arc, range: Range) -> Self { Self { buffer, @@ -1221,24 +1195,13 @@ impl VertexState { /// /// `slot` is the index of the vertex buffer slot that `self` tracks. fn flush(&mut self, slot: u32) -> Option { - // This was all checked before, but let's check again just in case. - let binding_size = self - .range - .end - .checked_sub(self.range.start) - .and_then(wgt::BufferSize::new); - assert!( - self.range.end <= self.buffer.size && binding_size.is_some(), - "vertex buffer range must have non-zero size and be contained in buffer", - ); - if self.is_dirty { self.is_dirty = false; Some(ArcRenderCommand::SetVertexBuffer { slot, buffer: self.buffer.clone(), offset: self.range.start, - size: binding_size, + size: wgt::BufferSize::new(self.range.end - self.range.start), }) } else { None diff --git a/wgpu-core/src/command/draw.rs b/wgpu-core/src/command/draw.rs index 7dadc8bfa4e..53a3f204fcc 100644 --- a/wgpu-core/src/command/draw.rs +++ b/wgpu-core/src/command/draw.rs @@ -7,7 +7,7 @@ use wgt::error::{ErrorType, WebGpuError}; use super::bind::BinderError; use crate::command::pass; use crate::{ - binding_model::{BindingError, LateMinBufferBindingSizeMismatch, PushConstantUploadError}, + binding_model::{LateMinBufferBindingSizeMismatch, PushConstantUploadError}, resource::{ DestroyedResourceError, MissingBufferUsageError, MissingTextureUsageError, ResourceErrorIdent, @@ -89,8 +89,6 @@ pub enum RenderCommandError { MissingTextureUsage(#[from] MissingTextureUsageError), #[error(transparent)] PushConstants(#[from] PushConstantUploadError), - #[error(transparent)] - BindingError(#[from] BindingError), #[error("Viewport size {{ w: {w}, h: {h} }} greater than device's requested `max_texture_dimension_2d` limit {max}, or less than zero")] InvalidViewportRectSize { w: f32, h: f32, max: u32 }, #[error("Viewport has invalid rect {rect:?} for device's requested `max_texture_dimension_2d` limit; Origin less than -2 * `max_texture_dimension_2d` ({min}), or rect extends past 2 * `max_texture_dimension_2d` - 1 ({max})")] @@ -112,7 +110,6 @@ impl WebGpuError for RenderCommandError { Self::MissingBufferUsage(e) => e, Self::MissingTextureUsage(e) => e, Self::PushConstants(e) => e, - Self::BindingError(e) => e, Self::BindGroupIndexOutOfRange { .. } | Self::VertexBufferIndexOutOfRange { .. } diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index d1596a5c474..19129f891c9 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -1,5 +1,5 @@ use alloc::{borrow::Cow, sync::Arc, vec::Vec}; -use core::{fmt, num::NonZeroU32, str}; +use core::{fmt, num::NonZeroU32, ops::Range, str}; use arrayvec::ArrayVec; use thiserror::Error; @@ -356,17 +356,13 @@ struct IndexState { } impl IndexState { - fn update_buffer( - &mut self, - binding: &hal::BufferBinding<'_, B>, - format: IndexFormat, - ) { + fn update_buffer(&mut self, range: Range, format: IndexFormat) { self.buffer_format = Some(format); let shift = match format { IndexFormat::Uint16 => 1, IndexFormat::Uint32 => 2, }; - self.limit = binding.size.get() >> shift; + self.limit = (range.end - range.start) >> shift; } fn reset(&mut self) { @@ -2326,7 +2322,6 @@ fn set_pipeline( Ok(()) } -// This function is duplicative of `bundle::set_index_buffer`. fn set_index_buffer( state: &mut State, cmd_buf: &Arc, @@ -2346,27 +2341,33 @@ fn set_index_buffer( buffer.same_device_as(cmd_buf.as_ref())?; buffer.check_usage(BufferUsages::INDEX)?; + let buf_raw = buffer.try_raw(state.general.snatch_guard)?; - let binding = buffer - .binding(offset, size, state.general.snatch_guard) - .map_err(RenderCommandError::from)?; - state.index.update_buffer(&binding, index_format); + let end = match size { + Some(s) => offset + s.get(), + None => buffer.size, + }; + state.index.update_buffer(offset..end, index_format); state.general.buffer_memory_init_actions.extend( buffer.initialization_status.read().create_action( &buffer, - offset..(offset + binding.size.get()), + offset..end, MemoryInitKind::NeedsInitializedMemory, ), ); + let bb = hal::BufferBinding { + buffer: buf_raw, + offset, + size, + }; unsafe { - hal::DynCommandEncoder::set_index_buffer(state.general.raw_encoder, binding, index_format); + hal::DynCommandEncoder::set_index_buffer(state.general.raw_encoder, bb, index_format); } Ok(()) } -// This function is duplicative of `render::set_vertex_buffer`. fn set_vertex_buffer( state: &mut State, cmd_buf: &Arc, @@ -2398,22 +2399,30 @@ fn set_vertex_buffer( } buffer.check_usage(BufferUsages::VERTEX)?; + let buf_raw = buffer.try_raw(state.general.snatch_guard)?; - let binding = buffer - .binding(offset, size, state.general.snatch_guard) - .map_err(RenderCommandError::from)?; - state.vertex.buffer_sizes[slot as usize] = Some(binding.size.get()); + //TODO: where are we checking that the offset is in bound? + let buffer_size = match size { + Some(s) => s.get(), + None => buffer.size - offset, + }; + state.vertex.buffer_sizes[slot as usize] = Some(buffer_size); state.general.buffer_memory_init_actions.extend( buffer.initialization_status.read().create_action( &buffer, - offset..(offset + binding.size.get()), + offset..(offset + buffer_size), MemoryInitKind::NeedsInitializedMemory, ), ); + let bb = hal::BufferBinding { + buffer: buf_raw, + offset, + size, + }; unsafe { - hal::DynCommandEncoder::set_vertex_buffer(state.general.raw_encoder, slot, binding); + hal::DynCommandEncoder::set_vertex_buffer(state.general.raw_encoder, slot, bb); } if let Some(pipeline) = state.pipeline.as_ref() { state.vertex.update_limits(&pipeline.vertex_steps); diff --git a/wgpu-core/src/command/render_command.rs b/wgpu-core/src/command/render_command.rs index 606d3fe9498..6fc4cbf5cf5 100644 --- a/wgpu-core/src/command/render_command.rs +++ b/wgpu-core/src/command/render_command.rs @@ -392,17 +392,6 @@ impl RenderCommand { } /// Equivalent to `RenderCommand` with the Ids resolved into resource Arcs. -/// -/// In a render pass, commands are stored in this format between when they are -/// added to the pass, and when the pass is `end()`ed and the commands are -/// replayed to the HAL encoder. Validation occurs when the pass is ended, which -/// means that parameters stored in an `ArcRenderCommand` for a pass operation -/// have generally not been validated. -/// -/// In a render bundle, commands are stored in this format between when the bundle -/// is `finish()`ed and when the bundle is executed. Validation occurs when the -/// bundle is finished, which means that parameters stored in an `ArcRenderCommand` -/// for a render bundle operation must have been validated. #[doc(hidden)] #[derive(Clone, Debug)] pub enum ArcRenderCommand { @@ -416,22 +405,12 @@ pub enum ArcRenderCommand { buffer: Arc, index_format: wgt::IndexFormat, offset: BufferAddress, - - // For a render pass, this reflects the argument passed by the - // application, which may be `None`. For a render bundle, this reflects - // the validated size of the binding, and will be populated even in the - // case that the application omitted the size. size: Option, }, SetVertexBuffer { slot: u32, buffer: Arc, offset: BufferAddress, - - // For a render pass, this reflects the argument passed by the - // application, which may be `None`. For a render bundle, this reflects - // the validated size of the binding, and will be populated even in the - // case that the application omitted the size. size: Option, }, SetBlendConstant(Color), diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index d05fb8c8cb8..d61be9613be 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -383,7 +383,6 @@ impl Global { /// - `hal_buffer` must be created from `device_id` corresponding raw handle. /// - `hal_buffer` must be created respecting `desc` /// - `hal_buffer` must be initialized - /// - `hal_buffer` must not have zero size. pub unsafe fn create_buffer_from_hal( &self, hal_buffer: A::Buffer, @@ -405,7 +404,7 @@ impl Global { trace.add(trace::Action::CreateBuffer(fid.id(), desc.clone())); } - let (buffer, err) = unsafe { device.create_buffer_from_hal(Box::new(hal_buffer), desc) }; + let (buffer, err) = device.create_buffer_from_hal(Box::new(hal_buffer), desc); let id = fid.assign(buffer); api_log!("Device::create_buffer -> {id:?}"); diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index a668f270a89..f68b8d69329 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -702,8 +702,7 @@ impl Device { let buffer = unsafe { self.raw().create_buffer(&hal_desc) } .map_err(|e| self.handle_hal_error_with_nonfatal_oom(e))?; - let timestamp_normalization_bind_group = Snatchable::new(unsafe { - // SAFETY: The size passed here must not overflow the buffer. + let timestamp_normalization_bind_group = Snatchable::new( self.timestamp_normalizer .get() .unwrap() @@ -711,10 +710,10 @@ impl Device { self, &*buffer, desc.label.as_deref(), - wgt::BufferSize::new(hal_desc.size).unwrap(), + desc.size, desc.usage, - ) - }?); + )?, + ); let indirect_validation_bind_groups = self.create_indirect_validation_bind_groups(buffer.as_ref(), desc.size, desc.usage)?; @@ -810,36 +809,28 @@ impl Device { Ok(texture) } - /// # Safety - /// - /// - `hal_buffer` must have been created on this device. - /// - `hal_buffer` must have been created respecting `desc` (in particular, the size). - /// - `hal_buffer` must be initialized. - /// - `hal_buffer` must not have zero size. - pub(crate) unsafe fn create_buffer_from_hal( + pub(crate) fn create_buffer_from_hal( self: &Arc, hal_buffer: Box, desc: &resource::BufferDescriptor, ) -> (Fallible, Option) { - let timestamp_normalization_bind_group = unsafe { - match self - .timestamp_normalizer - .get() - .unwrap() - .create_normalization_bind_group( - self, - &*hal_buffer, - desc.label.as_deref(), - wgt::BufferSize::new(desc.size).unwrap(), - desc.usage, - ) { - Ok(bg) => Snatchable::new(bg), - Err(e) => { - return ( - Fallible::Invalid(Arc::new(desc.label.to_string())), - Some(e.into()), - ) - } + let timestamp_normalization_bind_group = match self + .timestamp_normalizer + .get() + .unwrap() + .create_normalization_bind_group( + self, + &*hal_buffer, + desc.label.as_deref(), + desc.size, + desc.usage, + ) { + Ok(bg) => Snatchable::new(bg), + Err(e) => { + return ( + Fallible::Invalid(Arc::new(desc.label.to_string())), + Some(e.into()), + ) } }; @@ -2196,9 +2187,31 @@ impl Device { buffer.same_device(self)?; buffer.check_usage(pub_usage)?; - - let bb = buffer.binding(bb.offset, bb.size, snatch_guard)?; - let bind_size = bb.size.get(); + let raw_buffer = buffer.try_raw(snatch_guard)?; + + let (bind_size, bind_end) = match bb.size { + Some(size) => { + let end = bb.offset + size.get(); + if end > buffer.size { + return Err(Error::BindingRangeTooLarge { + buffer: buffer.error_ident(), + range: bb.offset..end, + size: buffer.size, + }); + } + (size.get(), end) + } + None => { + if buffer.size < bb.offset { + return Err(Error::BindingRangeTooLarge { + buffer: buffer.error_ident(), + range: bb.offset..bb.offset, + size: buffer.size, + }); + } + (buffer.size - bb.offset, buffer.size) + } + }; if bind_size > range_limit as u64 { return Err(Error::BufferRangeTooLarge { @@ -2213,8 +2226,8 @@ impl Device { dynamic_binding_info.push(binding_model::BindGroupDynamicBindingData { binding_idx: binding, buffer_size: buffer.size, - binding_range: bb.offset..bb.offset + bind_size, - maximum_dynamic_offset: buffer.size - bb.offset - bind_size, + binding_range: bb.offset..bind_end, + maximum_dynamic_offset: buffer.size - bind_end, binding_type: binding_ty, }); } @@ -2252,7 +2265,11 @@ impl Device { MemoryInitKind::NeedsInitializedMemory, )); - Ok(bb) + Ok(hal::BufferBinding { + buffer: raw_buffer, + offset: bb.offset, + size: bb.size, + }) } fn create_sampler_binding<'a>( diff --git a/wgpu-core/src/indirect_validation/dispatch.rs b/wgpu-core/src/indirect_validation/dispatch.rs index e9fe4971bf7..00e3798e9ba 100644 --- a/wgpu-core/src/indirect_validation/dispatch.rs +++ b/wgpu-core/src/indirect_validation/dispatch.rs @@ -232,9 +232,10 @@ impl Dispatch { resource_index: 0, count: 1, }], - buffers: &[unsafe { - // SAFETY: We just created the buffer with this size. - hal::BufferBinding::new_unchecked(dst_buffer.as_ref(), 0, DST_BUFFER_SIZE) + buffers: &[hal::BufferBinding { + buffer: dst_buffer.as_ref(), + offset: 0, + size: Some(DST_BUFFER_SIZE), }], samplers: &[], textures: &[], @@ -277,9 +278,10 @@ impl Dispatch { resource_index: 0, count: 1, }], - buffers: &[unsafe { - // SAFETY: We calculated the binding size to fit within the buffer. - hal::BufferBinding::new_unchecked(buffer, 0, binding_size) + buffers: &[hal::BufferBinding { + buffer, + offset: 0, + size: Some(binding_size), }], samplers: &[], textures: &[], diff --git a/wgpu-core/src/indirect_validation/draw.rs b/wgpu-core/src/indirect_validation/draw.rs index af0e1a2c54c..d88acb8d60d 100644 --- a/wgpu-core/src/indirect_validation/draw.rs +++ b/wgpu-core/src/indirect_validation/draw.rs @@ -135,9 +135,10 @@ impl Draw { resource_index: 0, count: 1, }], - buffers: &[unsafe { - // SAFETY: We calculated the binding size to fit within the buffer. - hal::BufferBinding::new_unchecked(buffer, 0, binding_size) + buffers: &[hal::BufferBinding { + buffer, + offset: 0, + size: Some(binding_size), }], samplers: &[], textures: &[], @@ -683,9 +684,10 @@ fn create_buffer_and_bind_group( resource_index: 0, count: 1, }], - buffers: &[unsafe { - // SAFETY: We just created the buffer with this size. - hal::BufferBinding::new_unchecked(buffer.as_ref(), 0, BUFFER_SIZE) + buffers: &[hal::BufferBinding { + buffer: buffer.as_ref(), + offset: 0, + size: Some(BUFFER_SIZE), }], samplers: &[], textures: &[], diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index df18ae83e2e..022b1ba59f2 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -17,7 +17,7 @@ use wgt::{ #[cfg(feature = "trace")] use crate::device::trace; use crate::{ - binding_model::{BindGroup, BindingError}, + binding_model::BindGroup, device::{ queue, resource::DeferredDestroy, BufferMapPendingClosure, Device, DeviceError, DeviceMismatch, HostMap, MissingDownlevelFlags, MissingFeatures, @@ -485,76 +485,6 @@ impl Buffer { } } - /// Resolve the size of a binding for buffer with `offset` and `size`. - /// - /// If `size` is `None`, then the remainder of the buffer starting from - /// `offset` is used. - /// - /// If the binding would overflow the buffer or is empty (see - /// [`hal::BufferBinding`]), then an error is returned. - pub fn resolve_binding_size( - &self, - offset: wgt::BufferAddress, - binding_size: Option, - ) -> Result { - let buffer_size = self.size; - - match binding_size { - Some(binding_size) => { - match offset.checked_add(binding_size.get()) { - // `binding_size` is not zero which means `end == buffer_size` is ok. - Some(end) if end <= buffer_size => Ok(binding_size), - _ => Err(BindingError::BindingRangeTooLarge { - buffer: self.error_ident(), - offset, - binding_size: binding_size.get(), - buffer_size, - }), - } - } - None => { - // We require that `buffer_size - offset` converts to - // `BufferSize` (`NonZeroU64`) because bindings must not be - // empty. - buffer_size - .checked_sub(offset) - .and_then(wgt::BufferSize::new) - .ok_or_else(|| BindingError::BindingOffsetTooLarge { - buffer: self.error_ident(), - offset, - buffer_size, - }) - } - } - } - - /// Create a new [`hal::BufferBinding`] for the buffer with `offset` and - /// `size`. - /// - /// If `size` is `None`, then the remainder of the buffer starting from - /// `offset` is used. - /// - /// If the binding would overflow the buffer or is empty (see - /// [`hal::BufferBinding`]), then an error is returned. - pub fn binding<'a>( - &'a self, - offset: wgt::BufferAddress, - binding_size: Option, - snatch_guard: &'a SnatchGuard, - ) -> Result, BindingError> { - let buf_raw = self.try_raw(snatch_guard)?; - let resolved_size = self.resolve_binding_size(offset, binding_size)?; - unsafe { - // SAFETY: The offset and size passed to hal::BufferBinding::new_unchecked must - // define a binding contained within the buffer. - Ok(hal::BufferBinding::new_unchecked( - buf_raw, - offset, - resolved_size, - )) - } - } - /// Returns the mapping callback in case of error so that the callback can be fired outside /// of the locks that are held in this function. pub(crate) fn map_async( diff --git a/wgpu-core/src/timestamp_normalization/mod.rs b/wgpu-core/src/timestamp_normalization/mod.rs index e5a9ef9a8ad..dd4d466235c 100644 --- a/wgpu-core/src/timestamp_normalization/mod.rs +++ b/wgpu-core/src/timestamp_normalization/mod.rs @@ -242,16 +242,12 @@ impl TimestampNormalizer { } } - /// Create a bind group for normalizing timestamps in `buffer`. - /// - /// This function is unsafe because it does not know that `buffer_size` is - /// the true size of the buffer. - pub unsafe fn create_normalization_bind_group( + pub fn create_normalization_bind_group( &self, device: &Device, buffer: &dyn hal::DynBuffer, buffer_label: Option<&str>, - buffer_size: wgt::BufferSize, + buffer_size: u64, buffer_usages: wgt::BufferUsages, ) -> Result { unsafe { @@ -267,7 +263,7 @@ impl TimestampNormalizer { // at once to normalize the timestamps, we can't use it. We force the buffer to fail // to allocate. The lowest max binding size is 128MB, and query sets must be small // (no more than 4096), so this should never be hit in practice by sane programs. - if buffer_size.get() > device.adapter.limits().max_storage_buffer_binding_size as u64 { + if buffer_size > device.adapter.limits().max_storage_buffer_binding_size as u64 { return Err(DeviceError::OutOfMemory); } @@ -286,7 +282,11 @@ impl TimestampNormalizer { .create_bind_group(&hal::BindGroupDescriptor { label: Some(label), layout: &*state.temporary_bind_group_layout, - buffers: &[hal::BufferBinding::new_unchecked(buffer, 0, buffer_size)], + buffers: &[hal::BufferBinding { + buffer, + offset: 0, + size: None, + }], samplers: &[], textures: &[], acceleration_structures: &[], diff --git a/wgpu-hal/examples/halmark/main.rs b/wgpu-hal/examples/halmark/main.rs index 5641eb4de2f..75f3bc2fb9a 100644 --- a/wgpu-hal/examples/halmark/main.rs +++ b/wgpu-hal/examples/halmark/main.rs @@ -445,13 +445,10 @@ impl Example { let texture_view = unsafe { device.create_texture_view(&texture, &view_desc).unwrap() }; let global_group = { - let global_buffer_binding = unsafe { - // SAFETY: This is the same size that was specified for buffer creation. - hal::BufferBinding::new_unchecked( - &global_buffer, - 0, - global_buffer_desc.size.try_into().unwrap(), - ) + let global_buffer_binding = hal::BufferBinding { + buffer: &global_buffer, + offset: 0, + size: None, }; let texture_binding = hal::TextureBinding { view: &texture_view, @@ -486,13 +483,10 @@ impl Example { }; let local_group = { - let local_buffer_binding = unsafe { - // SAFETY: The size must fit within the buffer. - hal::BufferBinding::new_unchecked( - &local_buffer, - 0, - wgpu_types::BufferSize::new(size_of::() as _).unwrap(), - ) + let local_buffer_binding = hal::BufferBinding { + buffer: &local_buffer, + offset: 0, + size: wgpu_types::BufferSize::new(size_of::() as _), }; let local_group_desc = hal::BindGroupDescriptor { label: Some("local"), diff --git a/wgpu-hal/examples/ray-traced-triangle/main.rs b/wgpu-hal/examples/ray-traced-triangle/main.rs index 93e687ff1b5..a8d3a77b916 100644 --- a/wgpu-hal/examples/ray-traced-triangle/main.rs +++ b/wgpu-hal/examples/ray-traced-triangle/main.rs @@ -603,13 +603,10 @@ impl Example { let texture_view = unsafe { device.create_texture_view(&texture, &view_desc).unwrap() }; let bind_group = { - let buffer_binding = unsafe { - // SAFETY: The size matches the buffer allocation. - hal::BufferBinding::new_unchecked( - &uniform_buffer, - 0, - wgpu_types::BufferSize::new_unchecked(uniforms_size as u64), - ) + let buffer_binding = hal::BufferBinding { + buffer: &uniform_buffer, + offset: 0, + size: None, }; let texture_binding = hal::TextureBinding { view: &texture_view, diff --git a/wgpu-hal/src/gles/device.rs b/wgpu-hal/src/gles/device.rs index 0b5718cf0e9..0f36f734b8c 100644 --- a/wgpu-hal/src/gles/device.rs +++ b/wgpu-hal/src/gles/device.rs @@ -534,6 +534,7 @@ impl crate::Device for super::Device { return Ok(super::Buffer { raw: None, target, + size: desc.size, map_flags: 0, data: Some(Arc::new(MaybeMutex::new(vec![0; desc.size as usize]))), offset_of_current_mapping: Arc::new(MaybeMutex::new(0)), @@ -633,6 +634,7 @@ impl crate::Device for super::Device { Ok(super::Buffer { raw, target, + size: desc.size, map_flags, data, offset_of_current_mapping: Arc::new(MaybeMutex::new(0)), @@ -1263,8 +1265,11 @@ impl crate::Device for super::Device { let bb = &desc.buffers[entry.resource_index as usize]; super::RawBinding::Buffer { raw: bb.buffer.raw.unwrap(), - offset: bb.offset.try_into().unwrap(), - size: bb.size.get().try_into().unwrap(), + offset: bb.offset as i32, + size: match bb.size { + Some(s) => s.get() as i32, + None => (bb.buffer.size - bb.offset) as i32, + }, } } wgt::BindingType::Sampler { .. } => { diff --git a/wgpu-hal/src/gles/mod.rs b/wgpu-hal/src/gles/mod.rs index c1b226f8cd6..a6073b4ec8f 100644 --- a/wgpu-hal/src/gles/mod.rs +++ b/wgpu-hal/src/gles/mod.rs @@ -342,6 +342,7 @@ impl Drop for Queue { pub struct Buffer { raw: Option, target: BindTarget, + size: wgt::BufferAddress, map_flags: u32, data: Option>>>, offset_of_current_mapping: Arc>, diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index 65e42180d01..6f05edbb168 100644 --- a/wgpu-hal/src/lib.rs +++ b/wgpu-hal/src/lib.rs @@ -1968,13 +1968,6 @@ pub struct PipelineLayoutDescriptor<'a, B: DynBindGroupLayout + ?Sized> { /// /// [`BindGroup`]: Api::BindGroup /// -/// ## Construction -/// -/// The recommended way to construct a `BufferBinding` is using the `binding` -/// method on a wgpu-core `Buffer`, which will validate the binding size -/// against the buffer size. An unsafe `new_unchecked` constructor is also -/// provided for cases where direct construction is necessary. -/// /// ## Accessible region /// /// `wgpu_hal` guarantees that shaders compiled with @@ -1999,48 +1992,39 @@ pub struct PipelineLayoutDescriptor<'a, B: DynBindGroupLayout + ?Sized> { /// parts of which buffers shaders might observe. This optimization is only /// sound if shader access is bounds-checked. /// -/// ## Zero-length bindings -/// -/// Some back ends cannot tolerate zero-length regions; for example, see -/// [VUID-VkDescriptorBufferInfo-offset-00340][340] and -/// [VUID-VkDescriptorBufferInfo-range-00341][341], or the -/// documentation for GLES's [glBindBufferRange][bbr]. For this reason, a valid -/// `BufferBinding` must have `offset` strictly less than the size of the -/// buffer. -/// -/// WebGPU allows zero-length bindings, and there is not currently a mechanism -/// in place -/// /// [`buffer`]: BufferBinding::buffer /// [`offset`]: BufferBinding::offset /// [`size`]: BufferBinding::size /// [`Storage`]: wgt::BufferBindingType::Storage /// [`Uniform`]: wgt::BufferBindingType::Uniform -/// [340]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorBufferInfo-offset-00340 -/// [341]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorBufferInfo-range-00341 -/// [bbr]: https://registry.khronos.org/OpenGL-Refpages/es3.0/html/glBindBufferRange.xhtml /// [woob]: https://gpuweb.github.io/gpuweb/wgsl/#out-of-bounds-access-sec #[derive(Debug)] pub struct BufferBinding<'a, B: DynBuffer + ?Sized> { /// The buffer being bound. - /// - /// This is not fully `pub` to prevent direct construction of - /// `BufferBinding`s, while still allowing public read access to the `offset` - /// and `size` properties. - pub(crate) buffer: &'a B, + pub buffer: &'a B, /// The offset at which the bound region starts. /// - /// Because zero-length bindings are not permitted (see above), this must be - /// strictly less than the size of the buffer. + /// This must be less than the size of the buffer. Some back ends + /// cannot tolerate zero-length regions; for example, see + /// [VUID-VkDescriptorBufferInfo-offset-00340][340] and + /// [VUID-VkDescriptorBufferInfo-range-00341][341], or the + /// documentation for GLES's [glBindBufferRange][bbr]. + /// + /// [340]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorBufferInfo-offset-00340 + /// [341]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorBufferInfo-range-00341 + /// [bbr]: https://registry.khronos.org/OpenGL-Refpages/es3.0/html/glBindBufferRange.xhtml pub offset: wgt::BufferAddress, /// The size of the region bound, in bytes. - pub size: wgt::BufferSize, + /// + /// If `None`, the region extends from `offset` to the end of the + /// buffer. Given the restrictions on `offset`, this means that + /// the size is always greater than zero. + pub size: Option, } -// We must implement this manually because `B` is not necessarily `Clone`. -impl Clone for BufferBinding<'_, B> { +impl<'a, T: DynBuffer + ?Sized> Clone for BufferBinding<'a, T> { fn clone(&self) -> Self { BufferBinding { buffer: self.buffer, @@ -2050,31 +2034,6 @@ impl Clone for BufferBinding<'_, B> { } } -impl<'a, B: DynBuffer + ?Sized> BufferBinding<'a, B> { - /// Construct a `BufferBinding` with the given contents. - /// - /// When possible, use the `binding` method on a wgpu-core `Buffer` instead - /// of this method. `Buffer::binding` validates the size of the binding - /// against the size of the buffer. - /// - /// It is more difficult to provide a validating constructor here, due to - /// not having direct access to the size of a `DynBuffer`. - /// - /// SAFETY: The caller is responsible for ensuring that a binding of `size` - /// bytes starting at `offset` is contained within the buffer. - pub unsafe fn new_unchecked( - buffer: &'a B, - offset: wgt::BufferAddress, - size: wgt::BufferSize, - ) -> Self { - Self { - buffer, - offset, - size, - } - } -} - #[derive(Debug)] pub struct TextureBinding<'a, T: DynTextureView + ?Sized> { pub view: &'a T, diff --git a/wgpu-hal/src/metal/command.rs b/wgpu-hal/src/metal/command.rs index 4fc1987ce99..72a799a0275 100644 --- a/wgpu-hal/src/metal/command.rs +++ b/wgpu-hal/src/metal/command.rs @@ -977,9 +977,15 @@ impl crate::CommandEncoder for super::CommandEncoder { let encoder = self.state.render.as_ref().unwrap(); encoder.set_vertex_buffer(buffer_index, Some(&binding.buffer.raw), binding.offset); - self.state - .vertex_buffer_size_map - .insert(buffer_index, binding.size); + let buffer_size = binding.resolve_size(); + if buffer_size > 0 { + self.state.vertex_buffer_size_map.insert( + buffer_index, + core::num::NonZeroU64::new(buffer_size).unwrap(), + ); + } else { + self.state.vertex_buffer_size_map.remove(&buffer_index); + } if let Some((index, sizes)) = self .state diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs index 3835fd022b8..ef8e7c83a52 100644 --- a/wgpu-hal/src/metal/device.rs +++ b/wgpu-hal/src/metal/device.rs @@ -340,6 +340,10 @@ impl super::Device { } } + pub unsafe fn buffer_from_raw(raw: metal::Buffer, size: wgt::BufferAddress) -> super::Buffer { + super::Buffer { raw, size } + } + pub fn raw_device(&self) -> &Mutex { &self.shared.device } @@ -369,7 +373,10 @@ impl crate::Device for super::Device { raw.set_label(label); } self.counters.buffers.add(1); - Ok(super::Buffer { raw }) + Ok(super::Buffer { + raw, + size: desc.size, + }) }) } unsafe fn destroy_buffer(&self, _buffer: super::Buffer) { @@ -928,9 +935,14 @@ impl crate::Device for super::Device { let end = start + 1; bg.buffers .extend(desc.buffers[start..end].iter().map(|source| { + // Given the restrictions on `BufferBinding::offset`, + // this should never be `None`. + let remaining_size = wgt::BufferSize::new( + source.buffer.size - source.offset, + ); let binding_size = match ty { wgt::BufferBindingType::Storage { .. } => { - Some(source.size) + source.size.or(remaining_size) } _ => None, }; diff --git a/wgpu-hal/src/metal/mod.rs b/wgpu-hal/src/metal/mod.rs index 30af14a33fa..b5ae1dd5d5d 100644 --- a/wgpu-hal/src/metal/mod.rs +++ b/wgpu-hal/src/metal/mod.rs @@ -502,6 +502,7 @@ impl crate::Queue for Queue { #[derive(Debug)] pub struct Buffer { raw: metal::Buffer, + size: wgt::BufferAddress, } unsafe impl Send for Buffer {} @@ -515,6 +516,15 @@ impl Buffer { } } +impl crate::BufferBinding<'_, Buffer> { + fn resolve_size(&self) -> wgt::BufferAddress { + match self.size { + Some(size) => size.get(), + None => self.buffer.size - self.offset, + } + } +} + #[derive(Debug)] pub struct Texture { raw: metal::Texture, diff --git a/wgpu-hal/src/vulkan/device.rs b/wgpu-hal/src/vulkan/device.rs index ee0a88a469c..e760c49462b 100644 --- a/wgpu-hal/src/vulkan/device.rs +++ b/wgpu-hal/src/vulkan/device.rs @@ -1807,7 +1807,9 @@ impl crate::Device for super::Device { vk::DescriptorBufferInfo::default() .buffer(binding.buffer.raw) .offset(binding.offset) - .range(binding.size.get()) + .range( + binding.size.map_or(vk::WHOLE_SIZE, wgt::BufferSize::get), + ) }, )); write.buffer_info(local_buffer_infos) diff --git a/wgpu/src/api/device.rs b/wgpu/src/api/device.rs index 224c688fd83..99ed5071df9 100644 --- a/wgpu/src/api/device.rs +++ b/wgpu/src/api/device.rs @@ -322,7 +322,6 @@ impl Device { /// - `hal_buffer` must be created from this device internal handle /// - `hal_buffer` must be created respecting `desc` /// - `hal_buffer` must be initialized - /// - `hal_buffer` must not have zero size #[cfg(wgpu_core)] #[must_use] pub unsafe fn create_buffer_from_hal( diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index 87573dae34f..6a86d44b238 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -170,12 +170,6 @@ impl ContextWgpuCore { } } - /// # Safety - /// - /// - `hal_buffer` must be created from `device`. - /// - `hal_buffer` must be created respecting `desc` - /// - `hal_buffer` must be initialized - /// - `hal_buffer` must not have zero size. pub unsafe fn create_buffer_from_hal( &self, hal_buffer: A::Buffer,