bevy/crates/bevy_ecs/src/storage/blob_vec.rs
TheRawMeatball 73c78c3667 Use lifetimed, type erased pointers in bevy_ecs (#3001)
# Objective

`bevy_ecs` has large amounts of unsafe code which is hard to get right and makes it difficult to audit for soundness.

## Solution

Introduce lifetimed, type-erased pointers: `Ptr<'a>` `PtrMut<'a>` `OwningPtr<'a>'` and `ThinSlicePtr<'a, T>` which are newtypes around a raw pointer with a lifetime and conceptually representing strong invariants about the pointee and validity of the pointer.

The process of converting bevy_ecs to use these has already caught multiple cases of unsound behavior.

## Changelog

TL;DR for release notes: `bevy_ecs` now uses lifetimed, type-erased pointers internally, significantly improving safety and legibility without sacrificing performance. This should have approximately no end user impact, unless you were meddling with the (unfortunately public) internals of `bevy_ecs`.

- `Fetch`, `FilterFetch` and `ReadOnlyFetch` trait no longer have a `'state` lifetime
    - this was unneeded
- `ReadOnly/Fetch` associated types on `WorldQuery` are now on a new `WorldQueryGats<'world>` trait
    - was required to work around lack of Generic Associated Types (we wish to express `type Fetch<'a>: Fetch<'a>`)
- `derive(WorldQuery)` no longer requires `'w` lifetime on struct
    - this was unneeded, and improves the end user experience
- `EntityMut::get_unchecked_mut` returns `&'_ mut T` not `&'w mut T`
    - allows easier use of unsafe API with less footguns, and can be worked around via lifetime transmutery as a user
- `Bundle::from_components` now takes a `ctx` parameter to pass to the `FnMut` closure
    - required because closure return types can't borrow from captures
- `Fetch::init` takes `&'world World`, `Fetch::set_archetype` takes `&'world Archetype` and `&'world Tables`, `Fetch::set_table` takes `&'world Table`
    - allows types implementing `Fetch` to store borrows into world
- `WorldQuery` trait now has a `shrink` fn to shorten the lifetime in `Fetch::<'a>::Item`
    - this works around lack of subtyping of assoc types, rust doesnt allow you to turn `<T as Fetch<'static>>::Item'` into `<T as Fetch<'a>>::Item'`
    - `QueryCombinationsIter` requires this
- Most types implementing `Fetch` now have a lifetime `'w`
    - allows the fetches to store borrows of world data instead of using raw pointers

## Migration guide

- `EntityMut::get_unchecked_mut` returns a more restricted lifetime, there is no general way to migrate this as it depends on your code
- `Bundle::from_components` implementations must pass the `ctx` arg to `func`
- `Bundle::from_components` callers have to use a fn arg instead of closure captures for borrowing from world
- Remove lifetime args on `derive(WorldQuery)` structs as it is nonsensical
- `<Q as WorldQuery>::ReadOnly/Fetch` should be changed to either `RO/QueryFetch<'world>` or `<Q as WorldQueryGats<'world>>::ReadOnly/Fetch`
- `<F as Fetch<'w, 's>>` should be changed to `<F as Fetch<'w>>`
- Change the fn sigs of `Fetch::init/set_archetype/set_table` to match respective trait fn sigs
- Implement the required `fn shrink` on any `WorldQuery` implementations
- Move assoc types `Fetch` and `ReadOnlyFetch` on `WorldQuery` impls to `WorldQueryGats` impls
- Pass an appropriate `'world` lifetime to whatever fetch struct you are for some reason using

### Type inference regression

in some cases rustc may give spurrious errors when attempting to infer the `F` parameter on a query/querystate this can be fixed by manually specifying the type, i.e. `QueryState:🆕:<_, ()>(world)`. The error is rather confusing:

```rust=
error[E0271]: type mismatch resolving `<() as Fetch<'_>>::Item == bool`
    --> crates/bevy_pbr/src/render/light.rs:1413:30
     |
1413 |             main_view_query: QueryState::new(world),
     |                              ^^^^^^^^^^^^^^^ expected `bool`, found `()`
     |
     = note: required because of the requirements on the impl of `for<'x> FilterFetch<'x>` for `<() as WorldQueryGats<'x>>::Fetch`
note: required by a bound in `bevy_ecs::query::QueryState::<Q, F>::new`
    --> crates/bevy_ecs/src/query/state.rs:49:32
     |
49   |     for<'x> QueryFetch<'x, F>: FilterFetch<'x>,
     |                                ^^^^^^^^^^^^^^^ required by this bound in `bevy_ecs::query::QueryState::<Q, F>::new`
```

---

Made with help from @BoxyUwU and @alice-i-cecile 

Co-authored-by: Boxy <supbscripter@gmail.com>
2022-04-27 23:44:06 +00:00

480 lines
17 KiB
Rust

use std::{
alloc::{handle_alloc_error, Layout},
cell::UnsafeCell,
ptr::NonNull,
};
use crate::ptr::{OwningPtr, Ptr, PtrMut};
/// A flat, type-erased data storage type
///
/// Used to densely store homogeneous ECS data.
pub struct BlobVec {
item_layout: Layout,
capacity: usize,
/// Number of elements, not bytes
len: usize,
data: NonNull<u8>,
swap_scratch: NonNull<u8>,
drop: unsafe fn(OwningPtr<'_>),
}
// We want to ignore the `drop` field in our `Debug` impl
impl std::fmt::Debug for BlobVec {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("BlobVec")
.field("item_layout", &self.item_layout)
.field("capacity", &self.capacity)
.field("len", &self.len)
.field("data", &self.data)
.field("swap_scratch", &self.swap_scratch)
.finish()
}
}
impl BlobVec {
/// # Safety
///
/// `drop` should be safe to call with an [`OwningPtr`] pointing to any item that's been pushed into this [`BlobVec`].
pub unsafe fn new(
item_layout: Layout,
drop: unsafe fn(OwningPtr<'_>),
capacity: usize,
) -> BlobVec {
if item_layout.size() == 0 {
BlobVec {
swap_scratch: NonNull::dangling(),
data: NonNull::dangling(),
capacity: usize::MAX,
len: 0,
item_layout,
drop,
}
} else {
let swap_scratch = NonNull::new(std::alloc::alloc(item_layout))
.unwrap_or_else(|| std::alloc::handle_alloc_error(item_layout));
let mut blob_vec = BlobVec {
swap_scratch,
data: NonNull::dangling(),
capacity: 0,
len: 0,
item_layout,
drop,
};
blob_vec.reserve_exact(capacity);
blob_vec
}
}
#[inline]
pub fn len(&self) -> usize {
self.len
}
#[inline]
pub fn is_empty(&self) -> bool {
self.len == 0
}
#[inline]
pub fn capacity(&self) -> usize {
self.capacity
}
pub fn reserve_exact(&mut self, additional: usize) {
let available_space = self.capacity - self.len;
if available_space < additional {
self.grow_exact(additional - available_space);
}
}
// FIXME: this should probably be an unsafe fn as it shouldn't be called if the layout
// is for a ZST
fn grow_exact(&mut self, increment: usize) {
debug_assert!(self.item_layout.size() != 0);
let new_capacity = self.capacity + increment;
let new_layout =
array_layout(&self.item_layout, new_capacity).expect("array layout should be valid");
unsafe {
let new_data = if self.capacity == 0 {
std::alloc::alloc(new_layout)
} else {
std::alloc::realloc(
self.get_ptr_mut().inner().as_ptr(),
array_layout(&self.item_layout, self.capacity)
.expect("array layout should be valid"),
new_layout.size(),
)
};
self.data = NonNull::new(new_data).unwrap_or_else(|| handle_alloc_error(new_layout));
}
self.capacity = new_capacity;
}
/// # Safety
/// - index must be in bounds
/// - the memory in the [`BlobVec`] starting at index `index`, of a size matching this [`BlobVec`]'s
/// `item_layout`, must have been previously allocated.
#[inline]
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(),
);
}
/// # Safety
/// - index must be in-bounds
/// - the memory in the [`BlobVec`] starting at index `index`, of a size matching this
/// [`BlobVec`]'s `item_layout`, must have been previously initialized with an item matching
/// this [`BlobVec`]'s `item_layout`
/// - the memory at `*value` must also be previously initialized with an item matching this
/// [`BlobVec`]'s `item_layout`
pub unsafe fn replace_unchecked(&mut self, index: usize, value: OwningPtr<'_>) {
debug_assert!(index < self.len());
// If `drop` panics, then when the collection is dropped during stack unwinding, the
// collection's `Drop` impl will call `drop` again for the old value (which is still stored
// 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();
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.len = old_len;
}
/// Pushes a value to the [`BlobVec`].
///
/// # Safety
/// `value` must be valid to add to this [`BlobVec`]
#[inline]
pub unsafe fn push(&mut self, value: OwningPtr<'_>) {
self.reserve_exact(1);
let index = self.len;
self.len += 1;
self.initialize_unchecked(index, value);
}
/// # Safety
/// `len` must be <= `capacity`. if length is decreased, "out of bounds" items must be dropped.
/// Newly added items must be immediately populated with valid values and length must be
/// increased. For better unwind safety, call [`BlobVec::set_len`] _after_ populating a new
/// value.
pub unsafe fn set_len(&mut self, len: usize) {
debug_assert!(len <= self.capacity());
self.len = len;
}
/// Performs a "swap remove" at the given `index`, which removes the item at `index` and moves
/// the last item in the [`BlobVec`] to `index` (if `index` is not the last item). It is the
/// caller's responsibility to drop the returned pointer, if that is desirable.
///
/// # Safety
/// It is the caller's responsibility to ensure that `index` is < `self.len()`
#[inline]
#[must_use = "The returned pointer should be used to dropped the removed element"]
pub unsafe fn swap_remove_and_forget_unchecked(&mut self, index: usize) -> OwningPtr<'_> {
// FIXME: This should probably just use `core::ptr::swap` and return an `OwningPtr`
// into the underlying `BlobVec` allocation, and remove swap_scratch
debug_assert!(index < self.len());
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(),
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.item_layout.size(),
);
self.len -= 1;
OwningPtr::new(self.swap_scratch)
}
/// # Safety
/// It is the caller's responsibility to ensure that `index` is < self.len()
#[inline]
pub unsafe fn swap_remove_and_drop_unchecked(&mut self, index: usize) {
debug_assert!(index < self.len());
let drop = self.drop;
let value = self.swap_remove_and_forget_unchecked(index);
(drop)(value);
}
/// # Safety
/// It is the caller's responsibility to ensure that `index` is < self.len()
#[inline]
pub unsafe fn get_unchecked(&self, index: usize) -> Ptr<'_> {
debug_assert!(index < self.len());
self.get_ptr().add(index * self.item_layout.size())
}
/// # Safety
/// It is the caller's responsibility to ensure that `index` is < self.len()
#[inline]
pub unsafe fn get_unchecked_mut(&mut self, index: usize) -> PtrMut<'_> {
debug_assert!(index < self.len());
let layout_size = self.item_layout.size();
self.get_ptr_mut().add(index * layout_size)
}
/// Gets a [`Ptr`] to the start of the vec
#[inline]
pub fn get_ptr(&self) -> Ptr<'_> {
// SAFE: the inner data will remain valid for as long as 'self.
unsafe { Ptr::new(self.data) }
}
/// Gets a [`PtrMut`] to the start of the vec
#[inline]
pub fn get_ptr_mut(&mut self) -> PtrMut<'_> {
// SAFE: the inner data will remain valid for as long as 'self.
unsafe { PtrMut::new(self.data) }
}
/// Get a reference to the entire [`BlobVec`] as if it were an array with elements of type `T`
///
/// # Safety
/// The type `T` must be the type of the items in this [`BlobVec`].
pub unsafe fn get_slice<T>(&self) -> &[UnsafeCell<T>] {
// SAFE: the inner data will remain valid for as long as 'self.
std::slice::from_raw_parts(self.data.as_ptr() as *const UnsafeCell<T>, self.len)
}
pub fn clear(&mut self) {
let len = self.len;
// We set len to 0 _before_ dropping elements for unwind safety. This ensures we don't
// accidentally drop elements twice in the event of a drop impl panicking.
self.len = 0;
let drop = self.drop;
let layout_size = self.item_layout.size();
for i in 0..len {
unsafe {
// NOTE: this doesn't use self.get_unchecked(i) because the debug_assert on index
// will panic here due to self.len being set to 0
let ptr = self.get_ptr_mut().add(i * layout_size).promote();
(drop)(ptr);
}
}
}
}
impl Drop for BlobVec {
fn drop(&mut self) {
self.clear();
let array_layout =
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.swap_scratch.as_ptr(), self.item_layout);
}
}
}
}
/// From <https://doc.rust-lang.org/beta/src/core/alloc/layout.rs.html>
fn array_layout(layout: &Layout, n: usize) -> Option<Layout> {
let (array_layout, offset) = repeat_layout(layout, n)?;
debug_assert_eq!(layout.size(), offset);
Some(array_layout)
}
// TODO: replace with `Layout::repeat` if/when it stabilizes
/// From <https://doc.rust-lang.org/beta/src/core/alloc/layout.rs.html>
fn repeat_layout(layout: &Layout, n: usize) -> Option<(Layout, usize)> {
// This cannot overflow. Quoting from the invariant of Layout:
// > `size`, when rounded up to the nearest multiple of `align`,
// > must not overflow (i.e., the rounded value must be less than
// > `usize::MAX`)
let padded_size = layout.size() + padding_needed_for(layout, layout.align());
let alloc_size = padded_size.checked_mul(n)?;
// SAFETY: self.align is already known to be valid and alloc_size has been
// padded already.
unsafe {
Some((
Layout::from_size_align_unchecked(alloc_size, layout.align()),
padded_size,
))
}
}
/// From <https://doc.rust-lang.org/beta/src/core/alloc/layout.rs.html>
const fn padding_needed_for(layout: &Layout, align: usize) -> usize {
let len = layout.size();
// Rounded up value is:
// len_rounded_up = (len + align - 1) & !(align - 1);
// and then we return the padding difference: `len_rounded_up - len`.
//
// We use modular arithmetic throughout:
//
// 1. align is guaranteed to be > 0, so align - 1 is always
// valid.
//
// 2. `len + align - 1` can overflow by at most `align - 1`,
// so the &-mask with `!(align - 1)` will ensure that in the
// case of overflow, `len_rounded_up` will itself be 0.
// Thus the returned padding, when added to `len`, yields 0,
// which trivially satisfies the alignment `align`.
//
// (Of course, attempts to allocate blocks of memory whose
// size and padding overflow in the above manner should cause
// the allocator to yield an error anyway.)
let len_rounded_up = len.wrapping_add(align).wrapping_sub(1) & !align.wrapping_sub(1);
len_rounded_up.wrapping_sub(len)
}
#[cfg(test)]
mod tests {
use crate::ptr::OwningPtr;
use super::BlobVec;
use std::{alloc::Layout, cell::RefCell, rc::Rc};
// 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()
}
/// # Safety
///
/// `blob_vec` must have a layout that matches `Layout::new::<T>()`
unsafe fn push<T>(blob_vec: &mut BlobVec, value: T) {
OwningPtr::make(value, |ptr| {
blob_vec.push(ptr);
});
}
/// # Safety
///
/// `blob_vec` must have a layout that matches `Layout::new::<T>()`
unsafe fn swap_remove<T>(blob_vec: &mut BlobVec, index: usize) -> T {
assert!(index < blob_vec.len());
let value = blob_vec.swap_remove_and_forget_unchecked(index);
value.read::<T>()
}
/// # Safety
///
/// `blob_vec` must have a layout that matches `Layout::new::<T>()`, it most store a valid `T`
/// value at the given `index`
unsafe fn get_mut<T>(blob_vec: &mut BlobVec, index: usize) -> &mut T {
assert!(index < blob_vec.len());
blob_vec.get_unchecked_mut(index).deref_mut::<T>()
}
#[test]
fn resize_test() {
let item_layout = Layout::new::<usize>();
let drop = drop_ptr::<usize>;
let mut blob_vec = unsafe { BlobVec::new(item_layout, drop, 64) };
unsafe {
for i in 0..1_000 {
push(&mut blob_vec, i as usize);
}
}
assert_eq!(blob_vec.len(), 1_000);
assert_eq!(blob_vec.capacity(), 1_000);
}
#[derive(Debug, Eq, PartialEq, Clone)]
struct Foo {
a: u8,
b: String,
drop_counter: Rc<RefCell<usize>>,
}
impl Drop for Foo {
fn drop(&mut self) {
*self.drop_counter.borrow_mut() += 1;
}
}
#[test]
fn blob_vec() {
let drop_counter = Rc::new(RefCell::new(0));
{
let item_layout = Layout::new::<Foo>();
let drop = drop_ptr::<Foo>;
let mut blob_vec = unsafe { BlobVec::new(item_layout, drop, 2) };
assert_eq!(blob_vec.capacity(), 2);
unsafe {
let foo1 = Foo {
a: 42,
b: "abc".to_string(),
drop_counter: drop_counter.clone(),
};
push(&mut blob_vec, foo1.clone());
assert_eq!(blob_vec.len(), 1);
assert_eq!(get_mut::<Foo>(&mut blob_vec, 0), &foo1);
let mut foo2 = Foo {
a: 7,
b: "xyz".to_string(),
drop_counter: drop_counter.clone(),
};
push::<Foo>(&mut blob_vec, foo2.clone());
assert_eq!(blob_vec.len(), 2);
assert_eq!(blob_vec.capacity(), 2);
assert_eq!(get_mut::<Foo>(&mut blob_vec, 0), &foo1);
assert_eq!(get_mut::<Foo>(&mut blob_vec, 1), &foo2);
get_mut::<Foo>(&mut blob_vec, 1).a += 1;
assert_eq!(get_mut::<Foo>(&mut blob_vec, 1).a, 8);
let foo3 = Foo {
a: 16,
b: "123".to_string(),
drop_counter: drop_counter.clone(),
};
push(&mut blob_vec, foo3.clone());
assert_eq!(blob_vec.len(), 3);
assert_eq!(blob_vec.capacity(), 3);
let last_index = blob_vec.len() - 1;
let value = swap_remove::<Foo>(&mut blob_vec, last_index);
assert_eq!(foo3, value);
assert_eq!(blob_vec.len(), 2);
assert_eq!(blob_vec.capacity(), 3);
let value = swap_remove::<Foo>(&mut blob_vec, 0);
assert_eq!(foo1, value);
assert_eq!(blob_vec.len(), 1);
assert_eq!(blob_vec.capacity(), 3);
foo2.a = 8;
assert_eq!(get_mut::<Foo>(&mut blob_vec, 0), &foo2);
}
}
assert_eq!(*drop_counter.borrow(), 6);
}
#[test]
fn blob_vec_drop_empty_capacity() {
let item_layout = Layout::new::<Foo>();
let drop = drop_ptr::<Foo>;
let _ = unsafe { BlobVec::new(item_layout, drop, 0) };
}
}