Rollup merge of #144500 - joboet:thread-name-stack-overflow, r=ChrisDenton

thread name in stack overflow message

Fixes rust-lang/rust#144481, which is caused by the thread name not being initialised yet when setting up the stack overflow information. Unfortunately, the stack overflow UI test did not test for the correct thread name being present, and testing this separately didn't occur to me when writing https://github.com/rust-lang/rust/pull/140628.

This PR contains the smallest possible fix I could think of: passing the thread name explicitly to the platform thread creation function. In the future I'd very much like to explore some possibilities around merging the thread packet and thread handle into one structure and using that in the platform code instead – but that's best left for another PR.

This PR also amends the stack overflow test to check for thread names, so we don't run into this again.

``@rustbot`` label +beta-nominated

library/std/src/sys/pal/hermit/thread.rs

@@ -58,7 +58,11 @@ impl Thread {
         }
     }
 
-    pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
+    pub unsafe fn new(
+        stack: usize,
+        _name: Option<&str>,
+        p: Box<dyn FnOnce()>,
+    ) -> io::Result<Thread> {
         unsafe {
             Thread::new_with_coreid(stack, p, -1 /* = no specific core */)
         }

library/std/src/sys/pal/itron/thread.rs

@@ -86,7 +86,11 @@ impl Thread {
     /// # Safety
     ///
     /// See `thread::Builder::spawn_unchecked` for safety requirements.
-    pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
+    pub unsafe fn new(
+        stack: usize,
+        _name: Option<&str>,
+        p: Box<dyn FnOnce()>,
+    ) -> io::Result<Thread> {
         let inner = Box::new(ThreadInner {
             start: UnsafeCell::new(ManuallyDrop::new(p)),
             lifecycle: AtomicUsize::new(LIFECYCLE_INIT),

library/std/src/sys/pal/sgx/thread.rs

@@ -96,7 +96,11 @@ pub mod wait_notify {
 
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
-    pub unsafe fn new(_stack: usize, p: Box<dyn FnOnce() + Send>) -> io::Result<Thread> {
+    pub unsafe fn new(
+        _stack: usize,
+        _name: Option<&str>,
+        p: Box<dyn FnOnce() + Send>,
+    ) -> io::Result<Thread> {
         let mut queue_lock = task_queue::lock();
         unsafe { usercalls::launch_thread()? };
         let (task, handle) = task_queue::Task::new(p);

library/std/src/sys/pal/teeos/thread.rs

@@ -22,7 +22,11 @@ unsafe extern "C" {
 
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
-    pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
+    pub unsafe fn new(
+        stack: usize,
+        _name: Option<&str>,
+        p: Box<dyn FnOnce()>,
+    ) -> io::Result<Thread> {
         let p = Box::into_raw(Box::new(p));
         let mut native: libc::pthread_t = unsafe { mem::zeroed() };
         let mut attr: libc::pthread_attr_t = unsafe { mem::zeroed() };

library/std/src/sys/pal/uefi/thread.rs

@@ -11,7 +11,11 @@ pub const DEFAULT_MIN_STACK_SIZE: usize = 64 * 1024;
 
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
-    pub unsafe fn new(_stack: usize, _p: Box<dyn FnOnce()>) -> io::Result<Thread> {
+    pub unsafe fn new(
+        _stack: usize,
+        _name: Option<&str>,
+        _p: Box<dyn FnOnce()>,
+    ) -> io::Result<Thread> {
         unsupported()
     }
 

library/std/src/sys/pal/unix/stack_overflow.rs

@@ -8,8 +8,8 @@ pub struct Handler {
 }
 
 impl Handler {
-    pub unsafe fn new() -> Handler {
-        make_handler(false)
+    pub unsafe fn new(thread_name: Option<Box<str>>) -> Handler {
+        make_handler(false, thread_name)
     }
 
     fn null() -> Handler {
@@ -72,7 +72,6 @@ mod imp {
     use crate::sync::OnceLock;
     use crate::sync::atomic::{Atomic, AtomicBool, AtomicPtr, AtomicUsize, Ordering};
     use crate::sys::pal::unix::os;
-    use crate::thread::with_current_name;
     use crate::{io, mem, panic, ptr};
 
     // Signal handler for the SIGSEGV and SIGBUS handlers. We've got guard pages
@@ -158,13 +157,12 @@ mod imp {
                 if !NEED_ALTSTACK.load(Ordering::Relaxed) {
                     // haven't set up our sigaltstack yet
                     NEED_ALTSTACK.store(true, Ordering::Release);
-                    let handler = unsafe { make_handler(true) };
+                    let handler = unsafe { make_handler(true, None) };
                     MAIN_ALTSTACK.store(handler.data, Ordering::Relaxed);
                     mem::forget(handler);
 
                     if let Some(guard_page_range) = guard_page_range.take() {
-                        let thread_name = with_current_name(|name| name.map(Box::from));
-                        set_current_info(guard_page_range, thread_name);
+                        set_current_info(guard_page_range, Some(Box::from("main")));
                     }
                 }
 
@@ -230,14 +228,13 @@ mod imp {
     /// # Safety
     /// Mutates the alternate signal stack
     #[forbid(unsafe_op_in_unsafe_fn)]
-    pub unsafe fn make_handler(main_thread: bool) -> Handler {
+    pub unsafe fn make_handler(main_thread: bool, thread_name: Option<Box<str>>) -> Handler {
         if !NEED_ALTSTACK.load(Ordering::Acquire) {
             return Handler::null();
         }
 
         if !main_thread {
             if let Some(guard_page_range) = unsafe { current_guard() } {
-                let thread_name = with_current_name(|name| name.map(Box::from));
                 set_current_info(guard_page_range, thread_name);
             }
         }
@@ -634,7 +631,10 @@ mod imp {
 
     pub unsafe fn cleanup() {}
 
-    pub unsafe fn make_handler(_main_thread: bool) -> super::Handler {
+    pub unsafe fn make_handler(
+        _main_thread: bool,
+        _thread_name: Option<Box<str>>,
+    ) -> super::Handler {
         super::Handler::null()
     }
 
@@ -717,7 +717,10 @@ mod imp {
 
     pub unsafe fn cleanup() {}
 
-    pub unsafe fn make_handler(main_thread: bool) -> super::Handler {
+    pub unsafe fn make_handler(
+        main_thread: bool,
+        _thread_name: Option<Box<str>>,
+    ) -> super::Handler {
         if !main_thread {
             reserve_stack();
         }

library/std/src/sys/pal/unix/thread.rs

@@ -22,6 +22,11 @@ pub const DEFAULT_MIN_STACK_SIZE: usize = 256 * 1024;
 #[cfg(any(target_os = "espidf", target_os = "nuttx"))]
 pub const DEFAULT_MIN_STACK_SIZE: usize = 0; // 0 indicates that the stack size configured in the ESP-IDF/NuttX menuconfig system should be used
 
+struct ThreadData {
+    name: Option<Box<str>>,
+    f: Box<dyn FnOnce()>,
+}
+
 pub struct Thread {
     id: libc::pthread_t,
 }
@@ -34,8 +39,12 @@ unsafe impl Sync for Thread {}
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
     #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
-    pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
-        let p = Box::into_raw(Box::new(p));
+    pub unsafe fn new(
+        stack: usize,
+        name: Option<&str>,
+        f: Box<dyn FnOnce()>,
+    ) -> io::Result<Thread> {
+        let data = Box::into_raw(Box::new(ThreadData { name: name.map(Box::from), f }));
         let mut native: libc::pthread_t = mem::zeroed();
         let mut attr: mem::MaybeUninit<libc::pthread_attr_t> = mem::MaybeUninit::uninit();
         assert_eq!(libc::pthread_attr_init(attr.as_mut_ptr()), 0);
@@ -73,7 +82,7 @@ impl Thread {
             };
         }
 
-        let ret = libc::pthread_create(&mut native, attr.as_ptr(), thread_start, p as *mut _);
+        let ret = libc::pthread_create(&mut native, attr.as_ptr(), thread_start, data as *mut _);
         // Note: if the thread creation fails and this assert fails, then p will
         // be leaked. However, an alternative design could cause double-free
         // which is clearly worse.
@@ -82,19 +91,20 @@ impl Thread {
         return if ret != 0 {
             // The thread failed to start and as a result p was not consumed. Therefore, it is
             // safe to reconstruct the box so that it gets deallocated.
-            drop(Box::from_raw(p));
+            drop(Box::from_raw(data));
             Err(io::Error::from_raw_os_error(ret))
         } else {
             Ok(Thread { id: native })
         };
 
-        extern "C" fn thread_start(main: *mut libc::c_void) -> *mut libc::c_void {
+        extern "C" fn thread_start(data: *mut libc::c_void) -> *mut libc::c_void {
             unsafe {
+                let data = Box::from_raw(data as *mut ThreadData);
                 // Next, set up our stack overflow handler which may get triggered if we run
                 // out of stack.
-                let _handler = stack_overflow::Handler::new();
+                let _handler = stack_overflow::Handler::new(data.name);
                 // Finally, let's run some code.
-                Box::from_raw(main as *mut Box<dyn FnOnce()>)();
+                (data.f)();
             }
             ptr::null_mut()
         }

library/std/src/sys/pal/unsupported/thread.rs

@@ -10,7 +10,11 @@ pub const DEFAULT_MIN_STACK_SIZE: usize = 64 * 1024;
 
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
-    pub unsafe fn new(_stack: usize, _p: Box<dyn FnOnce()>) -> io::Result<Thread> {
+    pub unsafe fn new(
+        _stack: usize,
+        _name: Option<&str>,
+        _p: Box<dyn FnOnce()>,
+    ) -> io::Result<Thread> {
         unsupported()
     }
 

library/std/src/sys/pal/wasi/thread.rs

@@ -73,7 +73,7 @@ impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
     cfg_if::cfg_if! {
         if #[cfg(target_feature = "atomics")] {
-            pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
+            pub unsafe fn new(stack: usize, _name: Option<&str>, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
                 let p = Box::into_raw(Box::new(p));
                 let mut native: libc::pthread_t = unsafe { mem::zeroed() };
                 let mut attr: libc::pthread_attr_t = unsafe { mem::zeroed() };
@@ -120,7 +120,7 @@ impl Thread {
                 }
             }
         } else {
-            pub unsafe fn new(_stack: usize, _p: Box<dyn FnOnce()>) -> io::Result<Thread> {
+            pub unsafe fn new(_stack: usize, _name: Option<&str>, _p: Box<dyn FnOnce()>) -> io::Result<Thread> {
                 crate::sys::unsupported()
             }
         }

library/std/src/sys/pal/wasm/atomics/thread.rs

@@ -10,7 +10,11 @@ pub const DEFAULT_MIN_STACK_SIZE: usize = 1024 * 1024;
 
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
-    pub unsafe fn new(_stack: usize, _p: Box<dyn FnOnce()>) -> io::Result<Thread> {
+    pub unsafe fn new(
+        _stack: usize,
+        _name: Option<&str>,
+        _p: Box<dyn FnOnce()>,
+    ) -> io::Result<Thread> {
         unsupported()
     }
 

library/std/src/sys/pal/windows/thread.rs

@@ -20,7 +20,11 @@ pub struct Thread {
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
     #[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
-    pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
+    pub unsafe fn new(
+        stack: usize,
+        _name: Option<&str>,
+        p: Box<dyn FnOnce()>,
+    ) -> io::Result<Thread> {
         let p = Box::into_raw(Box::new(p));
 
         // CreateThread rounds up values for the stack size to the nearest page size (at least 4kb).

library/std/src/sys/pal/xous/thread.rs

@@ -20,7 +20,11 @@ pub const GUARD_PAGE_SIZE: usize = 4096;
 
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
-    pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
+    pub unsafe fn new(
+        stack: usize,
+        _name: Option<&str>,
+        p: Box<dyn FnOnce()>,
+    ) -> io::Result<Thread> {
         let p = Box::into_raw(Box::new(p));
         let mut stack_size = crate::cmp::max(stack, MIN_STACK_SIZE);
 

library/std/src/thread/mod.rs

@@ -595,7 +595,7 @@ impl Builder {
             // Similarly, the `sys` implementation must guarantee that no references to the closure
             // exist after the thread has terminated, which is signaled by `Thread::join`
             // returning.
-            native: unsafe { imp::Thread::new(stack_size, main)? },
+            native: unsafe { imp::Thread::new(stack_size, my_thread.name(), main)? },
             thread: my_thread,
             packet: my_packet,
         })

tests/ui/runtime/out-of-stack.rs

@@ -19,7 +19,7 @@ extern crate libc;
 use std::env;
 use std::hint::black_box;
 use std::process::Command;
-use std::thread;
+use std::thread::Builder;
 
 fn silent_recurse() {
     let buf = [0u8; 1000];
@@ -56,9 +56,9 @@ fn main() {
     } else if args.len() > 1 && args[1] == "loud" {
         loud_recurse();
     } else if args.len() > 1 && args[1] == "silent-thread" {
-        thread::spawn(silent_recurse).join();
+        Builder::new().name("ferris".to_string()).spawn(silent_recurse).unwrap().join();
     } else if args.len() > 1 && args[1] == "loud-thread" {
-        thread::spawn(loud_recurse).join();
+        Builder::new().name("ferris".to_string()).spawn(loud_recurse).unwrap().join();
     } else {
         let mut modes = vec![
             "silent-thread",
@@ -82,6 +82,12 @@ fn main() {
             let error = String::from_utf8_lossy(&silent.stderr);
             assert!(error.contains("has overflowed its stack"),
                     "missing overflow message: {}", error);
+
+            if mode.contains("thread") {
+                assert!(error.contains("ferris"), "missing thread name: {}", error);
+            } else {
+                assert!(error.contains("main"), "missing thread name: {}", error);
+            }
         }
     }
 }