Fix incorrect offset in get_mapped_range (#3233)

* Add a test.

* Fix incorrect offset in get_mapped_range.

* Add an entry in the changelog.

CHANGELOG.md

@@ -144,6 +144,14 @@ Additionally `Surface::get_default_config` now returns an Option and returns Non
 
 - Don't use a pointer to a local copy of a `PhysicalDeviceDriverProperties` struct after it has gone out of scope. In fact, don't make a local copy at all. Introduce a helper function for building `CStr`s from C character arrays, and remove some `unsafe` blocks. By @jimblandy in [#3076](https://github.com/gfx-rs/wgpu/pull/3076).
 
+
+## wgpu-0.14.2 (2022-11-28)
+
+### Bug Fixes
+
+- Fix incorrect offset in `get_mapped_range` by @nical in [#3233](https://github.com/gfx-rs/wgpu/pull/3233)
+
+
 ## wgpu-0.14.1 (2022-11-02)
 
 ### Bug Fixes

wgpu-core/src/device/mod.rs

@@ -5677,7 +5677,10 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                         max: range.end,
                     });
                 }
-                unsafe { Ok((ptr.as_ptr().offset(offset as isize), range_size)) }
+                // ptr points to the beginning of the range we mapped in map_async
+                // rather thant the beginning of the buffer.
+                let relative_offset = (offset - range.start) as isize;
+                unsafe { Ok((ptr.as_ptr().offset(relative_offset), range_size)) }
             }
             resource::BufferMapState::Idle | resource::BufferMapState::Waiting(_) => {
                 Err(BufferAccessError::NotMapped)

wgpu/tests/buffer.rs

@@ -1,9 +1,6 @@
 use crate::common::{initialize_test, TestParameters, TestingContext};
-use std::sync::{Arc, atomic::{AtomicBool, Ordering}};
 
 fn test_empty_buffer_range(ctx: &TestingContext, buffer_size: u64, label: &str) {
-    let status = Arc::new(AtomicBool::new(false));
-
     let r = wgpu::BufferUsages::MAP_READ;
     let rw = wgpu::BufferUsages::MAP_READ | wgpu::BufferUsages::MAP_WRITE;
     for usage in [r, rw] {
@@ -14,15 +11,10 @@ fn test_empty_buffer_range(ctx: &TestingContext, buffer_size: u64, label: &str)
             mapped_at_creation: false,
         });
 
-        let done = status.clone();
-        b0.slice(0..0).map_async(wgpu::MapMode::Read, move |result| {
-            assert!(result.is_ok());
-            done.store(true, Ordering::SeqCst);
-        });
+        b0.slice(0..0)
+            .map_async(wgpu::MapMode::Read, Result::unwrap);
 
-        while !status.load(Ordering::SeqCst) {
-            ctx.device.poll(wgpu::MaintainBase::Poll);
-        }
+        ctx.device.poll(wgpu::MaintainBase::Wait);
 
         {
             let view = b0.slice(0..0).get_mapped_range();
@@ -37,30 +29,26 @@ fn test_empty_buffer_range(ctx: &TestingContext, buffer_size: u64, label: &str)
 
         // 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.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();
 
-        status.store(false, Ordering::SeqCst);
-
         // Write mode.
         if usage == rw {
-            let done = status.clone();
-            b0.slice(0..0).map_async(wgpu::MapMode::Write, move |result| {
-                assert!(result.is_ok());
-                done.store(true, Ordering::SeqCst);
-            });
+            b0.slice(0..0)
+                .map_async(wgpu::MapMode::Write, Result::unwrap);
 
-            while !status.load(Ordering::SeqCst) {
-                ctx.device.poll(wgpu::MaintainBase::Poll);
-            }
+            ctx.device.poll(wgpu::MaintainBase::Wait);
 
             //{
             //    let view = b0.slice(0..0).get_mapped_range_mut();
@@ -72,11 +60,9 @@ fn test_empty_buffer_range(ctx: &TestingContext, buffer_size: u64, label: &str)
             // Map and unmap right away.
             b0.slice(0..0).map_async(wgpu::MapMode::Write, move |_| {});
             b0.unmap();
-
         }
     }
 
-
     let b1 = ctx.device.create_buffer(&wgpu::BufferDescriptor {
         label: Some(label),
         size: buffer_size,
@@ -91,18 +77,85 @@ fn test_empty_buffer_range(ctx: &TestingContext, buffer_size: u64, label: &str)
 
     b1.unmap();
 
-    for _ in  0..10 {
-        ctx.device.poll(wgpu::MaintainBase::Poll);
-    }
+    ctx.device.poll(wgpu::MaintainBase::Wait);
 }
 
 #[test]
+#[ignore]
 fn empty_buffer() {
-    initialize_test(
-        TestParameters::default(),
-        |ctx| {
-            test_empty_buffer_range(&ctx, 2048, "regular buffer");
-            test_empty_buffer_range(&ctx, 0, "zero-sized buffer");
-        }
-    )
+    // TODO: Currently wgpu does not accept empty buffer slices, which
+    // is what test is about.
+    initialize_test(TestParameters::default(), |ctx| {
+        test_empty_buffer_range(&ctx, 2048, "regular buffer");
+        test_empty_buffer_range(&ctx, 0, "zero-sized buffer");
+    })
+}
+
+#[test]
+fn test_map_offset() {
+    initialize_test(TestParameters::default(), |ctx| {
+        // This test writes 16 bytes at the beginning of buffer mapped mapped with
+        // an offset of 32 bytes. Then the buffer is copied into another buffer that
+        // is read back and we check that the written bytes are correctly placed at
+        // offset 32..48.
+        // The goal is to check that get_mapped_range did not accidentally double-count
+        // the mapped offset.
+
+        let write_buf = ctx.device.create_buffer(&wgpu::BufferDescriptor {
+            label: None,
+            size: 256,
+            usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
+            mapped_at_creation: false,
+        });
+        let read_buf = ctx.device.create_buffer(&wgpu::BufferDescriptor {
+            label: None,
+            size: 256,
+            usage: wgpu::BufferUsages::MAP_READ | wgpu::BufferUsages::COPY_DST,
+            mapped_at_creation: false,
+        });
+
+        write_buf
+            .slice(32..)
+            .map_async(wgpu::MapMode::Write, move |result| {
+                result.unwrap();
+            });
+
+        ctx.device.poll(wgpu::MaintainBase::Wait);
+
+        {
+            let slice = write_buf.slice(32..48);
+            let mut view = slice.get_mapped_range_mut();
+            for byte in &mut view[..] {
+                *byte = 2;
+            }
+        }
+
+        write_buf.unmap();
+
+        let mut encoder = ctx
+            .device
+            .create_command_encoder(&wgpu::CommandEncoderDescriptor { label: None });
+
+        encoder.copy_buffer_to_buffer(&write_buf, 0, &read_buf, 0, 256);
+
+        ctx.queue.submit(Some(encoder.finish()));
+
+        read_buf
+            .slice(..)
+            .map_async(wgpu::MapMode::Read, Result::unwrap);
+
+        ctx.device.poll(wgpu::MaintainBase::Wait);
+
+        let slice = read_buf.slice(..);
+        let view = slice.get_mapped_range();
+        for byte in &view[0..32] {
+            assert_eq!(*byte, 0);
+        }
+        for byte in &view[32..48] {
+            assert_eq!(*byte, 2);
+        }
+        for byte in &view[48..] {
+            assert_eq!(*byte, 0);
+        }
+    });
 }

wgpu/tests/root.rs

@@ -1,6 +1,7 @@
 // All files containing tests
 mod common;
 
+mod buffer;
 mod buffer_copy;
 mod buffer_usages;
 mod clear_texture;