Skip to content

Commit 5bd459a

Browse files
committed
Adjust some comments.
This also makes `Tid::set` unsafe, as incorrect usage may lead to unsoundness.
1 parent 2296ffb commit 5bd459a

File tree

2 files changed

+22
-10
lines changed

2 files changed

+22
-10
lines changed

library/std/src/sync/reentrant_lock.rs

+20-10
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,13 @@ use crate::thread::{current_id, ThreadId};
7575
// synchronization is left to the mutex, making relaxed memory ordering for
7676
// the `owner` field fine in all cases.
7777
//
78-
// On systems without 64 bit atomics we use a simple seqlock to emulate a 64 bit Tid using
79-
// 32 bit atomics (which should be supported on all platforms with `std`). This works
80-
// because only one thread at a time (the one holding the mutex) writes to it.
78+
// On systems without 64 bit atomics we also store the address of a TLS variable
79+
// along the 64-bit TID. We then first check that address against the address
80+
// of that variable on the current thread, and only if they compare equal do we
81+
// compare the actual TIDs. Because we only ever read the TID on the same thread
82+
// that it was written on (or a thread sharing the TLS block with that writer thread),
83+
// we don't need to further synchronize the TID accesses, so they can be regular 64-bit
84+
// non-atomic accesses.
8185
#[unstable(feature = "reentrant_lock", issue = "121440")]
8286
pub struct ReentrantLock<T: ?Sized> {
8387
mutex: sys::Mutex,
@@ -103,7 +107,8 @@ cfg_if!(
103107
}
104108

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

159164
#[inline]
160-
// This may only be called by one thread at a time.
161-
fn set(&self, tid: Option<ThreadId>) {
162-
// It's important that we set `self.tls_addr` to 0 if the
163-
// tid is cleared. Otherwise, there might be race conditions between
165+
// This may only be called by one thread at a time, and can lead to
166+
// race conditions otherwise.
167+
unsafe fn set(&self, tid: Option<ThreadId>) {
168+
// It's important that we set `self.tls_addr` to 0 if the tid is
169+
// cleared. Otherwise, there might be race conditions between
164170
// `set()` and `get()`.
165171
let tls_addr = if tid.is_some() { tls_addr() } else { 0 };
166172
let value = tid.map_or(0, |tid| tid.as_u64().get());
@@ -273,7 +279,9 @@ impl<T: ?Sized> ReentrantLock<T> {
273279
/// ```
274280
pub fn lock(&self) -> ReentrantLockGuard<'_, T> {
275281
let this_thread = current_id();
276-
// Safety: We only touch lock_count when we own the lock.
282+
// Safety: We only touch lock_count when we own the inner mutex.
283+
// Additionally, we only call `self.owner.set()` while holding
284+
// the inner mutex, so no two threads can call it concurrently.
277285
unsafe {
278286
if self.owner.contains(this_thread) {
279287
self.increment_lock_count().expect("lock count overflow in reentrant mutex");
@@ -315,7 +323,9 @@ impl<T: ?Sized> ReentrantLock<T> {
315323
/// This function does not block.
316324
pub(crate) fn try_lock(&self) -> Option<ReentrantLockGuard<'_, T>> {
317325
let this_thread = current_id();
318-
// Safety: We only touch lock_count when we own the lock.
326+
// Safety: We only touch lock_count when we own the inner mutex.
327+
// Additionally, we only call `self.owner.set()` while holding
328+
// the inner mutex, so no two threads can call it concurrently.
319329
unsafe {
320330
if self.owner.contains(this_thread) {
321331
self.increment_lock_count()?;

library/std/src/thread/mod.rs

+2
Original file line numberDiff line numberDiff line change
@@ -699,6 +699,8 @@ where
699699

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

0 commit comments

Comments
 (0)