-
Notifications
You must be signed in to change notification settings - Fork 441
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
Resolve restrictions of BufferContents
, add support for allocating all types of buffers, and rework vulkano-shaders
#2132
Conversation
Can you explain exactly why it is not unsound for the user to provide a value that contains uninitialised bytes? |
Yes you're right, forgot to mention that. It is undefined behavior to interpret uninitialized memory in Rust, which at no point happens. Since we go directly from the mapped pointer to a reference to the type of the buffer contents, even if this type has padding, by definition Rust won't be able to read or otherwise reference those padding bytes. |
That's if you're interpreting Vulkan mapped memory. But what about user-provided data, like writing to a host buffer, specialisation constants, |
That's what I meant as well. As long as no reference to the padding bytes exists in Rust, there is no UB. |
The compiler by definition can't exploit UB across FFI boudaries. |
In the case of writing to a host buffer from user-provided data, a reference does exist, since the copy is done within Rust. |
I'm not sure I entirely understand - we can have a reference to a type that has padding in Rust. |
Ok, I think I understand. But I still feel like there's some edge case that we haven't thought of yet... |
I agree it's scary, but I hope I documented all the |
BufferContents
, add support for allocating all types of buffers, and other QoL improvementsBufferContents
, add support for allocating all types of buffers, and rework vulkano-shaders
I wanted to update the description again, to make clear the changes that have been added, but I'm afraid I don't have the energy at the moment. Hopefully the changelog will do. The PR is now ready for final review. |
I updated the description again now, hopefully it conveys all the changes. |
I added another section to the description - this time it should be complete I hope. |
Can you fix the conflicts? |
Ah, thanks, didn't notice. Done now. |
…all types of buffers, and rework vulkano-shaders (vulkano-rs#2132) * Add `BufferContentsLayout` * Rework `BufferContents` * Add `BufferContents` derive macro * Relax restrictions of shader code generation * Fix examples * Add additional invariant to `Subbuffer` * Shorten paths a bit * Remove a bit of bloat * Add `Sized` constraint to element traits * Fix an oopsie * Add `Aligned` * Add support for deriving `BufferContents` for sized types * Fix alignment and padding issues * Fix docs and add examples for `BufferContents` * Adjust shader macro * Add tests * Adjust `Vertex` example * Remove bytemuck re-export * Update examples * Workaround bytemuck's array elements that are `AnyBitPattern` limitation * Add more alignments * Fix an earlier oopsie * Rework vulkano-shaders * Fix examples * Fix some rogue tabs in examples * Add `AsRef` and `AsMut` implementation for `Padded` * Remove useless code duplication * Make the `BufferContents` derive macro the same for all types * Add example docs for `Padded` * Work around trivial bounds * More example docs * Minor consistency adjustment * Add `serde` to the list of cargo features * Make clippy happy again
This PR aims to loosen the restrictions of
BufferContents
, allow allocating and mapping of user-defined DSTs, and fix some bugs related to shader struct generation, all of which are tightly connected.BufferContents
Previously, buffer contents must have been POD, because they were cast to and from a slice of bytes. This doesn't have to be the case though: we don't actually need to read/write a byte slice at any point, all we need is to be able to interpret the mapped memory as some Rust type (the buffer contents). For this, the data still has to be POD because the memory could be random, but the type having padding is no issue, since we don't hold any references to those padding bytes at any point.
Therefore, the constraint on types that are
BufferContents
has been lowered fromPod
toAnyBitPattern
. Unfortunately, this means that we can no longer use bytemuck to convert between the mapped bytes and the desired type, since this necessitates using raw pointers instead of a byte slice. Which makes the trait more unsafe, but this is precisely what we have derive macros for. I can't think of any situation where derivingAnyBitPattern
orBufferContents
wouldn't suffice.Allocating memory for user-defined DSTs
The issue of not being able to allocate memory for user-defined DSTs has been resolved, which I think fixes #1329. The way this was done is that a
BufferContentsLayout
has been added, which is capable of encoding all buffer layouts that we can use with Vulkan, and this layout is specified when implementingBufferContents
. Since there is currently no way to query layout of DSTs in Rust, the only way we can support this is with a derive macro, for which aBufferContents
derive macro was added to safely implement the trait.Here's an example of a user-defined DST:
The memory for which can simply be allocated using
Buffer::new_unsized
orSubbufferAllocator::allocate_unsized
. Both of these only need you to input the length of the slice, instead of needing to do the (very error-prone) calculation of the number of bytes manually, as was the case previously. Also, the data is aligned correctly this way.You can even use generics:
This works because the derive macro adds
T: BufferContents, U: BufferContents
constraints. (And crucially, because padding is allowed.)Reading and writing user-defined DSTs from the host
Once allocated, these DSTs can also be read/written from the host, which fixes #1609. Note that this also necessitates working with raw pointers, as for instance bytemuck can't convert a byte slice to a user-defined DST.
Improvements and bug fixes to vulkano-shaders
Added a new type
Padded
, which aims to solve the following:[Padded<T, N>; M]
.[Padded<T, N>; M]
.Padded<T, N>
.Other fixes include:
The bigger picture
All the above together means that the shader interface now matches the Rust side exactly.
For example the following GLSL code:
Leads vulkano-shaders to generates this:
(Note the padding between
a
andb
.)And we can read/write the contents just fine. You can also see that the shader macro adds a
BufferContents
derive. This solves the issue of the user having to addbytemuck
to theirCargo.toml
for the sole purpose of using the shader macro. The user now also doesn't need to use bytemuck when they define their own structs, they can simply use our derive macro.Another example, showcasing how fields pad each other when necessary:
And like above,
BufferContents
is derived automatically for unsized types as well. The hope here is that by not generating auxilary fields, this will lead to less confusion from new users. They can read the docs ofPadded
to understand why its there, as well as get advice on how to mitigate the need for it.And another example, showcasing the use of
Padded
in arrays when necessary:Other miscallaneous changes
While working on breaking changes to shader struct generation, some other breaking changes were made:
ty
module was removed, so all types are now generated in the module in which theshader!
call resides. The reason being that the user can, and is already encouraged, to put theshader!
call in its own module, rendering thisty
module unnecessary. The user also has no control over the module generated by the macro, unlike when they make their own module, which manifests itself for example when they want to use derive macros imported in the outer module. Previously, thetypes_meta
option of the macro had the possibility to adduse
declarations in thety
module, which is now obsolete.types_meta
option of the macro previously also lead the macro to derive certain traits for the generated structs itself, ignoring the fields used for padding. Since these fields are no longer generated, andPadded
is used instead which is a passthrough wrapper, all this logic is also obsolete.types_meta
option enabled the user to derive certain traits which are either not supposed to be derived (Display
) or were being derived incorrectly (Eq
for structs containingf32
, which are notEq
). This functionality has also been removed. The user can deriveEq
for the structs if none of them containf32
s (orf64
s orf16
s), orimpl Eq
for select structs.Changelog:
Add:
Remove: