Skip to content

Commit

Permalink
Use the now-stabilized pointer_bytes_offsets instead of what is tec…
Browse files Browse the repository at this point in the history 
…hnically 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
  • Loading branch information
@marc0246
marc0246 authored Sep 12, 2024
1 parent a0b909f commit f508cab
Show file tree
Hide file tree
Showing 4 changed files with 179 additions and 243 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down
243 changes: 98 additions & 145 deletions vulkano-macros/src/derive_buffer_contents.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand All @@ -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!(
Expand All @@ -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
Expand All @@ -52,136 +63,111 @@ pub fn derive_buffer_contents(crate_ident: &Ident, mut ast: DeriveInput) -> Resu
ast.generics.split_for_impl()
};

let layout = write_layout(crate_ident, &ast)?;

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;

#[inline(always)]
unsafe fn ptr_from_slice(slice: ::std::ptr::NonNull<[u8]>) -> *mut Self {
#[repr(C)]
union PtrRepr<T: ?Sized> {
components: PtrComponents,
ptr: *mut T,
}

#[derive(Clone, Copy)]
#[repr(C)]
struct PtrComponents {
data: *mut u8,
len: usize,
}

let data = <*mut [u8]>::cast::<u8>(slice.as_ptr());

let head_size = <Self as ::#crate_ident::buffer::BufferContents>::LAYOUT
.head_size() as usize;
let element_size = <Self as ::#crate_ident::buffer::BufferContents>::LAYOUT
.element_size()
.unwrap_or(1) as usize;

::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<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 Some(last_field_type) = field_types.next_back() else {
bail!("zero-sized types are not valid buffer contents");
};
let mut field_layouts = Vec::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.
// 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));

layout = quote! {
extend_layout(#layout, ::std::alloc::Layout::new::<#field_type>())
};
field_layouts.push(quote! { ::std::alloc::Layout::new::<#field_type>() });
}

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, and therefore we
// can't use `BufferContents::extend_from_layout` on it.
// 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 same as above, and therefore we can't use
// `BufferContents::extend_from_layout` on it either.
// 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_head_element_layout(
#layout,
::std::alloc::Layout::new::<#elem>(),
::#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! {
<#last_field_type as ::#crate_ident::buffer::BufferContents>::LAYOUT
.extend_from_layout(&#layout)
::#crate_ident::buffer::BufferContentsLayout::from_field_layouts(
&[ #( #field_layouts ),* ],
<#last_field_type as ::#crate_ident::buffer::BufferContents>::LAYOUT,
)
};
}
}

let (impl_generics, _, where_clause) = ast.generics.split_for_impl();
ptr_from_slice = quote! {
let data = <*mut [u8]>::cast::<u8>(slice.as_ptr());

let head_size = <Self as ::#crate_ident::buffer::BufferContents>::LAYOUT
.head_size() as usize;
let element_size = <Self as ::#crate_ident::buffer::BufferContents>::LAYOUT
.element_size()
.unwrap_or(1) as usize;

::std::debug_assert!(slice.len() >= head_size);
let tail_size = slice.len() - head_size;
::std::debug_assert!(tail_size % element_size == 0);

<#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
};
}
};

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.
Expand All @@ -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 )*

// 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!()
}
}
const LAYOUT: ::#crate_ident::buffer::BufferContentsLayout = #layout;

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
Expand Down
Loading

0 comments on commit f508cab

Please sign in to comment.