diff --git a/CHANGELOG.md b/CHANGELOG.md index ffca24947b..c85b769741 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -125,6 +125,7 @@ By @cwfitzgerald in [#8162](https://github.com/gfx-rs/wgpu/pull/8162). 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). - Require new `F16_IN_F32` downlevel flag for `quantizeToF16`, `pack2x16float`, and `unpack2x16float` in WGSL input. By @aleiserson in [#8130](https://github.com/gfx-rs/wgpu/pull/8130). - The error message for non-copyable depth/stencil formats no longer mentions the aspect when it is not relevant. By @reima in [#8156](https://github.com/gfx-rs/wgpu/pull/8156). diff --git a/tests/tests/wgpu-validation/api/buffer_mapping.rs b/tests/tests/wgpu-validation/api/buffer_mapping.rs new file mode 100644 index 0000000000..62fcf0a806 --- /dev/null +++ b/tests/tests/wgpu-validation/api/buffer_mapping.rs @@ -0,0 +1,191 @@ +fn mapping_is_zeroed(array: &[u8]) { + for (i, &byte) in array.iter().enumerate() { + assert_eq!(byte, 0, "Byte at index {i} is not zero"); + } +} + +// Ensure that a simple immutable mapping works and it is zeroed. +#[test] +fn full_immutable_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_READ, + mapped_at_creation: false, + }); + + buffer.map_async(wgpu::MapMode::Read, .., |_| {}); + device.poll(wgpu::PollType::Wait).unwrap(); + + let mapping = buffer.slice(..).get_mapped_range(); + + mapping_is_zeroed(&mapping); + + drop(mapping); + + buffer.unmap(); +} + +// Ensure that a simple mutable binding works and it is zeroed. +#[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(); +} + +// Ensure that you can make two non-overlapping immutable ranges, which are both zeroed +#[test] +fn split_immutable_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_READ, + mapped_at_creation: false, + }); + + buffer.map_async(wgpu::MapMode::Read, .., |_| {}); + device.poll(wgpu::PollType::Wait).unwrap(); + + let mapping0 = buffer.slice(0..512).get_mapped_range(); + let mapping1 = buffer.slice(512..1024).get_mapped_range(); + + mapping_is_zeroed(&mapping0); + mapping_is_zeroed(&mapping1); + + drop(mapping0); + drop(mapping1); + + buffer.unmap(); +} + +/// Ensure that you can make two non-overlapping mapped ranges, which are both zeroed +#[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(); +} + +/// Ensure that you can make two overlapping immutablely mapped ranges. +#[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(); +} + +/// Ensure that two overlapping mutably mapped ranges panics. +#[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(); +} + +/// Ensure that when you try to get a mapped range from an unmapped buffer, it panics with +/// an error mentioning a completely unmapped buffer. +#[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(); +} + +/// Ensure that when you partially map a buffer, then try to read outside of that range, it panics +/// mentioning the mapped indices. +#[test] +#[should_panic( + expected = "Attempted to get range 512..1024 (Mutable), 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(); +} + +/// Ensure that you cannot unmap a buffer while there are still accessible mapped views. +#[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(); +} diff --git a/tests/tests/wgpu-validation/api/mod.rs b/tests/tests/wgpu-validation/api/mod.rs index 1266113fa3..9d591fa6b9 100644 --- a/tests/tests/wgpu-validation/api/mod.rs +++ b/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 experimental; diff --git a/wgpu/src/api/buffer.rs b/wgpu/src/api/buffer.rs index 28b43b76b0..537b95b819 100644 --- a/wgpu/src/api/buffer.rs +++ b/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>(&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>(&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,12 +559,17 @@ 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 range = self.buffer.inner.get_mapped_range(self.offset..end); + let subrange = Subrange::new(self.offset, self.size, RangeMappingKind::Immutable); + self.buffer + .map_context + .lock() + .validate_and_add(subrange.clone()); + let range = self.buffer.inner.get_mapped_range(subrange.index); BufferView { buffer: self.buffer.clone(), size: self.size, @@ -585,12 +592,17 @@ 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 range = self.buffer.inner.get_mapped_range(self.offset..end); + let subrange = Subrange::new(self.offset, self.size, RangeMappingKind::Mutable); + self.buffer + .map_context + .lock() + .validate_and_add(subrange.clone()); + let range = self.buffer.inner.get_mapped_range(subrange.index); BufferViewMut { buffer: self.buffer.clone(), size: self.size, @@ -640,6 +652,53 @@ impl<'a> From> for crate::BindingResource<'a> { } } +fn range_overlaps(a: &Range, b: &Range) -> bool { + a.start < b.end && b.start < a.end +} + +#[derive(Debug, Copy, Clone)] +enum RangeMappingKind { + Mutable, + Immutable, +} + +impl RangeMappingKind { + /// Returns true if a range of this kind can touch the same bytes as a range of the other kind. + /// + /// This is Rust's Mutable XOR Shared rule. + fn allowed_concurrently_with(self, other: Self) -> bool { + matches!( + (self, other), + (RangeMappingKind::Immutable, RangeMappingKind::Immutable) + ) + } +} + +#[derive(Debug, Clone)] +struct Subrange { + index: Range, + kind: RangeMappingKind, +} + +impl Subrange { + fn new(offset: BufferAddress, size: BufferSize, kind: RangeMappingKind) -> Self { + Self { + index: offset..(offset + size.get()), + kind, + } + } +} + +impl fmt::Display for Subrange { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "{}..{} ({:?})", + self.index.start, self.index.end, self.kind + ) + } +} + /// 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 +712,31 @@ 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, + mapped_range: Range, /// 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>, + /// within `mapped_range`. + sub_ranges: Vec, } impl MapContext { - pub(crate) fn new() -> Self { + /// Creates a new `MapContext`. + /// + /// For [`mapped_at_creation`] buffers, pass the full buffer range in the + /// `mapped_range` argument. For other buffers, pass `None`. + /// + /// [`mapped_at_creation`]: BufferDescriptor::mapped_at_creation + pub(crate) fn new(mapped_range: Option>) -> Self { Self { - initial_range: 0..0, + mapped_range: mapped_range.unwrap_or(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(), @@ -681,25 +746,38 @@ impl MapContext { /// Record that the `size` bytes of the buffer at `offset` are now viewed. /// - /// Return the byte offset within the buffer of the end of the viewed range. - /// /// # Panics /// - /// This panics if the given range overlaps with any existing range. - fn add(&mut self, offset: BufferAddress, size: BufferSize) -> BufferAddress { - let end = offset + size.get(); - assert!(self.initial_range.start <= offset && end <= self.initial_range.end); + /// This panics if the given range is invalid. + #[track_caller] + fn validate_and_add(&mut self, new_sub: Subrange) { + if self.mapped_range.is_empty() { + panic!("tried to call get_mapped_range(_mut) on an unmapped buffer"); + } + if !range_overlaps(&self.mapped_range, &new_sub.index) { + panic!( + "tried to call get_mapped_range(_mut) on a range that is not entirely mapped. \ + Attempted to get range {}, but the mapped range is {}..{}", + new_sub, 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, &new_sub.index) + && !sub.kind.allowed_concurrently_with(new_sub.kind) + { + panic!( + "tried to call get_mapped_range(_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 {}", + new_sub, sub + ); + } } - self.sub_ranges.push(offset..end); - end + self.sub_ranges.push(new_sub); } /// Record that the `size` bytes of the buffer at `offset` are no longer viewed. @@ -707,16 +785,14 @@ impl MapContext { /// # Panics /// /// This panics if the given range does not exactly match one previously - /// passed to [`add`]. - /// - /// [`add]`: MapContext::add + /// passed to [`MapContext::validate_and_add`]. fn remove(&mut self, offset: BufferAddress, size: BufferSize) { let end = offset + size.get(); 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 +998,9 @@ fn range_to_offset_size>( #[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 +1050,20 @@ mod tests { fn check_buffer_bounds_panics_for_end_wraparound() { check_buffer_bounds(u64::MAX, 1, bs(u64::MAX)); } + + #[test] + 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); + } } diff --git a/wgpu/src/api/device.rs b/wgpu/src/api/device.rs index a0b51c4be7..c392958703 100644 --- a/wgpu/src/api/device.rs +++ b/wgpu/src/api/device.rs @@ -262,10 +262,7 @@ impl Device { /// Creates a [`Buffer`]. #[must_use] 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; - } + let map_context = MapContext::new(desc.mapped_at_creation.then_some(0..desc.size)); let buffer = self.inner.create_buffer(desc); @@ -371,10 +368,7 @@ impl Device { hal_buffer: A::Buffer, desc: &BufferDescriptor<'_>, ) -> Buffer { - let mut map_context = MapContext::new(); - if desc.mapped_at_creation { - map_context.initial_range = 0..desc.size; - } + let map_context = MapContext::new(desc.mapped_at_creation.then_some(0..desc.size)); let buffer = unsafe { let core_device = self.inner.as_core(); diff --git a/wgpu/src/lib.rs b/wgpu/src/lib.rs index 85d9b901ba..a9f5d89820 100644 --- a/wgpu/src/lib.rs +++ b/wgpu/src/lib.rs @@ -45,7 +45,10 @@ )] #![allow( // We need to investiagate these. - clippy::large_enum_variant + clippy::large_enum_variant, + // These degrade readability significantly. + clippy::bool_assert_comparison, + clippy::bool_comparison, )] // NOTE: Keep this in sync with `wgpu-core`. #![cfg_attr(not(send_sync), allow(clippy::arc_with_non_send_sync))]