forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Pointerfication followup: Type safety and cleanup (#4621)

Browse Source 
# Objective
The `Ptr` types gives free access to the underlying `NonNull<u8>`, which adds more publicly visible pointer wrangling than there needs to be. There are also a few edge cases where Ptr types could be more readily utilized for properly validating the soundness of ECS operations.

## Solution
 - Replace `*Ptr(Mut)::inner` with `cast` which requires a concrete type to give the pointer. This function could also have a `debug_assert` with an alignment check to ensure that the pointer is aligned properly, but is currently not included.
 - Use `OwningPtr::read` in ECS macros over casting the inner pointer around.
This commit is contained in:
James Liu 2022-05-03 20:07:58 +00:00
parent 4a9932fa8e
commit 3e24b725af
5 changed files with 91 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
crates/bevy_ecs/macros/src/lib.rs
View File

@ -134,7 +134,7 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
#ecs_path::ptr::OwningPtr::make(self.#field, &mut func);
});
field_from_components.push(quote! {
#field: func(ctx).inner().as_ptr().cast::<#field_type>().read(),
#field: func(ctx).read::<#field_type>(),
});
}
}

5
crates/bevy_ecs/src/bundle.rs
View File

@ -112,10 +112,7 @@ macro_rules! tuple_impl {
F: FnMut(&mut T) -> OwningPtr<'_>
{
#[allow(non_snake_case)]
let ($(mut $name,)*) = (
$(func(ctx).inner().cast::<$name>(),)*
);
($($name.as_ptr().read(),)*)
($(func(ctx).read::<$name>(),)*)
}
#[allow(unused_variables, unused_mut)]

2
crates/bevy_ecs/src/component.rs
View File

@ -186,7 +186,7 @@ impl std::fmt::Debug for ComponentDescriptor {
impl ComponentDescriptor {
// SAFETY: The pointer points to a valid value of type `T` and it is safe to drop this value.
unsafe fn drop_ptr<T>(x: OwningPtr<'_>) {
x.inner().cast::<T>().as_ptr().drop_in_place()
x.drop_as::<T>()
}
pub fn new<T: Component>() -> Self {

90
crates/bevy_ecs/src/ptr.rs
View File

@ -43,57 +43,86 @@ pub struct OwningPtr<'a>(NonNull<u8>, PhantomData<&'a mut u8>);
macro_rules! impl_ptr {
($ptr:ident) => {
impl $ptr<'_> {
/// Calculates the offset from a pointer.
/// As the pointer is type-erased, there is no size information available. The provided
/// `count` parameter is in raw bytes.
///
/// *See also: [`ptr::offset`][ptr_offset]*
///
/// # Safety
/// the offset cannot make the existing ptr null, or take it out of bounds for its allocation.
///
/// [ptr_offset]: https://doc.rust-lang.org/std/primitive.pointer.html#method.offset
#[inline]
pub unsafe fn offset(self, count: isize) -> Self {
Self(
NonNull::new_unchecked(self.0.as_ptr().offset(count)),
NonNull::new_unchecked(self.as_ptr().offset(count)),
PhantomData,
)
}
/// Calculates the offset from a pointer (convenience for `.offset(count as isize)`).
/// As the pointer is type-erased, there is no size information available. The provided
/// `count` parameter is in raw bytes.
///
/// *See also: [`ptr::add`][ptr_add]*
///
/// # Safety
/// the offset cannot make the existing ptr null, or take it out of bounds for its allocation.
///
/// [ptr_add]: https://doc.rust-lang.org/std/primitive.pointer.html#method.add
#[inline]
pub unsafe fn add(self, count: usize) -> Self {
Self(
NonNull::new_unchecked(self.0.as_ptr().add(count)),
NonNull::new_unchecked(self.as_ptr().add(count)),
PhantomData,
)
}
/// # Safety
/// Creates a new instance from a raw pointer.
///
/// # Safety
/// The lifetime for the returned item must not exceed the lifetime `inner` is valid for
#[inline]
pub unsafe fn new(inner: NonNull<u8>) -> Self {
Self(inner, PhantomData)
}
#[inline]
pub fn inner(&self) -> NonNull<u8> {
self.0
}
}
};
}
impl_ptr!(Ptr);
impl<'a> Ptr<'a> {
/// # Safety
/// Transforms this [`Ptr`] into an [`PtrMut`]
///
/// # Safety
/// Another [`PtrMut`] for the same [`Ptr`] must not be created until the first is dropped.
#[inline]
pub unsafe fn assert_unique(self) -> PtrMut<'a> {
PtrMut(self.0, PhantomData)
}
/// Transforms this [`Ptr<T>`] into a `&T` with the same lifetime
///
/// # Safety
/// Must point to a valid `T`
#[inline]
pub unsafe fn deref<T>(self) -> &'a T {
&*self.0.as_ptr().cast()
&*self.as_ptr().cast()
}
/// Gets the underlying pointer, erasing the associated lifetime.
///
/// If possible, it is strongly encouraged to use [`deref`](Self::deref) over this function,
/// as it retains the lifetime.
///
/// # Safety
/// All subsequent operations to the returned pointer must be valid inside the
/// associated lifetime.
#[inline]
#[allow(clippy::wrong_self_convention)]
pub unsafe fn as_ptr(self) -> *mut u8 {
self.0.as_ptr()
}
}
impl_ptr!(PtrMut);
@ -113,7 +142,21 @@ impl<'a> PtrMut<'a> {
/// Must point to a valid `T`
#[inline]
pub unsafe fn deref_mut<T>(self) -> &'a mut T {
&mut *self.inner().as_ptr().cast()
&mut *self.as_ptr().cast()
}
/// Gets the underlying pointer, erasing the associated lifetime.
///
/// If possible, it is strongly encouraged to use [`deref_mut`](Self::deref_mut) over
/// this function, as it retains the lifetime.
///
/// # Safety
/// All subsequent operations to the returned pointer must be valid inside the
/// associated lifetime.
#[inline]
#[allow(clippy::wrong_self_convention)]
pub unsafe fn as_ptr(self) -> *mut u8 {
self.0.as_ptr()
}
}
impl_ptr!(OwningPtr);
@ -132,7 +175,30 @@ impl<'a> OwningPtr<'a> {
/// Must point to a valid `T`.
#[inline]
pub unsafe fn read<T>(self) -> T {
self.inner().as_ptr().cast::<T>().read()
self.as_ptr().cast::<T>().read()
}
//// Consumes the [`OwningPtr`] to drop the underlying data of type `T`.
///
/// # Safety
/// Must point to a valid `T`.
#[inline]
pub unsafe fn drop_as<T>(self) {
self.as_ptr().cast::<T>().drop_in_place()
}
/// Gets the underlying pointer, erasing the associated lifetime.
///
/// If possible, it is strongly encouraged to use the other more type-safe functions
/// over this function.
///
/// # Safety
/// All subsequent operations to the returned pointer must be valid inside the
/// associated lifetime.
#[inline]
#[allow(clippy::wrong_self_convention)]
pub unsafe fn as_ptr(self) -> *mut u8 {
self.0.as_ptr()
}
}

28
crates/bevy_ecs/src/storage/blob_vec.rs
View File

@ -101,7 +101,7 @@ impl BlobVec {
std::alloc::alloc(new_layout)
} else {
std::alloc::realloc(
self.get_ptr_mut().inner().as_ptr(),
self.get_ptr_mut().as_ptr(),
array_layout(&self.item_layout, self.capacity)
.expect("array layout should be valid"),
new_layout.size(),
@ -121,11 +121,7 @@ impl BlobVec {
pub unsafe fn initialize_unchecked(&mut self, index: usize, value: OwningPtr<'_>) {
debug_assert!(index < self.len());
let ptr = self.get_unchecked_mut(index);
std::ptr::copy_nonoverlapping::<u8>(
value.inner().as_ptr(),
ptr.inner().as_ptr(),
self.item_layout.size(),
);
std::ptr::copy_nonoverlapping::<u8>(value.as_ptr(), ptr.as_ptr(), self.item_layout.size());
}
/// # Safety
@ -142,15 +138,11 @@ impl BlobVec {
// in the collection), so we get a double drop. To prevent that, we set len to 0 until we're
// done.
let old_len = self.len;
let ptr = self.get_unchecked_mut(index).promote().inner();
let ptr = self.get_unchecked_mut(index).promote().as_ptr();
self.len = 0;
// Drop the old value, then write back, justifying the promotion
(self.drop)(OwningPtr::new(ptr));
std::ptr::copy_nonoverlapping::<u8>(
value.inner().as_ptr(),
ptr.as_ptr(),
self.item_layout.size(),
);
(self.drop)(OwningPtr::new(NonNull::new_unchecked(ptr)));
std::ptr::copy_nonoverlapping::<u8>(value.as_ptr(), ptr, self.item_layout.size());
self.len = old_len;
}
@ -192,13 +184,13 @@ impl BlobVec {
let last = self.len - 1;
let swap_scratch = self.swap_scratch.as_ptr();
std::ptr::copy_nonoverlapping::<u8>(
self.get_unchecked_mut(index).inner().as_ptr(),
self.get_unchecked_mut(index).as_ptr(),
swap_scratch,
self.item_layout.size(),
);
std::ptr::copy::<u8>(
self.get_unchecked_mut(last).inner().as_ptr(),
self.get_unchecked_mut(index).inner().as_ptr(),
self.get_unchecked_mut(last).as_ptr(),
self.get_unchecked_mut(index).as_ptr(),
self.item_layout.size(),
);
self.len -= 1;
@ -280,7 +272,7 @@ impl Drop for BlobVec {
array_layout(&self.item_layout, self.capacity).expect("array layout should be valid");
if array_layout.size() > 0 {
unsafe {
std::alloc::dealloc(self.get_ptr_mut().inner().as_ptr(), array_layout);
std::alloc::dealloc(self.get_ptr_mut().as_ptr(), array_layout);
std::alloc::dealloc(self.swap_scratch.as_ptr(), self.item_layout);
}
}
@ -350,7 +342,7 @@ mod tests {
// SAFETY: The pointer points to a valid value of type `T` and it is safe to drop this value.
unsafe fn drop_ptr<T>(x: OwningPtr<'_>) {
x.inner().cast::<T>().as_ptr().drop_in_place()
x.drop_as::<T>()
}
/// # Safety