References in FFI types/signatures

[Ralith]

Rust's &'a T is guaranteed to be interpretable as a nonnull raw pointer. Wrapping them in Option adds the null case. Given that Vulkan generally does not capture passed-in pointers, I think we could replace most uses of raw pointers to single objects in FFI signatures and structures with Rust references. This would help mitigate the footgun of storing a raw pointer in a struct and then dropping the pointee before passing the struct to Vulkan, and make builders less needed, though still nice for arrays.

Can anyone think of a reason not to do this?

[MarijnS95]

That'd be a very welcome change indeed! Though we have to keep in mind to not "accidentally" generate this for DSTs like dynamic types and slices.

raw pointers to single objects

Any idea if we can do something similar for arrays? Those have been most problematic in our code (even with the builder pattern, ie. .some_array(&[single_item]).build();).

Finally, the builders already seem "unnecessary" in hiding the functions behind a separate struct to not have lifetimes on the FFI structs. If we go ahead with this change (and add 'a to FFI structs) we might as well drop the builders entirely and move its functions directly on the main struct.

We should go ahead and make this change, and dog-feed it to our own code as an initial feasibility test. Likewise should we reconsider making fn build unsafe?

[Ralith]

Unfortunately I think this is impossible for slices, because Vulkan lays out the length inline in the struct, and we have absolutely no guarantee that the resulting layout would match that of a slice even if/when Rust slices do get a defined ABI. However, if we subsumed the builders into methods on Vulkan structs as you suggest, I think we'd have the next best thing: such methods could easily enforce that a passed-in array outlives the struct, just like they do for builders today. It'd be a bit weird to use struct construction syntax for most things and a setter for arrays, but it'd work.

[Ralith]

Likewise should we reconsider making fn build unsafe?

I don't think this accomplishes anything, for two reasons:

• Vulkan code is almost invariably deep inside a big unsafe block anyway, so if you're not reading the docs it won't change anything, and we already document the hazard
• This hazard is strictly orthogonal to the formal notion of safety in Rust: there is no way to invoke UB by calling build; the hazard only arises when you pass the structure to an (already-unsafe) function that has a documented requirement that the pointer is valid.

Anyway, it sounds like we have a good case for getting rid of builders entirely (which might help our compile times besides!)

[Rua]

Would every pointer be replaced with an Option of a reference in this case? I think that would be necessary in order to keep the Default impl working.

[MaikKlein]

• ffi signatures: Yes probably? But we are already exposing &T in our higher level bindings, so the win doesn't seem that big.
• ffi types: Potentially yes? I was initially a bit worried that people who want to store these types would have to worry about the lifetimes. But you usually just construct them just before the function call anyway. Although would like to have a look at all the affected types.

However, if we subsumed the builders into methods on Vulkan structs as you suggest, I think we'd have the next best thing: such methods could easily enforce that a passed-in array outlives the struct, just like they do for builders today. It'd be a bit weird to use struct construction syntax for most things and a setter for arrays, but it'd work.

How would that look like?

Like this? let foo = Foo { f, .. Default::default() }.with_bar_slice(&bar)

[Rua]

A C pointer to a slice maps to a reference to its first element. So if slices can't go into structs due to ABI issues, then a separate count + reference to first element can work. Of course, this means you lose checking on the length, so methods may still be needed.

[Ralith]

Like this?

Yep, that's what I was thinking of.

a separate count + reference to first element can work. Of course, this means you lose checking on the length, so methods may still be needed.

This feels really weird to me. I've also gotten the length wrong far more times than I'd gotten the lifetimes wrong, pre-builders.

[Ralith]

I was initially a bit worried that people who want to store these types would have to worry about the lifetimes.

You could store a 'static version using None or pointers to constants, which should cover the cases where you'd want to do this. Note that Foo<'static> can be used with FRU syntax to produce a non-'static value.

[MarijnS95]

Unfortunately I think this is impossible for slices, because Vulkan lays out the length inline in the struct, and we have absolutely no guarantee that the resulting layout would match that of a slice even if/when Rust slices do get a defined ABI.

I wasn't planning on laying out the slice one way or another inside Vulkan structs, rather how to model a safe borrow to it in the first place. Would a &'a borrowing the first element work, considering it's just a pointer? That only opens up confusing API though.

Lengths are troublesome anyway, consider for example SubmitInfo where wait_semaphores and wait_dst_stage_mask share the same field. Ideally that's turned into a single builder function taking both slices, and asserting their lengths to be equal.

However, if we subsumed the builders into methods on Vulkan structs as you suggest, I think we'd have the next best thing: such methods could easily enforce that a passed-in array outlives the struct, just like they do for builders today. It'd be a bit weird to use struct construction syntax for most things and a setter for arrays, but it'd work.

With this we'd basically be moving the lifetime parameter over to the struct itself, and automatically get rid of the .build() footgun - and we can turn single-object references into borrows too in case users like to interact with these fields directly.

Likewise should we reconsider making fn build unsafe?

I don't think this accomplishes anything, for two reasons:

• Vulkan code is almost invariably deep inside a big unsafe block anyway, so if you're not reading the docs it won't change anything, and we already document the hazard

At least in our codebase we're minimizing the scope of unsafe, even the .unwrap() or ? sits outside it where possible (and this usually allows rustfmt to place everything on a single line).

Regardless we have already accepted that Vulkan functions are unsafe, but builders are not. Making .build() unsafe would be another nudge to revisit all calls to it.

• This hazard is strictly orthogonal to the formal notion of safety in Rust: there is no way to invoke UB by calling build; the hazard only arises when you pass the structure to an (already-unsafe) function that has a documented requirement that the pointer is valid.

Just like how .as_ptr() on various types looses all lifetime/borrow information, but isn't inherently unsafe itself. I have mixed feelings about this and appreciate some leeway to allow custom "hey, you better think twice before calling this" unsafe markings.

Anyway, it sounds like we have a good case for getting rid of builders entirely (which might help our compile times besides!)

🥳

ffi types: Potentially yes? I was initially a bit worried that people who want to store these types would have to worry about the lifetimes. But you usually just construct them just before the function call anyway.

That's the purpose of lifetimes and this issue, to prevent users from having objects that reference dropped memory - they have to worry about lifetimes one way or another.

---

In an ideal world we should have all primitives and single-object borrows as public fields, but hide lengths and array pointers behind appropriate slice getters and setters to minimize the possibility for UB; both in terms of pointee-lifetime and slice length mishaps.

[MarijnS95]

A C pointer to a slice maps to a reference to its first element. So if slices can't go into structs due to ABI issues, then a separate count + reference to first element can work. Of course, this means you lose checking on the length, so methods may still be needed.

I probably don't want to worry about storing slice lengths separately if they are already encoded in the Vulkan structs itself. However, they might address the above problem of multiple array-pointer-fields sharing the same length field while still being able to re-set a slice field later, with different sizes.

[Ralith]

Would a &'a borrowing the first element work, considering it's just a pointer? That only opens up confusing API though.

Yes, but I agree it's a bit confusing. Then again, using setters only for array fields is weird too, unless we keep all the setters, which is a bit disappointing from a code savings perspective but makes for more consistent style. Not sure what the best balance is.

With this we'd basically be moving the lifetime parameter over to the struct itself, and automatically get rid of the .build() footgun - and we can turn single-object references into borrows too in case users like to interact with these fields directly.

Yes, that's the idea.

At least in our codebase we're minimizing the scope of unsafe

Most of my Vulkan code lives in functions that are almost exclusively concerned with making Vulkan calls or invoking methods that have unchecked assumptions about Vulkan state, so any narrowing further than the function level would lead to ~every expression being separately wrapped in unsafe, which does no good. I don't use ash's builders anywhere else.

Just like how .as_ptr() on various types looses all lifetime/borrow information, but isn't inherently unsafe itself

Yes, exactly. I can see both sides here, but my feeling is that using unsafe as a general-purpose red-flag cheapens it; if it's routinely used in places that have no direct semantics, then it's easier to get in the habit of skimming past it, and harder to identify and thoroughly audit code that genuinely does have unchecked soundness requirements.

[MarijnS95]

using setters only for array fields is weird too, unless we keep all the setters, which is a bit disappointing from a code savings perspective but makes for more consistent style

Indeed, the end result inevitably has to look like #441 (comment) with fields and a default in FRU followed by one or more explicit setters. It's not consistent but we might as well try this and see what impact it has on our own programs (OTOH that's a lot of effort to implement when we already know what it looks like).

Yes, exactly. I can see both sides here, but my feeling is that using unsafe as a general-purpose red-flag cheapens it; if it's routinely used in places that have no direct semantics, then it's easier to get in the habit of skimming past it, and harder to identify and thoroughly audit code that genuinely does have unchecked soundness requirements.

I guess that already is the case for us in our Vulkan backend, unsafe simply doesn't stand out anymore whereas .build() sticks out like a sore thumb. Let's leave this as it is then, it becomes obsolete if we go ahead with this change anyway.

[Ralith]

Adopting references for non-array stuff and moving all setters to the actual Vulkan structs rather than builders should be uncontroversial and a big win on its own; we can then separately consider stripping things down further.

[MarijnS95]

The only controversy might be in the lifetime parameters that will have to be specified all across the generated functions and function-pointers which use these struct types directly, that could become annoying generator-wise (unless the new generator is used, which seems to have better support for a distinct parsing+analysis phase, separate from the code generation phase).

[Ralith]

Sounds like a good reason to pursue this as part of the new generator, then!

[MarijnS95]

https://twitter.com/dotstdy/status/1454505264246312980?t=Og0n2mof83gW_mmLQLzOdg Got linked this relevant thread that judges Ash build time and uses exactly the things we proposed here! (except the VulkanSlice1 as that might not work when the size field isn't right before the pointer field, including the ambiguity where multiple pointers share the same size field).

All in all this is probably something we can start working on right now, as it doesn't seem there's a lot of traction on the new one yet. @MaikKlein what's its status, is it something that everyone just needs to start working on?

[Ralith]

#602 is a big step in this direction.

[MarijnS95]

@Ralith is this still something you want to chase up? It might be relevant for the few structs one could construct directly without default() (i.e. making sure all fields are filled), and that's slightly ugly with the _marker: PhantomData.

[Ralith]

I'm pretty bandwidth-starved at the moment, at least. The slice representation problem means we'll always need to rely on setters in some cases, so at least off the cuff I don't mind leaning into them.

Is vk.xml precise enough to let us judge where to use Option<&T> vs &T? I know that didn't pan out for Vulkan handles, but I forget whether it does for pointers. If not, this might be a step backwards in ergonomics. If so, we could leverage that to significantly improve readability of fields and setters both.

[Ralith]

If we can't eliminate _marker entirely then eliminating it in most cases might just make the remaining ones even more confusing, since there won't be consistent norms to deal with them.

[MarijnS95]

The slice representation problem means we'll always need to rely on setters in some cases, so at least off the cuff I don't mind leaning into them.

I previously linked a Twitter post but it seems to have gotten removed. It had a screenshot of some VulkanSlice1<', u32, T> type off the top of my head, for the common case where a count is directly followed a pointer, and I guess the type would safely abstract that away with slice access. I presume there's also a VulkanSlice2 when two slices share the same counter (see SubpassDescription::color_attachment_count which is immediately followed by p_color_attachments and p_resolve_attachments). It probably uses some trick to deal with the 8-byte alignment for pointers:

struct VkFoo {
    count: u32, // off=0
    _implicit_padding: u32, // off=4
    ptr: *const u32, // off=8
}

But that padding is not there in the parent struct if the previous members sum up to an uneven multiplier of 4-bytes

struct VkFoo {
    flags: u32,  // off=0
    count: u32, // off=4
    // No padding needed!
    ptr: *const u32, // off=8
}

And if I remember correctly there was one more template parameter in the screenshot which must have been driving this.

If we can't eliminate _marker entirely then eliminating it in most cases might just make the remaining ones even more confusing, since there won't be consistent norms to deal with them.

Only slices would prevent this, right? Then the VulkanSliceX type, if we can apply it globally, would have this as private member and we won't have it publicly anywhere (see below: perhaps it can have a const constructor).

Is vk.xml precise enough to let us judge where to use Option<&T> vs &T? I know that didn't pan out for Vulkan handles, but I forget whether it does for pointers. If not, this might be a step backwards in ergonomics. If so, we could leverage that to significantly improve readability of fields and setters both.

That's a question for @oddhack I think, I don't know if optional attributes are accurately absent.

TBH I don't think writing some_ptr: Some(&other_thing) is much worse than .some_ptr(&other_thing), though it'd be great if Option<> is only present where the spec mandates it.

---

On yet another note, especially if we end up going this route of VulkanSlice1 which needs a constructor, I wonder if we can and should annotate builder functions as fn const.

[Ralith]

Only slices would prevent this, right? Then the VulkanSliceX type, if we can apply it globally, would have this as private member and we won't have it publicly anywhere

I don't think such types can ever be sound. A nested struct is not guaranteed to have the same layout as the equivalent fields inlined into a struct.

I wonder if we can and should annotate builder functions as fn const.

Not sure when this would be very important, but it seems harmless since we're inlining them all anyway.

[MarijnS95]

fn const would only be necessary if building and wanting to store these structs in const variables, but then because of inlining that may already be what the compiler emits. It's more of a usability thing for the user, though I don't have any concrete examples where this'd be useful (but there probably are!) 😅

[MarijnS95]

Catching up on this very old issue because of Matrix discussions, I think we might build the slice wrapper from the removed Twitter comment as long as we have different slice wrapper types around with different size/packing rules. That means the generator will have to keep track of the size and alignment of every field to pick the right one.

Example lacks an implementation and appropriate lifetimes. And we need a VulkanSlice2 variant for the waitSemaphoreCount linked above, which has one counter for two slices.

#[repr(C)]
#[cfg_attr(feature = "debug", derive(Debug))]
#[derive(Copy, Clone)]
pub struct VulkanSlice1<T> {
    size: u32,
    // Implicit u32 padding here
    pointer: *const T,
}
#[repr(packed(4))]
#[cfg_attr(feature = "debug", derive(Debug))]
#[derive(Copy, Clone)]
pub struct VulkanSlice1Packed<T> {
    size: u32,
    pointer: *const T,
}
#[repr(C)]
#[cfg_attr(feature = "debug", derive(Debug))]
#[derive(Copy, Clone)]
#[doc = "<https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VkDeviceCreateInfo.html>"]
#[must_use]
pub struct DeviceCreateInfoSlices<'a> {
    pub s_type: StructureType,
    pub p_next: *const c_void,
    pub flags: DeviceCreateFlags,
    pub queue_create_infos: VulkanSlice1Packed<DeviceQueueCreateInfo<'a>>,
    #[deprecated = "functionality described by this member no longer operates"]
    pub enabled_layer_names: VulkanSlice1<*const c_char>,
    pub enabled_extension_names: VulkanSlice1<*const c_char>,
    pub p_enabled_features: *const PhysicalDeviceFeatures,
    pub _marker: PhantomData<&'a ()>,
}
/// Ensure the size stays the same
const CHECK: usize = (std::mem::size_of::<DeviceCreateInfoSlices>()
    == std::mem::size_of::<DeviceCreateInfo>()) as usize
    - 1;

Keep in mind that the common pattern to have a size field before the pointer field might not apply everywhere. We'd have to scan through vk.xml to ensure there are few enough outliers to make this worth our time.