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

Resolve restrictions of BufferContents, add support for allocating all types of buffers, and rework vulkano-shaders #2132

Merged
merged 36 commits into from
Mar 5, 2023

Conversation

marc0246
Copy link
Contributor

@marc0246 marc0246 commented Jan 17, 2023

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 from Pod to AnyBitPattern. 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 deriving AnyBitPattern or BufferContents 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 implementing BufferContents. 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 a BufferContents derive macro was added to safely implement the trait.

Here's an example of a user-defined DST:

#[derive(BufferContents)]
#[repr(C)]
struct Test {
    x: u32,
    y: u32,
    slice: [u32],
}

The memory for which can simply be allocated using Buffer::new_unsized or SubbufferAllocator::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:

#[derive(BufferContents)]
#[repr(C)]
struct Test<T, U> {
    x: T,
    y: T,
    slice: [U],
}

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:

  • Previously, arrays whose stride is greater than the size of the element in Rust were rejected by vulkano-shaders. These are now accepted and generate as [Padded<T, N>; M].
  • Matrices whose stride exceeds the size of the column/row in Rust are now generated correctly, also as [Padded<T, N>; M].
  • There are no more mystery fields generated, instead struct fields pad eachother using Padded<T, N>.

Other fixes include:

  • Matrix majorness is now taken into consideration when generating the types.

The bigger picture

All the above together means that the shader interface now matches the Rust side exactly.

For example the following GLSL code:

layout(binding = 0) uniform Test {
    int a;
    double b;
};

Leads vulkano-shaders to generates this:

#[allow(non_camel_case_types, non_snake_case)]
#[derive(::vulkano::buffer::BufferContents, ::std::clone::Clone, ::std::marker::Copy)]
#[repr(C)]
pub struct Test {
    pub a: i32,
    pub b: f64,
}

(Note the padding between a and b.)
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 add bytemuck to their Cargo.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:

layout(binding = 0) buffer Test {
    int x;
    vec3 y;
    float z[];
};
#[allow(non_camel_case_types, non_snake_case)]
#[derive(::vulkano::buffer::BufferContents)]
#[repr(C)]
pub struct Test {
    pub x: ::vulkano::padded::Padded<i32, 12usize>,
    pub y: [f32; 3usize],
    pub z: [f32],
}

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 of Padded 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:

struct Test {
    float a;
};

layout(binding = 0) buffer Test1 {
    Test a[4];
};

layout(binding = 1) uniform Test2 {
    Test b[4];
};
#[allow(non_camel_case_types, non_snake_case)]
#[derive(::vulkano::buffer::BufferContents, ::std::clone::Clone, ::std::marker::Copy)]
#[repr(C)]
pub struct Test {
    pub a: f32,
}
#[allow(non_camel_case_types, non_snake_case)]
#[derive(::vulkano::buffer::BufferContents, ::std::clone::Clone, ::std::marker::Copy)]
#[repr(C)]
pub struct Test1 {
    pub a: [Test; 4usize],
}
#[allow(non_camel_case_types, non_snake_case)]
#[derive(::vulkano::buffer::BufferContents, ::std::clone::Clone, ::std::marker::Copy)]
#[repr(C)]
pub struct Test2 {
    pub b: [::vulkano::padded::Padded<Test, 12usize>; 4usize],
}

Other miscallaneous changes

While working on breaking changes to shader struct generation, some other breaking changes were made:

  • The generation of the ty module was removed, so all types are now generated in the module in which the shader! call resides. The reason being that the user can, and is already encouraged, to put the shader! call in its own module, rendering this ty 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, the types_meta option of the macro had the possibility to add use declarations in the ty module, which is now obsolete.
  • The 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, and Padded is used instead which is a passthrough wrapper, all this logic is also obsolete.
  • The 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 containing f32, which are not Eq). This functionality has also been removed. The user can derive Eq for the structs if none of them contain f32s (or f64s or f16s), or impl Eq for select structs.

Changelog:
Add:

### Breaking changes
- All methonds on `BufferContents` have been replaced.
- Vulkano-shaders: Struct fields are now padded using `Padded`, instead of generating additional fields.
- Vulkano-shaders: The `types_meta` option has been removed in favor of `custom_derives`. Additionally, a derive for `BufferContents` is automatically added to the generated structs, so there is no need to specify anything in order to use the structs in buffers.
- Vulkano-shaders: The `ty` module is no longer generated. All types are generated in the same module where the macro call resides.

### Additions
- Added a `BufferContents` derive macro. You no longer need to use `bytemuck` to read/write your structs form/to buffers.
- Added `BufferContentsLayout`.
- Added support for allocating buffers with any kind of unsized contents, not just slices. These can also be read/written from the host.
- Added `Padded`, which can be used to pad struct fields as well as array elements/matrix columns.
- Vulkano-shaders: All error messages are now appropriately spanned compile-errors, instead of being panics.
- Vulkano-shaders: Added support for arrays (and by extension matrices) whose stride exceeds the size of the element type in Rust (e.g. `vec3[]`).
- Vulkano-shaders: Added a `linalg_type` option to the macro, for generating types from external linear algebra crates.

### Bugs fixed
- Vulkano-shaders: `mat3xN` is now generated correctly, taking its matrix stride into account.
- Vulkano-shaders: Row-major matrices are now generated correctly.

Remove:

- Vulkano-shaders: shader_cgmath and shader_nalgebra macros, next to the existing shader macro.
- Vulkano-shaders: The bytemuck traits `Pod` and `Zeroable` are now automatically derived for generated structs, so there is no need to specify them with `types_meta` anymore.

@marc0246 marc0246 marked this pull request as draft January 17, 2023 14:12
@Rua
Copy link
Contributor

Rua commented Jan 17, 2023

Can you explain exactly why it is not unsound for the user to provide a value that contains uninitialised bytes?

@marc0246
Copy link
Contributor Author

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.

@Rua
Copy link
Contributor

Rua commented Jan 17, 2023

That's if you're interpreting Vulkan mapped memory. But what about user-provided data, like writing to a host buffer, specialisation constants, update_buffer, push_constants etc?

@marc0246
Copy link
Contributor Author

marc0246 commented Jan 17, 2023

That's what I meant as well. As long as no reference to the padding bytes exists in Rust, there is no UB.

@marc0246
Copy link
Contributor Author

The compiler by definition can't exploit UB across FFI boudaries.

@Rua
Copy link
Contributor

Rua commented Jan 17, 2023

In the case of writing to a host buffer from user-provided data, a reference does exist, since the copy is done within Rust.

@marc0246
Copy link
Contributor Author

I'm not sure I entirely understand - we can have a reference to a type that has padding in Rust.

@Rua
Copy link
Contributor

Rua commented Jan 17, 2023

Ok, I think I understand. But I still feel like there's some edge case that we haven't thought of yet...

@marc0246
Copy link
Contributor Author

I agree it's scary, but I hope I documented all the unsafe well enough, at least I really tried.

@marc0246 marc0246 changed the title Resolve restrictions of BufferContents, add support for allocating all types of buffers, and other QoL improvements Resolve restrictions of BufferContents, add support for allocating all types of buffers, and rework vulkano-shaders Feb 7, 2023
@marc0246 marc0246 marked this pull request as ready for review February 7, 2023 15:47
@marc0246
Copy link
Contributor Author

marc0246 commented Feb 7, 2023

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.

@marc0246
Copy link
Contributor Author

I updated the description again now, hopefully it conveys all the changes.

@marc0246
Copy link
Contributor Author

I added another section to the description - this time it should be complete I hope.

@Rua
Copy link
Contributor

Rua commented Mar 5, 2023

Can you fix the conflicts?

@marc0246
Copy link
Contributor Author

marc0246 commented Mar 5, 2023

Ah, thanks, didn't notice. Done now.

@Rua Rua merged commit baf863c into vulkano-rs:master Mar 5, 2023
Rua added a commit that referenced this pull request Mar 5, 2023
@marc0246 marc0246 deleted the custom-dsts branch March 17, 2023 01:35
@yshui yshui mentioned this pull request Mar 30, 2023
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
…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
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 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
2 participants