From f508cab5da055809895f50bbb661de814073016e Mon Sep 17 00:00:00 2001 From: marc0246 <40955683+marc0246@users.noreply.github.com> Date: Thu, 12 Sep 2024 10:27:00 +0200 Subject: [PATCH] Use the now-stabilized `pointer_bytes_offsets` instead of what is technically UB and clean up the `BufferContents` derive macro expansion (#2563) * Clean up `BufferContents` derive expansion * Use the now-stabilized `pointer_bytes_offsets` instead of technical UB --- Cargo.toml | 2 +- vulkano-macros/src/derive_buffer_contents.rs | 263 ++++++++----------- vulkano/src/buffer/subbuffer.rs | 182 ++++++------- vulkano/src/padded.rs | 9 +- 4 files changed, 196 insertions(+), 260 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index b5d1fbf5..15fb5194 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,7 +12,7 @@ resolver = "2" [workspace.package] edition = "2021" -rust-version = "1.72.0" +rust-version = "1.75.0" license = "MIT OR Apache-2.0" homepage = "https://vulkano.rs" keywords = ["vulkan", "bindings", "graphics", "gpu", "rendering"] diff --git a/vulkano-macros/src/derive_buffer_contents.rs b/vulkano-macros/src/derive_buffer_contents.rs index 15f8a751..378cf660 100644 --- a/vulkano-macros/src/derive_buffer_contents.rs +++ b/vulkano-macros/src/derive_buffer_contents.rs @@ -13,7 +13,7 @@ pub fn derive_buffer_contents(crate_ident: &Ident, mut ast: DeriveInput) -> Resu .attrs .iter() .filter(|&attr| attr.path().is_ident("repr")) - .map(|attr| { + .all(|attr| { let mut is_repr_rust = true; let _ = attr.parse_nested_meta(|meta| { @@ -25,8 +25,7 @@ pub fn derive_buffer_contents(crate_ident: &Ident, mut ast: DeriveInput) -> Resu }); is_repr_rust - }) - .all(|b| b); + }); if is_repr_rust { bail!( @@ -35,6 +34,18 @@ pub fn derive_buffer_contents(crate_ident: &Ident, mut ast: DeriveInput) -> Resu ); } + let data = match &ast.data { + Data::Struct(data) => data, + Data::Enum(_) => bail!("deriving `BufferContents` for enums is not supported"), + Data::Union(_) => bail!("deriving `BufferContents` for unions is not supported"), + }; + + let fields = match &data.fields { + Fields::Named(FieldsNamed { named, .. }) => named, + Fields::Unnamed(FieldsUnnamed { unnamed, .. }) => unnamed, + Fields::Unit => bail!("zero-sized types are not valid buffer contents"), + }; + let (impl_generics, type_generics, where_clause) = { let predicates = ast .generics @@ -52,30 +63,85 @@ pub fn derive_buffer_contents(crate_ident: &Ident, mut ast: DeriveInput) -> Resu ast.generics.split_for_impl() }; - let layout = write_layout(crate_ident, &ast)?; + let mut field_types = fields.iter().map(|field| &field.ty); + let Some(last_field_type) = field_types.next_back() else { + bail!("zero-sized types are not valid buffer contents"); + }; + let mut field_layouts = Vec::new(); - Ok(quote! { - #[allow(unsafe_code)] - unsafe impl #impl_generics ::#crate_ident::buffer::BufferContents - for #struct_ident #type_generics #where_clause - { - const LAYOUT: ::#crate_ident::buffer::BufferContentsLayout = #layout; + let mut bound_types = Vec::new(); - #[inline(always)] - unsafe fn ptr_from_slice(slice: ::std::ptr::NonNull<[u8]>) -> *mut Self { - #[repr(C)] - union PtrRepr { - components: PtrComponents, - ptr: *mut T, - } + // Accumulate the field layouts and types that have to implement `BufferContents` in order for + // the struct to implement the trait as well. + for field_type in field_types { + bound_types.push(find_innermost_element_type(field_type)); + field_layouts.push(quote! { ::std::alloc::Layout::new::<#field_type>() }); + } - #[derive(Clone, Copy)] - #[repr(C)] - struct PtrComponents { - data: *mut u8, - len: usize, - } + let layout; + let ptr_from_slice; + // The last field needs special treatment. + match last_field_type { + // An array might not implement `BufferContents` depending on the element. However, we know + // the type must be sized, so we can generate the layout and function easily. + Type::Array(TypeArray { elem, .. }) => { + bound_types.push(find_innermost_element_type(elem)); + + layout = quote! { + ::#crate_ident::buffer::BufferContentsLayout::from_sized( + ::std::alloc::Layout::new::() + ) + }; + + ptr_from_slice = quote! { + debug_assert_eq!(slice.len(), ::std::mem::size_of::()); + + <*mut [u8]>::cast::(slice.as_ptr()) + }; + } + // A slice might contain an array which might not implement `BufferContents`. However, we + // know the type must be unsized, so can generate the layout and function easily. + Type::Slice(TypeSlice { elem, .. }) => { + bound_types.push(find_innermost_element_type(elem)); + + layout = quote! { + ::#crate_ident::buffer::BufferContentsLayout::from_field_layouts( + &[ #( #field_layouts ),* ], + ::#crate_ident::buffer::BufferContentsLayout::from_slice( + ::std::alloc::Layout::new::<#elem>(), + ), + ) + }; + + ptr_from_slice = quote! { + let data = <*mut [u8]>::cast::<#elem>(slice.as_ptr()); + + let head_size = ::LAYOUT + .head_size() as usize; + let element_size = ::std::mem::size_of::<#elem>(); + + ::std::debug_assert!(slice.len() >= head_size); + let tail_size = slice.len() - head_size; + ::std::debug_assert!(tail_size % element_size == 0); + let len = tail_size / element_size; + + ::std::ptr::slice_from_raw_parts_mut(data, len) as *mut Self + }; + } + // Any other type may be either sized or unsized and the macro has no way of knowing. But + // it surely implements `BufferContents`, so we can use the existing layout and function. + ty => { + bound_types.push(ty); + + layout = quote! { + ::#crate_ident::buffer::BufferContentsLayout::from_field_layouts( + &[ #( #field_layouts ),* ], + <#last_field_type as ::#crate_ident::buffer::BufferContents>::LAYOUT, + ) + }; + + ptr_from_slice = quote! { let data = <*mut [u8]>::cast::(slice.as_ptr()); let head_size = ::LAYOUT @@ -87,101 +153,21 @@ pub fn derive_buffer_contents(crate_ident: &Ident, mut ast: DeriveInput) -> Resu ::std::debug_assert!(slice.len() >= head_size); let tail_size = slice.len() - head_size; ::std::debug_assert!(tail_size % element_size == 0); - let len = tail_size / element_size; - let components = PtrComponents { data, len }; - - // SAFETY: All fields must implement `BufferContents`. The last field, if it is - // unsized, must therefore be a slice or a DST derived from a slice. It cannot be - // any other kind of DST, unless unsafe code was used to achieve that. - // - // That means we can safely rely on knowing what kind of DST the implementing type - // is, but it doesn't tell us what the correct representation for the pointer of - // this kind of DST is. For that we have to rely on what the docs tell us, namely - // that for structs where the last field is a DST, the metadata is the same as the - // last field's. We also know that the metadata of a slice is its length measured - // in the number of elements. This tells us that the components of a pointer to the - // implementing type are the address to the start of the data, and a length. It - // still does not tell us what the representation of the pointer is though. - // - // In fact, there is no way to be certain that this representation is correct. - // *Theoretically* rustc could decide tomorrow that the metadata comes first and - // the address comes last, but the chance of that ever happening is zero. - // - // But what if the implementing type is actually sized? In that case this - // conversion will simply discard the length field, and only leave the pointer. - PtrRepr { components }.ptr - } - } - }) -} - -fn write_layout(crate_ident: &Ident, ast: &DeriveInput) -> Result { - let data = match &ast.data { - Data::Struct(data) => data, - Data::Enum(_) => bail!("deriving `BufferContents` for enums is not supported"), - Data::Union(_) => bail!("deriving `BufferContents` for unions is not supported"), - }; - - let fields = match &data.fields { - Fields::Named(FieldsNamed { named, .. }) => named, - Fields::Unnamed(FieldsUnnamed { unnamed, .. }) => unnamed, - Fields::Unit => bail!("zero-sized types are not valid buffer contents"), - }; - - let mut field_types = fields.iter().map(|field| &field.ty); - let last_field_type = field_types.next_back().unwrap(); - let mut layout = quote! { ::std::alloc::Layout::new::<()>() }; - - let mut bound_types = Vec::new(); - - // Construct the layout of the head and accumulate the types that have to implement - // `BufferContents` in order for the struct to implement the trait as well. - for field_type in field_types { - bound_types.push(find_innermost_element_type(field_type)); - - layout = quote! { - extend_layout(#layout, ::std::alloc::Layout::new::<#field_type>()) - }; - } - - // The last field needs special treatment. - match last_field_type { - // An array might not implement `BufferContents` depending on the element, and therefore we - // can't use `BufferContents::extend_from_layout` on it. - Type::Array(TypeArray { elem, .. }) => { - bound_types.push(find_innermost_element_type(elem)); - layout = quote! { - ::#crate_ident::buffer::BufferContentsLayout::from_sized( - ::std::alloc::Layout::new::() + <#last_field_type as ::#crate_ident::buffer::BufferContents>::ptr_from_slice( + ::std::ptr::NonNull::new_unchecked(::std::ptr::slice_from_raw_parts_mut( + data.add(head_size), + tail_size, + )), ) + .byte_sub(head_size) as *mut Self }; } - // A slice might contain an array same as above, and therefore we can't use - // `BufferContents::extend_from_layout` on it either. - Type::Slice(TypeSlice { elem, .. }) => { - bound_types.push(find_innermost_element_type(elem)); - layout = quote! { - ::#crate_ident::buffer::BufferContentsLayout::from_head_element_layout( - #layout, - ::std::alloc::Layout::new::<#elem>(), - ) - }; - } - ty => { - bound_types.push(ty); - layout = quote! { - <#last_field_type as ::#crate_ident::buffer::BufferContents>::LAYOUT - .extend_from_layout(&#layout) - }; - } - } - - let (impl_generics, _, where_clause) = ast.generics.split_for_impl(); + }; let bounds = bound_types.into_iter().map(|ty| { quote_spanned! { ty.span() => - { + const _: () = { // HACK: This works around Rust issue #48214, which makes it impossible to put // these bounds in the where clause of the trait implementation where they actually // belong until that is resolved. @@ -190,58 +176,25 @@ fn write_layout(crate_ident: &Ident, ast: &DeriveInput) -> Result { fn assert_impl() {} assert_impl::<#ty>(); } - } + }; } }); - let layout = quote! { + Ok(quote! { + #[allow(unsafe_code)] + unsafe impl #impl_generics ::#crate_ident::buffer::BufferContents + for #struct_ident #type_generics #where_clause { - #( #bounds )* + const LAYOUT: ::#crate_ident::buffer::BufferContentsLayout = #layout; - // HACK: Very depressingly, `Layout::extend` is not const. - const fn extend_layout( - layout: ::std::alloc::Layout, - next: ::std::alloc::Layout, - ) -> ::std::alloc::Layout { - let padded_size = if let Some(val) = - layout.size().checked_add(next.align() - 1) - { - val & !(next.align() - 1) - } else { - ::std::unreachable!() - }; - - // TODO: Replace with `Ord::max` once its constness is stabilized. - let align = if layout.align() >= next.align() { - layout.align() - } else { - next.align() - }; - - if let Some(size) = padded_size.checked_add(next.size()) { - if let Ok(layout) = ::std::alloc::Layout::from_size_align(size, align) { - layout - } else { - ::std::unreachable!() - } - } else { - ::std::unreachable!() - } - } - - if let Some(layout) = #layout { - if let Some(layout) = layout.pad_to_alignment() { - layout - } else { - ::std::unreachable!() - } - } else { - ::std::panic!("zero-sized types are not valid buffer contents") + #[inline(always)] + unsafe fn ptr_from_slice(slice: ::std::ptr::NonNull<[u8]>) -> *mut Self { + #ptr_from_slice } } - }; - Ok(layout) + #( #bounds )* + }) } // HACK: This works around an inherent limitation of bytemuck, namely that an array where the diff --git a/vulkano/src/buffer/subbuffer.rs b/vulkano/src/buffer/subbuffer.rs index 12c1b73e..85a42fa0 100644 --- a/vulkano/src/buffer/subbuffer.rs +++ b/vulkano/src/buffer/subbuffer.rs @@ -471,7 +471,7 @@ impl Subbuffer { impl Subbuffer<[T]> { /// Returns the number of elements in the slice. pub fn len(&self) -> DeviceSize { - debug_assert!(self.size % size_of::() as DeviceSize == 0); + debug_assert_eq!(self.size % size_of::() as DeviceSize, 0); self.size / size_of::() as DeviceSize } @@ -830,16 +830,11 @@ unsafe impl BufferContents for T where T: AnyBitPattern + Send + Sync, { - const LAYOUT: BufferContentsLayout = - if let Some(layout) = BufferContentsLayout::from_sized(Layout::new::()) { - layout - } else { - panic!("zero-sized types are not valid buffer contents"); - }; + const LAYOUT: BufferContentsLayout = BufferContentsLayout::from_sized(Layout::new::()); #[inline(always)] unsafe fn ptr_from_slice(slice: NonNull<[u8]>) -> *mut Self { - debug_assert!(slice.len() == size_of::()); + debug_assert_eq!(slice.len(), size_of::()); <*mut [u8]>::cast::(slice.as_ptr()) } @@ -849,16 +844,13 @@ unsafe impl BufferContents for [T] where T: BufferContents, { - const LAYOUT: BufferContentsLayout = BufferContentsLayout(BufferContentsLayoutInner::Unsized { - head_layout: None, - element_layout: T::LAYOUT.unwrap_sized(), - }); + const LAYOUT: BufferContentsLayout = BufferContentsLayout::from_slice(Layout::new::()); #[inline(always)] unsafe fn ptr_from_slice(slice: NonNull<[u8]>) -> *mut Self { let data = <*mut [u8]>::cast::(slice.as_ptr()); let len = slice.len() / size_of::(); - debug_assert!(slice.len() % size_of::() == 0); + debug_assert_eq!(slice.len() % size_of::(), 0); ptr::slice_from_raw_parts_mut(data, len) } @@ -961,116 +953,112 @@ impl BufferContentsLayout { /// derive macro only. #[doc(hidden)] #[inline] - pub const fn from_sized(sized: Layout) -> Option { + pub const fn from_sized(sized: Layout) -> Self { assert!( sized.align() <= 64, "types with alignments above 64 are not valid buffer contents", ); if let Ok(sized) = DeviceLayout::from_layout(sized) { - Some(Self(BufferContentsLayoutInner::Sized(sized))) + Self(BufferContentsLayoutInner::Sized(sized)) } else { - None + unreachable!() } } - /// Creates a new `BufferContentsLayout` from a head and element layout. This is intended for - /// use by the derive macro only. + /// Creates a new `BufferContentsLayout` from an element layout. This is intended for use by + /// the derive macro only. #[doc(hidden)] #[inline] - pub const fn from_head_element_layout( - head_layout: Layout, - element_layout: Layout, - ) -> Option { - if head_layout.align() > 64 || element_layout.align() > 64 { - panic!("types with alignments above 64 are not valid buffer contents"); - } - - // The head of a `BufferContentsLayout` can be zero-sized. - // TODO: Replace with `Result::ok` once its constness is stabilized. - let head_layout = if let Ok(head_layout) = DeviceLayout::from_layout(head_layout) { - Some(head_layout) - } else { - None - }; - - if let Ok(element_layout) = DeviceLayout::from_layout(element_layout) { - Some(Self(BufferContentsLayoutInner::Unsized { - head_layout, - element_layout, - })) - } else { - None - } - } - - /// Extends the given `previous` [`Layout`] by `self`. This is intended for use by the derive - /// macro only. - #[doc(hidden)] - #[inline] - pub const fn extend_from_layout(self, previous: &Layout) -> Option { + pub const fn from_slice(element_layout: Layout) -> Self { assert!( - previous.align() <= 64, + element_layout.align() <= 64, "types with alignments above 64 are not valid buffer contents", ); - match self.0 { - BufferContentsLayoutInner::Sized(sized) => { - let (sized, _) = try_opt!(sized.extend_from_layout(previous)); - - Some(Self(BufferContentsLayoutInner::Sized(sized))) - } - BufferContentsLayoutInner::Unsized { + if let Ok(element_layout) = DeviceLayout::from_layout(element_layout) { + Self(BufferContentsLayoutInner::Unsized { head_layout: None, element_layout, - } => { - // The head of a `BufferContentsLayout` can be zero-sized. - // TODO: Replace with `Result::ok` once its constness is stabilized. - let head_layout = if let Ok(head_layout) = DeviceLayout::from_layout(*previous) { - Some(head_layout) - } else { - None - }; + }) + } else { + unreachable!() + } + } - Some(Self(BufferContentsLayoutInner::Unsized { - head_layout, - element_layout, - })) - } - BufferContentsLayoutInner::Unsized { - head_layout: Some(head_layout), - element_layout, - } => { - let (head_layout, _) = try_opt!(head_layout.extend_from_layout(previous)); - - Some(Self(BufferContentsLayoutInner::Unsized { - head_layout: Some(head_layout), - element_layout, - })) + /// Creates a new `BufferContentsLayout` from the given field layouts. This is intended for use + /// by the derive macro only. + #[doc(hidden)] + #[inline] + pub const fn from_field_layouts(field_layouts: &[Layout], last_field_layout: Self) -> Self { + const fn extend(previous: DeviceLayout, next: DeviceLayout) -> DeviceLayout { + match previous.extend(next) { + Some((layout, _)) => layout, + None => unreachable!(), } } + + let mut head_layout = None; + let mut i = 0; + + while i < field_layouts.len() { + head_layout = match DeviceLayout::from_layout(field_layouts[i]) { + Ok(field_layout) => Some(match head_layout { + Some(layout) => extend(layout, field_layout), + None => field_layout, + }), + Err(_) => unreachable!(), + }; + + i += 1; + } + + let layout = Self(match last_field_layout.0 { + BufferContentsLayoutInner::Sized(field_layout) => { + BufferContentsLayoutInner::Sized(match head_layout { + Some(layout) => extend(layout, field_layout), + None => field_layout, + }) + } + BufferContentsLayoutInner::Unsized { + head_layout: field_head_layout, + element_layout, + } => BufferContentsLayoutInner::Unsized { + head_layout: match (head_layout, field_head_layout) { + (Some(layout), Some(field_layout)) => Some(extend(layout, field_layout)), + (Some(layout), None) => Some(layout), + (None, Some(field_layout)) => Some(field_layout), + (None, None) => None, + }, + element_layout, + }, + }); + + assert!( + layout.alignment().as_devicesize() <= 64, + "types with alignments above 64 are not valid buffer contents", + ); + + layout.pad_to_alignment() } /// Creates a new `BufferContentsLayout` by rounding up the size of the head to the nearest /// multiple of its alignment if the layout is sized, or by rounding up the size of the head to /// the nearest multiple of the alignment of the element type and aligning the head to the /// alignment of the element type if there is a sized part. Doesn't do anything if there is no - /// sized part. Returns [`None`] if the new head size would exceed [`DeviceLayout::MAX_SIZE`]. - /// This is intended for use by the derive macro only. - #[doc(hidden)] - #[inline] - pub const fn pad_to_alignment(&self) -> Option { - match &self.0 { - BufferContentsLayoutInner::Sized(sized) => Some(Self( - BufferContentsLayoutInner::Sized(sized.pad_to_alignment()), - )), + /// sized part. + const fn pad_to_alignment(&self) -> Self { + Self(match &self.0 { + BufferContentsLayoutInner::Sized(sized) => { + BufferContentsLayoutInner::Sized(sized.pad_to_alignment()) + } BufferContentsLayoutInner::Unsized { head_layout: None, element_layout, - } => Some(Self(BufferContentsLayoutInner::Unsized { + } => BufferContentsLayoutInner::Unsized { head_layout: None, element_layout: *element_layout, - })), + }, BufferContentsLayoutInner::Unsized { head_layout: Some(head_layout), element_layout, @@ -1109,15 +1097,15 @@ impl BufferContentsLayout { DeviceAlignment::max(head_layout.alignment(), element_layout.alignment()); if let Some(head_layout) = DeviceLayout::new(padded_head_size, alignment) { - Some(Self(BufferContentsLayoutInner::Unsized { + BufferContentsLayoutInner::Unsized { head_layout: Some(head_layout), element_layout: *element_layout, - })) + } } else { - None + unreachable!() } } - } + }) } fn is_sized(&self) -> bool { @@ -1169,7 +1157,7 @@ mod tests { #[derive(BufferContents)] #[repr(C)] - struct Composite1(Test1, [f32; 10], Test1); + struct Composite1(Test1, [f32; 9], Test1); assert_eq!( Composite1::LAYOUT.head_size() as usize, @@ -1200,7 +1188,7 @@ mod tests { #[derive(BufferContents)] #[repr(C)] - struct Composite2(Test1, [f32; 10], Test2); + struct Composite2(Test1, [f32; 9], Test2); assert_eq!( Composite2::LAYOUT.head_size() as usize, diff --git a/vulkano/src/padded.rs b/vulkano/src/padded.rs index 63cc7893..4f3d3493 100644 --- a/vulkano/src/padded.rs +++ b/vulkano/src/padded.rs @@ -287,15 +287,10 @@ unsafe impl BufferContents for Padded where T: BufferContents, { - const LAYOUT: BufferContentsLayout = - if let Some(layout) = BufferContentsLayout::from_sized(Layout::new::()) { - layout - } else { - panic!("zero-sized types are not valid buffer contents"); - }; + const LAYOUT: BufferContentsLayout = BufferContentsLayout::from_sized(Layout::new::()); unsafe fn ptr_from_slice(slice: NonNull<[u8]>) -> *mut Self { - debug_assert!(slice.len() == size_of::>()); + debug_assert_eq!(slice.len(), size_of::>()); <*mut [u8]>::cast::>(slice.as_ptr()) }