From 4a2bfec15696090493cb7ecb8def0500d20f49ae Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Mon, 9 Dec 2024 17:48:08 +0100 Subject: [PATCH 01/11] Run TLS destructors at process exit on all platforms This calls TLS destructors for the thread that initiates a process exit. This is done by registering a process-wide exit callback. Previously UNIX platforms other than Linux and Apple did not destruct TLS variables on the thread that initiated the process exit (either by returning from main or calling std::process::exit). --- library/std/src/lib.rs | 1 + library/std/src/sys/thread_local/exit/unix.rs | 10 ++ library/std/src/sys/thread_local/guard/key.rs | 93 +++++++++++++------ .../std/src/sys/thread_local/guard/statik.rs | 24 +++++ library/std/src/sys/thread_local/key/racy.rs | 76 ++++++++++++++- library/std/src/sys/thread_local/mod.rs | 52 +++++++++-- library/std/src/sys/thread_local/statik.rs | 51 +++++++++- 7 files changed, 266 insertions(+), 41 deletions(-) create mode 100644 library/std/src/sys/thread_local/exit/unix.rs create mode 100644 library/std/src/sys/thread_local/guard/statik.rs diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs index 5c12236617c98..c106310238a2b 100644 --- a/library/std/src/lib.rs +++ b/library/std/src/lib.rs @@ -279,6 +279,7 @@ #![feature(asm_experimental_arch)] #![feature(autodiff)] #![feature(cfg_sanitizer_cfi)] +#![feature(cfg_target_has_atomic)] #![feature(cfg_target_thread_local)] #![feature(cfi_encoding)] #![feature(concat_idents)] diff --git a/library/std/src/sys/thread_local/exit/unix.rs b/library/std/src/sys/thread_local/exit/unix.rs new file mode 100644 index 0000000000000..0bc2419dfd29e --- /dev/null +++ b/library/std/src/sys/thread_local/exit/unix.rs @@ -0,0 +1,10 @@ +use crate::mem; + +pub unsafe fn at_process_exit(cb: unsafe extern "C" fn()) { + // Miri does not support atexit. + #[cfg(not(miri))] + assert_eq!(unsafe { libc::atexit(mem::transmute(cb)) }, 0); + + #[cfg(miri)] + let _ = cb; +} diff --git a/library/std/src/sys/thread_local/guard/key.rs b/library/std/src/sys/thread_local/guard/key.rs index 59581e6f281e6..260db888dfd01 100644 --- a/library/std/src/sys/thread_local/guard/key.rs +++ b/library/std/src/sys/thread_local/guard/key.rs @@ -3,21 +3,40 @@ //! that will run all native TLS destructors in the destructor list. use crate::ptr; +use crate::sync::atomic::{AtomicBool, Ordering}; +use crate::sys::thread_local::exit::at_process_exit; use crate::sys::thread_local::key::{LazyKey, set}; #[cfg(target_thread_local)] pub fn enable() { - use crate::sys::thread_local::destructors; + fn enable_thread() { + static DTORS: LazyKey = LazyKey::new(Some(run_thread)); - static DTORS: LazyKey = LazyKey::new(Some(run)); + // Setting the key value to something other than NULL will result in the + // destructor being run at thread exit. + unsafe { + set(DTORS.force(), ptr::without_provenance_mut(1)); + } + + unsafe extern "C" fn run_thread(_: *mut u8) { + run() + } + } - // Setting the key value to something other than NULL will result in the - // destructor being run at thread exit. - unsafe { - set(DTORS.force(), ptr::without_provenance_mut(1)); + fn enable_process() { + static REGISTERED: AtomicBool = AtomicBool::new(false); + if !REGISTERED.swap(true, Ordering::Relaxed) { + unsafe { at_process_exit(run_process) }; + } + + unsafe extern "C" fn run_process() { + run() + } } - unsafe extern "C" fn run(_: *mut u8) { + fn run() { + use crate::sys::thread_local::destructors; + unsafe { destructors::run(); // On platforms with `__cxa_thread_atexit_impl`, `destructors::run` @@ -28,33 +47,55 @@ pub fn enable() { crate::rt::thread_cleanup(); } } + + enable_thread(); + enable_process(); } /// On platforms with key-based TLS, the system runs the destructors for us. /// We still have to make sure that [`crate::rt::thread_cleanup`] is called, /// however. This is done by defering the execution of a TLS destructor to /// the next round of destruction inside the TLS destructors. +/// +/// POSIX systems do not run TLS destructors at process exit. +/// Thus we register our own callback to invoke them in that case. #[cfg(not(target_thread_local))] pub fn enable() { - const DEFER: *mut u8 = ptr::without_provenance_mut(1); - const RUN: *mut u8 = ptr::without_provenance_mut(2); - - static CLEANUP: LazyKey = LazyKey::new(Some(run)); - - unsafe { set(CLEANUP.force(), DEFER) } - - unsafe extern "C" fn run(state: *mut u8) { - if state == DEFER { - // Make sure that this function is run again in the next round of - // TLS destruction. If there is no futher round, there will be leaks, - // but that's okay, `thread_cleanup` is not guaranteed to be called. - unsafe { set(CLEANUP.force(), RUN) } - } else { - debug_assert_eq!(state, RUN); - // If the state is still RUN in the next round of TLS destruction, - // it means that no other TLS destructors defined by this runtime - // have been run, as they would have set the state to DEFER. - crate::rt::thread_cleanup(); + fn enable_thread() { + const DEFER: *mut u8 = ptr::without_provenance_mut(1); + const RUN: *mut u8 = ptr::without_provenance_mut(2); + + static CLEANUP: LazyKey = LazyKey::new(Some(run_thread)); + + unsafe { set(CLEANUP.force(), DEFER) } + + unsafe extern "C" fn run_thread(state: *mut u8) { + if state == DEFER { + // Make sure that this function is run again in the next round of + // TLS destruction. If there is no futher round, there will be leaks, + // but that's okay, `thread_cleanup` is not guaranteed to be called. + unsafe { set(CLEANUP.force(), RUN) } + } else { + debug_assert_eq!(state, RUN); + // If the state is still RUN in the next round of TLS destruction, + // it means that no other TLS destructors defined by this runtime + // have been run, as they would have set the state to DEFER. + crate::rt::thread_cleanup(); + } + } + } + + fn enable_process() { + static REGISTERED: AtomicBool = AtomicBool::new(false); + if !REGISTERED.swap(true, Ordering::Relaxed) { + unsafe { at_process_exit(run_process) }; + } + + unsafe extern "C" fn run_process() { + unsafe { crate::sys::thread_local::key::run_dtors() }; } } + + enable_thread(); + enable_process(); } diff --git a/library/std/src/sys/thread_local/guard/statik.rs b/library/std/src/sys/thread_local/guard/statik.rs new file mode 100644 index 0000000000000..bd687ff4b3188 --- /dev/null +++ b/library/std/src/sys/thread_local/guard/statik.rs @@ -0,0 +1,24 @@ +//! The platform has no threads, so we just need to register +//! a process exit callback. + +use crate::cell::Cell; +use crate::sys::thread_local::exit::at_process_exit; +use crate::sys::thread_local::statik::run_dtors; + +pub fn enable() { + struct Registered(Cell); + // SAFETY: the target doesn't have threads. + unsafe impl Sync for Registered {} + + static REGISTERED: Registered = Registered(Cell::new(false)); + + if !REGISTERED.0.get() { + REGISTERED.0.set(true); + unsafe { at_process_exit(run_process) }; + } + + unsafe extern "C" fn run_process() { + unsafe { run_dtors() }; + crate::rt::thread_cleanup(); + } +} diff --git a/library/std/src/sys/thread_local/key/racy.rs b/library/std/src/sys/thread_local/key/racy.rs index e1bc08eabb358..4fb928c613179 100644 --- a/library/std/src/sys/thread_local/key/racy.rs +++ b/library/std/src/sys/thread_local/key/racy.rs @@ -17,6 +17,9 @@ pub struct LazyKey { key: AtomicUsize, /// Destructor for the TLS value. dtor: Option, + /// Next element of process-wide destructor list. + #[cfg(not(target_thread_local))] + next: atomic::AtomicPtr, } // Define a sentinel value that is likely not to be returned @@ -31,18 +34,23 @@ const KEY_SENTVAL: usize = libc::PTHREAD_KEYS_MAX + 1; impl LazyKey { pub const fn new(dtor: Option) -> LazyKey { - LazyKey { key: atomic::AtomicUsize::new(KEY_SENTVAL), dtor } + LazyKey { + key: atomic::AtomicUsize::new(KEY_SENTVAL), + dtor, + #[cfg(not(target_thread_local))] + next: atomic::AtomicPtr::new(crate::ptr::null_mut()), + } } #[inline] - pub fn force(&self) -> super::Key { + pub fn force(&'static self) -> super::Key { match self.key.load(Ordering::Acquire) { KEY_SENTVAL => self.lazy_init() as super::Key, n => n as super::Key, } } - fn lazy_init(&self) -> usize { + fn lazy_init(&'static self) -> usize { // POSIX allows the key created here to be KEY_SENTVAL, but the compare_exchange // below relies on using KEY_SENTVAL as a sentinel value to check who won the // race to set the shared TLS key. As far as I know, there is no @@ -70,7 +78,13 @@ impl LazyKey { Ordering::Acquire, ) { // The CAS succeeded, so we've created the actual key - Ok(_) => key as usize, + Ok(_) => { + #[cfg(not(target_thread_local))] + if self.dtor.is_some() { + unsafe { register_dtor(self) }; + } + key as usize + } // If someone beat us to the punch, use their key instead Err(n) => unsafe { super::destroy(key); @@ -79,3 +93,57 @@ impl LazyKey { } } } + +/// POSIX does not run TLS destructors on process exit. +/// Thus we keep our own global list for that purpose. +#[cfg(not(target_thread_local))] +static DTORS: atomic::AtomicPtr = atomic::AtomicPtr::new(crate::ptr::null_mut()); + +/// Registers destructor to run at process exit. +#[cfg(not(target_thread_local))] +unsafe fn register_dtor(key: &'static LazyKey) { + crate::sys::thread_local::guard::enable(); + + let this = <*const LazyKey>::cast_mut(key); + // Use acquire ordering to pass along the changes done by the previously + // registered keys when we store the new head with release ordering. + let mut head = DTORS.load(Ordering::Acquire); + loop { + key.next.store(head, Ordering::Relaxed); + match DTORS.compare_exchange_weak(head, this, Ordering::Release, Ordering::Acquire) { + Ok(_) => break, + Err(new) => head = new, + } + } +} + +/// Run destructors at process exit. +/// +/// SAFETY: This will and must only be run by the destructor callback in [`guard`]. +#[cfg(not(target_thread_local))] +pub unsafe fn run_dtors() { + for _ in 0..5 { + let mut any_run = false; + + // Use acquire ordering to observe key initialization. + let mut cur = DTORS.load(Ordering::Acquire); + while !cur.is_null() { + let key = unsafe { (*cur).key.load(Ordering::Acquire) }; + let dtor = unsafe { (*cur).dtor.unwrap() }; + cur = unsafe { (*cur).next.load(Ordering::Relaxed) }; + + let ptr = unsafe { super::get(key as _) }; + if !ptr.is_null() { + unsafe { + super::set(key as _, crate::ptr::null_mut()); + dtor(ptr as *mut _); + any_run = true; + } + } + } + + if !any_run { + break; + } + } +} diff --git a/library/std/src/sys/thread_local/mod.rs b/library/std/src/sys/thread_local/mod.rs index f0a13323ec93f..438e9873eca4a 100644 --- a/library/std/src/sys/thread_local/mod.rs +++ b/library/std/src/sys/thread_local/mod.rs @@ -85,18 +85,16 @@ pub(crate) mod guard { } else if #[cfg(target_os = "windows")] { mod windows; pub(crate) use windows::enable; - } else if #[cfg(any( - all(target_family = "wasm", not( - all(target_os = "wasi", target_env = "p1", target_feature = "atomics") - )), - target_os = "uefi", - target_os = "zkvm", + } else if #[cfg(all( + target_family = "wasm", + target_feature = "atomics", + not(all(target_os = "wasi", target_env = "p1")) ))] { pub(crate) fn enable() { // FIXME: Right now there is no concept of "thread exit" on - // wasm, but this is likely going to show up at some point in - // the form of an exported symbol that the wasm runtime is going - // to be expected to call. For now we just leak everything, but + // wasm-unknown-unknown, but this is likely going to show up at some + // point in the form of an exported symbol that the wasm runtime is + // going to be expected to call. For now we just leak everything, but // if such a function starts to exist it will probably need to // iterate the destructor list with these functions: #[cfg(all(target_family = "wasm", target_feature = "atomics"))] @@ -115,6 +113,13 @@ pub(crate) mod guard { } else if #[cfg(target_os = "solid_asp3")] { mod solid; pub(crate) use solid::enable; + } else if #[cfg(any( + all(target_family = "wasm", not(target_feature = "atomics")), + target_os = "uefi", + target_os = "zkvm", + ))] { + mod statik; + pub(crate) use statik::enable; } else { mod key; pub(crate) use key::enable; @@ -144,6 +149,8 @@ pub(crate) mod key { #[cfg(test)] mod tests; pub(super) use racy::LazyKey; + #[cfg(not(target_thread_local))] + pub(super) use racy::run_dtors; pub(super) use unix::{Key, set}; #[cfg(any(not(target_thread_local), test))] pub(super) use unix::get; @@ -158,7 +165,7 @@ pub(crate) mod key { mod sgx; #[cfg(test)] mod tests; - pub(super) use racy::LazyKey; + pub(super) use racy::{LazyKey, run_dtors}; pub(super) use sgx::{Key, get, set}; use sgx::{create, destroy}; } else if #[cfg(target_os = "xous")] { @@ -174,6 +181,31 @@ pub(crate) mod key { } } +/// Process exit callback. +/// +/// Some platforms (POSIX) do not run TLS destructors at process exit. +/// Thus we need to register an exit callback to run them in that case. +pub(crate) mod exit { + cfg_if::cfg_if! { + if #[cfg(any( + all( + not(target_vendor = "apple"), + not(target_family = "wasm"), + target_family = "unix", + ), + target_os = "teeos", + all(target_os = "wasi", target_env = "p1"), + ))] { + mod unix; + pub(super) use unix::at_process_exit; + } else if #[cfg(target_family = "wasm")] { + pub unsafe fn at_process_exit(cb: unsafe extern "C" fn()) { + let _ = cb; + } + } + } +} + /// Run a callback in a scenario which must not unwind (such as a `extern "C" /// fn` declared in a user crate). If the callback unwinds anyway, then /// `rtabort` with a message about thread local panicking on drop. diff --git a/library/std/src/sys/thread_local/statik.rs b/library/std/src/sys/thread_local/statik.rs index 4da01a84acf68..eda2cc027ab7a 100644 --- a/library/std/src/sys/thread_local/statik.rs +++ b/library/std/src/sys/thread_local/statik.rs @@ -1,8 +1,9 @@ //! On some targets like wasm there's no threads, so no need to generate //! thread locals and we can instead just use plain statics! -use crate::cell::{Cell, UnsafeCell}; +use crate::cell::{Cell, RefCell, UnsafeCell}; use crate::ptr; +use crate::sys::thread_local::guard; #[doc(hidden)] #[allow_internal_unstable(thread_local_internals)] @@ -79,6 +80,10 @@ impl LazyStorage { #[cold] fn initialize(&'static self, i: Option<&mut Option>, f: impl FnOnce() -> T) -> *const T { + unsafe { + register_dtor(|| Self::destroy_value(&self.value)); + } + let value = i.and_then(Option::take).unwrap_or_else(f); // Destroy the old value, after updating the TLS variable as the // destructor might reference it. @@ -89,6 +94,13 @@ impl LazyStorage { // SAFETY: we just set this to `Some`. unsafe { (*self.value.get()).as_ref().unwrap_unchecked() } } + + /// Destroy contained value. + /// + /// Returns whether a value was contained. + fn destroy_value(value: &UnsafeCell>) -> bool { + unsafe { value.get().replace(None).is_some() } + } } // SAFETY: the target doesn't have threads. @@ -123,3 +135,40 @@ impl LocalPointer { // SAFETY: the target doesn't have threads. unsafe impl Sync for LocalPointer {} + +/// Destructor list wrapper. +struct Dtors(RefCell bool + 'static>>>); +// SAFETY: the target doesn't have threads. +unsafe impl Sync for Dtors {} + +/// List of destructors to run at process exit. +static DTORS: Dtors = Dtors(RefCell::new(Vec::new())); + +/// Registers destructor to run at process exit. +unsafe fn register_dtor(dtor: impl Fn() -> bool + 'static) { + guard::enable(); + + DTORS.0.borrow_mut().push(Box::new(dtor)); +} + +/// Run destructors at process exit. +/// +/// SAFETY: This will and must only be run by the destructor callback in [`guard`]. +pub unsafe fn run_dtors() { + let mut dtors = DTORS.0.take(); + + for _ in 0..5 { + let mut any_run = false; + for dtor in &dtors { + any_run |= dtor(); + } + + let mut new_dtors = DTORS.0.borrow_mut(); + + if !any_run && new_dtors.is_empty() { + break; + } + + dtors.extend(new_dtors.drain(..)); + } +} From 2494b97d07e5e20caa70b512c6426508a0381e94 Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Mon, 9 Dec 2024 17:57:43 +0100 Subject: [PATCH 02/11] Add test for main thread thread-local destructors --- tests/ui/thread-local/main-thread-dtor.rs | 28 +++++++++++++++++++ .../thread-local/main-thread-dtor.run.stdout | 2 ++ 2 files changed, 30 insertions(+) create mode 100644 tests/ui/thread-local/main-thread-dtor.rs create mode 100644 tests/ui/thread-local/main-thread-dtor.run.stdout diff --git a/tests/ui/thread-local/main-thread-dtor.rs b/tests/ui/thread-local/main-thread-dtor.rs new file mode 100644 index 0000000000000..aac836acd646a --- /dev/null +++ b/tests/ui/thread-local/main-thread-dtor.rs @@ -0,0 +1,28 @@ +//! Ensure that TLS destructors run on the main thread. +//@ run-pass +//@ check-run-results + +struct Bar; + +impl Drop for Bar { + fn drop(&mut self) { + println!("Bar dtor"); + } +} + +struct Foo; + +impl Drop for Foo { + fn drop(&mut self) { + println!("Foo dtor"); + // We initialize another thread-local inside the dtor, which is an interesting corner case. + thread_local!(static BAR: Bar = Bar); + BAR.with(|_| {}); + } +} + +thread_local!(static FOO: Foo = Foo); + +fn main() { + FOO.with(|_| {}); +} diff --git a/tests/ui/thread-local/main-thread-dtor.run.stdout b/tests/ui/thread-local/main-thread-dtor.run.stdout new file mode 100644 index 0000000000000..6160f2726492d --- /dev/null +++ b/tests/ui/thread-local/main-thread-dtor.run.stdout @@ -0,0 +1,2 @@ +Foo dtor +Bar dtor From 60cdb6a5be92080c89e9fceaece54f01c5056bc2 Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Mon, 13 Jan 2025 14:35:08 +0100 Subject: [PATCH 03/11] Remove unnecessary feature --- library/std/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs index c106310238a2b..5c12236617c98 100644 --- a/library/std/src/lib.rs +++ b/library/std/src/lib.rs @@ -279,7 +279,6 @@ #![feature(asm_experimental_arch)] #![feature(autodiff)] #![feature(cfg_sanitizer_cfi)] -#![feature(cfg_target_has_atomic)] #![feature(cfg_target_thread_local)] #![feature(cfi_encoding)] #![feature(concat_idents)] From bfea6a3ceac27ffcece8234b8d62c9ed11caf1a7 Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Mon, 13 Jan 2025 22:08:55 +0100 Subject: [PATCH 04/11] Use thread local list to store destructors --- library/std/src/sys/thread_local/key/racy.rs | 112 +++++++++++-------- 1 file changed, 67 insertions(+), 45 deletions(-) diff --git a/library/std/src/sys/thread_local/key/racy.rs b/library/std/src/sys/thread_local/key/racy.rs index 4fb928c613179..575ad9b26460e 100644 --- a/library/std/src/sys/thread_local/key/racy.rs +++ b/library/std/src/sys/thread_local/key/racy.rs @@ -17,9 +17,6 @@ pub struct LazyKey { key: AtomicUsize, /// Destructor for the TLS value. dtor: Option, - /// Next element of process-wide destructor list. - #[cfg(not(target_thread_local))] - next: atomic::AtomicPtr, } // Define a sentinel value that is likely not to be returned @@ -34,12 +31,7 @@ const KEY_SENTVAL: usize = libc::PTHREAD_KEYS_MAX + 1; impl LazyKey { pub const fn new(dtor: Option) -> LazyKey { - LazyKey { - key: atomic::AtomicUsize::new(KEY_SENTVAL), - dtor, - #[cfg(not(target_thread_local))] - next: atomic::AtomicPtr::new(crate::ptr::null_mut()), - } + LazyKey { key: atomic::AtomicUsize::new(KEY_SENTVAL), dtor } } #[inline] @@ -71,50 +63,79 @@ impl LazyKey { key2 }; rtassert!(key as usize != KEY_SENTVAL); - match self.key.compare_exchange( + + let final_key = match self.key.compare_exchange( KEY_SENTVAL, key as usize, Ordering::Release, Ordering::Acquire, ) { // The CAS succeeded, so we've created the actual key - Ok(_) => { - #[cfg(not(target_thread_local))] - if self.dtor.is_some() { - unsafe { register_dtor(self) }; - } - key as usize - } + Ok(_) => key as usize, // If someone beat us to the punch, use their key instead Err(n) => unsafe { super::destroy(key); n }, + }; + + #[cfg(not(target_thread_local))] + if self.dtor.is_some() { + unsafe { register_dtor(self) }; } + + final_key } } /// POSIX does not run TLS destructors on process exit. -/// Thus we keep our own global list for that purpose. +/// Thus we keep our own thread-local list for that purpose. #[cfg(not(target_thread_local))] -static DTORS: atomic::AtomicPtr = atomic::AtomicPtr::new(crate::ptr::null_mut()); +fn lazy_keys() -> &'static crate::cell::RefCell> { + static KEY: atomic::AtomicUsize = atomic::AtomicUsize::new(KEY_SENTVAL); + + unsafe extern "C" fn drop_lazy_keys(ptr: *mut u8) { + let ptr = ptr as *mut crate::cell::RefCell>; + if !ptr.is_null() { + drop(unsafe { Box::from_raw(ptr) }); + } + } + + // Allocate a TLS key to store the thread local destructor list. + let mut key = KEY.load(Ordering::Acquire) as super::Key; + if key == KEY_SENTVAL as _ { + let new_key = super::create(Some(drop_lazy_keys)); + match KEY.compare_exchange_weak( + KEY_SENTVAL, + new_key as _, + Ordering::Release, + Ordering::Acquire, + ) { + Ok(_) => key = new_key, + Err(other_key) => { + unsafe { super::destroy(new_key) }; + key = other_key as _; + } + } + } + + // And allocate the list for this thread if necessary. + let mut ptr = unsafe { super::get(key) as *const crate::cell::RefCell> }; + if ptr.is_null() { + let list = Box::new(crate::cell::RefCell::new(Vec::new())); + ptr = Box::into_raw(list); + unsafe { super::set(key, ptr as _) }; + } + + unsafe { &*ptr } +} /// Registers destructor to run at process exit. #[cfg(not(target_thread_local))] -unsafe fn register_dtor(key: &'static LazyKey) { +unsafe fn register_dtor(lazy_key: &'static LazyKey) { crate::sys::thread_local::guard::enable(); - let this = <*const LazyKey>::cast_mut(key); - // Use acquire ordering to pass along the changes done by the previously - // registered keys when we store the new head with release ordering. - let mut head = DTORS.load(Ordering::Acquire); - loop { - key.next.store(head, Ordering::Relaxed); - match DTORS.compare_exchange_weak(head, this, Ordering::Release, Ordering::Acquire) { - Ok(_) => break, - Err(new) => head = new, - } - } + lazy_keys().borrow_mut().push(lazy_key); } /// Run destructors at process exit. @@ -122,28 +143,29 @@ unsafe fn register_dtor(key: &'static LazyKey) { /// SAFETY: This will and must only be run by the destructor callback in [`guard`]. #[cfg(not(target_thread_local))] pub unsafe fn run_dtors() { + let lazy_keys_cell = lazy_keys(); + let mut lazy_keys = lazy_keys_cell.take(); + for _ in 0..5 { let mut any_run = false; - - // Use acquire ordering to observe key initialization. - let mut cur = DTORS.load(Ordering::Acquire); - while !cur.is_null() { - let key = unsafe { (*cur).key.load(Ordering::Acquire) }; - let dtor = unsafe { (*cur).dtor.unwrap() }; - cur = unsafe { (*cur).next.load(Ordering::Relaxed) }; - - let ptr = unsafe { super::get(key as _) }; - if !ptr.is_null() { - unsafe { - super::set(key as _, crate::ptr::null_mut()); - dtor(ptr as *mut _); + for lazy_key in &lazy_keys { + if let Some(dtor) = &lazy_key.dtor { + let key = lazy_key.force(); + let ptr = unsafe { super::get(key) }; + if !ptr.is_null() { + unsafe { dtor(ptr) }; + unsafe { super::set(key, crate::ptr::null_mut()) }; any_run = true; } } } - if !any_run { + let mut new_lazy_keys = lazy_keys_cell.borrow_mut(); + + if !any_run && new_lazy_keys.is_empty() { break; } + + lazy_keys.extend(new_lazy_keys.drain(..)); } } From 5b6cc98805daf85011d8e927a33f338a97f84557 Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Mon, 13 Jan 2025 22:31:15 +0100 Subject: [PATCH 05/11] Add TODO --- library/std/src/sys/thread_local/key/racy.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/std/src/sys/thread_local/key/racy.rs b/library/std/src/sys/thread_local/key/racy.rs index 575ad9b26460e..cb1f43aa679a2 100644 --- a/library/std/src/sys/thread_local/key/racy.rs +++ b/library/std/src/sys/thread_local/key/racy.rs @@ -79,6 +79,7 @@ impl LazyKey { }, }; + // TODO: This must be called for every thread that uses this LazyKey once! #[cfg(not(target_thread_local))] if self.dtor.is_some() { unsafe { register_dtor(self) }; From 8e9f7bcf50f5a0ebdb8d05aaf9b7e1eacf945bfb Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Tue, 14 Jan 2025 13:59:44 +0100 Subject: [PATCH 06/11] Add LazyKey to destructor list on value initialization --- library/std/src/sys/thread_local/key/racy.rs | 50 ++++++++------------ library/std/src/sys/thread_local/os.rs | 12 +++-- 2 files changed, 29 insertions(+), 33 deletions(-) diff --git a/library/std/src/sys/thread_local/key/racy.rs b/library/std/src/sys/thread_local/key/racy.rs index cb1f43aa679a2..83a7827e21a99 100644 --- a/library/std/src/sys/thread_local/key/racy.rs +++ b/library/std/src/sys/thread_local/key/racy.rs @@ -64,7 +64,7 @@ impl LazyKey { }; rtassert!(key as usize != KEY_SENTVAL); - let final_key = match self.key.compare_exchange( + match self.key.compare_exchange( KEY_SENTVAL, key as usize, Ordering::Release, @@ -77,15 +77,18 @@ impl LazyKey { super::destroy(key); n }, - }; + } + } - // TODO: This must be called for every thread that uses this LazyKey once! - #[cfg(not(target_thread_local))] - if self.dtor.is_some() { - unsafe { register_dtor(self) }; + /// Registers destructor to run at process exit. + #[cfg(not(target_thread_local))] + pub fn register_process_dtor(&'static self) { + if self.dtor.is_none() { + return; } - final_key + crate::sys::thread_local::guard::enable(); + lazy_keys().borrow_mut().push(self); } } @@ -131,42 +134,31 @@ fn lazy_keys() -> &'static crate::cell::RefCell> { unsafe { &*ptr } } -/// Registers destructor to run at process exit. -#[cfg(not(target_thread_local))] -unsafe fn register_dtor(lazy_key: &'static LazyKey) { - crate::sys::thread_local::guard::enable(); - - lazy_keys().borrow_mut().push(lazy_key); -} - /// Run destructors at process exit. /// /// SAFETY: This will and must only be run by the destructor callback in [`guard`]. #[cfg(not(target_thread_local))] pub unsafe fn run_dtors() { let lazy_keys_cell = lazy_keys(); - let mut lazy_keys = lazy_keys_cell.take(); for _ in 0..5 { let mut any_run = false; - for lazy_key in &lazy_keys { - if let Some(dtor) = &lazy_key.dtor { - let key = lazy_key.force(); - let ptr = unsafe { super::get(key) }; - if !ptr.is_null() { - unsafe { dtor(ptr) }; - unsafe { super::set(key, crate::ptr::null_mut()) }; - any_run = true; + + for lazy_key in lazy_keys_cell.take() { + let key = lazy_key.force(); + let ptr = unsafe { super::get(key) }; + if !ptr.is_null() { + // SAFETY: only keys with destructors are registered. + unsafe { + let Some(dtor) = &lazy_key.dtor else { crate::hint::unreachable_unchecked() }; + dtor(ptr); } + any_run = true; } } - let mut new_lazy_keys = lazy_keys_cell.borrow_mut(); - - if !any_run && new_lazy_keys.is_empty() { + if !any_run { break; } - - lazy_keys.extend(new_lazy_keys.drain(..)); } } diff --git a/library/std/src/sys/thread_local/os.rs b/library/std/src/sys/thread_local/os.rs index 00d2e30bd6036..bbc7feb906940 100644 --- a/library/std/src/sys/thread_local/os.rs +++ b/library/std/src/sys/thread_local/os.rs @@ -79,7 +79,11 @@ impl Storage { unsafe { &(*ptr).value } } else { // SAFETY: trivially correct. - unsafe { Self::try_initialize(key, ptr, i, f) } + let (ptr, was_null) = unsafe { Self::try_initialize(key, ptr, i, f) }; + if was_null { + self.key.register_process_dtor(); + } + ptr } } @@ -91,10 +95,10 @@ impl Storage { ptr: *mut Value, i: Option<&mut Option>, f: impl FnOnce() -> T, - ) -> *const T { + ) -> (*const T, bool) { if ptr.addr() == 1 { // destructor is running - return ptr::null(); + return (ptr::null(), false); } let value = Box::new(Value { value: i.and_then(Option::take).unwrap_or_else(f), key }); @@ -120,7 +124,7 @@ impl Storage { } // SAFETY: We just created this value above. - unsafe { &(*ptr).value } + (unsafe { &(*ptr).value }, old.is_null()) } } From 8f516220ea424e67e02a324446662c6872004515 Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Tue, 14 Jan 2025 14:12:12 +0100 Subject: [PATCH 07/11] Add test for process exit initiated by thread --- tests/ui/thread-local/spawned-thread-dtor.rs | 26 +++++++++++++++++++ .../spawned-thread-dtor.rs.stdout | 1 + 2 files changed, 27 insertions(+) create mode 100644 tests/ui/thread-local/spawned-thread-dtor.rs create mode 100644 tests/ui/thread-local/spawned-thread-dtor.rs.stdout diff --git a/tests/ui/thread-local/spawned-thread-dtor.rs b/tests/ui/thread-local/spawned-thread-dtor.rs new file mode 100644 index 0000000000000..4f01f8c4a6b93 --- /dev/null +++ b/tests/ui/thread-local/spawned-thread-dtor.rs @@ -0,0 +1,26 @@ +//! Ensure that TLS destructors run on a spawned thread that +//! exits the process. +//@ run-pass +//@ needs-threads +//@ check-run-results + +struct Foo; + +impl Drop for Foo { + fn drop(&mut self) { + println!("Foo dtor"); + } +} + +thread_local!(static FOO: Foo = Foo); + +fn main() { + std::thread::spawn(|| { + FOO.with(|_| {}); + std::process::exit(0); + }); + + loop { + std::thread::park(); + } +} diff --git a/tests/ui/thread-local/spawned-thread-dtor.rs.stdout b/tests/ui/thread-local/spawned-thread-dtor.rs.stdout new file mode 100644 index 0000000000000..9237ba8b1b40f --- /dev/null +++ b/tests/ui/thread-local/spawned-thread-dtor.rs.stdout @@ -0,0 +1 @@ +Foo dtor From 3d640a9ef8ad69a8eac8a8a86acd23b9124f243a Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Tue, 14 Jan 2025 14:13:12 +0100 Subject: [PATCH 08/11] Test: ensure thread does not initialize the TLS key --- tests/ui/thread-local/spawned-thread-dtor.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/ui/thread-local/spawned-thread-dtor.rs b/tests/ui/thread-local/spawned-thread-dtor.rs index 4f01f8c4a6b93..52d2586e7303c 100644 --- a/tests/ui/thread-local/spawned-thread-dtor.rs +++ b/tests/ui/thread-local/spawned-thread-dtor.rs @@ -15,6 +15,8 @@ impl Drop for Foo { thread_local!(static FOO: Foo = Foo); fn main() { + FOO.with(|_| {}); + std::thread::spawn(|| { FOO.with(|_| {}); std::process::exit(0); From cc68e0eebae624aa12d6510ef43b5275355c8c1e Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Tue, 14 Jan 2025 14:17:28 +0100 Subject: [PATCH 09/11] Fix Windows build --- library/std/src/sys/thread_local/key/windows.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/std/src/sys/thread_local/key/windows.rs b/library/std/src/sys/thread_local/key/windows.rs index f4e0f25a476ee..dd3d17f19d1f2 100644 --- a/library/std/src/sys/thread_local/key/windows.rs +++ b/library/std/src/sys/thread_local/key/windows.rs @@ -126,6 +126,10 @@ impl LazyKey { } } } + + pub fn register_process_dtor(&'static self) { + // On Windows destructor registration is performed in LazyKey::init. + } } unsafe impl Send for LazyKey {} From 58220d7249224838a131431072207328d8167bf4 Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Tue, 14 Jan 2025 14:30:46 +0100 Subject: [PATCH 10/11] Revert unnecessary changes --- library/std/src/sys/thread_local/key/racy.rs | 31 ++++---------------- 1 file changed, 5 insertions(+), 26 deletions(-) diff --git a/library/std/src/sys/thread_local/key/racy.rs b/library/std/src/sys/thread_local/key/racy.rs index 83a7827e21a99..d6b90b7d86000 100644 --- a/library/std/src/sys/thread_local/key/racy.rs +++ b/library/std/src/sys/thread_local/key/racy.rs @@ -35,14 +35,14 @@ impl LazyKey { } #[inline] - pub fn force(&'static self) -> super::Key { + pub fn force(&self) -> super::Key { match self.key.load(Ordering::Acquire) { KEY_SENTVAL => self.lazy_init() as super::Key, n => n as super::Key, } } - fn lazy_init(&'static self) -> usize { + fn lazy_init(&self) -> usize { // POSIX allows the key created here to be KEY_SENTVAL, but the compare_exchange // below relies on using KEY_SENTVAL as a sentinel value to check who won the // race to set the shared TLS key. As far as I know, there is no @@ -63,7 +63,6 @@ impl LazyKey { key2 }; rtassert!(key as usize != KEY_SENTVAL); - match self.key.compare_exchange( KEY_SENTVAL, key as usize, @@ -96,34 +95,14 @@ impl LazyKey { /// Thus we keep our own thread-local list for that purpose. #[cfg(not(target_thread_local))] fn lazy_keys() -> &'static crate::cell::RefCell> { - static KEY: atomic::AtomicUsize = atomic::AtomicUsize::new(KEY_SENTVAL); + static KEY: LazyKey = LazyKey::new(Some(drop_lazy_keys)); unsafe extern "C" fn drop_lazy_keys(ptr: *mut u8) { let ptr = ptr as *mut crate::cell::RefCell>; - if !ptr.is_null() { - drop(unsafe { Box::from_raw(ptr) }); - } - } - - // Allocate a TLS key to store the thread local destructor list. - let mut key = KEY.load(Ordering::Acquire) as super::Key; - if key == KEY_SENTVAL as _ { - let new_key = super::create(Some(drop_lazy_keys)); - match KEY.compare_exchange_weak( - KEY_SENTVAL, - new_key as _, - Ordering::Release, - Ordering::Acquire, - ) { - Ok(_) => key = new_key, - Err(other_key) => { - unsafe { super::destroy(new_key) }; - key = other_key as _; - } - } + drop(unsafe { Box::from_raw(ptr) }); } - // And allocate the list for this thread if necessary. + let key = KEY.force(); let mut ptr = unsafe { super::get(key) as *const crate::cell::RefCell> }; if ptr.is_null() { let list = Box::new(crate::cell::RefCell::new(Vec::new())); From 82b363b53ffeecc71f9516bb400d73f79f58b348 Mon Sep 17 00:00:00 2001 From: Sebastian Urban Date: Tue, 14 Jan 2025 20:10:08 +0100 Subject: [PATCH 11/11] Fix test stdout filename --- ...awned-thread-dtor.rs.stdout => spawned-thread-dtor.run.stdout} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/ui/thread-local/{spawned-thread-dtor.rs.stdout => spawned-thread-dtor.run.stdout} (100%) diff --git a/tests/ui/thread-local/spawned-thread-dtor.rs.stdout b/tests/ui/thread-local/spawned-thread-dtor.run.stdout similarity index 100% rename from tests/ui/thread-local/spawned-thread-dtor.rs.stdout rename to tests/ui/thread-local/spawned-thread-dtor.run.stdout