From 3e24b725afe143db5db006b146cd94339e155251 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 3 May 2022 20:07:58 +0000 Subject: [PATCH] Pointerfication followup: Type safety and cleanup (#4621) # Objective The `Ptr` types gives free access to the underlying `NonNull`, 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. --- crates/bevy_ecs/macros/src/lib.rs | 2 +- crates/bevy_ecs/src/bundle.rs | 5 +- crates/bevy_ecs/src/component.rs | 2 +- crates/bevy_ecs/src/ptr.rs | 90 +++++++++++++++++++++---- crates/bevy_ecs/src/storage/blob_vec.rs | 28 +++----- 5 files changed, 91 insertions(+), 36 deletions(-) diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 7dfdaf0a86c0c..f67ce35bbb997 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -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>(), }); } } diff --git a/crates/bevy_ecs/src/bundle.rs b/crates/bevy_ecs/src/bundle.rs index ec40ee8443752..1fa9fa9653112 100644 --- a/crates/bevy_ecs/src/bundle.rs +++ b/crates/bevy_ecs/src/bundle.rs @@ -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)] diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 6d465f1a36e93..fac3bfb9dbeff 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -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(x: OwningPtr<'_>) { - x.inner().cast::().as_ptr().drop_in_place() + x.drop_as::() } pub fn new() -> Self { diff --git a/crates/bevy_ecs/src/ptr.rs b/crates/bevy_ecs/src/ptr.rs index 502e91eb598e5..2f81e4632b376 100644 --- a/crates/bevy_ecs/src/ptr.rs +++ b/crates/bevy_ecs/src/ptr.rs @@ -43,57 +43,86 @@ pub struct OwningPtr<'a>(NonNull, 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) -> Self { Self(inner, PhantomData) } - - #[inline] - pub fn inner(&self) -> NonNull { - 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`] into a `&T` with the same lifetime + /// /// # Safety /// Must point to a valid `T` #[inline] pub unsafe fn deref(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(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(self) -> T { - self.inner().as_ptr().cast::().read() + self.as_ptr().cast::().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(self) { + self.as_ptr().cast::().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() } } diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index a5f2366649f04..f46ab409e4cfb 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -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::( - value.inner().as_ptr(), - ptr.inner().as_ptr(), - self.item_layout.size(), - ); + std::ptr::copy_nonoverlapping::(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::( - value.inner().as_ptr(), - ptr.as_ptr(), - self.item_layout.size(), - ); + (self.drop)(OwningPtr::new(NonNull::new_unchecked(ptr))); + std::ptr::copy_nonoverlapping::(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::( - self.get_unchecked_mut(index).inner().as_ptr(), + self.get_unchecked_mut(index).as_ptr(), swap_scratch, self.item_layout.size(), ); std::ptr::copy::( - 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(x: OwningPtr<'_>) { - x.inner().cast::().as_ptr().drop_in_place() + x.drop_as::() } /// # Safety