From a58c2b83075f034427927c8b4af48533bc71fede Mon Sep 17 00:00:00 2001 From: Jake Hughes Date: Tue, 23 Jan 2024 11:23:35 +0000 Subject: [PATCH] Finalise `Gc` objects on a separate thread This prevents deadlocks in user code which acquire locks in drop methods. The new approach to finalisation works as follows: 1. During the entry_point, a finalisation thread, and an mpsc channel are initialised. 2. New threads (including the main thread) are given thread local storage (TLS) containing a sender, which is used to pass finalisers to the finalisation thread. This does not require synchronisation. 3. At the end of the GC pause, Boehm will run finalisers for objects which were not marked. This does not run the drop method for the object directly. Instead, it sends the drop method to the finalisation thread to be run later. 4. The finalisation thread iterates through the message queue, calling the finaliser callback for each unreachable object. This finaliser callback amounts to a call to `std::ptr::drop_in_place` on the object. I believe this is sound for three reasons: 1. Finaliser Safety Analysis (FSA) does not allow us to use non-Send and non-Sync fields inside a drop method, so a drop method run on another thread cannot accidentally access memory in non-thread-safe way. 2. Our modification to the BDWGC [1] which adds a memory fence at the end of a GC pause means that when a finaliser is run, it will have an up-to-date view of the object graph. If a finalisable object is mutated after a collection but before it is finalised, then this is still fine. This is because that can only happen using Sync-Send-safe containers such as a `Mutex` (as this is the only form of interior mutability supported by `Gc`.) 3. Before a finaliser is run, Boehm will re-mark the object (and any others which reachable from that finaliser) which means that the object's memory will not be swept until at least the next GC (or potentially even later, if the finalisation thread has not yet run!) [1]: softdevteam/bdwgc#3 --- library/std/src/gc.rs | 98 ++++++++++++++-------- library/std/src/rt.rs | 2 + library/std/src/thread/mod.rs | 6 ++ tests/ui/runtime/gc/run_finalizers.rs | 6 ++ tests/ui/runtime/gc/unchecked_finalizer.rs | 6 ++ tests/ui/runtime/gc/zero_vecs.rs | 5 ++ 6 files changed, 87 insertions(+), 36 deletions(-) diff --git a/library/std/src/gc.rs b/library/std/src/gc.rs index 1bd9220bfd309..97c6b2ad8b686 100644 --- a/library/std/src/gc.rs +++ b/library/std/src/gc.rs @@ -39,15 +39,18 @@ use core::{ any::Any, + cell::RefCell, cmp::Ordering, fmt, hash::{Hash, Hasher}, marker::{FinalizerSafe, PhantomData, Unsize}, - mem::MaybeUninit, + mem::{transmute, MaybeUninit}, ops::{CoerceUnsized, Deref, DispatchFromDyn, Receiver}, ptr::{drop_in_place, null_mut, NonNull}, }; +use crate::{sync::mpsc, sync::mpsc::Sender, thread}; + pub use alloc::gc::thread_registered; pub use alloc::gc::GcAllocator; pub use core::gc::*; @@ -258,11 +261,15 @@ impl Gc { #[cfg(profile_gc)] FINALIZERS_REGISTERED.fetch_add(1, atomic::Ordering::Relaxed); - unsafe extern "C" fn finalizer(obj: *mut u8, _meta: *mut u8) { + // This function gets called by Boehm when an object becomes unreachable. + unsafe extern "C" fn finalizer(object: *mut u8, _meta: *mut u8) { unsafe { - drop_in_place(obj as *mut T); - #[cfg(profile_gc)] - FINALIZERS_COMPLETED.fetch_add(1, atomic::Ordering::Relaxed); + let cb = finalizer_callback::; + let fo = FinalizableObj { + callback: transmute(cb as unsafe extern "C" fn(*mut T)), + object, + }; + FINALIZER_QUEUE.with(|q| q.borrow().as_ref().unwrap().send(fo).unwrap()); } } @@ -276,37 +283,6 @@ impl Gc { ) } } - - #[unstable(feature = "gc", issue = "none")] - pub fn unregister_finalizer(&mut self) { - let ptr = self.ptr.as_ptr() as *mut GcBox as *mut u8; - unsafe { - bdwgc::GC_register_finalizer( - ptr, - None, - ::core::ptr::null_mut(), - ::core::ptr::null_mut(), - ::core::ptr::null_mut(), - ); - } - } -} - -#[cfg(profile_gc)] -#[derive(Debug)] -pub struct FinalizerInfo { - pub registered: u64, - pub completed: u64, -} - -#[cfg(profile_gc)] -impl FinalizerInfo { - pub fn finalizer_info() -> FinalizerInfo { - FinalizerInfo { - registered: FINALIZERS_REGISTERED.load(atomic::Ordering::Relaxed), - completed: FINALIZERS_COMPLETED.load(atomic::Ordering::Relaxed), - } - } } impl Gc { @@ -663,3 +639,53 @@ impl AsRef for Gc { &**self } } + +thread_local! { + pub static FINALIZER_QUEUE: RefCell> = RefCell::new(None); +} + +type FinalizerQueue = Sender; + +#[derive(Debug)] +pub struct FinalizableObj { + callback: FinalizerCallback, + object: *mut u8, +} + +unsafe impl Send for FinalizableObj {} + +pub(crate) fn init_finalization_thread() { + let (sender, receiver) = mpsc::channel(); + FINALIZER_QUEUE.with(|q| *q.borrow_mut() = Some(sender)); + + thread::spawn(move || { + for finalisable in receiver.iter() { + unsafe { (finalisable.callback)(finalisable.object) }; + } + }); +} + +unsafe extern "C" fn finalizer_callback(obj: *mut T) { + unsafe { drop_in_place(obj as *mut T) }; + #[cfg(profile_gc)] + FINALIZERS_COMPLETED.fetch_add(1, atomic::Ordering::Relaxed); +} + +type FinalizerCallback = unsafe extern "C" fn(data: *mut u8); + +#[cfg(profile_gc)] +#[derive(Debug)] +pub struct FinalizerInfo { + pub registered: u64, + pub completed: u64, +} + +#[cfg(profile_gc)] +impl FinalizerInfo { + pub fn finalizer_info() -> FinalizerInfo { + FinalizerInfo { + registered: FINALIZERS_REGISTERED.load(atomic::Ordering::Relaxed), + completed: FINALIZERS_COMPLETED.load(atomic::Ordering::Relaxed), + } + } +} diff --git a/library/std/src/rt.rs b/library/std/src/rt.rs index b47254fa086a8..c052f4e82147a 100644 --- a/library/std/src/rt.rs +++ b/library/std/src/rt.rs @@ -112,6 +112,8 @@ unsafe fn init(argc: isize, argv: *const *const u8, sigpipe: u8) { // info about the stack bounds. let thread = Thread::new(Some(rtunwrap!(Ok, CString::new("main")))); thread_info::set(main_guard, thread); + + crate::gc::init_finalization_thread(); } } diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index 59529ceb4d999..388f972ccc0fa 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -162,6 +162,7 @@ use crate::any::Any; use crate::cell::UnsafeCell; use crate::ffi::{CStr, CString}; use crate::fmt; +use crate::gc::FINALIZER_QUEUE; use crate::io; use crate::marker::PhantomData; use crate::mem::{self, forget}; @@ -512,11 +513,16 @@ impl Builder { } let f = MaybeDangling::new(f); + + let fin_q_sender = FINALIZER_QUEUE.with(|q| q.borrow().clone()); + let main = move || { if let Some(name) = their_thread.cname() { imp::Thread::set_name(name); } + FINALIZER_QUEUE.with(|q| *q.borrow_mut() = fin_q_sender); + crate::io::set_output_capture(output_capture); // SAFETY: we constructed `f` initialized. diff --git a/tests/ui/runtime/gc/run_finalizers.rs b/tests/ui/runtime/gc/run_finalizers.rs index 1ee1268963a60..a3a08a566e05a 100644 --- a/tests/ui/runtime/gc/run_finalizers.rs +++ b/tests/ui/runtime/gc/run_finalizers.rs @@ -4,6 +4,8 @@ use std::gc::{Gc, GcAllocator}; use std::sync::atomic::{self, AtomicUsize}; +use std::thread; +use std::time; struct Finalizable(usize); @@ -30,6 +32,10 @@ fn foo() { fn main() { foo(); GcAllocator::force_gc(); + + // Wait enough time for the finaliser thread to finish running. + + thread::sleep(time::Duration::from_millis(100)); // On some platforms, the last object might not be finalised because it's // kept alive by a lingering reference. assert!(FINALIZER_COUNT.load(atomic::Ordering::Relaxed) >= ALLOCATED_COUNT -1); diff --git a/tests/ui/runtime/gc/unchecked_finalizer.rs b/tests/ui/runtime/gc/unchecked_finalizer.rs index 39f2309568a4d..3e0ba41526a7c 100644 --- a/tests/ui/runtime/gc/unchecked_finalizer.rs +++ b/tests/ui/runtime/gc/unchecked_finalizer.rs @@ -5,6 +5,8 @@ use std::gc::{Gc, GcAllocator, FinalizeUnchecked}; use std::sync::atomic::{self, AtomicUsize}; +use std::thread; +use std::time; struct UnsafeContainer(usize); @@ -33,6 +35,10 @@ fn foo() { fn main() { foo(); GcAllocator::force_gc(); + + // Wait enough time for the finaliser thread to finish running. + thread::sleep(time::Duration::from_millis(100)); + // On some platforms, the last object might not be finalised because it's // kept alive by a lingering reference. assert!(FINALIZER_COUNT.load(atomic::Ordering::Relaxed) >= ALLOCATED_COUNT -1); diff --git a/tests/ui/runtime/gc/zero_vecs.rs b/tests/ui/runtime/gc/zero_vecs.rs index 89adb30717da3..21bb92618f0c4 100644 --- a/tests/ui/runtime/gc/zero_vecs.rs +++ b/tests/ui/runtime/gc/zero_vecs.rs @@ -8,6 +8,8 @@ use std::gc::{Gc, GcAllocator}; use std::sync::atomic::{self, AtomicUsize}; +use std::thread; +use std::time; struct Finalizable(usize); @@ -41,6 +43,9 @@ fn main() { GcAllocator::force_gc(); + // Wait enough time for the finaliser thread to finish running. + thread::sleep(time::Duration::from_millis(100)); + // This tests that finalisation happened indirectly by trying to overwrite references to live GC // objects in order for Boehm to consider them dead. This is inherently flaky because we might // miss some references which linger on the stack or in registers. This tends to happen for the