diff --git a/cts_runner/test.lst b/cts_runner/test.lst index 163944db9ad..c509d87bbee 100644 --- a/cts_runner/test.lst +++ b/cts_runner/test.lst @@ -27,6 +27,10 @@ webgpu:api,validation,encoding,cmds,copyTextureToTexture:sample_count:* //FAIL: webgpu:api,validation,encoding,cmds,copyTextureToTexture:copy_ranges_with_compressed_texture_formats:* webgpu:api,validation,encoding,cmds,index_access:* //FAIL: webgpu:api,validation,encoding,cmds,render,draw:* +webgpu:api,validation,encoding,cmds,render,draw:index_buffer_OOB:* +webgpu:api,validation,encoding,cmds,render,draw:unused_buffer_bound:* +webgpu:api,validation,encoding,cmds,render,setIndexBuffer:index_buffer_state:* +webgpu:api,validation,encoding,cmds,render,setVertexBuffer:vertex_buffer_state:* webgpu:api,validation,encoding,encoder_state:* webgpu:api,validation,encoding,encoder_open_state:non_pass_commands:* webgpu:api,validation,encoding,encoder_open_state:render_pass_commands:* diff --git a/deno_webgpu/device.rs b/deno_webgpu/device.rs index 5416f53798c..211cdc513d0 100644 --- a/deno_webgpu/device.rs +++ b/deno_webgpu/device.rs @@ -680,29 +680,26 @@ impl GPUDevice { .buffers .into_iter() .map(|b| { - let layout = b.into_option().ok_or_else(|| { - JsErrorBox::type_error( - "Nullable GPUVertexBufferLayouts are currently not supported", - ) - })?; - - Ok(wgpu_core::pipeline::VertexBufferLayout { - array_stride: layout.array_stride, - step_mode: layout.step_mode.into(), - attributes: Cow::Owned( - layout - .attributes - .into_iter() - .map(|attr| wgpu_types::VertexAttribute { - format: attr.format.into(), - offset: attr.offset, - shader_location: attr.shader_location, - }) - .collect(), - ), - }) + b.into_option().map_or_else( + wgpu_core::pipeline::VertexBufferLayout::default, + |layout| wgpu_core::pipeline::VertexBufferLayout { + array_stride: layout.array_stride, + step_mode: layout.step_mode.into(), + attributes: Cow::Owned( + layout + .attributes + .into_iter() + .map(|attr| wgpu_types::VertexAttribute { + format: attr.format.into(), + offset: attr.offset, + shader_location: attr.shader_location, + }) + .collect(), + ), + }, + ) }) - .collect::>()?, + .collect(), ), }; diff --git a/wgpu-core/src/binding_model.rs b/wgpu-core/src/binding_model.rs index 8075887ed98..b049ceb108b 100644 --- a/wgpu-core/src/binding_model.rs +++ b/wgpu-core/src/binding_model.rs @@ -94,8 +94,39 @@ impl WebGpuError for CreateBindGroupLayoutError { } } -//TODO: refactor this to move out `enum BindingError`. +#[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 buffer size {buffer_size}")] + BindingOffsetTooLarge { + buffer: ResourceErrorIdent, + offset: wgt::BufferAddress, + buffer_size: u64, + }, +} +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 { @@ -103,6 +134,8 @@ 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" )] @@ -113,12 +146,6 @@ 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, @@ -233,6 +260,7 @@ 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, @@ -240,7 +268,6 @@ 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 7a64502b14d..ed6c9181b98 100644 --- a/wgpu-core/src/command/bundle.rs +++ b/wgpu-core/src/command/bundle.rs @@ -93,6 +93,7 @@ use core::{ use arrayvec::ArrayVec; use thiserror::Error; +use wgpu_hal::ShouldBeNonZeroExt; use wgt::error::{ErrorType, WebGpuError}; use crate::{ @@ -602,6 +603,7 @@ 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>, @@ -620,21 +622,20 @@ fn set_index_buffer( buffer.same_device(&state.device)?; buffer.check_usage(wgt::BufferUsages::INDEX)?; - let end = match size { - Some(s) => offset + s.get(), - None => buffer.size, - }; + let end = offset + buffer.resolve_binding_size(offset, size)?; + state .buffer_memory_init_actions .extend(buffer.initialization_status.read().create_action( &buffer, - offset..end, + offset..end.get(), MemoryInitKind::NeedsInitializedMemory, )); - state.set_index_buffer(buffer, index_format, offset..end); + state.set_index_buffer(buffer, index_format, offset..end.get()); Ok(()) } +// This function is duplicative of `render::set_vertex_buffer`. fn set_vertex_buffer( state: &mut State, buffer_guard: &crate::storage::Storage>, @@ -662,18 +663,16 @@ fn set_vertex_buffer( buffer.same_device(&state.device)?; buffer.check_usage(wgt::BufferUsages::VERTEX)?; - let end = match size { - Some(s) => offset + s.get(), - None => buffer.size, - }; + let end = offset + buffer.resolve_binding_size(offset, size)?; + state .buffer_memory_init_actions .extend(buffer.initialization_status.read().create_action( &buffer, - offset..end, + offset..end.get(), MemoryInitKind::NeedsInitializedMemory, )); - state.vertex[slot as usize] = Some(VertexState::new(buffer, offset..end)); + state.vertex[slot as usize] = Some(VertexState::new(buffer, offset..end.get())); Ok(()) } @@ -965,11 +964,9 @@ impl RenderBundle { size, } => { let buffer = buffer.try_raw(snatch_guard)?; - let bb = hal::BufferBinding { - buffer, - offset: *offset, - size: *size, - }; + // SAFETY: The binding size was checked against the buffer size + // in `set_index_buffer` and again in `IndexState::flush`. + let bb = hal::BufferBinding::new_unchecked(buffer, *offset, *size); unsafe { raw.set_index_buffer(bb, *index_format) }; } Cmd::SetVertexBuffer { @@ -979,11 +976,9 @@ impl RenderBundle { size, } => { let buffer = buffer.try_raw(snatch_guard)?; - let bb = hal::BufferBinding { - buffer, - offset: *offset, - size: *size, - }; + // SAFETY: The binding size was checked against the buffer size + // in `set_vertex_buffer` and again in `VertexState::flush`. + let bb = hal::BufferBinding::new_unchecked(buffer, *offset, *size); unsafe { raw.set_vertex_buffer(*slot, bb) }; } Cmd::SetPushConstant { @@ -1131,6 +1126,9 @@ 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, @@ -1152,13 +1150,21 @@ 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) + .filter(|_| self.range.end <= self.buffer.size) + .expect("index range must 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: wgt::BufferSize::new(self.range.end - self.range.start), + size: NonZeroU64::new(binding_size), }) } else { None @@ -1174,6 +1180,9 @@ 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 { @@ -1183,6 +1192,9 @@ struct VertexState { } impl VertexState { + /// Create a new `VertexState`. + /// + /// The `range` must be contained within `buffer`. fn new(buffer: Arc, range: Range) -> Self { Self { buffer, @@ -1195,13 +1207,20 @@ impl VertexState { /// /// `slot` is the index of the vertex buffer slot that `self` tracks. fn flush(&mut self, slot: u32) -> Option { + 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"); + if self.is_dirty { self.is_dirty = false; Some(ArcRenderCommand::SetVertexBuffer { slot, buffer: self.buffer.clone(), offset: self.range.start, - size: wgt::BufferSize::new(self.range.end - self.range.start), + size: NonZeroU64::new(binding_size), }) } else { None diff --git a/wgpu-core/src/command/draw.rs b/wgpu-core/src/command/draw.rs index 53a3f204fcc..7dadc8bfa4e 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::{LateMinBufferBindingSizeMismatch, PushConstantUploadError}, + binding_model::{BindingError, LateMinBufferBindingSizeMismatch, PushConstantUploadError}, resource::{ DestroyedResourceError, MissingBufferUsageError, MissingTextureUsageError, ResourceErrorIdent, @@ -89,6 +89,8 @@ 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})")] @@ -110,6 +112,7 @@ 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/mod.rs b/wgpu-core/src/command/mod.rs index 80133c5b8ae..9587669c73d 100644 --- a/wgpu-core/src/command/mod.rs +++ b/wgpu-core/src/command/mod.rs @@ -31,6 +31,7 @@ pub use timestamp_writes::PassTimestampWrites; use self::memory_init::CommandBufferTextureMemoryActions; +use crate::binding_model::BindingError; use crate::command::transition_resources::TransitionResourcesError; use crate::device::queue::TempResource; use crate::device::{Device, DeviceError, MissingFeatures}; @@ -1060,6 +1061,18 @@ impl CommandEncoderError { inner: RenderPassErrorInner::DestroyedResource(_), .. }) + | Self::RenderPass(RenderPassError { + inner: RenderPassErrorInner::RenderCommand( + RenderCommandError::DestroyedResource(_) + ), + .. + }) + | Self::RenderPass(RenderPassError { + inner: RenderPassErrorInner::RenderCommand(RenderCommandError::BindingError( + BindingError::DestroyedResource(_) + )), + .. + }) ) } } diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index 19129f891c9..f30d8929dd6 100644 --- a/wgpu-core/src/command/render.rs +++ b/wgpu-core/src/command/render.rs @@ -2322,6 +2322,7 @@ fn set_pipeline( Ok(()) } +// This function is duplicative of `bundle::set_index_buffer`. fn set_index_buffer( state: &mut State, cmd_buf: &Arc, @@ -2341,12 +2342,11 @@ 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 end = match size { - Some(s) => offset + s.get(), - None => buffer.size, - }; + let (binding, resolved_size) = buffer + .binding(offset, size, state.general.snatch_guard) + .map_err(RenderCommandError::from)?; + let end = offset + resolved_size; state.index.update_buffer(offset..end, index_format); state.general.buffer_memory_init_actions.extend( @@ -2357,17 +2357,13 @@ fn set_index_buffer( ), ); - let bb = hal::BufferBinding { - buffer: buf_raw, - offset, - size, - }; unsafe { - hal::DynCommandEncoder::set_index_buffer(state.general.raw_encoder, bb, index_format); + hal::DynCommandEncoder::set_index_buffer(state.general.raw_encoder, binding, index_format); } Ok(()) } +// This function is duplicative of `render::set_vertex_buffer`. fn set_vertex_buffer( state: &mut State, cmd_buf: &Arc, @@ -2399,13 +2395,10 @@ fn set_vertex_buffer( } buffer.check_usage(BufferUsages::VERTEX)?; - let buf_raw = buffer.try_raw(state.general.snatch_guard)?; - //TODO: where are we checking that the offset is in bound? - let buffer_size = match size { - Some(s) => s.get(), - None => buffer.size - offset, - }; + let (binding, buffer_size) = buffer + .binding(offset, size, state.general.snatch_guard) + .map_err(RenderCommandError::from)?; state.vertex.buffer_sizes[slot as usize] = Some(buffer_size); state.general.buffer_memory_init_actions.extend( @@ -2416,13 +2409,8 @@ fn set_vertex_buffer( ), ); - let bb = hal::BufferBinding { - buffer: buf_raw, - offset, - size, - }; unsafe { - hal::DynCommandEncoder::set_vertex_buffer(state.general.raw_encoder, slot, bb); + hal::DynCommandEncoder::set_vertex_buffer(state.general.raw_encoder, slot, binding); } 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 6fc4cbf5cf5..6564238548f 100644 --- a/wgpu-core/src/command/render_command.rs +++ b/wgpu-core/src/command/render_command.rs @@ -392,6 +392,17 @@ 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 { diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index d61be9613be..d05fb8c8cb8 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -383,6 +383,7 @@ 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, @@ -404,7 +405,7 @@ impl Global { trace.add(trace::Action::CreateBuffer(fid.id(), desc.clone())); } - let (buffer, err) = device.create_buffer_from_hal(Box::new(hal_buffer), desc); + let (buffer, err) = unsafe { 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 f68b8d69329..6e6512af1a6 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -11,6 +11,7 @@ use core::{ num::NonZeroU32, sync::atomic::{AtomicBool, Ordering}, }; +use hal::ShouldBeNonZeroExt; use arrayvec::ArrayVec; use bitflags::Flags; @@ -702,7 +703,8 @@ 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( + let timestamp_normalization_bind_group = Snatchable::new(unsafe { + // SAFETY: The size passed here must not overflow the buffer. self.timestamp_normalizer .get() .unwrap() @@ -710,10 +712,10 @@ impl Device { self, &*buffer, desc.label.as_deref(), - desc.size, + wgt::BufferSize::new(hal_desc.size).unwrap(), desc.usage, - )?, - ); + ) + }?); let indirect_validation_bind_groups = self.create_indirect_validation_bind_groups(buffer.as_ref(), desc.size, desc.usage)?; @@ -809,28 +811,36 @@ impl Device { Ok(texture) } - pub(crate) fn create_buffer_from_hal( + /// # 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( self: &Arc, hal_buffer: Box, desc: &resource::BufferDescriptor, ) -> (Fallible, Option) { - 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()), - ) + 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()), + ) + } } }; @@ -2187,31 +2197,9 @@ impl Device { buffer.same_device(self)?; buffer.check_usage(pub_usage)?; - 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) - } - }; + + let (bb, bind_size) = buffer.binding(bb.offset, bb.size, snatch_guard)?; + let bind_end = bb.offset + bind_size; if bind_size > range_limit as u64 { return Err(Error::BufferRangeTooLarge { @@ -2265,11 +2253,7 @@ impl Device { MemoryInitKind::NeedsInitializedMemory, )); - Ok(hal::BufferBinding { - buffer: raw_buffer, - offset: bb.offset, - size: bb.size, - }) + Ok(bb) } fn create_sampler_binding<'a>( diff --git a/wgpu-core/src/indirect_validation/dispatch.rs b/wgpu-core/src/indirect_validation/dispatch.rs index 00e3798e9ba..a1c1627fd3a 100644 --- a/wgpu-core/src/indirect_validation/dispatch.rs +++ b/wgpu-core/src/indirect_validation/dispatch.rs @@ -232,11 +232,12 @@ impl Dispatch { resource_index: 0, count: 1, }], - buffers: &[hal::BufferBinding { - buffer: dst_buffer.as_ref(), - offset: 0, - size: Some(DST_BUFFER_SIZE), - }], + // SAFETY: We just created the buffer with this size. + buffers: &[hal::BufferBinding::new_unchecked( + dst_buffer.as_ref(), + 0, + Some(DST_BUFFER_SIZE), + )], samplers: &[], textures: &[], acceleration_structures: &[], @@ -278,11 +279,8 @@ impl Dispatch { resource_index: 0, count: 1, }], - buffers: &[hal::BufferBinding { - buffer, - offset: 0, - size: Some(binding_size), - }], + // SAFETY: We calculated the binding size to fit within the buffer. + buffers: &[hal::BufferBinding::new_unchecked(buffer, 0, binding_size)], samplers: &[], textures: &[], acceleration_structures: &[], diff --git a/wgpu-core/src/indirect_validation/draw.rs b/wgpu-core/src/indirect_validation/draw.rs index d88acb8d60d..8e6943f3c34 100644 --- a/wgpu-core/src/indirect_validation/draw.rs +++ b/wgpu-core/src/indirect_validation/draw.rs @@ -135,11 +135,8 @@ impl Draw { resource_index: 0, count: 1, }], - buffers: &[hal::BufferBinding { - buffer, - offset: 0, - size: Some(binding_size), - }], + // SAFETY: We calculated the binding size to fit within the buffer. + buffers: &[hal::BufferBinding::new_unchecked(buffer, 0, binding_size)], samplers: &[], textures: &[], acceleration_structures: &[], @@ -684,11 +681,12 @@ fn create_buffer_and_bind_group( resource_index: 0, count: 1, }], - buffers: &[hal::BufferBinding { - buffer: buffer.as_ref(), - offset: 0, - size: Some(BUFFER_SIZE), - }], + // SAFETY: We just created the buffer with this size. + buffers: &[hal::BufferBinding::new_unchecked( + buffer.as_ref(), + 0, + BUFFER_SIZE, + )], samplers: &[], textures: &[], acceleration_structures: &[], diff --git a/wgpu-core/src/pipeline.rs b/wgpu-core/src/pipeline.rs index 3c4b52cc757..3bc3f7f6d79 100644 --- a/wgpu-core/src/pipeline.rs +++ b/wgpu-core/src/pipeline.rs @@ -371,6 +371,17 @@ pub struct VertexBufferLayout<'a> { pub attributes: Cow<'a, [wgt::VertexAttribute]>, } +/// A null vertex buffer layout that may be placed in unused slots. +impl Default for VertexBufferLayout<'_> { + fn default() -> Self { + Self { + array_stride: Default::default(), + step_mode: Default::default(), + attributes: Cow::Borrowed(&[]), + } + } +} + /// Describes the vertex process in a render pipeline. #[derive(Clone, Debug)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index 022b1ba59f2..824847a105a 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, + binding_model::{BindGroup, BindingError}, device::{ queue, resource::DeferredDestroy, BufferMapPendingClosure, Device, DeviceError, DeviceMismatch, HostMap, MissingDownlevelFlags, MissingFeatures, @@ -485,6 +485,82 @@ 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, 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). + 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()) { + Some(end) if end <= buffer_size => Ok(binding_size.get()), + _ => Err(BindingError::BindingRangeTooLarge { + buffer: self.error_ident(), + offset, + binding_size: binding_size.get(), + buffer_size, + }), + }, + None => { + buffer_size + .checked_sub(offset) + .ok_or_else(|| BindingError::BindingOffsetTooLarge { + buffer: self.error_ident(), + offset, + buffer_size, + }) + } + } + } + + /// Create a new [`hal::BufferBinding`] for the buffer with `offset` and + /// `binding_size`. + /// + /// If `binding_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. + /// + /// A zero-size binding at the end of the buffer is permitted here for historical reasons. Although + /// zero-size bindings are permitted by WebGPU, they are not permitted by + /// some backends. The zero-size binding 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). + /// + /// Although it seems like it would be simpler and safer to use the resolved + /// size in the returned [`hal::BufferBinding`], doing this (and removing + /// redundant logic in backends to resolve the implicit size) was observed + /// to cause problems in certain CTS tests, so an implicit size + /// specification is preserved in the output. + pub fn binding<'a>( + &'a self, + offset: wgt::BufferAddress, + binding_size: Option, + snatch_guard: &'a SnatchGuard, + ) -> Result<(hal::BufferBinding<'a, dyn hal::DynBuffer>, u64), BindingError> { + let buf_raw = self.try_raw(snatch_guard)?; + let resolved_size = self.resolve_binding_size(offset, binding_size)?; + // 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, binding_size), + 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 dd4d466235c..e5a9ef9a8ad 100644 --- a/wgpu-core/src/timestamp_normalization/mod.rs +++ b/wgpu-core/src/timestamp_normalization/mod.rs @@ -242,12 +242,16 @@ impl TimestampNormalizer { } } - pub fn create_normalization_bind_group( + /// 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( &self, device: &Device, buffer: &dyn hal::DynBuffer, buffer_label: Option<&str>, - buffer_size: u64, + buffer_size: wgt::BufferSize, buffer_usages: wgt::BufferUsages, ) -> Result { unsafe { @@ -263,7 +267,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 > device.adapter.limits().max_storage_buffer_binding_size as u64 { + if buffer_size.get() > device.adapter.limits().max_storage_buffer_binding_size as u64 { return Err(DeviceError::OutOfMemory); } @@ -282,11 +286,7 @@ impl TimestampNormalizer { .create_bind_group(&hal::BindGroupDescriptor { label: Some(label), layout: &*state.temporary_bind_group_layout, - buffers: &[hal::BufferBinding { - buffer, - offset: 0, - size: None, - }], + buffers: &[hal::BufferBinding::new_unchecked(buffer, 0, buffer_size)], samplers: &[], textures: &[], acceleration_structures: &[], diff --git a/wgpu-hal/examples/halmark/main.rs b/wgpu-hal/examples/halmark/main.rs index 75f3bc2fb9a..153bd49adb5 100644 --- a/wgpu-hal/examples/halmark/main.rs +++ b/wgpu-hal/examples/halmark/main.rs @@ -14,7 +14,9 @@ use winit::{ use std::{ borrow::{Borrow, Cow}, - iter, ptr, + iter, + num::NonZeroU64, + ptr, time::Instant, }; @@ -445,11 +447,12 @@ impl Example { let texture_view = unsafe { device.create_texture_view(&texture, &view_desc).unwrap() }; let global_group = { - let global_buffer_binding = hal::BufferBinding { - buffer: &global_buffer, - offset: 0, - size: None, - }; + // SAFETY: This is the same size that was specified for buffer creation. + let global_buffer_binding = hal::BufferBinding::new_unchecked( + &global_buffer, + 0, + NonZeroU64::new(global_buffer_desc.size), + ); let texture_binding = hal::TextureBinding { view: &texture_view, usage: wgpu_types::TextureUses::RESOURCE, @@ -483,11 +486,12 @@ impl Example { }; let local_group = { - let local_buffer_binding = hal::BufferBinding { - buffer: &local_buffer, - offset: 0, - size: wgpu_types::BufferSize::new(size_of::() as _), - }; + // SAFETY: The size must fit within the buffer. + let local_buffer_binding = hal::BufferBinding::new_unchecked( + &local_buffer, + 0, + wgpu_types::BufferSize::new(size_of::() as _), + ); let local_group_desc = hal::BindGroupDescriptor { label: Some("local"), layout: &local_group_layout, diff --git a/wgpu-hal/examples/ray-traced-triangle/main.rs b/wgpu-hal/examples/ray-traced-triangle/main.rs index a8d3a77b916..93e687ff1b5 100644 --- a/wgpu-hal/examples/ray-traced-triangle/main.rs +++ b/wgpu-hal/examples/ray-traced-triangle/main.rs @@ -603,10 +603,13 @@ impl Example { let texture_view = unsafe { device.create_texture_view(&texture, &view_desc).unwrap() }; let bind_group = { - let buffer_binding = hal::BufferBinding { - buffer: &uniform_buffer, - offset: 0, - size: None, + 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 texture_binding = hal::TextureBinding { view: &texture_view, diff --git a/wgpu-hal/src/dx12/command.rs b/wgpu-hal/src/dx12/command.rs index f57e6b9238a..32953455927 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.resolve_size() as u32, + SizeInBytes: binding.resolve_size().try_into().unwrap(), 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.resolve_size() as u32; + vb.SizeInBytes = binding.resolve_size().try_into().unwrap(); self.pass.dirty_vertex_buffers |= 1 << index; } diff --git a/wgpu-hal/src/dx12/device.rs b/wgpu-hal/src/dx12/device.rs index 24cd3826d4b..19e7743d6f7 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.resolve_size() as u32; + let mut size = data.resolve_size().try_into().unwrap(); if has_dynamic_offset { match ty { diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs index 6f05edbb168..75be3ee734b 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, + num::{NonZeroU32, NonZeroU64}, ops::{Range, RangeInclusive}, ptr::NonNull, }; @@ -1968,11 +1968,18 @@ 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. A `new_unchecked` constructor is also provided for +/// cases where direct construction is necessary. +/// /// ## Accessible region /// /// `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 [`buffer`]: +/// write data via this binding outside the *accessible region* of a buffer: /// /// - The accessible region starts at [`offset`]. /// @@ -1992,28 +1999,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. /// -/// [`buffer`]: BufferBinding::buffer +/// ## 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]. 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). +/// /// [`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. - pub buffer: &'a B, + /// + /// 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, /// The offset at which the bound region starts. /// - /// 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 + /// This must be less or equal to the size of the buffer. pub offset: wgt::BufferAddress, /// The size of the region bound, in bytes. @@ -2024,7 +2042,8 @@ pub struct BufferBinding<'a, B: DynBuffer + ?Sized> { pub size: Option, } -impl<'a, T: DynBuffer + ?Sized> Clone for BufferBinding<'a, T> { +// We must implement this manually because `B` is not necessarily `Clone`. +impl Clone for BufferBinding<'_, B> { fn clone(&self) -> Self { BufferBinding { buffer: self.buffer, @@ -2034,6 +2053,64 @@ impl<'a, T: DynBuffer + ?Sized> Clone for BufferBinding<'a, T> { } } +/// 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 ShouldBeNonZeroExt for Option { + fn get(&self) -> u64 { + match *self { + Some(non_zero) => non_zero.get(), + None => 0, + } + } +} + +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. + /// + /// The `S` type parameter is a temporary convenience to allow callers to + /// pass a zero size. When the zero-size binding issue is resolved, the + /// argument should just match the type of the member. + /// TODO(): remove the parameter + pub fn new_unchecked>>( + buffer: &'a B, + offset: wgt::BufferAddress, + size: S, + ) -> Self { + Self { + buffer, + offset, + size: size.into(), + } + } +} + #[derive(Debug)] pub struct TextureBinding<'a, T: DynTextureView + ?Sized> { pub view: &'a T, diff --git a/wgpu/src/api/device.rs b/wgpu/src/api/device.rs index 99ed5071df9..224c688fd83 100644 --- a/wgpu/src/api/device.rs +++ b/wgpu/src/api/device.rs @@ -322,6 +322,7 @@ 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 6a86d44b238..87573dae34f 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -170,6 +170,12 @@ 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,