Open BigAngryPanda opened 4 months ago
Let us first link some relevant sources:
The PR that made this MaybeUninit::uninit()
: #798
The bit of code that you're bringing up for discussion:
Also relevant to mention that the "545.29.06 driver" you're mentioning is an Nvidia driver. Other hardware vendors, which also implement the Vulkan API, have a different numbering scheme.
Have you looked at the memory_type_count
field of vk::PhysicalDeviceMemoryProperties
? It describes how many elements in memory_types
are initialized when Instance::get_physical_device_memory_properties()
returns. Keying off of heap_index
on its own wouldn't make much sense (if it was mem::zeroed()
) as heap_index 0
is valid if there's at least one heap.
Specifically we added slice getters in #858 to make it easier to extract the valid portion of this struct.
Now, given that the fields of struct vk::PhysicalDeviceMemoryProperties
are safe to access, I don't know if unsafe
ness of Instance::get_physical_device_memory_properties()
allows us to say that the caller is responsible for only looking at the first ..memory_type_count
number of elements in memory_types
(same for heaps) in the returned struct instance.
https://doc.rust-lang.org/std/pin/struct.Pin.html#method.get_unchecked_mut might be a nice example of this, where the safety docs don't specify any preconditions to uphold, but do say how you have to handle the returned value to not cause any UB.
On the other hand the safety docs for MaybeUninit::assume_init()
specify that T
must be fully initialized, which is not the case here.
Maybe this is one of the few cases where we should move back to ::zeroed()
though these uninitialized invalid values do force callers to actually look at the _count
field or (better yet) use our helper slice getters.
Thanks for answering
You are right I didn't check memory_type_count
so it works with zeroed memory but not with uninitialized
As it is my bad I guess issue is resolved
I don't want to close this prematurely as there's definitely something actionable on the Ash side, even if the Vulkan specification specifically calls out that the *Count
fields define how many valid elements there are in either array.
In the case that unsafe
on the function is enough to return uninitialized memory in a safely-accessible struct (which I doubt, because we're already violating the safety constraints on MaybeUninit::assume_init()
), the action point is to extend the method with a # Safety
paragraph.
Otherwise, we could move back to MaybeUninit::zeroed()
(or rather: Default::default()
and drop assume_init()
altogether), and still update the docs that the user is strongly encouraged to use our _as_slice()
helpers instead of piecing together the size manually (or uselessly read completely zeroed vk::MemoryType
elements). This wouldn't be a breaking change and can easily go into a patch release (on the side we also need to consider whether we want to solve #905).
Finally, we could wrap this struct in such a way that the fields are not exposed, and that the user can only access a slice view through a helper (also without being able to update/overwrite the *_count
fields safely). After all this struct is returnedonly
and doesn't need to be altered by the user (except for layers).
That would go nicely with changing the sized array type (because it now has a len
attribute in vk.xml
) to MaybeUninit<T>
, putting even more emphasis on using the helpers. But again, we should make sure the user cannot safely update *_count
in this case.
All of the above applies to a few more functions that return ::uninit()
structs with length-bounded static-sized arrays.
I started to inventorize all static-sized arrays that are runtime length bounded (either by a field or for strings a NULL terminator) at https://github.com/ash-rs/ash/compare/static-runtime-bounded-array-maybeuninit. Might get a little nasty to deal with this properly, as all accessor functions now have to be unsafe
because we can never assume that the "bound" was initialized or tampered with.
(either by overwriting the count field or rewriting when/where the NULL terminator is)
And since there isn't always a "guarantee" that (parts of) the struct was uninitialized (or left uninitialized), specifically when the caller passes a Default::default()
structure into a &mut
-taking function or p_next
chain, forcing these fields to always be MaybeUninit
seems too harsh. It would definitely be easier to revert parts of #798 and properly document when and why we didn't zero-initialize. Above all that's a change we can push out in a patch.
In ash 0.38 memory for
VkPhysicalDeviceMemoryProperties
became uninitializedlet mut memory_prop = mem::MaybeUninit::uninit();
This change produce invalid data in (at least)
memory_types
field (specifically heap_index becomes random) which violates restrictions inVkMemoryType
As for ash 0.37 it is fine as memory is zeroed
let mut memory_prop = mem::zeroed();
Driver 545.29.06-0ubuntu0.22.04.2