Allow empty buffer slices to be instantiated (gfx-rs#8505)

Author: beholdnec

CHANGELOG.md

@@ -85,6 +85,33 @@ GLSL:
 
 By @andyleiserson in [#9321](https://github.com/gfx-rs/wgpu/pull/9321).
 
+#### Empty buffer slices are now permitted
+
+Creating a `BufferSlice` with a length of 0 no longer causes a panic.
+
+Empty buffer slices can be:
+
+- Instantiated
+- Mapped (the result is an empty slice of bytes)
+
+Empty buffer slices cannot be:
+
+- Used in buffer bindings
+- Passed to `set_index_buffer` or `set_vertex_buffer`
+
+#3170 tracks making it possible to pass a zero-size `BufferSlice` to `set_vertex_buffer` and `set_index_buffer` in the future.
+
+Zero-size buffer bindings are still not permitted. `BufferBinding` now implements `TryFrom<BufferSlice>` instead of `From<BufferSlice>`. The `TryFrom` conversion will fail if the slice is zero-size.
+
+```diff
+-let slice = buffer.slice(0..0); // panic!
+-let mapping = BufferBinding::from(slice); // infallible
++let slice = buffer.slice(0..0); // okay
++let mapping = BufferBinding::try_from(slice).unwrap(); // panic
+```
+
+By @beholdnec in [#8505](https://github.com/gfx-rs/wgpu/pull/8505).
+
 ### Added/New Features
 
 #### General

tests/tests/wgpu-gpu/buffer.rs

@@ -4,7 +4,8 @@ use wgpu_test::{
 
 pub fn all_tests(vec: &mut Vec<GpuTestInitializer>) {
     vec.extend([
-        EMPTY_BUFFER,
+        EMPTY_BUFFER_READ,
+        EMPTY_BUFFER_READ_WRITE,
         MAP_OFFSET,
         MAP_WITHOUT_SUBMIT,
         MINIMUM_BUFFER_BINDING_SIZE_LAYOUT,
@@ -14,19 +15,72 @@ pub fn all_tests(vec: &mut Vec<GpuTestInitializer>) {
     ]);
 }
 
-async fn test_empty_buffer_range(ctx: &TestingContext, buffer_size: u64, label: &str) {
-    let r = wgpu::BufferUsages::MAP_READ;
-    let rw = wgpu::BufferUsages::MAP_READ | wgpu::BufferUsages::MAP_WRITE;
-    for usage in [r, rw] {
-        let b0 = ctx.device.create_buffer(&wgpu::BufferDescriptor {
-            label: Some(label),
-            size: buffer_size,
-            usage,
-            mapped_at_creation: false,
+async fn test_empty_buffer_range_with_usage(
+    ctx: &TestingContext,
+    buffer_size: u64,
+    label: &str,
+    usage: wgpu::BufferUsages,
+) {
+    let b0 = ctx.device.create_buffer(&wgpu::BufferDescriptor {
+        label: Some(label),
+        size: buffer_size,
+        usage,
+        mapped_at_creation: false,
+    });
+
+    b0.slice(0..0)
+        .map_async(wgpu::MapMode::Read, Result::unwrap);
+
+    ctx.async_poll(wgpu::PollType::wait_indefinitely())
+        .await
+        .unwrap();
+
+    {
+        let view = b0.slice(0..0).get_mapped_range().unwrap();
+        assert!(view.is_empty());
+    }
+
+    b0.unmap();
+
+    // Map and unmap right away.
+    b0.slice(0..0).map_async(wgpu::MapMode::Read, move |_| {});
+    b0.unmap();
+
+    // Map multiple times before unmapping.
+    b0.slice(0..0).map_async(wgpu::MapMode::Read, move |_| {});
+    b0.slice(0..0)
+        .map_async(wgpu::MapMode::Read, move |result| {
+            assert!(result.is_err());
+        });
+    b0.slice(0..0)
+        .map_async(wgpu::MapMode::Read, move |result| {
+            assert!(result.is_err());
+        });
+    b0.slice(0..0)
+        .map_async(wgpu::MapMode::Read, move |result| {
+            assert!(result.is_err());
         });
+    b0.unmap();
+
+    // Ensure mapping errors didn't break it permanently.
+    b0.slice(0..0)
+        .map_async(wgpu::MapMode::Read, Result::unwrap);
+
+    ctx.async_poll(wgpu::PollType::wait_indefinitely())
+        .await
+        .unwrap();
+
+    {
+        let view = b0.slice(0..0).get_mapped_range().unwrap();
+        assert!(view.is_empty());
+    }
+
+    b0.unmap();
 
+    // Write mode.
+    if usage.contains(wgpu::BufferUsages::MAP_WRITE) {
         b0.slice(0..0)
-            .map_async(wgpu::MapMode::Read, Result::unwrap);
+            .map_async(wgpu::MapMode::Write, Result::unwrap);
 
         ctx.async_poll(wgpu::PollType::wait_indefinitely())
             .await
@@ -37,60 +91,35 @@ async fn test_empty_buffer_range(ctx: &TestingContext, buffer_size: u64, label:
             assert!(view.is_empty());
         }
 
-        b0.unmap();
+        {
+            let view = b0.slice(0..0).get_mapped_range_mut().unwrap();
+            assert!(view.is_empty());
+        }
 
-        // Map and unmap right away.
-        b0.slice(0..0).map_async(wgpu::MapMode::Read, move |_| {});
         b0.unmap();
 
-        // Map multiple times before unmapping.
-        b0.slice(0..0).map_async(wgpu::MapMode::Read, move |_| {});
-        b0.slice(0..0)
-            .map_async(wgpu::MapMode::Read, move |result| {
-                assert!(result.is_err());
-            });
-        b0.slice(0..0)
-            .map_async(wgpu::MapMode::Read, move |result| {
-                assert!(result.is_err());
-            });
+        // Map and unmap right away.
         b0.slice(0..0)
-            .map_async(wgpu::MapMode::Read, move |result| {
-                assert!(result.is_err());
-            });
+            .map_async(wgpu::MapMode::Write, Result::unwrap);
         b0.unmap();
-
-        // Write mode.
-        if usage == rw {
-            b0.slice(0..0)
-                .map_async(wgpu::MapMode::Write, Result::unwrap);
-
-            ctx.async_poll(wgpu::PollType::wait_indefinitely())
-                .await
-                .unwrap();
-
-            //{
-            //    let view = b0.slice(0..0).get_mapped_range_mut();
-            //    assert!(view.is_empty());
-            //}
-
-            b0.unmap();
-
-            // Map and unmap right away.
-            b0.slice(0..0).map_async(wgpu::MapMode::Write, move |_| {});
-            b0.unmap();
-        }
     }
 
+    // Test buffer mapped at creation.
     let b1 = ctx.device.create_buffer(&wgpu::BufferDescriptor {
         label: Some(label),
         size: buffer_size,
-        usage: rw,
+        usage,
         mapped_at_creation: true,
     });
 
     {
+        let view = b1.slice(0..0).get_mapped_range().unwrap();
+        assert!(view.is_empty());
+    }
+
+    if usage.contains(wgpu::BufferUsages::MAP_WRITE) {
         let view = b1.slice(0..0).get_mapped_range_mut().unwrap();
-        assert_eq!(view.len(), 0);
+        assert!(view.is_empty());
     }
 
     b1.unmap();
@@ -101,15 +130,25 @@ async fn test_empty_buffer_range(ctx: &TestingContext, buffer_size: u64, label:
 }
 
 #[gpu_test]
-static EMPTY_BUFFER: GpuTestConfiguration = GpuTestConfiguration::new()
+static EMPTY_BUFFER_READ: GpuTestConfiguration = GpuTestConfiguration::new()
+    .parameters(TestParameters::default().enable_noop())
+    .run_async(|ctx| async move {
+        let usage = wgpu::BufferUsages::MAP_READ;
+        test_empty_buffer_range_with_usage(&ctx, 2048, "regular buffer", usage).await;
+        test_empty_buffer_range_with_usage(&ctx, 0, "zero-sized buffer", usage).await;
+    });
+
+#[gpu_test]
+static EMPTY_BUFFER_READ_WRITE: GpuTestConfiguration = GpuTestConfiguration::new()
     .parameters(
         TestParameters::default()
             .expect_fail(FailureCase::always())
             .enable_noop(),
     )
     .run_async(|ctx| async move {
-        test_empty_buffer_range(&ctx, 2048, "regular buffer").await;
-        test_empty_buffer_range(&ctx, 0, "zero-sized buffer").await;
+        let usage = wgpu::BufferUsages::MAP_READ | wgpu::BufferUsages::MAP_WRITE;
+        test_empty_buffer_range_with_usage(&ctx, 2048, "regular buffer", usage).await;
+        test_empty_buffer_range_with_usage(&ctx, 0, "zero-sized buffer", usage).await;
     });
 
 #[gpu_test]

tests/tests/wgpu-validation/api/buffer_slice.rs

@@ -36,7 +36,7 @@ fn getters() {
             slice_with_size.offset(),
             slice_with_size.size()
         ),
-        (&buffer, 10, NonZero::new(80).unwrap())
+        (&buffer, 10, 80)
     );
 
     let slice_without_size = buffer.slice(10..);
@@ -46,7 +46,7 @@ fn getters() {
             slice_without_size.offset(),
             slice_without_size.size()
         ),
-        (&buffer, 10, NonZero::new(90).unwrap())
+        (&buffer, 10, 90)
     );
 }
 
@@ -60,7 +60,7 @@ fn into_buffer_binding() {
         buffer: b,
         offset: 50,
         size: Some(size),
-    }) = wgpu::BindingResource::from(buffer.slice(50..80))
+    }) = buffer.slice(50..80).try_into().unwrap()
     else {
         panic!("didn't match")
     };

tests/tests/wgpu-validation/api/command_buffer_actions.rs

@@ -41,19 +41,6 @@ fn encoder_map_buffer_on_submit_defers_until_submit() {
     assert!(fired.load(SeqCst));
 }
 
-/// Empty ranges panic immediately when registering the deferred map.
-#[test]
-#[should_panic = "buffer slices can not be empty"]
-fn encoder_map_buffer_on_submit_empty_range_panics_immediately() {
-    let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
-    let buffer = make_read_buffer(&device, 16);
-
-    let encoder = device.create_command_encoder(&wgpu::CommandEncoderDescriptor { label: None });
-
-    // This panics inside map_buffer_on_submit (range_to_offset_size).
-    encoder.map_buffer_on_submit(&buffer, wgpu::MapMode::Read, 8..8, |_| {});
-}
-
 /// Out-of-bounds ranges panic during submit (when the deferred map executes).
 #[test]
 #[should_panic = "is out of range for buffer of size"]

wgpu-hal/src/gles/adapter.rs

@@ -1317,14 +1317,17 @@ impl super::AdapterShared {
         } else {
             log::error!("Fake map");
             let length = dst_data.len();
-            let buffer_mapping =
-                unsafe { gl.map_buffer_range(target, offset, length as _, glow::MAP_READ_BIT) };
+            // glMapBufferRange throws an error if length is 0.
+            if length != 0 {
+                let buffer_mapping =
+                    unsafe { gl.map_buffer_range(target, offset, length as _, glow::MAP_READ_BIT) };
 
-            unsafe {
-                core::ptr::copy_nonoverlapping(buffer_mapping, dst_data.as_mut_ptr(), length)
-            };
+                unsafe {
+                    core::ptr::copy_nonoverlapping(buffer_mapping, dst_data.as_mut_ptr(), length)
+                };
 
-            unsafe { gl.unmap_buffer(target) };
+                unsafe { gl.unmap_buffer(target) };
+            }
         }
     }
 }