From addbc8e273a222bd312d025f5896ff7525f31bb1 Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Sat, 6 Apr 2024 13:00:19 +0100 Subject: [PATCH 01/10] Remove last rt::init allocation for thread info --- library/std/src/lib.rs | 1 + library/std/src/thread/mod.rs | 122 +++++++++++++++++++++++----------- 2 files changed, 83 insertions(+), 40 deletions(-) diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs index fb63445c22a93..74e421bc5bd63 100644 --- a/library/std/src/lib.rs +++ b/library/std/src/lib.rs @@ -356,6 +356,7 @@ #![feature(str_internals)] #![feature(strict_provenance)] #![feature(strict_provenance_atomic_ptr)] +#![feature(sync_unsafe_cell)] #![feature(ub_checks)] // tidy-alphabetical-end // diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index 604eb05040b2d..f3420ac07280d 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -159,12 +159,14 @@ mod tests; use crate::any::Any; +use crate::cell::SyncUnsafeCell; use crate::cell::{OnceCell, UnsafeCell}; use crate::env; use crate::ffi::{CStr, CString}; use crate::fmt; use crate::io; use crate::marker::PhantomData; +use crate::mem::MaybeUninit; use crate::mem::{self, forget}; use crate::num::NonZero; use crate::panic; @@ -530,7 +532,7 @@ impl Builder { let f = MaybeDangling::new(f); let main = move || { - if let Some(name) = their_thread.cname() { + if let Some(name) = their_thread.0.name() { imp::Thread::set_name(name); } @@ -1162,7 +1164,7 @@ pub fn park_timeout(dur: Duration) { let guard = PanicGuard; // SAFETY: park_timeout is called on the parker owned by this thread. unsafe { - current().inner.as_ref().parker().park_timeout(dur); + current().0.parker().park_timeout(dur); } // No panic occurred, do not abort. forget(guard); @@ -1201,7 +1203,12 @@ pub fn park_timeout(dur: Duration) { pub struct ThreadId(NonZero); impl ThreadId { - // Generate a new unique thread ID. + /// Generate a new unique thread ID. + /// + /// The current implementation starts at 2 and increments from there. + /// + /// This is as `1` is the value for the main thread, so std::thread::Thread does not + /// have to store this value when creating the main thread's information. fn new() -> ThreadId { #[cold] fn exhausted() -> ! { @@ -1212,7 +1219,7 @@ impl ThreadId { if #[cfg(target_has_atomic = "64")] { use crate::sync::atomic::AtomicU64; - static COUNTER: AtomicU64 = AtomicU64::new(0); + static COUNTER: AtomicU64 = AtomicU64::new(1); let mut last = COUNTER.load(Ordering::Relaxed); loop { @@ -1228,7 +1235,7 @@ impl ThreadId { } else { use crate::sync::{Mutex, PoisonError}; - static COUNTER: Mutex = Mutex::new(0); + static COUNTER: Mutex = Mutex::new(1); let mut counter = COUNTER.lock().unwrap_or_else(PoisonError::into_inner); let Some(id) = counter.checked_add(1) else { @@ -1245,6 +1252,11 @@ impl ThreadId { } } + /// Creates a ThreadId with the ID of the main thread. + fn new_main() -> Self { + Self(NonZero::::MIN) + } + /// This returns a numeric identifier for the thread identified by this /// `ThreadId`. /// @@ -1264,23 +1276,54 @@ impl ThreadId { // Thread //////////////////////////////////////////////////////////////////////////////// -/// The internal representation of a `Thread`'s name. -enum ThreadName { - Main, - Other(CString), - Unnamed, -} +/// The parker for the main thread. This avoids having to allocate an Arc in `fn main() {}`. +static MAIN_PARKER: SyncUnsafeCell> = + SyncUnsafeCell::new(MaybeUninit::uninit()); -/// The internal representation of a `Thread` handle -struct Inner { - name: ThreadName, // Guaranteed to be UTF-8 +/// The internal representation of a `Thread` that is not the main thread. +struct OtherInner { + name: Option, // Guaranteed to be UTF-8 id: ThreadId, parker: Parker, } +/// The internal representation of a `Thread` handle. +#[derive(Clone)] +enum Inner { + Main, + Other(Pin>), +} + impl Inner { - fn parker(self: Pin<&Self>) -> Pin<&Parker> { - unsafe { Pin::map_unchecked(self, |inner| &inner.parker) } + fn id(&self) -> ThreadId { + match self { + Self::Main => ThreadId::new_main(), + Self::Other(other) => other.id, + } + } + + fn name(&self) -> Option<&CStr> { + match self { + Self::Main => Some(c"main"), + Self::Other(other) => other.name.as_deref(), + } + } + + fn parker(&self) -> Pin<&Parker> { + match self { + Self::Main => { + // Safety: MAIN_PARKER only ever has a mutable reference when Inner::Main is initialised. + let static_ref: &'static MaybeUninit = unsafe { &*MAIN_PARKER.get() }; + + // Safety: MAIN_PARKER is initialised when Inner::Main is initialised. + let parker_ref = unsafe { static_ref.assume_init_ref() }; + + Pin::static_ref(parker_ref) + } + Self::Other(inner) => unsafe { + Pin::map_unchecked(inner.as_ref(), |inner| &inner.parker) + }, + } } } @@ -1304,9 +1347,7 @@ impl Inner { /// docs of [`Builder`] and [`spawn`] for more details. /// /// [`thread::current`]: current -pub struct Thread { - inner: Pin>, -} +pub struct Thread(Inner); impl Thread { /// Used only internally to construct a thread object without spawning. @@ -1314,28 +1355,37 @@ impl Thread { /// # Safety /// `name` must be valid UTF-8. pub(crate) unsafe fn new(name: CString) -> Thread { - unsafe { Self::new_inner(ThreadName::Other(name)) } + unsafe { Self::new_inner(Some(name)) } } pub(crate) fn new_unnamed() -> Thread { - unsafe { Self::new_inner(ThreadName::Unnamed) } + unsafe { Self::new_inner(None) } } - // Used in runtime to construct main thread - pub(crate) fn new_main() -> Thread { - unsafe { Self::new_inner(ThreadName::Main) } + /// Used in runtime to construct main thread + /// + /// # Safety + /// + /// This must only ever be called once, and must be called on the main thread. + pub(crate) unsafe fn new_main() -> Thread { + // Safety: Caller responsible for holding the mutable invariant and + // Parker::new_in_place does not read from the uninit value. + unsafe { Parker::new_in_place(MAIN_PARKER.get().cast()) } + + Self(Inner::Main) } /// # Safety - /// If `name` is `ThreadName::Other(_)`, the contained string must be valid UTF-8. - unsafe fn new_inner(name: ThreadName) -> Thread { + /// + /// If `name` is `Some(_)`, the contained string must be valid UTF-8. + unsafe fn new_inner(name: Option) -> Thread { // We have to use `unsafe` here to construct the `Parker` in-place, // which is required for the UNIX implementation. // // SAFETY: We pin the Arc immediately after creation, so its address never // changes. let inner = unsafe { - let mut arc = Arc::::new_uninit(); + let mut arc = Arc::::new_uninit(); let ptr = Arc::get_mut_unchecked(&mut arc).as_mut_ptr(); addr_of_mut!((*ptr).name).write(name); addr_of_mut!((*ptr).id).write(ThreadId::new()); @@ -1343,7 +1393,7 @@ impl Thread { Pin::new_unchecked(arc.assume_init()) }; - Thread { inner } + Self(Inner::Other(inner)) } /// Like the public [`park`], but callable on any handle. This is used to @@ -1352,7 +1402,7 @@ impl Thread { /// # Safety /// May only be called from the thread to which this handle belongs. pub(crate) unsafe fn park(&self) { - unsafe { self.inner.as_ref().parker().park() } + unsafe { self.0.parker().park() } } /// Atomically makes the handle's token available if it is not already. @@ -1388,7 +1438,7 @@ impl Thread { #[stable(feature = "rust1", since = "1.0.0")] #[inline] pub fn unpark(&self) { - self.inner.as_ref().parker().unpark(); + self.0.parker().unpark(); } /// Gets the thread's unique identifier. @@ -1408,7 +1458,7 @@ impl Thread { #[stable(feature = "thread_id", since = "1.19.0")] #[must_use] pub fn id(&self) -> ThreadId { - self.inner.id + self.0.id() } /// Gets the thread's name. @@ -1451,15 +1501,7 @@ impl Thread { #[stable(feature = "rust1", since = "1.0.0")] #[must_use] pub fn name(&self) -> Option<&str> { - self.cname().map(|s| unsafe { str::from_utf8_unchecked(s.to_bytes()) }) - } - - fn cname(&self) -> Option<&CStr> { - match &self.inner.name { - ThreadName::Main => Some(c"main"), - ThreadName::Other(other) => Some(&other), - ThreadName::Unnamed => None, - } + self.0.name().map(|s| unsafe { str::from_utf8_unchecked(s.to_bytes()) }) } } From f7f4025d4528f7b6199ae1f85a6cf99726e3c553 Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Sat, 6 Apr 2024 14:52:39 +0100 Subject: [PATCH 02/10] Fix test relying on allocation pre-main --- tests/rustdoc/demo-allocator-54478.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/rustdoc/demo-allocator-54478.rs b/tests/rustdoc/demo-allocator-54478.rs index dd98e80f03ade..80acfc0ff58a1 100644 --- a/tests/rustdoc/demo-allocator-54478.rs +++ b/tests/rustdoc/demo-allocator-54478.rs @@ -40,6 +40,7 @@ //! } //! //! fn main() { +//! drop(String::from("An allocation")); //! assert!(unsafe { HIT }); //! } //! ``` From 3c98cac21c069c6b7228829e16e8fa713245ed3b Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Sat, 6 Apr 2024 18:57:46 +0100 Subject: [PATCH 03/10] Add UI test to ensure no more allocations pre-main --- tests/ui/runtime/aborting-alloc.rs | 26 +++++++++++++++++++ tests/ui/runtime/no-allocation-before-main.rs | 13 ++++++++++ 2 files changed, 39 insertions(+) create mode 100644 tests/ui/runtime/aborting-alloc.rs create mode 100644 tests/ui/runtime/no-allocation-before-main.rs diff --git a/tests/ui/runtime/aborting-alloc.rs b/tests/ui/runtime/aborting-alloc.rs new file mode 100644 index 0000000000000..f078e7dc3f60e --- /dev/null +++ b/tests/ui/runtime/aborting-alloc.rs @@ -0,0 +1,26 @@ +//! Helper for 'no-allocation-before-main'. +//! +//! This also contains a meta-test to make sure that the AbortingAllocator does indeed abort. +//! +//! -Cprefer-dynamic=no is required as otherwise #[global_allocator] does nothing. +//@ run-fail +//@ compile-flags: -Cprefer-dynamic=no + +pub struct AbortingAllocator; + +unsafe impl std::alloc::GlobalAlloc for AbortingAllocator { + unsafe fn alloc(&self, _: std::alloc::Layout) -> *mut u8 { + std::process::abort() + } + + unsafe fn dealloc(&self, _: *mut u8, _: std::alloc::Layout) { + std::process::abort() + } +} + +#[global_allocator] +static ALLOCATOR: AbortingAllocator = AbortingAllocator; + +fn main() { + std::hint::black_box(String::from("An allocation")); +} diff --git a/tests/ui/runtime/no-allocation-before-main.rs b/tests/ui/runtime/no-allocation-before-main.rs new file mode 100644 index 0000000000000..6c4e06d946fcb --- /dev/null +++ b/tests/ui/runtime/no-allocation-before-main.rs @@ -0,0 +1,13 @@ +//! Tests that a program with no body does not allocate. +//! +//! The initial runtime should not allocate for performance/binary size reasons. +//! +//! -Cprefer-dynamic=no is required as otherwise #[global_allocator] does nothing. +//@ run-pass +//@ compile-flags: -Cprefer-dynamic=no + +#[allow(dead_code)] +#[path = "aborting-alloc.rs"] +mod aux; + +fn main() {} From 2769dd6925935d116b3fac489c2483af1be236e0 Mon Sep 17 00:00:00 2001 From: Gnome! Date: Tue, 23 Apr 2024 17:28:46 +0000 Subject: [PATCH 04/10] Add additional safety documentation Co-authored-by: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> --- library/std/src/thread/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index f3420ac07280d..2539454e152d6 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -1312,7 +1312,10 @@ impl Inner { fn parker(&self) -> Pin<&Parker> { match self { Self::Main => { - // Safety: MAIN_PARKER only ever has a mutable reference when Inner::Main is initialised. + // Safety: MAIN_PARKER is only ever read in this function, which requires access + // to an existing `&Inner` value, which can only be accessed via the main thread + // giving away such an instance from `current()`, implying that initialization, + // the only write to `MAIN_PARKER`, has been completed. let static_ref: &'static MaybeUninit = unsafe { &*MAIN_PARKER.get() }; // Safety: MAIN_PARKER is initialised when Inner::Main is initialised. From 2e56bc202cc26a4a1feafcf03a5904bfd931335e Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Wed, 24 Apr 2024 12:16:23 +0100 Subject: [PATCH 05/10] Reword new_main safety comment --- library/std/src/thread/mod.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index 2539454e152d6..06db2db118967 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -1371,8 +1371,10 @@ impl Thread { /// /// This must only ever be called once, and must be called on the main thread. pub(crate) unsafe fn new_main() -> Thread { - // Safety: Caller responsible for holding the mutable invariant and - // Parker::new_in_place does not read from the uninit value. + // Safety: As this is only called once and on the main thread, nothing else is accessing MAIN_PARKER + // as the only other read occurs after Inner::Main has been constructed, after this call finishes. + // + // Pre-main thread spawning cannot hit this either, as the caller holds that this is only called on the main thread. unsafe { Parker::new_in_place(MAIN_PARKER.get().cast()) } Self(Inner::Main) From 282fd6df4982008803c0474de24568dcc4a6610c Mon Sep 17 00:00:00 2001 From: Gnome! Date: Wed, 24 Apr 2024 13:30:11 +0100 Subject: [PATCH 06/10] More documentation improvements Co-authored-by: Ralf Jung --- library/std/src/thread/mod.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index 06db2db118967..3a0b68f8d62ba 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -1290,7 +1290,9 @@ struct OtherInner { /// The internal representation of a `Thread` handle. #[derive(Clone)] enum Inner { + /// Represents the main thread. May only be constructed by Thread::new_main. Main, + /// Represents any other thread. Other(Pin>), } @@ -1372,9 +1374,10 @@ impl Thread { /// This must only ever be called once, and must be called on the main thread. pub(crate) unsafe fn new_main() -> Thread { // Safety: As this is only called once and on the main thread, nothing else is accessing MAIN_PARKER - // as the only other read occurs after Inner::Main has been constructed, after this call finishes. + // as the only other read occurs in Inner::parker *after* Inner::Main has been constructed, + // and this function is the only one that constructs Inner::Main. // - // Pre-main thread spawning cannot hit this either, as the caller holds that this is only called on the main thread. + // Pre-main thread spawning cannot hit this either, as the caller promises that this is only called on the main thread. unsafe { Parker::new_in_place(MAIN_PARKER.get().cast()) } Self(Inner::Main) From 398c74f858898e08b725cfa715b4abe2a6ebd99b Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Thu, 25 Apr 2024 21:24:45 +0100 Subject: [PATCH 07/10] Print backtrace in AbortingAllocator --- tests/ui/runtime/aborting-alloc.rs | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/tests/ui/runtime/aborting-alloc.rs b/tests/ui/runtime/aborting-alloc.rs index f078e7dc3f60e..167c654d50d20 100644 --- a/tests/ui/runtime/aborting-alloc.rs +++ b/tests/ui/runtime/aborting-alloc.rs @@ -6,20 +6,34 @@ //@ run-fail //@ compile-flags: -Cprefer-dynamic=no -pub struct AbortingAllocator; +use std::{sync::atomic::{AtomicBool, Ordering}, alloc::System}; + +static ABORT: AtomicBool = AtomicBool::new(true); + +pub struct AbortingAllocator(System); unsafe impl std::alloc::GlobalAlloc for AbortingAllocator { - unsafe fn alloc(&self, _: std::alloc::Layout) -> *mut u8 { - std::process::abort() + unsafe fn alloc(&self, layout: std::alloc::Layout) -> *mut u8 { + if ABORT.swap(false, Ordering::SeqCst) { + println!("{}", std::backtrace::Backtrace::force_capture()); + std::process::abort(); + } + + self.0.alloc(layout) } - unsafe fn dealloc(&self, _: *mut u8, _: std::alloc::Layout) { - std::process::abort() + unsafe fn dealloc(&self, ptr: *mut u8, layout: std::alloc::Layout) { + if ABORT.swap(false, Ordering::SeqCst) { + println!("{}", std::backtrace::Backtrace::force_capture()); + std::process::abort(); + } + + self.0.dealloc(ptr, layout) } } #[global_allocator] -static ALLOCATOR: AbortingAllocator = AbortingAllocator; +static ALLOCATOR: AbortingAllocator = AbortingAllocator(System); fn main() { std::hint::black_box(String::from("An allocation")); From d635cb04fe8a1a1e505da8088ad5cd344491432d Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Thu, 25 Apr 2024 21:25:04 +0100 Subject: [PATCH 08/10] Only run no-allocation-before-main on linux-gnu. --- tests/ui/runtime/no-allocation-before-main.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/ui/runtime/no-allocation-before-main.rs b/tests/ui/runtime/no-allocation-before-main.rs index 6c4e06d946fcb..536f3e045ea06 100644 --- a/tests/ui/runtime/no-allocation-before-main.rs +++ b/tests/ui/runtime/no-allocation-before-main.rs @@ -3,8 +3,11 @@ //! The initial runtime should not allocate for performance/binary size reasons. //! //! -Cprefer-dynamic=no is required as otherwise #[global_allocator] does nothing. +//! We only test linux-gnu as other targets currently need allocation for thread dtors. //@ run-pass //@ compile-flags: -Cprefer-dynamic=no +//@ only-linux +//@ only-gnu #[allow(dead_code)] #[path = "aborting-alloc.rs"] From 36ea953b9790eb8d6f22c94f5fb7f0fdafc5e4f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20W=C3=B6rister?= Date: Mon, 29 Apr 2024 11:05:00 +0200 Subject: [PATCH 09/10] Update debuginfo/thread.rs test to account for changed type definition in libstd. --- tests/debuginfo/thread.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/debuginfo/thread.rs b/tests/debuginfo/thread.rs index 0415f586f5d90..db7e166dbb4b8 100644 --- a/tests/debuginfo/thread.rs +++ b/tests/debuginfo/thread.rs @@ -12,9 +12,10 @@ // cdb-check:join_handle,d [Type: std::thread::JoinHandle >] // cdb-check: [...] __0 [Type: std::thread::JoinInner >] // -// cdb-command:dx t,d +// cdb-command:dx -r3 t,d // cdb-check:t,d : [...] [Type: std::thread::Thread *] -// cdb-check:[...] inner [...][Type: core::pin::Pin >] +// cdb-check: [...] __0 : Other [Type: enum2$] +// cdb-check: [...] __0 [Type: core::pin::Pin >] use std::thread; From 519fe58175d4e1e36b5a1fef486f3ddb4928a1e7 Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Tue, 30 Apr 2024 12:54:04 +0100 Subject: [PATCH 10/10] Add debuginfo to no-allocation test --- tests/ui/runtime/no-allocation-before-main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/runtime/no-allocation-before-main.rs b/tests/ui/runtime/no-allocation-before-main.rs index 536f3e045ea06..041838ac63b9d 100644 --- a/tests/ui/runtime/no-allocation-before-main.rs +++ b/tests/ui/runtime/no-allocation-before-main.rs @@ -5,7 +5,7 @@ //! -Cprefer-dynamic=no is required as otherwise #[global_allocator] does nothing. //! We only test linux-gnu as other targets currently need allocation for thread dtors. //@ run-pass -//@ compile-flags: -Cprefer-dynamic=no +//@ compile-flags: -Cprefer-dynamic=no -Cdebuginfo=full //@ only-linux //@ only-gnu