Skip to content
This repository was archived by the owner on Jun 18, 2021. It is now read-only.

Safety of buffer mapping #119

Closed
jacob-greenfield wants to merge 2 commits into from
Closed

Safety of buffer mapping #119

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62bbe5f
Run `cargo fmt`
jacob-greenfield Nov 8, 2019
66a1bf0
Remove unnecessary generic bounds
jacob-greenfield Nov 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,18 +494,18 @@ pub struct CreateBufferMapped<'a, T> {
pub data: &'a mut [T],
}

impl<'a, T> CreateBufferMapped<'a, T>
where
T: Copy,
{
impl<'a, T> CreateBufferMapped<'a, T> {
/// Copies a slice into the mapped buffer and unmaps it, returning a [`Buffer`].
///
/// `slice` and `self.data` must have the same length.
///
/// # Panics
///
/// Panics if the slices have different lengths.
pub fn fill_from_slice(self, slice: &[T]) -> Buffer {
pub fn fill_from_slice(self, slice: &[T]) -> Buffer
where
T: Copy,
{
self.data.copy_from_slice(slice);
self.finish()
}
Expand Down Expand Up @@ -794,10 +794,7 @@ impl Device {
&'a self,
count: usize,
usage: BufferUsage,
) -> CreateBufferMapped<'a, T>
where
T: 'static + Copy,
{
) -> CreateBufferMapped<'a, T> {
Copy link
Member

@kvark kvark Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to at least have T: 'a (i.e. T type lives at least as long as 'a) unless it's inferred automatically

Copy link
Contributor Author

@jacob-greenfield jacob-greenfield Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized... the current function is actually unsafe:

let test_buf = device.create_buffer_mapped::<&u8>(8, wgpu::BufferUsage::VERTEX);
println!("{:?}", test_buf.data); // Segmentation fault

I'll need to think about how to fix this...

let type_size = std::mem::size_of::<T>() as BufferAddress;
assert_ne!(type_size, 0);

Expand Down Expand Up @@ -888,8 +885,8 @@ where
impl Buffer {
pub fn map_read_async<T, F>(&self, start: BufferAddress, size: BufferAddress, callback: F)
where
T: 'static + FromBytes,
Copy link
Member

@kvark kvark Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is static removed from here? We don't know when the call back is going to be called, and for this time frame T has to be valid. static is a conservative bound to enforce that

F: FnOnce(BufferMapAsyncResult<&[T]>) + 'static,
T: FromBytes,
F: FnOnce(BufferMapAsyncResult<&[T]>),
{
extern "C" fn buffer_map_read_callback_wrapper<T, F>(
status: wgn::BufferMapAsyncStatus,
Expand Down Expand Up @@ -934,8 +931,8 @@ impl Buffer {

pub fn map_write_async<T, F>(&self, start: BufferAddress, size: BufferAddress, callback: F)
where
T: 'static + AsBytes + FromBytes,
F: FnOnce(BufferMapAsyncResult<&mut [T]>) + 'static,
T: AsBytes + FromBytes,
F: FnOnce(BufferMapAsyncResult<&mut [T]>),
{
extern "C" fn buffer_map_write_callback_wrapper<T, F>(
status: wgn::BufferMapAsyncStatus,
Expand Down