util::Align and AlignIter are unsound

[fu5ha]

As of now, Align and AlignIter both trigger undefined behavior unless the full mapped range used to create it is initialized (not guaranteed by vulkan spec afaik). A reference is never allowed to point to uninitialized memory in rust, even if it is never read... this happens in the implementation of copy_from_slice and when iterating in AlignIter.

[MarijnS95]

I'd assume this is why the only constructor - fn new - is marked unsafe: the caller has to "guarantee" that the pointer is valid and pointing to initialized memory.

This may not be a valid assumption for the cases (ie. in the examples) where copy_from_slice is exactly used to initialize the data, but it is necessary for readbacks from mapped buffers.

Perhaps we should switch to a model where this is made explicit with MaybeUninit and typical helpers. We started doing the same for gpu-allocator but the bug-reporter went dark and I've never validated+finished it.

[Ralith]

Alternatively, we could drop these entirely. IME they're often used unnecessarily, and similar functionality can be (more?) easily achieved with e.g.

#[repr(align(256))] 
#[repr(C)]
struct DynamicUniformValue {
    // ...
}

and regular slices.

[MaikKlein]

I am not too happy with Align myself. I wrote it an eternity ago as dynamic uniform buffers are a bit special. I am a bit hesitant about removing it as I don't know how many people rely on it.

Fwiw we can just switch to https://doc.rust-lang.org/stable/std/ptr/fn.slice_from_raw_parts_mut.html internally. No need to create borrows.

Using #[repr(align(256))] could also work, but potentially over aligns the buffers :shrug:

[fu5ha]

Raw slices are cool but really not usable outside of nightly (or compiler internals which of course can do what they want) at the moment which limits their use in this case. I've been working on a crate to solve similar issues we have internally at Embark and hopefully will be able to open source that soon -- ash could either depend on it (could be used to implement Align wrapper, though might require breaking change in api by making something marked unsafe) or just remove Align entirely and perhaps recommend it as a way to solve the issues it was meant to solve.

@MarijnS95 meant to reply to your comment sooner... opening a PR on gpu-allocator is on my todo list but didn't want to do so before I had a solution ready that I thought would be able to relatively ergonomically replace the current way things are implemented, that way it's not a huge hassle for end users. I think I have that in the crate I was talking about above, but haven't actually open sourced it yet. Anyhow, will take any more convo about gpu-allocator to that repo so we don't clog up ash issues with it ;P

[Ralith]

Raw slices are cool but really not usable outside of nightly

Huh? *mut [T] works just fine on stable. Did you mean something else?

IMO this functionality is just plain out of scope for ash regardless, since it's so rarely actually required.

[fu5ha]

*mut [T] works fine in that you can declare (and even create with the ptr functions) one, but in order to actually do basically anything with it (including query its length etc), you must deref it as &mut [T] because all the slice methods take &self / &mut self. Which in this case makes it useless because now we have to guarantee the more strict requirements of a reference over a raw pointer

[MarijnS95]

@termnh no worries, though I hope you've not spent an inordinate amount cleaning up that branch as I've done the same locally since writing this, just haven't found the right moment to dot the i's and cross the t's to make a PR. Feel free to make an issue and/or PR there to continue the discussion about safety versus ergonomics :grin: