vulkan: Mark fn mapped_(mut_)slice() as unsafe
#139
Conversation
| /// 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]> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
👍 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)
| /// 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]> { |
There was a problem hiding this comment.
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.
|
I think that this is likely a good way forward. It might be the best to just not expose |
|
It seems useful to me to have these functions available - with the right That said, @fu5ha I assume for this to really be useful we should replace What are your thoughts on also adding a |
|
I'm a fan. I think the list of apis could be something like /// `offset` is in bytes from start of allocation, `len` is the number of `T`s in the returned slice
unsafe fn mapped[_mut]slice_from_offset<T>(&[mut] self, offset: usize, len: usize) -> Option(&[mut] [T]) {}
/// Same as above but doesn't validate alignment or size requirements for you
unsafe fn mapped[_mut]_slice_from_offset_unchecked<T>(&[mut] self, offset: usize, len: usize) -> Option(&[mut] [T]) {}
/// Note this can be safe because `MaybeUninit` ships the unsafety to `assume_init` call.
/// `offset` is in bytes from start of allocation, `len` is the number of `T`s in the returned slice
fn mapped_maybe_uninit[_mut]_slice_from_offset<T>(&[mut] self, offset: usize, len: usize) -> Option(&[mut] [MaybeUninit<T>]) {}
/// Same as above but doesn't validate alignment or size requirements for you so unsafe again
unsafe fn mapped_maybe_uninit[_mut]_slice_from_offset_unchecked<T>(&[mut] self, offset: usize, len: usize) -> Option(&[mut] [MaybeUninit<T>]) {}Also arguably the non- Notably these are also the set of apis (plus ones that just return a bare |
| }) | ||
| /// | ||
| /// # Safety | ||
| /// Only to be called when the memory is known to be _fully_ initialized. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ah, I see, the unsafety is reading possibly uninitialized data.
There was a problem hiding this comment.
@attackgoat more concretely, will a MaybeUninit<T> API work for your use-case?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
|
@fu5ha Good point on the Not sure if we should:
|
MarijnS95
changed the title
fn mapped_(mut_)slice() as `unsafefn mapped_(mut_)slice() as unsafe
|
My take is probably that things are "fine" just as is for now, people are unlikely to change to new apis between now and the near-ish future where we could replace the whole api as discussed. Especially since I don't think it'll take too long to implement those in presser and then add the APIs here on top. Plus that will be one less breaking change for users to have to update to. |
|
Opened EmbarkStudios/presser#5 which implements basically the idea I discussed above, atop which it should be fairly easy to implement similar APIs in |
|
@fu5ha Sure! This PR only took a few minutes to rig up, I'm fine with closing it and migrating the whole public mapping API to We could keep the ( |
|
Whatever you think. I'm a bit biased but I think doing this would be a good order:
|
|
Sounds good! |
I think we should always provide a mapping api in order to not break this crate; however, I think we should support |
Yeah, it would always go through For this to work out I suggest we follow @fu5ha's points above - drop this PR - and use at least the examples in this repo and our codebase to test-drive a full public-API migration to |
|
Sounds good to me. We can/will also use our internal codebase to test things out! |
|
Would it be worth releasing a version that deprecates |
|
@djkoloski yeah.. this PR is likely to be closed rather than merge now, see the current plan described in first comment of #138 |
|
@djkoloski That is effectively - exactly - what I wrote two posts above yours 😉. This is planned, I just have had too much on my plate to look into EmbarkStudios/presser#5 followed by #138, apologies @fu5ha I'll queue it up asap! |
As discussed long ago, and recently in #138, it is undefined behaviour to create or transmute to `&[u8]` when the underlying data is possibly uninit. This also holds true for transmuting arbitrary `T: Copy` structures to `&[u8]` where eventual padding bytes are considered uninitialized, hence invalid for `u8`. Instead of coming up with a massive safety API that distinguishes between uninitialized and initialized buffers - which turn out to be really easy to invalidate by copying structures with padding bytes - place the onus on the user to keep track of initialization status by only ever providing mapped slices in an `unsafe` context. Users are expected to initialize the buffer using `ptr::copy(_nonoverlapping)()` when used from a CPU context instead of calling `.mapped_mut_slice()`, or switch to the new [presser] API from #138. [presser]: https://crates.io/crates/presser
MarijnS95
force-pushed
the
mapped-slice-unsafe
branch
from
1682e52 to
888cd2a
Compare
Motivated by Traverse-Research/gpu-allocator#139, scaffolded some read-helper functions. These helpers are mostly all unsafe and have both semi-checked and completely unchecked variants. They are relevant in either case because they lay out in documentation exactly what the needed safety requirements are to read the given data, given that we have a properly implemented `Slab`, and try to remove some common footguns (alignment, size within allocation) where possible in the checked variants. A note for reviewers is that you can skip the `copy.rs` file as that is just code movement from the old `lib.rs`. @eddyb, requested your review since you helped validate `presser` originally and you might have some valuable input on the safety comments/requirements here.
|
I am reopening this PR, if not in the least for tracking purposes. While the presser changes were merged as-is (adding lots of documentation and a crate dependency which I have yet to digest, for what is effectively a raw pointer API), while leaving the original UB We'd have to remove this API or mark it |
CC @fu5ha
As discussed long ago, and recently in #138, it is undefined behaviour to create or transmute to
&[u8]when the underlying data is possibly uninit. This also holds true for transmuting arbitraryT: Copystructures to&[u8]where eventual padding bytes are considered uninitialized, hence invalid foru8.Instead of coming up with a massive safety API that distinguishes between uninitialized and initialized buffers - which turn out to be really easy to invalidate by copying structures with padding bytes - place the onus on the user to keep track of initialization status by only ever providing mapped slices in an
unsafecontext. Users are expected to initialize the buffer usingptr::copy(_nonoverlapping)()when used from a CPU context instead of calling.mapped_mut_slice(), or switch to the new presser API from #138.