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

@@ -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"]

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! {
+    let mut bound_types = Vec::new();
-        #[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;
 
-            #[inline(always)]
+    // Accumulate the field layouts and types that have to implement `BufferContents` in order for
-            unsafe fn ptr_from_slice(slice: ::std::ptr::NonNull<[u8]>) -> *mut Self {
+    // the struct to implement the trait as well.
-                #[repr(C)]
+    for field_type in field_types {
-                union PtrRepr<T: ?Sized> {
+        bound_types.push(find_innermost_element_type(field_type));
-                    components: PtrComponents,
+        field_layouts.push(quote! { ::std::alloc::Layout::new::<#field_type>() });
-                    ptr: *mut T,
+    }
-                }
 
-                #[derive(Clone, Copy)]
+    let layout;
-                #[repr(C)]
+    let ptr_from_slice;
-                struct PtrComponents {
-                    data: *mut u8,
-                    len: usize,
-                }
 
+    // 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::<Self>()
+                )
+            };
+
+            ptr_from_slice = quote! {
+                debug_assert_eq!(slice.len(), ::std::mem::size_of::<Self>());
+
+                <*mut [u8]>::cast::<Self>(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 = <Self as ::#crate_ident::buffer::BufferContents>::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::<u8>(slice.as_ptr());
 
                 let head_size = <Self as ::#crate_ident::buffer::BufferContents>::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 };
+                <#last_field_type as ::#crate_ident::buffer::BufferContents>::ptr_from_slice(
-
+                    ::std::ptr::NonNull::new_unchecked(::std::ptr::slice_from_raw_parts_mut(
-                // SAFETY: All fields must implement `BufferContents`. The last field, if it is
+                        data.add(head_size),
-                // unsized, must therefore be a slice or a DST derived from a slice. It cannot be
+                        tail_size,
-                // 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<TokenStream> {
-    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::<Self>()
                 )
+                .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<TokenStream> {
                     fn assert_impl<T: ::#crate_ident::buffer::BufferContents + ?Sized>() {}
                     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.
+            #[inline(always)]
-            const fn extend_layout(
+            unsafe fn ptr_from_slice(slice: ::std::ptr::NonNull<[u8]>) -> *mut Self {
-                layout: ::std::alloc::Layout,
+                #ptr_from_slice
-                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")
             }
         }
-    };
 
-    Ok(layout)
+        #( #bounds )*
+    })
 }
 
 // HACK: This works around an inherent limitation of bytemuck, namely that an array where the

vulkano/src/buffer/subbuffer.rs

@@ -471,7 +471,7 @@ impl<T> Subbuffer<T> {
 impl<T> Subbuffer<[T]> {
     /// Returns the number of elements in the slice.
     pub fn len(&self) -> DeviceSize {
-        debug_assert!(self.size % size_of::<T>() as DeviceSize == 0);
+        debug_assert_eq!(self.size % size_of::<T>() as DeviceSize, 0);
 
         self.size / size_of::<T>() as DeviceSize
     }
@@ -830,16 +830,11 @@ unsafe impl<T> BufferContents for T
 where
     T: AnyBitPattern + Send + Sync,
 {
-    const LAYOUT: BufferContentsLayout =
+    const LAYOUT: BufferContentsLayout = BufferContentsLayout::from_sized(Layout::new::<T>());
-        if let Some(layout) = BufferContentsLayout::from_sized(Layout::new::<T>()) {
-            layout
-        } else {
-            panic!("zero-sized types are not valid buffer contents");
-        };
 
     #[inline(always)]
     unsafe fn ptr_from_slice(slice: NonNull<[u8]>) -> *mut Self {
-        debug_assert!(slice.len() == size_of::<T>());
+        debug_assert_eq!(slice.len(), size_of::<T>());
 
         <*mut [u8]>::cast::<T>(slice.as_ptr())
     }
@@ -849,16 +844,13 @@ unsafe impl<T> BufferContents for [T]
 where
     T: BufferContents,
 {
-    const LAYOUT: BufferContentsLayout = BufferContentsLayout(BufferContentsLayoutInner::Unsized {
+    const LAYOUT: BufferContentsLayout = BufferContentsLayout::from_slice(Layout::new::<T>());
-        head_layout: None,
-        element_layout: T::LAYOUT.unwrap_sized(),
-    });
 
     #[inline(always)]
     unsafe fn ptr_from_slice(slice: NonNull<[u8]>) -> *mut Self {
         let data = <*mut [u8]>::cast::<T>(slice.as_ptr());
         let len = slice.len() / size_of::<T>();
-        debug_assert!(slice.len() % size_of::<T>() == 0);
+        debug_assert_eq!(slice.len() % size_of::<T>(), 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<Self> {
+    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
+    /// Creates a new `BufferContentsLayout` from an element layout. This is intended for use by
-    /// use by the derive macro only.
+    /// the derive macro only.
     #[doc(hidden)]
     #[inline]
-    pub const fn from_head_element_layout(
+    pub const fn from_slice(element_layout: Layout) -> Self {
-        head_layout: Layout,
-        element_layout: Layout,
-    ) -> Option<Self> {
-        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<Self> {
         assert!(
-            previous.align() <= 64,
+            element_layout.align() <= 64,
             "types with alignments above 64 are not valid buffer contents",
         );
 
-        match self.0 {
+        if let Ok(element_layout) = DeviceLayout::from_layout(element_layout) {
-            BufferContentsLayoutInner::Sized(sized) => {
+            Self(BufferContentsLayoutInner::Unsized {
-                let (sized, _) = try_opt!(sized.extend_from_layout(previous));
-
-                Some(Self(BufferContentsLayoutInner::Sized(sized)))
-            }
-            BufferContentsLayoutInner::Unsized {
                 head_layout: None,
                 element_layout,
-            } => {
+            })
-                // The head of a `BufferContentsLayout` can be zero-sized.
+        } else {
-                // TODO: Replace with `Result::ok` once its constness is stabilized.
+            unreachable!()
-                let head_layout = if let Ok(head_layout) = DeviceLayout::from_layout(*previous) {
+        }
-                    Some(head_layout)
+    }
-                } else {
-                    None
-                };
 
-                Some(Self(BufferContentsLayoutInner::Unsized {
+    /// Creates a new `BufferContentsLayout` from the given field layouts. This is intended for use
-                    head_layout,
+    /// by the derive macro only.
-                    element_layout,
+    #[doc(hidden)]
-                }))
+    #[inline]
-            }
+    pub const fn from_field_layouts(field_layouts: &[Layout], last_field_layout: Self) -> Self {
-            BufferContentsLayoutInner::Unsized {
+        const fn extend(previous: DeviceLayout, next: DeviceLayout) -> DeviceLayout {
-                head_layout: Some(head_layout),
+            match previous.extend(next) {
-                element_layout,
+                Some((layout, _)) => layout,
-            } => {
+                None => unreachable!(),
-                let (head_layout, _) = try_opt!(head_layout.extend_from_layout(previous));
-
-                Some(Self(BufferContentsLayoutInner::Unsized {
-                    head_layout: Some(head_layout),
-                    element_layout,
-                }))
             }
         }
+
+        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`].
+    /// sized part.
-    /// This is intended for use by the derive macro only.
+    const fn pad_to_alignment(&self) -> Self {
-    #[doc(hidden)]
+        Self(match &self.0 {
-    #[inline]
+            BufferContentsLayoutInner::Sized(sized) => {
-    pub const fn pad_to_alignment(&self) -> Option<Self> {
+                BufferContentsLayoutInner::Sized(sized.pad_to_alignment())
-        match &self.0 {
+            }
-            BufferContentsLayoutInner::Sized(sized) => Some(Self(
-                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,

vulkano/src/padded.rs

@@ -287,15 +287,10 @@ unsafe impl<T, const N: usize> BufferContents for Padded<T, N>
 where
     T: BufferContents,
 {
-    const LAYOUT: BufferContentsLayout =
+    const LAYOUT: BufferContentsLayout = BufferContentsLayout::from_sized(Layout::new::<Self>());
-        if let Some(layout) = BufferContentsLayout::from_sized(Layout::new::<Self>()) {
-            layout
-        } else {
-            panic!("zero-sized types are not valid buffer contents");
-        };
 
     unsafe fn ptr_from_slice(slice: NonNull<[u8]>) -> *mut Self {
-        debug_assert!(slice.len() == size_of::<Padded<T, N>>());
+        debug_assert_eq!(slice.len(), size_of::<Padded<T, N>>());
 
         <*mut [u8]>::cast::<Padded<T, N>>(slice.as_ptr())
     }