Restore plumbing of implicit remainder-of-buffer size to backends

Author: andyleiserson

cts_runner/test.lst

@@ -27,6 +27,8 @@ 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,encoder_state:*
 webgpu:api,validation,encoding,encoder_open_state:non_pass_commands:*
 webgpu:api,validation,encoding,encoder_open_state:render_pass_commands:*

wgpu-core/src/command/bundle.rs

@@ -505,7 +505,7 @@ impl RenderBundleEncoder {
             buffer_id,
             index_format,
             offset,
-            size: size.map(NonZeroU64::get),
+            size,
         });
     }
 }
@@ -610,7 +610,7 @@ fn set_index_buffer(
     buffer_id: id::Id<id::markers::Buffer>,
     index_format: wgt::IndexFormat,
     offset: u64,
-    size: Option<wgt::BufferSizeOrZero>,
+    size: Option<NonZeroU64>,
 ) -> Result<(), RenderBundleErrorInner> {
     let buffer = buffer_guard.get(buffer_id).get()?;
 
@@ -642,7 +642,7 @@ fn set_vertex_buffer(
     slot: u32,
     buffer_id: id::Id<id::markers::Buffer>,
     offset: u64,
-    size: Option<wgt::BufferSizeOrZero>,
+    size: Option<NonZeroU64>,
 ) -> Result<(), RenderBundleErrorInner> {
     let max_vertex_buffers = state.device.limits.max_vertex_buffers;
     if slot >= max_vertex_buffers {
@@ -967,11 +967,7 @@ impl RenderBundle {
                     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`"),
-                        )
+                        hal::BufferBinding::new_unchecked(buffer, *offset, *size)
                     };
                     unsafe { raw.set_index_buffer(bb, *index_format) };
                 }
@@ -985,11 +981,7 @@ impl RenderBundle {
                     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`"),
-                        )
+                        hal::BufferBinding::new_unchecked(buffer, *offset, *size)
                     };
                     unsafe { raw.set_vertex_buffer(*slot, bb) };
                 }
@@ -1176,7 +1168,7 @@ impl IndexState {
                 buffer: self.buffer.clone(),
                 index_format: self.format,
                 offset: self.range.start,
-                size: Some(binding_size),
+                size: NonZeroU64::new(binding_size),
             })
         } else {
             None
@@ -1232,7 +1224,7 @@ impl VertexState {
                 slot,
                 buffer: self.buffer.clone(),
                 offset: self.range.start,
-                size: Some(binding_size),
+                size: NonZeroU64::new(binding_size),
             })
         } else {
             None
@@ -1596,7 +1588,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 +1647,7 @@ pub mod bundle_ffi {
             slot,
             buffer_id,
             offset,
-            size: size.map(NonZeroU64::get),
+            size,
         });
     }
 

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, ops::Range, 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::{
@@ -362,17 +357,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) {
@@ -2339,7 +2330,7 @@ fn set_index_buffer(
     buffer: Arc<crate::resource::Buffer>,
     index_format: IndexFormat,
     offset: u64,
-    size: Option<BufferSizeOrZero>,
+    size: Option<BufferSize>,
 ) -> Result<(), RenderPassErrorInner> {
     api_log!("RenderPass::set_index_buffer {}", buffer.error_ident());
 
@@ -2353,15 +2344,16 @@ fn set_index_buffer(
 
     buffer.check_usage(BufferUsages::INDEX)?;
 
-    let binding = buffer
+    let (binding, resolved_size) = buffer
         .binding(offset, size, state.general.snatch_guard)
         .map_err(RenderCommandError::from)?;
-    state.index.update_buffer(&binding, index_format);
+    let end = offset + resolved_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,
         ),
     );
@@ -2379,7 +2371,7 @@ fn set_vertex_buffer(
     slot: u32,
     buffer: Arc<crate::resource::Buffer>,
     offset: u64,
-    size: Option<BufferSizeOrZero>,
+    size: Option<BufferSize>,
 ) -> Result<(), RenderPassErrorInner> {
     api_log!(
         "RenderPass::set_vertex_buffer {slot} {}",
@@ -2405,15 +2397,15 @@ fn set_vertex_buffer(
 
     buffer.check_usage(BufferUsages::VERTEX)?;
 
-    let binding = buffer
+    let (binding, buffer_size) = buffer
         .binding(offset, size, state.general.snatch_guard)
         .map_err(RenderCommandError::from)?;
-    state.vertex.buffer_sizes[slot as usize] = Some(binding.size.get());
+    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,
         ),
     );
@@ -3090,7 +3082,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(())
@@ -3111,7 +3103,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(())

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<BufferSizeOrZero>,
+        size: Option<BufferSize>,
     },
     SetVertexBuffer {
         slot: u32,
         buffer_id: id::BufferId,
         offset: BufferAddress,
-        size: Option<BufferSizeOrZero>,
+        size: Option<BufferSize>,
     },
     SetBlendConstant(Color),
     SetStencilReference(u32),
@@ -416,20 +416,13 @@ pub enum ArcRenderCommand {
         buffer: Arc<Buffer>,
         index_format: wgt::IndexFormat,
         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<BufferSizeOrZero>,
+        size: Option<BufferSize>,
     },
     SetVertexBuffer {
         slot: u32,
         buffer: Arc<Buffer>,
         offset: BufferAddress,
-
-        // See comment in `SetIndexBuffer`.
-        size: Option<BufferSizeOrZero>,
+        size: Option<BufferSize>,
     },
     SetBlendConstant(Color),
     SetStencilReference(u32),

wgpu-core/src/device/resource.rs

@@ -8,7 +8,7 @@ use alloc::{
 use core::{
     fmt,
     mem::{self, ManuallyDrop},
-    num::{NonZeroU32, NonZeroU64},
+    num::NonZeroU32,
     sync::atomic::{AtomicBool, Ordering},
 };
 use hal::ShouldBeNonZeroExt;
@@ -2198,8 +2198,8 @@ impl Device {
 
         buffer.check_usage(pub_usage)?;
 
-        let bb = buffer.binding(bb.offset, bb.size.map(NonZeroU64::get), snatch_guard)?;
-        let bind_size = bb.size.get();
+        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 {
@@ -2214,8 +2214,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,
             });
         }

wgpu-core/src/indirect_validation/dispatch.rs

@@ -234,7 +234,7 @@ impl Dispatch {
             }],
             buffers: &[unsafe {
                 // SAFETY: We just created the buffer with this size.
-                hal::BufferBinding::new_unchecked(dst_buffer.as_ref(), 0, DST_BUFFER_SIZE)
+                hal::BufferBinding::new_unchecked(dst_buffer.as_ref(), 0, Some(DST_BUFFER_SIZE))
             }],
             samplers: &[],
             textures: &[],

wgpu-core/src/resource.rs

@@ -508,17 +508,17 @@ impl Buffer {
     pub fn resolve_binding_size(
         &self,
         offset: wgt::BufferAddress,
-        binding_size: Option<wgt::BufferSizeOrZero>,
-    ) -> Result<wgt::BufferSizeOrZero, BindingError> {
+        binding_size: Option<wgt::BufferSize>,
+    ) -> Result<u64, BindingError> {
         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),
+            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: binding_size.get(),
                     buffer_size,
                 }),
             },
@@ -535,35 +535,38 @@ impl Buffer {
     }
 
     /// Create a new [`hal::BufferBinding`] for the buffer with `offset` and
-    /// `size`.
+    /// `binding_size`.
     ///
-    /// If `size` is `None`, then the remainder of the buffer starting from
-    /// `offset` is used.
+    /// 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.
     ///
-    /// Zero-size bindings are permitted here for historical reasons. Although
+    /// 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. 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
+    /// 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<wgt::BufferSizeOrZero>,
+        binding_size: Option<wgt::BufferSize>,
         snatch_guard: &'a SnatchGuard,
-    ) -> Result<hal::BufferBinding<'a, dyn hal::DynBuffer>, BindingError> {
+    ) -> 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)?;
         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,
+            Ok((
+                hal::BufferBinding::new_unchecked(buf_raw, offset, binding_size),
                 resolved_size,
             ))
         }