Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vulkan: Mark fn mapped_(mut_)slice() as unsafe #139

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

vulkan: Mark fn mapped_(mut_)slice() as unsafe #139

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 14 additions & 8 deletions src/vulkan/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,18 +92,24 @@ impl Allocation {

/// Returns a valid mapped slice if the memory is host visible, otherwise it will return None.
/// The slice already references the exact memory region of the allocation, so no offset needs to be applied.
pub fn mapped_slice(&self) -> Option<&[u8]> {
self.mapped_ptr().map(|ptr| unsafe {
std::slice::from_raw_parts(ptr.cast().as_ptr(), self.size as usize)
})
///
/// # Safety
/// Only to be called when the memory is known to be _fully_ initialized.
Copy link

Choose a reason for hiding this comment

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

As a user of gpu_allocator, I can only get an Allocation using Allocator::allocate. The memory is at that point fully initialized because any bit pattern is a valid byte. Freeing likewise takes my instance, preventing unsafe usage.

Does this need to be unsafe?

Copy link
Member Author

Choose a reason for hiding this comment

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

The memory is at that point fully initialized because any bit pattern is a valid byte

According to https://crates.io/crates/presser / https://www.ralfj.de/blog/2019/07/14/uninit.html, and per the description/trigger of this PR, that is not the case.

Copy link
Contributor

@fu5ha fu5ha Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The memory is at that point fully initialized because any bit pattern is a valid byte.

But this does not logically follow. Memory initialization is not about whether some bit pattern is a valid byte or not, it's a specific meta-state change about a piece of memory that happens the first time data is written into it after being marked uninit. Memory returned from Allocator::allocate is not initialized. uninit memory is not just "any bit pattern", it is a completely separate "meta-state" about a piece of memory that means it could not just be any bit pattern but it could very well not exist at all, and thus the compiler is free to make optimizations, reorder your code, etc. based on this assumption. reading an uninit byte is always ub, even if your code is valid for any bit pattern -- see readme here https://github.com/EmbarkStudios/presser

Copy link

Choose a reason for hiding this comment

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

Ah, I see, the unsafety is reading possibly uninitialized data.

Copy link
Member Author

Choose a reason for hiding this comment

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

@attackgoat more concretely, will a MaybeUninit<T> API work for your use-case?

Copy link

Choose a reason for hiding this comment

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

I see the danger in using data which has "random" values, especially when transmuting to T; but I'm not entirely convinced that this is unsafe in the context. It's more unwise. The GPU is very likely to do things to our memory as we hold it; so additional safeguards don't seem fool proof.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whether the GPU is doing things or not is a wholly separate issue, though it does make things another level of unsafe. In that regard, most of the rust graphics libraries have taken the stance that the GPU overlapping access with CPU access doesn't constitute marking a function as unsafe because validating that at compile time is essentially impossible and classifying it as unsafe would mean pretty much every gpu api ever would be unsafe which just doesn't seem like it adds any value. So instead we reserve unsafe to demarcate when a function is extra unsafe and should be looked at even more carefully than just for synchronization with GPU access. Once again though, uninit =/= "random", in fact, if the GPU is writing data with uninit/padding and we are reading it back as &[u8] that's actually unsound whereas reading it back as &[T] which has the same layout as what the GPU wrote is completely sound and valid.

Copy link

Choose a reason for hiding this comment

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

I prefer a strong # Safety warning that states the danger involved, and at least an unsafe marker. I don't know if I'm knowledgeable enough to know about MaybeUnint exactly.

Copy link
Contributor

@fu5ha fu5ha Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The goal is to have an api that makes things almost-as-ergonomic (or even more ergonomic in many cases) while avoiding the most common safety pitfalls and calling out more explicitly the ways things can go wrong thru stronger safety warnings like you say :)

Copy link

Choose a reason for hiding this comment

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

Yes and I'll add that I like the idea of #138 because Screen 13 has the same old pattern using bytemuck and it would be nice to offer users a safer way of doing this.

pub unsafe fn mapped_slice(&self) -> Option<&[u8]> {
self.mapped_ptr()
.map(|ptr| std::slice::from_raw_parts(ptr.cast().as_ptr(), self.size as usize))
}

/// Returns a valid mapped mutable slice if the memory is host visible, otherwise it will return None.
/// The slice already references the exact memory region of the allocation, so no offset needs to be applied.
pub fn mapped_slice_mut(&mut self) -> Option<&mut [u8]> {
self.mapped_ptr().map(|ptr| unsafe {
std::slice::from_raw_parts_mut(ptr.cast().as_ptr(), self.size as usize)
})
///
/// # Safety
/// Only to be called when the memory is known to be _fully_ initialized. Use [`std::ptr::copy()`] or
/// [`std::ptr::copy_nonoverlapping()`] on [`mapped_ptr()`][Self::mapped_ptr()] to initialize this buffer
/// from the CPU instead.
pub unsafe fn mapped_mut_slice(&mut self) -> Option<&mut [u8]> {
Comment on lines +107 to +110
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of always requiring pointers for CPU-side initialization, we can also provide a safe fn that returns &(mut) [MaybeUninit<T>], allowing the caller to still initialize from slices more easily, without pulling in the whole (borked) "initialization tracking" from https://github.com/Traverse-Research/gpu-allocator/compare/uninit.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 yeah, though if we wanted it to be actually safe it would still need to be &mut [MaybeUninit<u8>], or check alignment and size requirements for T within (also doable)

Comment on lines +107 to +110
Copy link
Member Author

@MarijnS95 MarijnS95 Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Note that I sneaked a little mapped_slice_mut -> mapped_mut_slice renime in here, to better match as_slice/as_mut_slice in Rust std.

self.mapped_ptr()
.map(|ptr| std::slice::from_raw_parts_mut(ptr.cast().as_ptr(), self.size as usize))
}

pub fn is_null(&self) -> bool {
Expand Down
Loading