Skip to content

Commit

Permalink
Adjust some comments.
Browse files Browse the repository at this point in the history
This also makes `Tid::set` unsafe, as incorrect usage may lead to unsoundness.
  • Loading branch information
Sp00ph committed Jul 18, 2024
1 parent 2296ffb commit 5bd459a
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 10 deletions.
30 changes: 20 additions & 10 deletions library/std/src/sync/reentrant_lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,13 @@ use crate::thread::{current_id, ThreadId};
// synchronization is left to the mutex, making relaxed memory ordering for
// the `owner` field fine in all cases.
//
// On systems without 64 bit atomics we use a simple seqlock to emulate a 64 bit Tid using
// 32 bit atomics (which should be supported on all platforms with `std`). This works
// because only one thread at a time (the one holding the mutex) writes to it.
// On systems without 64 bit atomics we also store the address of a TLS variable
// along the 64-bit TID. We then first check that address against the address
// of that variable on the current thread, and only if they compare equal do we
// compare the actual TIDs. Because we only ever read the TID on the same thread
// that it was written on (or a thread sharing the TLS block with that writer thread),
// we don't need to further synchronize the TID accesses, so they can be regular 64-bit
// non-atomic accesses.
#[unstable(feature = "reentrant_lock", issue = "121440")]
pub struct ReentrantLock<T: ?Sized> {
mutex: sys::Mutex,
Expand All @@ -103,7 +107,8 @@ cfg_if!(
}

#[inline]
fn set(&self, tid: Option<ThreadId>) {
// This is just unsafe to match the API of the Tid type below.
unsafe fn set(&self, tid: Option<ThreadId>) {
let value = tid.map_or(0, |tid| tid.as_u64().get());
self.0.store(value, Relaxed);
}
Expand Down Expand Up @@ -157,10 +162,11 @@ cfg_if!(
}

#[inline]
// This may only be called by one thread at a time.
fn set(&self, tid: Option<ThreadId>) {
// It's important that we set `self.tls_addr` to 0 if the
// tid is cleared. Otherwise, there might be race conditions between
// This may only be called by one thread at a time, and can lead to
// race conditions otherwise.
unsafe fn set(&self, tid: Option<ThreadId>) {
// It's important that we set `self.tls_addr` to 0 if the tid is
// cleared. Otherwise, there might be race conditions between
// `set()` and `get()`.
let tls_addr = if tid.is_some() { tls_addr() } else { 0 };
let value = tid.map_or(0, |tid| tid.as_u64().get());
Expand Down Expand Up @@ -273,7 +279,9 @@ impl<T: ?Sized> ReentrantLock<T> {
/// ```
pub fn lock(&self) -> ReentrantLockGuard<'_, T> {
let this_thread = current_id();
// Safety: We only touch lock_count when we own the lock.
// Safety: We only touch lock_count when we own the inner mutex.
// Additionally, we only call `self.owner.set()` while holding
// the inner mutex, so no two threads can call it concurrently.
unsafe {
if self.owner.contains(this_thread) {
self.increment_lock_count().expect("lock count overflow in reentrant mutex");
Expand Down Expand Up @@ -315,7 +323,9 @@ impl<T: ?Sized> ReentrantLock<T> {
/// This function does not block.
pub(crate) fn try_lock(&self) -> Option<ReentrantLockGuard<'_, T>> {
let this_thread = current_id();
// Safety: We only touch lock_count when we own the lock.
// Safety: We only touch lock_count when we own the inner mutex.
// Additionally, we only call `self.owner.set()` while holding
// the inner mutex, so no two threads can call it concurrently.
unsafe {
if self.owner.contains(this_thread) {
self.increment_lock_count()?;
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/thread/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,8 @@ where

thread_local! {
// Invariant: `CURRENT` and `CURRENT_ID` will always be initialized together.
// If `CURRENT` is initialized, then `CURRENT_ID` will hold the same value
// as `CURRENT.id()`.
static CURRENT: OnceCell<Thread> = const { OnceCell::new() };
static CURRENT_ID: Cell<Option<ThreadId>> = const { Cell::new(None) };
}
Expand Down

0 comments on commit 5bd459a

Please sign in to comment.