From 2c0ad38563af55c415efeff748b30f1d97f7dc0f Mon Sep 17 00:00:00 2001 From: jedel1043 Date: Wed, 20 Sep 2023 14:34:05 -0600 Subject: [PATCH 1/2] Add `Gc::new_cyclic` --- boa_gc/src/internals/ephemeron_box.rs | 33 +++++++++++++++++++++--- boa_gc/src/pointers/ephemeron.rs | 3 ++- boa_gc/src/pointers/gc.rs | 36 +++++++++++++++++++++++++-- boa_gc/src/pointers/weak.rs | 8 +++++- 4 files changed, 72 insertions(+), 8 deletions(-) diff --git a/boa_gc/src/internals/ephemeron_box.rs b/boa_gc/src/internals/ephemeron_box.rs index 361b9ebf8b4..3db54195135 100644 --- a/boa_gc/src/internals/ephemeron_box.rs +++ b/boa_gc/src/internals/ephemeron_box.rs @@ -100,6 +100,7 @@ struct Data { } impl EphemeronBox { + /// Creates a new `EphemeronBox` that tracks `key` and has `value` as its inner data. pub(crate) fn new(key: &Gc, value: V) -> Self { Self { header: EphemeronBoxHeader::new(), @@ -110,6 +111,14 @@ impl EphemeronBox { } } + /// Creates a new `EphemeronBox` with its inner data in the invalidated state. + pub(crate) fn new_empty() -> Self { + Self { + header: EphemeronBoxHeader::new(), + data: UnsafeCell::new(None), + } + } + /// Returns `true` if the two references refer to the same `EphemeronBox`. pub(crate) fn ptr_eq(this: &Self, other: &Self) -> bool { // Use .header to ignore fat pointer vtables, to work around @@ -121,8 +130,8 @@ impl EphemeronBox { /// /// # Safety /// - /// The garbage collector must not run between the call to this function and the eventual - /// drop of the returned reference, since that could free the inner value. + /// The caller must ensure there are no live mutable references to the ephemeron box's data + /// before calling this method. pub(crate) unsafe fn value(&self) -> Option<&V> { // SAFETY: the garbage collector ensures the ephemeron doesn't mutate until // finalization. @@ -134,8 +143,8 @@ impl EphemeronBox { /// /// # Safety /// - /// The garbage collector must not run between the call to this function and the eventual - /// drop of the returned reference, since that could free the inner value. + /// The caller must ensure there are no live mutable references to the ephemeron box's data + /// before calling this method. pub(crate) unsafe fn key(&self) -> Option<&GcBox> { // SAFETY: the garbage collector ensures the ephemeron doesn't mutate until // finalization. @@ -153,6 +162,22 @@ impl EphemeronBox { self.header.mark(); } + /// Sets the inner data of the `EphemeronBox` to the specified key and value. + /// + /// # Safety + /// + /// The caller must ensure there are no live mutable references to the ephemeron box's data + /// before calling this method. + pub(crate) unsafe fn set(&self, key: &Gc, value: V) { + // SAFETY: The caller must ensure setting the key and value of the ephemeron box is safe. + unsafe { + *self.data.get() = Some(Data { + key: key.inner_ptr(), + value, + }); + } + } + #[inline] pub(crate) fn inc_ref_count(&self) { self.header.ref_count.set(self.header.ref_count.get() + 1); diff --git a/boa_gc/src/pointers/ephemeron.rs b/boa_gc/src/pointers/ephemeron.rs index c8268f7cd98..fd5fbab026a 100644 --- a/boa_gc/src/pointers/ephemeron.rs +++ b/boa_gc/src/pointers/ephemeron.rs @@ -44,6 +44,7 @@ impl Ephemeron { impl Ephemeron { /// Creates a new `Ephemeron`. + #[must_use] pub fn new(key: &Gc, value: V) -> Self { let inner_ptr = Allocator::alloc_ephemeron(EphemeronBox::new(key, value)); Self { inner_ptr } @@ -72,7 +73,7 @@ impl Ephemeron { /// This function is unsafe because improper use may lead to memory corruption, double-free, /// or misbehaviour of the garbage collector. #[must_use] - const unsafe fn from_raw(inner_ptr: NonNull>) -> Self { + pub(crate) const unsafe fn from_raw(inner_ptr: NonNull>) -> Self { Self { inner_ptr } } } diff --git a/boa_gc/src/pointers/gc.rs b/boa_gc/src/pointers/gc.rs index 675cba7b005..a8cb1476024 100644 --- a/boa_gc/src/pointers/gc.rs +++ b/boa_gc/src/pointers/gc.rs @@ -1,8 +1,8 @@ use crate::{ finalizer_safe, - internals::GcBox, + internals::{EphemeronBox, GcBox}, trace::{Finalize, Trace}, - Allocator, + Allocator, Ephemeron, WeakGc, }; use std::{ cmp::Ordering, @@ -22,6 +22,7 @@ pub struct Gc { impl Gc { /// Constructs a new `Gc` with the given value. + #[must_use] pub fn new(value: T) -> Self { // Create GcBox and allocate it to heap. // @@ -34,6 +35,37 @@ impl Gc { } } + /// Constructs a new `Gc` while giving you a `WeakGc` to the allocation, to allow + /// constructing a T which holds a weak pointer to itself. + /// + /// Since the new `Gc` is not fully-constructed until `Gc::new_cyclic` returns, calling + /// [`upgrade`][WeakGc::upgrade] on the weak reference inside the closure will fail and result + /// in a `None` value. + #[must_use] + pub fn new_cyclic(data_fn: F) -> Self + where + F: FnOnce(&WeakGc) -> T, + { + // SAFETY: The newly allocated ephemeron is only live here, meaning `Ephemeron` is the + // sole owner of the allocation after passing it to `from_raw`, making this operation safe. + let weak = unsafe { + Ephemeron::from_raw(Allocator::alloc_ephemeron(EphemeronBox::new_empty())).into() + }; + + // If `data_fn` panics, `weak` will be dropped here, giving the GC an opportunity to + // deallocate it before exiting. This ensures unwind safety. + let gc = Self::new(data_fn(&weak)); + + // SAFETY: + // - `as_mut`: `weak` is properly initialized by `alloc_ephemeron` and cannot escape the + // `unsafe` block. + // - `set_kv`: `weak` is a newly created `EphemeronBox`, meaning it isn't possible to + // collect it since `weak` is still live. + unsafe { weak.inner().inner_ptr().as_mut().set(&gc, gc.clone()) } + + gc + } + /// Consumes the `Gc`, returning a wrapped raw pointer. /// /// To avoid a memory leak, the pointer must be converted back to a `Gc` using [`Gc::from_raw`]. diff --git a/boa_gc/src/pointers/weak.rs b/boa_gc/src/pointers/weak.rs index 38596b7f24b..1acd98b3db1 100644 --- a/boa_gc/src/pointers/weak.rs +++ b/boa_gc/src/pointers/weak.rs @@ -20,7 +20,8 @@ impl WeakGc { } } - /// Upgrade returns a `Gc` pointer for the internal value if valid, or None if the value was already garbage collected. + /// Upgrade returns a `Gc` pointer for the internal value if the pointer is still live, or `None` + /// if the value was already garbage collected. #[must_use] pub fn upgrade(&self) -> Option> { self.inner.value() @@ -31,6 +32,11 @@ impl WeakGc { pub fn is_upgradable(&self) -> bool { self.inner.has_value() } + + #[must_use] + pub(crate) const fn inner(&self) -> &Ephemeron> { + &self.inner + } } impl Clone for WeakGc { From 2dc5f789e3788bf18c2f301fd445cc3b972820fa Mon Sep 17 00:00:00 2001 From: jedel1043 Date: Wed, 20 Sep 2023 16:52:30 -0600 Subject: [PATCH 2/2] Remove unrelated doc comment --- boa_gc/src/pointers/gc.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/boa_gc/src/pointers/gc.rs b/boa_gc/src/pointers/gc.rs index a8cb1476024..c49ae8b0f48 100644 --- a/boa_gc/src/pointers/gc.rs +++ b/boa_gc/src/pointers/gc.rs @@ -52,8 +52,6 @@ impl Gc { Ephemeron::from_raw(Allocator::alloc_ephemeron(EphemeronBox::new_empty())).into() }; - // If `data_fn` panics, `weak` will be dropped here, giving the GC an opportunity to - // deallocate it before exiting. This ensures unwind safety. let gc = Self::new(data_fn(&weak)); // SAFETY: