Restore the buffer size changes

Author: andyleiserson

wgpu-core/src/binding_model.rs

@@ -95,15 +95,48 @@ 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 {
     #[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"
     )]
@@ -114,12 +147,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<wgt::BufferAddress>,
-        size: u64,
-    },
     #[error("Binding size {actual} of {buffer} is less than minimum {min}")]
     BindingSizeTooSmall {
         buffer: ResourceErrorIdent,
@@ -234,14 +261,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

@@ -93,6 +93,7 @@ use core::{
 use arrayvec::ArrayVec;
 use thiserror::Error;
 
+use wgpu_hal::ShouldBeNonZeroExt;
 use wgt::error::{ErrorType, WebGpuError};
 
 use crate::{
@@ -504,7 +505,7 @@ impl RenderBundleEncoder {
             buffer_id,
             index_format,
             offset,
-            size,
+            size: size.map(NonZeroU64::get),
         });
     }
 }
@@ -602,13 +603,14 @@ 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>>,
     buffer_id: id::Id<id::markers::Buffer>,
     index_format: wgt::IndexFormat,
     offset: u64,
-    size: Option<NonZeroU64>,
+    size: Option<wgt::BufferSizeOrZero>,
 ) -> Result<(), RenderBundleErrorInner> {
     let buffer = buffer_guard.get(buffer_id).get()?;
 
@@ -620,28 +622,27 @@ 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 = 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<Fallible<Buffer>>,
     slot: u32,
     buffer_id: id::Id<id::markers::Buffer>,
     offset: u64,
-    size: Option<NonZeroU64>,
+    size: Option<wgt::BufferSizeOrZero>,
 ) -> Result<(), RenderBundleErrorInner> {
     let max_vertex_buffers = state.device.limits.max_vertex_buffers;
     if slot >= max_vertex_buffers {
@@ -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 = 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,10 +964,14 @@ impl RenderBundle {
                     size,
                 } => {
                     let buffer = buffer.try_raw(snatch_guard)?;
-                    let bb = hal::BufferBinding {
-                        buffer,
-                        offset: *offset,
-                        size: *size,
+                    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`"),
+                        )
                     };
                     unsafe { raw.set_index_buffer(bb, *index_format) };
                 }
@@ -979,10 +982,14 @@ impl RenderBundle {
                     size,
                 } => {
                     let buffer = buffer.try_raw(snatch_guard)?;
-                    let bb = hal::BufferBinding {
-                        buffer,
-                        offset: *offset,
-                        size: *size,
+                    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`"),
+                        )
                     };
                     unsafe { raw.set_vertex_buffer(*slot, bb) };
                 }
@@ -1131,6 +1138,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<Buffer>,
@@ -1152,13 +1162,21 @@ 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)
+            .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: Some(binding_size),
             })
         } else {
             None
@@ -1174,6 +1192,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 +1204,9 @@ 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,
@@ -1195,13 +1219,20 @@ impl VertexState {
     ///
     /// `slot` is the index of the vertex buffer slot that `self` tracks.
     fn flush(&mut self, slot: u32) -> Option<ArcRenderCommand> {
+        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: Some(binding_size),
             })
         } else {
             None
@@ -1565,7 +1596,7 @@ where
 pub mod bundle_ffi {
     use super::{RenderBundleEncoder, RenderCommand};
     use crate::{id, RawString};
-    use core::{convert::TryInto, slice};
+    use core::{convert::TryInto, num::NonZeroU64, slice};
     use wgt::{BufferAddress, BufferSize, DynamicOffset, IndexFormat};
 
     /// # Safety
@@ -1624,7 +1655,7 @@ pub mod bundle_ffi {
             slot,
             buffer_id,
             offset,
-            size,
+            size: size.map(NonZeroU64::get),
         });
     }
 

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