vulkan: Mark `fn mapped_(mut_)slice()` as `unsafe`

[MarijnS95]

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 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.

[fu5ha]

I think that this is likely a good way forward. It might be the best to just not expose mapped_slice at all and only ever expose the raw base pointer, though exposing it as a &[u8] under an unsafe fn and making it clear the guarantees that implies is also fine. I've also thought about adding support for some unsafe functions to presser for casting some subsection of a Slab into a &[T] for reading. These would need to be unsafe but could help avoid some common pitfalls and list all the actual needed safety invariants in an easy-to-see place on them, but that's not a reality now so having these is probably fine.

[MarijnS95]

It seems useful to me to have these functions available - with the right # Safety context - to hopefully prevent users from implementing it themselves without being aware of these problems.

That said, @fu5ha I assume for this to really be useful we should replace u8 with T so that a struct with padding can still be accessed safely, as &[T] is safe to access whereas &[u8] is not.

What are your thoughts on also adding a &mut [MaybeUninit<T>] API (possibly both mut and non-mut variants)?

[fu5ha]

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-mut version of the MaybeUninit api is pretty much useless because at least as of the current stable MaybeUninit interface you can't actually do anything with it and if you're just going to assume_init immediately you may as well just use the unsafe version that returns an actual slice.

Notably these are also the set of apis (plus ones that just return a bare &[mut] T rather than a slice) that I'd want to implement for this purpose in presser, so this could be a good excuse to do that and then just ship them thru to it in the gpu-allocator functions.

[MarijnS95]

@fu5ha Good point on the offset and len!

Not sure if we should:

1. Merge this PR as a stop-gap solution;
2. With or without MaybeUninit;
3. With or without u8 -> T, requiring an offset and size (otherwise the user needs to go through &[u8] anyway, offset, and from_raw_parts);
4. Wait for you to implement this in presser, and replace the entire public mapped_slice[_mut]() API with it at once.

[fu5ha]

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.

[fu5ha]

Opened https://github.com/EmbarkStudios/presser/pull/5 which implements basically the idea I discussed above, atop which it should be fairly easy to implement similar APIs in gpu-allocator, though probably not all of them are needed to be included directly, since we can just go through the Slab impl proposed in #138 for the rest.

[MarijnS95]

@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 presser as a whole.

We could keep the (default enabled) presser feature in #138, and just not provide any mapping API at all if it is disabled - or commit the changes provided by this PR, including the modifications (size+offset, and a safe MaybeUninit accessor) suggested above.

[fu5ha]

Whatever you think.

I'm a bit biased but I think doing this would be a good order:

1. Keep how it is for now
2. land #138
3. land EmbarkStudios/presser#5 and release presser 0.4
4. update to presser 0.4 here and deprecate the current slice apis, recommending using the presser ones thru as_mapped_slab instead
5. see if it feels too awkward to go thru an extra type for all read and writes or not, if so we can add back a couple helper functions like the ones discussed above, if not we can leave as-is and fully remove the old ones

[MarijnS95]

Sounds good!

CC @manon-traverse @Jasper-Bekkers :)

[Jasper-Bekkers]

We could keep the (default enabled) presser feature in https://github.com/Traverse-Research/gpu-allocator/pull/138, and just not provide any mapping API at all if it is disabled

I think we should always provide a mapping api in order to not break this crate; however, I think we should support presser as a first class citizen and potentially even not provide any other paths. However, if we go that route, we should make sure that it's minimal impact on exisiting users, and has great upgrade documentation.

[MarijnS95]

I think we should always provide a mapping api in order to not break this crate

Yeah, it would always go through presser in that case, and we should omit the feature flag altogether (if users really are against using presser in the public API, we could provide minimal mapping functions that are implemented on top of presser internally :grimacing:).

---

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 presser when point 4 is finished. We can always make beta releases or provide a non-breaking release with #[deprecated] statements, just to see how it all pans out without affecting anyone.

[fu5ha]

Sounds good to me. We can/will also use our internal codebase to test things out!

[djkoloski]

Would it be worth releasing a version that deprecates mapped_(mut_)?slice()? That would ensure that anyone using the current API should be using mapped_ptr() and size() instead.

[fu5ha]

@djkoloski yeah.. this PR is likely to be closed rather than merge now, see the current plan described in first comment of #138

[MarijnS95]

@djkoloski That is effectively - exactly - what I wrote two posts above yours :wink:. This is planned, I just have had too much on my plate to look into https://github.com/EmbarkStudios/presser/pull/5 followed by #138, apologies @fu5ha I'll queue it up asap!