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.

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

Convert mem::zeroed() / 0 to MaybeUninit::uninit() #798

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Conversation

MarijnS95
Copy link
Collaborator

As noted in #792 changes like this might help us more strictly identify and validate that the argument in question is only relevant as an output argument (i.e. structs with sType are read by the function call, even if the caller strictly expects return values there, so that it can fill in multiple structures in a pNext chain, and must hence be Default- initialized).

It does embed rather nicely with the assume_init_on_success() API, though.


Creating this as draft just to feel out where we stand with this; it might not net us too much, and doesn't seem like it would help us audit for accidental sType structs that are zero/uninit. There was also the idea to initialize handles with ::null() explicitly (instead of mem::zeroed() or the MaybeUninit::uninit() added here), but that also doesn't sound like much of an improvement.

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

I agree that this helps clarify whether an intention to pass in a specific value exists or not. Perhaps a marginal change, but certainly doesn't hurt.

@@ -828,14 +832,15 @@ impl Device {
&self,
info: &vk::ImageSparseMemoryRequirementsInfo2,
) -> usize {
let mut count = 0;
// TODO: when pSparseMemoryRequirements is NULL, is it okay/safe for count to be uninit?
let mut count = mem::MaybeUninit::uninit();
Copy link
Collaborator

@Ralith Ralith Oct 11, 2023

Choose a reason for hiding this comment

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

I think this is sound as written. The spec seems to be missing the relevant text, but consider this analogous section:

If pPhysicalDevices is NULL, then the number of physical devices available is returned in pPhysicalDeviceCount. Otherwise, pPhysicalDeviceCount must point to a variable set by the user to the number of elements in the pPhysicalDevices array [...]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I thought I had deleted all TODOs from this PR again 😅

Copy link
Collaborator Author

@MarijnS95 MarijnS95 Nov 17, 2023

Choose a reason for hiding this comment

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

@Ralith I'm marking this as unresolved because, at least for vkGetLatencyTimingsNV (#814, KhronosGroup/Vulkan-Docs#2269 (comment)), pTimingCount will be set to the available number of timings if it was 0, even if pTimings was non-NULL. Meaning that there's probably an if (!pTimings || !*pTimingCount) *pTimingCount = ...; in the driver.

This could also be written the wrong way around and read our uninitialized value: if (!*pTimingCount || !pTimings) *pTimingCount = ...;

@chansen-nv is adding that mention to the spec, but I don't think this behaviour is consistent/expected wrt other Vulkan functions?


However, https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetDeviceImageSparseMemoryRequirements.html only says:

pSparseMemoryRequirementCount is a pointer to an integer related to the number of sparse memory requirements available or queried, as described below.

(There is no description below)

And the VUID doesn't rule out a similar implementation to vkGetLatencyTimingsNV either:

If the value referenced by pSparseMemoryRequirementCount is not 0, and pSparseMemoryRequirements is not NULL, pSparseMemoryRequirements must be a valid pointer to an array of pSparseMemoryRequirementCount VkSparseImageMemoryRequirements2 structures

Perhaps @oddhack can help us clarify whether or not the count pointer/value should be written when it's zero even if the array-pointer is NULL (in the generic case). If we cannot get a guarantee on this for some/most functions, we should back this change out and keep a 0 init (let mut count = 0; or let mut count = mem::MaybeUninit::zeroed()).

@MarijnS95
Copy link
Collaborator Author

Sure thing! I'll get going on finding the remaining assignments for 0 / mem::zeroed()...

@MarijnS95
Copy link
Collaborator Author

@Ralith would you mind taking another look? Besides tackling the remaining obvious = 0; and mem::zeroed(), I've also converted the let fd = -1; initializations to this (assuming Vulkan always initializes them when it retrurns OK), and replaced a few remaining zero-initialized Vecs with Vec::with_capacity() + set_len().

@MarijnS95 MarijnS95 marked this pull request as ready for review November 15, 2023 11:03
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

LGTM. Didn't double-check the spec for every single command, but each case got at least a quick look.

ash/src/extensions/amdx/shader_enqueue.rs Outdated Show resolved Hide resolved
ash/src/prelude.rs Outdated Show resolved Hide resolved
As noted in #792 changes like this might help us more strictly identify
and validate that the argument in question is only relevant as an output
argument (i.e. structs with `sType` are read by the function call, even
if the caller strictly expects return values there, so that it can fill
in multiple structures in a `pNext` chain, and must hence be `Default`-
initialized).
@MarijnS95 MarijnS95 merged commit 4e99de1 into master Dec 1, 2023
22 checks passed
@MarijnS95 MarijnS95 deleted the maybeuninit branch December 1, 2023 21:25
@MarijnS95 MarijnS95 added this to the Ash 0.38 milestone Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants