[wgpu] Improve buffer mapping errors and allow multiple immutable borrows

Author: cwfitzgerald

CHANGELOG.md

@@ -100,6 +100,7 @@ This allows using precompiled shaders without manually checking which backend's
   By @kpreid in [#8011](https://github.com/gfx-rs/wgpu/pull/8011).
 - Make a compacted hal acceleration structure inherit a label from the base BLAS. By @Vecvec in [#8103](https://github.com/gfx-rs/wgpu/pull/8103).
 - The limits requested for a device must now satisfy `min_subgroup_size <= max_subgroup_size`. By @andyleiserson in [#8085](https://github.com/gfx-rs/wgpu/pull/8085).
+- Improve errors when buffer mapping is done incorrectly. Allow aliasing immutable [`BufferViews`]. By @cwfitzgerald in [#8150](https://github.com/gfx-rs/wgpu/pull/8150).
 
 #### Naga
 

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

@@ -0,0 +1,129 @@
+fn mapping_is_zeroed(array: &[u8]) {
+    for (i, &byte) in array.iter().enumerate() {
+        assert_eq!(byte, 0, "Byte at index {i} is not zero");
+    }
+}
+
+#[test]
+fn full_mut_binding() {
+    let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
+
+    let buffer = device.create_buffer(&wgpu::BufferDescriptor {
+        label: None,
+        size: 1024,
+        usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
+        mapped_at_creation: true,
+    });
+
+    let mapping = buffer.slice(..).get_mapped_range_mut();
+
+    mapping_is_zeroed(&mapping);
+
+    drop(mapping);
+
+    buffer.unmap();
+}
+
+#[test]
+fn split_mut_binding() {
+    let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
+
+    let buffer = device.create_buffer(&wgpu::BufferDescriptor {
+        label: None,
+        size: 1024,
+        usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
+        mapped_at_creation: true,
+    });
+
+    let mapping0 = buffer.slice(0..512).get_mapped_range_mut();
+    let mapping1 = buffer.slice(512..1024).get_mapped_range_mut();
+
+    mapping_is_zeroed(&mapping0);
+    mapping_is_zeroed(&mapping1);
+
+    drop(mapping0);
+    drop(mapping1);
+
+    buffer.unmap();
+}
+
+#[test]
+fn overlapping_ref_binding() {
+    let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
+
+    let buffer = device.create_buffer(&wgpu::BufferDescriptor {
+        label: None,
+        size: 1024,
+        usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
+        mapped_at_creation: true,
+    });
+
+    let _mapping0 = buffer.slice(0..512).get_mapped_range();
+    let _mapping1 = buffer.slice(256..768).get_mapped_range();
+}
+
+#[test]
+#[should_panic(expected = "break Rust memory aliasing rules")]
+fn overlapping_mut_binding() {
+    let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
+
+    let buffer = device.create_buffer(&wgpu::BufferDescriptor {
+        label: None,
+        size: 1024,
+        usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
+        mapped_at_creation: true,
+    });
+
+    let _mapping0 = buffer.slice(0..512).get_mapped_range_mut();
+    let _mapping1 = buffer.slice(256..768).get_mapped_range_mut();
+}
+
+#[test]
+#[should_panic(expected = "an unmapped buffer")]
+fn not_mapped() {
+    let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
+
+    let buffer = device.create_buffer(&wgpu::BufferDescriptor {
+        label: None,
+        size: 1024,
+        usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
+        mapped_at_creation: false,
+    });
+
+    let _mapping = buffer.slice(..).get_mapped_range_mut();
+}
+
+#[test]
+#[should_panic(expected = "Attempted to get range 512..1024, but the mapped range is 0..512")]
+fn partially_mapped() {
+    let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
+
+    let buffer = device.create_buffer(&wgpu::BufferDescriptor {
+        label: None,
+        size: 1024,
+        usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
+        mapped_at_creation: false,
+    });
+
+    buffer.map_async(wgpu::MapMode::Write, 0..512, |_| {});
+    device.poll(wgpu::PollType::Wait).unwrap();
+
+    let _mapping0 = buffer.slice(0..512).get_mapped_range_mut();
+    let _mapping1 = buffer.slice(512..1024).get_mapped_range_mut();
+}
+
+#[test]
+#[should_panic(expected = "You cannot unmap a buffer that still has accessible mapped views")]
+fn unmap_while_visible() {
+    let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
+
+    let buffer = device.create_buffer(&wgpu::BufferDescriptor {
+        label: None,
+        size: 1024,
+        usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
+        mapped_at_creation: true,
+    });
+
+    let _mapping0 = buffer.slice(..).get_mapped_range_mut();
+    buffer.unmap();
+}

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

@@ -1,5 +1,6 @@
 mod binding_arrays;
 mod buffer;
+mod buffer_mapping;
 mod buffer_slice;
 mod device;
 mod external_texture;

wgpu/src/api/buffer.rs

@@ -396,9 +396,10 @@ impl Buffer {
     /// - If `bounds` has a length less than 1.
     /// - If the start and end of `bounds` are not aligned to [`MAP_ALIGNMENT`].
     /// - If the buffer to which `self` refers is not currently [mapped].
-    /// - If you try to create overlapping views of a buffer, mutable or otherwise.
+    /// - If you try to create a view which overlaps an existing [`BufferViewMut`].
     ///
     /// [mapped]: Buffer#mapping-buffers
+    #[track_caller]
     pub fn get_mapped_range<S: RangeBounds<BufferAddress>>(&self, bounds: S) -> BufferView {
         self.slice(bounds).get_mapped_range()
     }
@@ -419,9 +420,10 @@ impl Buffer {
     /// - If `bounds` has a length less than 1.
     /// - If the start and end of `bounds` are not aligned to [`MAP_ALIGNMENT`].
     /// - If the buffer to which `self` refers is not currently [mapped].
-    /// - If you try to create overlapping views of a buffer, mutable or otherwise.
+    /// - If you try to create a view which overlaps an existing [`BufferView`] or [`BufferViewMut`].
     ///
     /// [mapped]: Buffer#mapping-buffers
+    #[track_caller]
     pub fn get_mapped_range_mut<S: RangeBounds<BufferAddress>>(&self, bounds: S) -> BufferViewMut {
         self.slice(bounds).get_mapped_range_mut()
     }
@@ -534,9 +536,9 @@ impl<'a> BufferSlice<'a> {
         callback: impl FnOnce(Result<(), BufferAsyncError>) + WasmNotSend + 'static,
     ) {
         let mut mc = self.buffer.map_context.lock();
-        assert_eq!(mc.initial_range, 0..0, "Buffer is already mapped");
+        assert_eq!(mc.mapped_range, 0..0, "Buffer is already mapped");
         let end = self.offset + self.size.get();
-        mc.initial_range = self.offset..end;
+        mc.mapped_range = self.offset..end;
 
         self.buffer
             .inner
@@ -557,11 +559,16 @@ impl<'a> BufferSlice<'a> {
     ///
     /// - If the endpoints of this slice are not aligned to [`MAP_ALIGNMENT`] within the buffer.
     /// - If the buffer to which `self` refers is not currently [mapped].
-    /// - If you try to create overlapping views of a buffer, mutable or otherwise.
+    /// - If you try to create a view which overlaps an existing [`BufferViewMut`].
     ///
     /// [mapped]: Buffer#mapping-buffers
+    #[track_caller]
     pub fn get_mapped_range(&self) -> BufferView {
-        let end = self.buffer.map_context.lock().add(self.offset, self.size);
+        let end = self
+            .buffer
+            .map_context
+            .lock()
+            .add(self.offset, self.size, RangeKind::Immutable);
         let range = self.buffer.inner.get_mapped_range(self.offset..end);
         BufferView {
             buffer: self.buffer.clone(),
@@ -585,11 +592,16 @@ impl<'a> BufferSlice<'a> {
     ///
     /// - If the endpoints of this slice are not aligned to [`MAP_ALIGNMENT`].
     /// - If the buffer to which `self` refers is not currently [mapped].
-    /// - If you try to create overlapping views of a buffer, mutable or otherwise.
+    /// - If you try to create a view which overlaps an existing [`BufferView`] or [`BufferViewMut`].
     ///
     /// [mapped]: Buffer#mapping-buffers
+    #[track_caller]
     pub fn get_mapped_range_mut(&self) -> BufferViewMut {
-        let end = self.buffer.map_context.lock().add(self.offset, self.size);
+        let end = self
+            .buffer
+            .map_context
+            .lock()
+            .add(self.offset, self.size, RangeKind::Mutable);
         let range = self.buffer.inner.get_mapped_range(self.offset..end);
         BufferViewMut {
             buffer: self.buffer.clone(),
@@ -640,6 +652,28 @@ impl<'a> From<BufferSlice<'a>> for crate::BindingResource<'a> {
     }
 }
 
+fn range_overlaps(a: &Range<BufferAddress>, b: &Range<BufferAddress>) -> bool {
+    a.start < b.end && b.start < a.end
+}
+
+#[derive(Debug)]
+enum RangeKind {
+    Mutable,
+    Immutable,
+}
+
+impl RangeKind {
+    fn compatible(&self, other: &Self) -> bool {
+        matches!((self, other), (RangeKind::Immutable, RangeKind::Immutable))
+    }
+}
+
+#[derive(Debug)]
+struct Subrange {
+    index: Range<BufferAddress>,
+    kind: RangeKind,
+}
+
 /// The mapped portion of a buffer, if any, and its outstanding views.
 ///
 /// This ensures that views fall within the mapped range and don't overlap.
@@ -653,25 +687,25 @@ pub(crate) struct MapContext {
     /// *or has been requested to be* mapped.)
     ///
     /// All [`BufferView`]s and [`BufferViewMut`]s must fall within this range.
-    pub(crate) initial_range: Range<BufferAddress>,
+    pub(crate) mapped_range: Range<BufferAddress>,
 
     /// The ranges covered by all outstanding [`BufferView`]s and
     /// [`BufferViewMut`]s. These are non-overlapping, and are all contained
-    /// within `initial_range`.
-    sub_ranges: Vec<Range<BufferAddress>>,
+    /// within `mapped_range`.
+    sub_ranges: Vec<Subrange>,
 }
 
 impl MapContext {
     pub(crate) fn new() -> Self {
         Self {
-            initial_range: 0..0,
+            mapped_range: 0..0,
             sub_ranges: Vec::new(),
         }
     }
 
     /// Record that the buffer is no longer mapped.
     fn reset(&mut self) {
-        self.initial_range = 0..0;
+        self.mapped_range = 0..0;
 
         assert!(
             self.sub_ranges.is_empty(),
@@ -686,19 +720,38 @@ impl MapContext {
     /// # Panics
     ///
     /// This panics if the given range overlaps with any existing range.
-    fn add(&mut self, offset: BufferAddress, size: BufferSize) -> BufferAddress {
+    #[track_caller]
+    fn add(&mut self, offset: BufferAddress, size: BufferSize, kind: RangeKind) -> BufferAddress {
         let end = offset + size.get();
-        assert!(self.initial_range.start <= offset && end <= self.initial_range.end);
+
+        if self.mapped_range.is_empty() {
+            panic!("Tried to call getMappedRange(Mut) on an unmapped buffer");
+        }
+        if !range_overlaps(&self.mapped_range, &(offset..end)) {
+            panic!(
+                "Tried to call getMappedRange(Mut) on a range that is not entirely mapped. \
+                 Attempted to get range {}..{}, but the mapped range is {}..{}",
+                offset, end, self.mapped_range.start, self.mapped_range.end
+            );
+        }
+
         // This check is essential for avoiding undefined behavior: it is the
         // only thing that ensures that `&mut` references to the buffer's
         // contents don't alias anything else.
         for sub in self.sub_ranges.iter() {
-            assert!(
-                end <= sub.start || offset >= sub.end,
-                "Intersecting map range with {sub:?}"
-            );
+            if range_overlaps(&sub.index, &(offset..end)) && !sub.kind.compatible(&kind) {
+                panic!(
+                    "Tried to call getMappedRange(Mut) on a range that has already \
+                     been mapped and would break Rust memory aliasing rules. Attempted \
+                     to get range {}..{} ({:?}), and the conflicting range is {}..{} ({:?})",
+                    offset, end, kind, sub.index.start, sub.index.end, sub.kind
+                );
+            }
         }
-        self.sub_ranges.push(offset..end);
+        self.sub_ranges.push(Subrange {
+            index: offset..end,
+            kind,
+        });
         end
     }
 
@@ -716,7 +769,7 @@ impl MapContext {
         let index = self
             .sub_ranges
             .iter()
-            .position(|r| *r == (offset..end))
+            .position(|r| r.index == (offset..end))
             .expect("unable to remove range from map context");
         self.sub_ranges.swap_remove(index);
     }
@@ -922,7 +975,9 @@ fn range_to_offset_size<S: RangeBounds<BufferAddress>>(
 
 #[cfg(test)]
 mod tests {
-    use super::{check_buffer_bounds, range_to_offset_size, BufferAddress, BufferSize};
+    use super::{
+        check_buffer_bounds, range_overlaps, range_to_offset_size, BufferAddress, BufferSize,
+    };
 
     fn bs(value: BufferAddress) -> BufferSize {
         BufferSize::new(value).unwrap()
@@ -972,4 +1027,21 @@ mod tests {
     fn check_buffer_bounds_panics_for_end_wraparound() {
         check_buffer_bounds(u64::MAX, 1, bs(u64::MAX));
     }
+
+    #[test]
+    #[expect(clippy::bool_assert_comparison)] // This degrades readability significantly.
+    fn range_overlapping() {
+        // First range to the left
+        assert_eq!(range_overlaps(&(0..1), &(1..3)), false);
+        // First range overlaps left edge
+        assert_eq!(range_overlaps(&(0..2), &(1..3)), true);
+        // First range completely inside second
+        assert_eq!(range_overlaps(&(1..2), &(0..3)), true);
+        // First range completely surrounds second
+        assert_eq!(range_overlaps(&(0..3), &(1..2)), true);
+        // First range overlaps right edge
+        assert_eq!(range_overlaps(&(1..3), &(0..2)), true);
+        // First range entirely to the right
+        assert_eq!(range_overlaps(&(2..3), &(0..2)), false);
+    }
 }

wgpu/src/api/device.rs

@@ -264,7 +264,7 @@ impl Device {
     pub fn create_buffer(&self, desc: &BufferDescriptor<'_>) -> Buffer {
         let mut map_context = MapContext::new();
         if desc.mapped_at_creation {
-            map_context.initial_range = 0..desc.size;
+            map_context.mapped_range = 0..desc.size;
         }
 
         let buffer = self.inner.create_buffer(desc);
@@ -373,7 +373,7 @@ impl Device {
     ) -> Buffer {
         let mut map_context = MapContext::new();
         if desc.mapped_at_creation {
-            map_context.initial_range = 0..desc.size;
+            map_context.mapped_range = 0..desc.size;
         }
 
         let buffer = unsafe {