Change thread_id's return type to be an Option. (#97)

If `thread_id` is called after a thread has exited, the thread no longer
has an ID, so return an `Option` indicating whether the ID is present.

Author: sunfishcode

src/thread/linux_raw.rs

@@ -231,10 +231,10 @@ pub(super) fn initialize_startup_thread_info() {
             current_phdr = current_phdr.cast::<u8>().add(phent).cast();
 
             match phdr.p_type {
-                // Compute the offset from the static virtual addresses
-                // in the `p_vaddr` fields to the dynamic addresses. We don't
-                // always get a `PT_PHDR` or `PT_DYNAMIC` header, so use
-                // whichever one we get.
+                // Compute the offset from the static virtual addresses in the
+                // `p_vaddr` fields to the dynamic addresses. We don't always
+                // get a `PT_PHDR` or `PT_DYNAMIC` header, so use whichever one
+                // we get.
                 PT_PHDR => offset = first_phdr.addr().wrapping_sub(phdr.p_vaddr),
                 PT_DYNAMIC => offset = dynamic_addr.addr().wrapping_sub(phdr.p_vaddr),
 
@@ -950,9 +950,17 @@ pub fn current_thread_id() -> ThreadId {
     // SAFETY: All threads have been initialized, including the main thread
     // with `initialize_main_thread`, so `current_thread()` returns a valid
     // pointer.
-    let tid = unsafe { thread_id(current_thread()) };
-    debug_assert_eq!(tid, gettid(), "`current_thread_id` disagrees with `gettid`");
-    tid
+    unsafe {
+        // Don't use the `thread_id` function here because it returns an
+        // `Option` to handle the case where the thread has exited. We're
+        // querying the current thread which we know is still running because
+        // we're on it.
+        let raw = current_thread().0.as_ref().thread_id.load(SeqCst);
+        debug_assert!(raw > 0);
+        let tid = ThreadId::from_raw_unchecked(raw);
+        debug_assert_eq!(tid, gettid(), "`current_thread_id` disagrees with `gettid`");
+        tid
+    }
 }
 
 /// Set the current thread id, after a `fork`.
@@ -1015,7 +1023,7 @@ pub fn current_thread_tls_addr(offset: usize) -> *mut c_void {
     }
 }
 
-/// Return the id of a thread.
+/// Return the id of a thread, or `None` if the thread has exited.
 ///
 /// This is the same as [`rustix::thread::gettid`], but loads the value from a
 /// field in the runtime rather than making a system call.
@@ -1024,10 +1032,9 @@ pub fn current_thread_tls_addr(offset: usize) -> *mut c_void {
 ///
 /// `thread` must point to a valid thread record.
 #[inline]
-pub unsafe fn thread_id(thread: Thread) -> ThreadId {
+pub unsafe fn thread_id(thread: Thread) -> Option<ThreadId> {
     let raw = thread.0.as_ref().thread_id.load(SeqCst);
-    debug_assert!(raw > 0);
-    ThreadId::from_raw_unchecked(raw)
+    ThreadId::from_raw(raw)
 }
 
 /// Return the current thread's stack address (lowest address), size, and guard

test-crates/tls/Cargo.toml test-crates/origin-start/Cargo.tomltest-crates/tls/Cargo.toml renamed to test-crates/origin-start/Cargo.toml

@@ -1,5 +1,5 @@
 [package]
-name = "tls"
+name = "origin-start-tests"
 version = "0.0.0"
 edition = "2021"
 publish = false
@@ -9,6 +9,13 @@ origin = { path = "../..", default-features = false, features = ["origin-thread"
 atomic-dbg = { version = "0.1.8", default-features = false }
 rustix-dlmalloc = { version = "0.1.0", features = ["global"] }
 compiler_builtins = { version = "0.1.101", features = ["mem"] }
+rustix = { version = "0.38", default-features = false, features = ["thread"] }
+rustix-futex-sync = { version = "0.1.5", default-features = false }
 
 # This is just a test crate, and not part of the origin workspace.
 [workspace]
+
+[profile.release]
+debug = true
+debug-assertions = true
+overflow-checks = true

test-crates/tls/build.rs test-crates/origin-start/build.rstest-crates/tls/build.rs renamed to test-crates/origin-start/build.rs

@@ -0,0 +1,127 @@
+//! Test `thread_id` and `current_thread_id`.
+
+#![no_std]
+#![no_main]
+#![allow(internal_features)]
+#![feature(lang_items)]
+#![feature(core_intrinsics)]
+
+extern crate alloc;
+extern crate compiler_builtins;
+
+use alloc::boxed::Box;
+use alloc::sync::Arc;
+use atomic_dbg::dbg;
+use core::ffi::c_void;
+use core::ptr::{null_mut, NonNull};
+use origin::program::*;
+use origin::thread::*;
+use rustix_futex_sync::{Condvar, Mutex};
+
+#[panic_handler]
+fn panic(panic: &core::panic::PanicInfo<'_>) -> ! {
+    dbg!(panic);
+    core::intrinsics::abort();
+}
+
+#[lang = "eh_personality"]
+extern "C" fn eh_personality() {}
+
+#[global_allocator]
+static GLOBAL_ALLOCATOR: rustix_dlmalloc::GlobalDlmalloc = rustix_dlmalloc::GlobalDlmalloc;
+
+#[derive(Eq, PartialEq, Debug)]
+enum Id {
+    Parent(ThreadId),
+    Child(ThreadId),
+}
+
+#[no_mangle]
+unsafe fn origin_main(_argc: usize, _argv: *mut *mut u8, _envp: *mut *mut u8) -> i32 {
+    assert_eq!(current_thread_id(), thread_id(current_thread()).unwrap());
+    at_exit(Box::new(|| {
+        assert_eq!(current_thread_id(), thread_id(current_thread()).unwrap())
+    }));
+    at_thread_exit(Box::new(|| {
+        assert_eq!(current_thread_id(), thread_id(current_thread()).unwrap());
+    }));
+
+    let cond = Arc::new((Mutex::new(None), Condvar::new()));
+    let cond_clone = NonNull::new(Box::into_raw(Box::new(Arc::clone(&cond))).cast());
+
+    let thread = create_thread(
+        move |args| {
+            assert_eq!(current_thread_id(), thread_id(current_thread()).unwrap());
+            at_thread_exit(Box::new(|| {
+                assert_eq!(current_thread_id(), thread_id(current_thread()).unwrap());
+            }));
+
+            let unpack = |x: Option<NonNull<c_void>>| match x {
+                Some(p) => p.as_ptr().cast::<Arc<(Mutex<Option<Id>>, Condvar)>>(),
+                None => null_mut(),
+            };
+            let cond = Box::from_raw(unpack(args[0]));
+
+            // Wait for the parent to tell the child its id, and check it.
+            let (lock, cvar) = &**cond;
+            {
+                let mut id = lock.lock();
+                while (*id).is_none() {
+                    id = cvar.wait(id);
+                }
+                assert_ne!(Id::Parent(current_thread_id()), *id.as_ref().unwrap());
+            }
+
+            // Tell the parent the child's id.
+            {
+                let mut id = lock.lock();
+                *id = Some(Id::Child(current_thread_id()));
+                cvar.notify_one();
+            }
+
+            assert_eq!(current_thread_id(), thread_id(current_thread()).unwrap());
+            None
+        },
+        &[cond_clone],
+        default_stack_size(),
+        default_guard_size(),
+    )
+    .unwrap();
+
+    // While the child is still running, examine its id.
+    let child_thread_id_from_main = thread_id(thread).unwrap();
+
+    // Tell the child the main thread id.
+    let (lock, cvar) = &*cond;
+    {
+        let mut id = lock.lock();
+        *id = Some(Id::Parent(current_thread_id()));
+        cvar.notify_one();
+    }
+
+    // Wait for the child to tell the main thread its id, and check it.
+    {
+        let mut id = lock.lock();
+        loop {
+            if let Some(Id::Child(id)) = *id {
+                assert_eq!(child_thread_id_from_main, id);
+                break;
+            }
+            id = cvar.wait(id);
+        }
+    }
+
+    // At some point the child thread should exit, at which point `thread_id`
+    // will return `None`.
+    while thread_id(thread).is_some() {
+        let _ = rustix::thread::nanosleep(&rustix::thread::Timespec {
+            tv_sec: 0,
+            tv_nsec: 100_000_000,
+        });
+    }
+
+    join_thread(thread);
+
+    assert_eq!(current_thread_id(), thread_id(current_thread()).unwrap());
+    exit(201);
+}

test-crates/origin-start/src/bin/thread-id.rs

@@ -1,5 +1,5 @@
-//! Like `origin-start`, but add a `#[thread_local]` variable and asserts
-//! to ensure that it works.
+//! Like the `origin-start` example, but add a `#[thread_local]` variable and
+//! asserts to ensure that it works.
 
 #![no_std]
 #![no_main]

test-crates/tls/src/main.rs test-crates/origin-start/src/bin/tls.rstest-crates/tls/src/main.rs renamed to test-crates/origin-start/src/bin/tls.rs

@@ -18,14 +18,21 @@ fn test_crate(
 
 #[test]
 fn test_crate_tls() {
-    test_crate("tls", &["--release"], &[], "", "", Some(200));
+    test_crate(
+        "origin-start",
+        &["--bin=tls", "--release"],
+        &[],
+        "",
+        "",
+        Some(200),
+    );
 }
 
 #[test]
 fn test_crate_tls_crt_static() {
     test_crate(
-        "tls",
-        &["--features=origin/experimental-relocate"],
+        "origin-start",
+        &["--bin=tls", "--features=origin/experimental-relocate"],
         &[("RUSTFLAGS", "-C target-feature=+crt-static")],
         "",
         "",
@@ -36,8 +43,8 @@ fn test_crate_tls_crt_static() {
 #[test]
 fn test_crate_tls_crt_static_relocation_static() {
     test_crate(
-        "tls",
-        &[],
+        "origin-start",
+        &["--bin=tls"],
         &[(
             "RUSTFLAGS",
             "-C target-feature=+crt-static -C relocation-model=static",
@@ -47,3 +54,8 @@ fn test_crate_tls_crt_static_relocation_static() {
         Some(200),
     );
 }
+
+#[test]
+fn test_crate_thread_id() {
+    test_crate("origin-start", &["--bin=thread-id"], &[], "", "", Some(201));
+}