From 5f0531da05114b473cb04edce0d4374d516b9125 Mon Sep 17 00:00:00 2001 From: joboet Date: Thu, 14 Mar 2024 14:25:09 +0100 Subject: [PATCH 1/2] std: simplify key-based thread locals --- library/std/src/sys/thread_local/mod.rs | 83 ---------- library/std/src/sys/thread_local/os_local.rs | 162 +++++++------------ 2 files changed, 60 insertions(+), 185 deletions(-) diff --git a/library/std/src/sys/thread_local/mod.rs b/library/std/src/sys/thread_local/mod.rs index 36f6f907e7213..1acf1f8d8c16f 100644 --- a/library/std/src/sys/thread_local/mod.rs +++ b/library/std/src/sys/thread_local/mod.rs @@ -24,89 +24,6 @@ cfg_if::cfg_if! { } } -// Not used by the fast-local TLS anymore. -// FIXME(#110897): remove this. -#[allow(unused)] -mod lazy { - use crate::cell::UnsafeCell; - use crate::hint; - use crate::mem; - - pub struct LazyKeyInner { - inner: UnsafeCell>, - } - - impl LazyKeyInner { - pub const fn new() -> LazyKeyInner { - LazyKeyInner { inner: UnsafeCell::new(None) } - } - - pub unsafe fn get(&self) -> Option<&'static T> { - // SAFETY: The caller must ensure no reference is ever handed out to - // the inner cell nor mutable reference to the Option inside said - // cell. This make it safe to hand a reference, though the lifetime - // of 'static is itself unsafe, making the get method unsafe. - unsafe { (*self.inner.get()).as_ref() } - } - - /// The caller must ensure that no reference is active: this method - /// needs unique access. - pub unsafe fn initialize T>(&self, init: F) -> &'static T { - // Execute the initialization up front, *then* move it into our slot, - // just in case initialization fails. - let value = init(); - let ptr = self.inner.get(); - - // SAFETY: - // - // note that this can in theory just be `*ptr = Some(value)`, but due to - // the compiler will currently codegen that pattern with something like: - // - // ptr::drop_in_place(ptr) - // ptr::write(ptr, Some(value)) - // - // Due to this pattern it's possible for the destructor of the value in - // `ptr` (e.g., if this is being recursively initialized) to re-access - // TLS, in which case there will be a `&` and `&mut` pointer to the same - // value (an aliasing violation). To avoid setting the "I'm running a - // destructor" flag we just use `mem::replace` which should sequence the - // operations a little differently and make this safe to call. - // - // The precondition also ensures that we are the only one accessing - // `self` at the moment so replacing is fine. - unsafe { - let _ = mem::replace(&mut *ptr, Some(value)); - } - - // SAFETY: With the call to `mem::replace` it is guaranteed there is - // a `Some` behind `ptr`, not a `None` so `unreachable_unchecked` - // will never be reached. - unsafe { - // After storing `Some` we want to get a reference to the contents of - // what we just stored. While we could use `unwrap` here and it should - // always work it empirically doesn't seem to always get optimized away, - // which means that using something like `try_with` can pull in - // panicking code and cause a large size bloat. - match *ptr { - Some(ref x) => x, - None => hint::unreachable_unchecked(), - } - } - } - - /// Watch out: unsynchronized internal mutability! - /// - /// # Safety - /// Causes UB if any reference to the value is used after this. - #[allow(unused)] - pub(crate) unsafe fn take(&self) -> Option { - let mutable: *mut _ = UnsafeCell::get(&self.inner); - // SAFETY: That's the caller's problem. - unsafe { mutable.replace(None) } - } - } -} - /// 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/os_local.rs b/library/std/src/sys/thread_local/os_local.rs index 3edffd7e4437c..6b24fe254a15e 100644 --- a/library/std/src/sys/thread_local/os_local.rs +++ b/library/std/src/sys/thread_local/os_local.rs @@ -1,7 +1,8 @@ -use super::lazy::LazyKeyInner; +use super::abort_on_dtor_unwind; use crate::cell::Cell; -use crate::sys_common::thread_local_key::StaticKey as OsStaticKey; -use crate::{fmt, marker, panic, ptr}; +use crate::marker::PhantomData; +use crate::sys_common::thread_local_key::StaticKey as OsKey; +use crate::{panic, ptr}; #[doc(hidden)] #[allow_internal_unstable(thread_local_internals)] @@ -10,38 +11,9 @@ use crate::{fmt, marker, panic, ptr}; #[rustc_macro_transparency = "semitransparent"] pub macro thread_local_inner { // used to generate the `LocalKey` value for const-initialized thread locals - (@key $t:ty, const $init:expr) => {{ - #[inline] - #[deny(unsafe_op_in_unsafe_fn)] - unsafe fn __getit( - _init: $crate::option::Option<&mut $crate::option::Option<$t>>, - ) -> $crate::option::Option<&'static $t> { - const INIT_EXPR: $t = $init; - - // On platforms without `#[thread_local]` we fall back to the - // same implementation as below for os thread locals. - #[inline] - const fn __init() -> $t { INIT_EXPR } - static __KEY: $crate::thread::local_impl::Key<$t> = - $crate::thread::local_impl::Key::new(); - unsafe { - __KEY.get(move || { - if let $crate::option::Option::Some(init) = _init { - if let $crate::option::Option::Some(value) = init.take() { - return value; - } else if $crate::cfg!(debug_assertions) { - $crate::unreachable!("missing initial value"); - } - } - __init() - }) - } - } - - unsafe { - $crate::thread::LocalKey::new(__getit) - } - }}, + (@key $t:ty, const $init:expr) => { + $crate::thread::local_impl::thread_local_inner!(@key $t, { const INIT_EXPR: $t = $init; INIT_EXPR }) + }, // used to generate the `LocalKey` value for `thread_local!` (@key $t:ty, $init:expr) => { @@ -55,20 +27,11 @@ pub macro thread_local_inner { unsafe fn __getit( init: $crate::option::Option<&mut $crate::option::Option<$t>>, ) -> $crate::option::Option<&'static $t> { - static __KEY: $crate::thread::local_impl::Key<$t> = - $crate::thread::local_impl::Key::new(); + use $crate::thread::local_impl::Key; + static __KEY: Key<$t> = Key::new(); unsafe { - __KEY.get(move || { - if let $crate::option::Option::Some(init) = init { - if let $crate::option::Option::Some(value) = init.take() { - return value; - } else if $crate::cfg!(debug_assertions) { - $crate::unreachable!("missing default value"); - } - } - __init() - }) + __KEY.get(init, __init) } } @@ -85,78 +48,78 @@ pub macro thread_local_inner { /// Use a regular global static to store this key; the state provided will then be /// thread-local. +#[allow(missing_debug_implementations)] pub struct Key { - // OS-TLS key that we'll use to key off. - os: OsStaticKey, - marker: marker::PhantomData>, -} - -impl fmt::Debug for Key { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("Key").finish_non_exhaustive() - } + os: OsKey, + marker: PhantomData>, } unsafe impl Sync for Key {} struct Value { - inner: LazyKeyInner, + value: T, key: &'static Key, } impl Key { #[rustc_const_unstable(feature = "thread_local_internals", issue = "none")] pub const fn new() -> Key { - Key { os: OsStaticKey::new(Some(destroy_value::)), marker: marker::PhantomData } + Key { os: OsKey::new(Some(destroy_value::)), marker: PhantomData } } - /// It is a requirement for the caller to ensure that no mutable - /// reference is active when this method is called. - pub unsafe fn get(&'static self, init: impl FnOnce() -> T) -> Option<&'static T> { - // SAFETY: See the documentation for this method. + /// Get the value associated with this key, initializating it if necessary. + /// + /// # Safety + /// * the returned reference must not be used after recursive initialization + /// or thread destruction occurs. + pub unsafe fn get( + &'static self, + i: Option<&mut Option>, + f: impl FnOnce() -> T, + ) -> Option<&'static T> { + // SAFETY: (FIXME: get should actually be safe) let ptr = unsafe { self.os.get() as *mut Value }; if ptr.addr() > 1 { // SAFETY: the check ensured the pointer is safe (its destructor // is not running) + it is coming from a trusted source (self). - if let Some(ref value) = unsafe { (*ptr).inner.get() } { - return Some(value); - } + unsafe { Some(&(*ptr).value) } + } else { + // SAFETY: At this point we are sure we have no value and so + // initializing (or trying to) is safe. + unsafe { self.try_initialize(ptr, i, f) } } - // SAFETY: At this point we are sure we have no value and so - // initializing (or trying to) is safe. - unsafe { self.try_initialize(init) } } - // `try_initialize` is only called once per os thread local variable, - // except in corner cases where thread_local dtors reference other - // thread_local's, or it is being recursively initialized. - unsafe fn try_initialize(&'static self, init: impl FnOnce() -> T) -> Option<&'static T> { - // SAFETY: No mutable references are ever handed out meaning getting - // the value is ok. - let ptr = unsafe { self.os.get() as *mut Value }; + unsafe fn try_initialize( + &'static self, + ptr: *mut Value, + i: Option<&mut Option>, + f: impl FnOnce() -> T, + ) -> Option<&'static T> { if ptr.addr() == 1 { // destructor is running return None; } - let ptr = if ptr.is_null() { - // If the lookup returned null, we haven't initialized our own - // local copy, so do that now. - let ptr = Box::into_raw(Box::new(Value { inner: LazyKeyInner::new(), key: self })); - // SAFETY: At this point we are sure there is no value inside - // ptr so setting it will not affect anyone else. - unsafe { - self.os.set(ptr as *mut u8); - } - ptr - } else { - // recursive initialization - ptr - }; + let value = i.and_then(Option::take).unwrap_or_else(f); + let ptr = Box::into_raw(Box::new(Value { value, key: self })); + // SAFETY: (FIXME: get should actually be safe) + let old = unsafe { self.os.get() as *mut Value }; + // SAFETY: `ptr` is a correct pointer that can be destroyed by the key destructor. + unsafe { + self.os.set(ptr as *mut u8); + } + if !old.is_null() { + // If the variable was recursively initialized, drop the old value. + // SAFETY: We cannot be inside a `LocalKey::with` scope, as the + // initializer has already returned and the next scope only starts + // after we return the pointer. Therefore, there can be no references + // to the old value. + drop(unsafe { Box::from_raw(old) }); + } - // SAFETY: ptr has been ensured as non-NUL just above an so can be - // dereferenced safely. - unsafe { Some((*ptr).inner.initialize(init)) } + // SAFETY: We just created this value above. + unsafe { Some(&(*ptr).value) } } } @@ -170,16 +133,11 @@ unsafe extern "C" fn destroy_value(ptr: *mut u8) { // // Note that to prevent an infinite loop we reset it back to null right // before we return from the destructor ourselves. - // - // Wrap the call in a catch to ensure unwinding is caught in the event - // a panic takes place in a destructor. - if let Err(_) = panic::catch_unwind(|| unsafe { - let ptr = Box::from_raw(ptr as *mut Value); + abort_on_dtor_unwind(|| { + let ptr = unsafe { Box::from_raw(ptr as *mut Value) }; let key = ptr.key; - key.os.set(ptr::without_provenance_mut(1)); + unsafe { key.os.set(ptr::without_provenance_mut(1)) }; drop(ptr); - key.os.set(ptr::null_mut()); - }) { - rtabort!("thread local panicked on drop"); - } + unsafe { key.os.set(ptr::null_mut()) }; + }); } From 0e7e75ebca2ee9cc9a6c019110797250094908f5 Mon Sep 17 00:00:00 2001 From: joboet Date: Fri, 24 May 2024 11:46:09 +0200 Subject: [PATCH 2/2] std: clean up the TLS implementation --- library/std/src/sys/mod.rs | 2 -- library/std/src/sys/thread_local/fast_local/lazy.rs | 1 - library/std/src/sys/thread_local/mod.rs | 4 +++- library/std/src/sys/thread_local/os_local.rs | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/library/std/src/sys/mod.rs b/library/std/src/sys/mod.rs index bbd1d840e92dc..8f70cefc60121 100644 --- a/library/std/src/sys/mod.rs +++ b/library/std/src/sys/mod.rs @@ -9,8 +9,6 @@ pub mod cmath; pub mod os_str; pub mod path; pub mod sync; -#[allow(dead_code)] -#[allow(unused_imports)] pub mod thread_local; // FIXME(117276): remove this, move feature implementations into individual diff --git a/library/std/src/sys/thread_local/fast_local/lazy.rs b/library/std/src/sys/thread_local/fast_local/lazy.rs index 14371768d30ce..c2e9a17145468 100644 --- a/library/std/src/sys/thread_local/fast_local/lazy.rs +++ b/library/std/src/sys/thread_local/fast_local/lazy.rs @@ -1,6 +1,5 @@ use crate::cell::UnsafeCell; use crate::hint::unreachable_unchecked; -use crate::mem::forget; use crate::ptr; use crate::sys::thread_local::abort_on_dtor_unwind; use crate::sys::thread_local_dtor::register_dtor; diff --git a/library/std/src/sys/thread_local/mod.rs b/library/std/src/sys/thread_local/mod.rs index 1acf1f8d8c16f..0a78a1a1cf02d 100644 --- a/library/std/src/sys/thread_local/mod.rs +++ b/library/std/src/sys/thread_local/mod.rs @@ -1,4 +1,5 @@ #![unstable(feature = "thread_local_internals", reason = "should not be necessary", issue = "none")] +#![cfg_attr(test, allow(unused))] // There are three thread-local implementations: "static", "fast", "OS". // The "OS" thread local key type is accessed via platform-specific API calls and is slow, while the @@ -28,7 +29,8 @@ cfg_if::cfg_if! { /// fn` declared in a user crate). If the callback unwinds anyway, then /// `rtabort` with a message about thread local panicking on drop. #[inline] -pub fn abort_on_dtor_unwind(f: impl FnOnce()) { +#[allow(dead_code)] +fn abort_on_dtor_unwind(f: impl FnOnce()) { // Using a guard like this is lower cost. let guard = DtorUnwindGuard; f(); diff --git a/library/std/src/sys/thread_local/os_local.rs b/library/std/src/sys/thread_local/os_local.rs index 6b24fe254a15e..d6ddbb78a9c86 100644 --- a/library/std/src/sys/thread_local/os_local.rs +++ b/library/std/src/sys/thread_local/os_local.rs @@ -1,8 +1,8 @@ use super::abort_on_dtor_unwind; use crate::cell::Cell; use crate::marker::PhantomData; +use crate::ptr; use crate::sys_common::thread_local_key::StaticKey as OsKey; -use crate::{panic, ptr}; #[doc(hidden)] #[allow_internal_unstable(thread_local_internals)]