Revert "Validate binding ranges against buffer size"

This reverts commit ef428fc.

Author: andyleiserson

wgpu-core/src/binding_model.rs

@@ -94,48 +94,15 @@ 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 {
     #[error(transparent)]
     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<wgt::BufferAddress>,
+        size: u64,
+    },
     #[error("Binding size {actual} of {buffer} is less than minimum {min}")]
     BindingSizeTooSmall {
         buffer: ResourceErrorIdent,
@@ -260,14 +233,14 @@ 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,
             Self::InvalidResource(e) => e,
             Self::BindingArrayPartialLengthMismatch { .. }
             | Self::BindingArrayLengthMismatch { .. }
             | Self::BindingArrayZeroLength
+            | Self::BindingRangeTooLarge { .. }
             | Self::BindingSizeTooSmall { .. }
             | Self::BindingsNumMismatch { .. }
             | Self::BindingZeroSize(_)

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<Fallible<Buffer>>,
@@ -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<Fallible<Buffer>>,
@@ -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<Buffer>,
@@ -1161,24 +1152,13 @@ impl IndexState {
     /// Generate a `SetIndexBuffer` command to prepare for an indexed draw
     /// command, if needed.
     fn flush(&mut self) -> Option<ArcRenderCommand> {
-        // 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<Buffer>, range: Range<wgt::BufferAddress>) -> 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<ArcRenderCommand> {
-        // 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

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 { .. }

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<B: hal::DynBuffer + ?Sized>(
-        &mut self,
-        binding: &hal::BufferBinding<'_, B>,
-        format: IndexFormat,
-    ) {
+    fn update_buffer(&mut self, range: Range<BufferAddress>, 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<CommandBuffer>,
@@ -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<CommandBuffer>,
@@ -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);