Skip to content

Commit

Permalink
Rollup merge of rust-lang#84687 - a1phyr:improve_rwlock, r=m-ou-se
Browse files Browse the repository at this point in the history
Multiple improvements to RwLocks

This PR replicates rust-lang#77147, rust-lang#77380 and rust-lang#84650 on RWLocks :
- Split `sys_common::RWLock` in `StaticRWLock` and `MovableRWLock`
- Unbox rwlocks on some platforms (Windows, Wasm and unsupported)
- Simplify `RwLock::into_inner`

Notes to reviewers :
- For each target, I copied `MovableMutex` to guess if `MovableRWLock` should be boxed.
- ~A comment says that `StaticMutex` is not re-entrant, I don't understand why and I don't know whether it applies to `StaticRWLock`.~

r? `@m-ou-se`
  • Loading branch information
JohnTitor authored Jun 10, 2021
2 parents 27e84b8 + ac470e9 commit 578eb6d
Show file tree
Hide file tree
Showing 10 changed files with 111 additions and 133 deletions.
15 changes: 7 additions & 8 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::process;
use crate::sync::atomic::{AtomicBool, Ordering};
use crate::sys::stdio::panic_output;
use crate::sys_common::backtrace::{self, RustBacktrace};
use crate::sys_common::rwlock::RWLock;
use crate::sys_common::rwlock::StaticRWLock;
use crate::sys_common::thread_info;
use crate::thread;

Expand Down Expand Up @@ -74,7 +74,7 @@ enum Hook {
Custom(*mut (dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send)),
}

static HOOK_LOCK: RWLock = RWLock::new();
static HOOK_LOCK: StaticRWLock = StaticRWLock::new();
static mut HOOK: Hook = Hook::Default;

/// Registers a custom panic hook, replacing any that was previously registered.
Expand Down Expand Up @@ -117,10 +117,10 @@ pub fn set_hook(hook: Box<dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send>) {
}

unsafe {
HOOK_LOCK.write();
let guard = HOOK_LOCK.write();
let old_hook = HOOK;
HOOK = Hook::Custom(Box::into_raw(hook));
HOOK_LOCK.write_unlock();
drop(guard);

if let Hook::Custom(ptr) = old_hook {
#[allow(unused_must_use)]
Expand Down Expand Up @@ -165,10 +165,10 @@ pub fn take_hook() -> Box<dyn Fn(&PanicInfo<'_>) + 'static + Sync + Send> {
}

unsafe {
HOOK_LOCK.write();
let guard = HOOK_LOCK.write();
let hook = HOOK;
HOOK = Hook::Default;
HOOK_LOCK.write_unlock();
drop(guard);

match hook {
Hook::Default => Box::new(default_hook),
Expand Down Expand Up @@ -608,7 +608,7 @@ fn rust_panic_with_hook(

unsafe {
let mut info = PanicInfo::internal_constructor(message, location);
HOOK_LOCK.read();
let _guard = HOOK_LOCK.read();
match HOOK {
// Some platforms (like wasm) know that printing to stderr won't ever actually
// print anything, and if that's the case we can skip the default
Expand All @@ -626,7 +626,6 @@ fn rust_panic_with_hook(
(*ptr)(&info);
}
};
HOOK_LOCK.read_unlock();
}

if panics > 1 {
Expand Down
34 changes: 4 additions & 30 deletions library/std/src/sync/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ mod tests;

use crate::cell::UnsafeCell;
use crate::fmt;
use crate::mem;
use crate::ops::{Deref, DerefMut};
use crate::ptr;
use crate::sync::{poison, LockResult, TryLockError, TryLockResult};
use crate::sys_common::rwlock as sys;

Expand Down Expand Up @@ -66,7 +64,7 @@ use crate::sys_common::rwlock as sys;
/// [`Mutex`]: super::Mutex
#[stable(feature = "rust1", since = "1.0.0")]
pub struct RwLock<T: ?Sized> {
inner: Box<sys::RWLock>,
inner: sys::MovableRWLock,
poison: poison::Flag,
data: UnsafeCell<T>,
}
Expand Down Expand Up @@ -130,7 +128,7 @@ impl<T> RwLock<T> {
#[stable(feature = "rust1", since = "1.0.0")]
pub fn new(t: T) -> RwLock<T> {
RwLock {
inner: box sys::RWLock::new(),
inner: sys::MovableRWLock::new(),
poison: poison::Flag::new(),
data: UnsafeCell::new(t),
}
Expand Down Expand Up @@ -376,24 +374,8 @@ impl<T: ?Sized> RwLock<T> {
where
T: Sized,
{
// We know statically that there are no outstanding references to
// `self` so there's no need to lock the inner lock.
//
// To get the inner value, we'd like to call `data.into_inner()`,
// but because `RwLock` impl-s `Drop`, we can't move out of it, so
// we'll have to destructure it manually instead.
unsafe {
// Like `let RwLock { inner, poison, data } = self`.
let (inner, poison, data) = {
let RwLock { ref inner, ref poison, ref data } = self;
(ptr::read(inner), ptr::read(poison), ptr::read(data))
};
mem::forget(self);
inner.destroy(); // Keep in sync with the `Drop` impl.
drop(inner);

poison::map_result(poison.borrow(), |_| data.into_inner())
}
let data = self.data.into_inner();
poison::map_result(self.poison.borrow(), |_| data)
}

/// Returns a mutable reference to the underlying data.
Expand Down Expand Up @@ -424,14 +406,6 @@ impl<T: ?Sized> RwLock<T> {
}
}

#[stable(feature = "rust1", since = "1.0.0")]
unsafe impl<#[may_dangle] T: ?Sized> Drop for RwLock<T> {
fn drop(&mut self) {
// IMPORTANT: This code needs to be kept in sync with `RwLock::into_inner`.
unsafe { self.inner.destroy() }
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized + fmt::Debug> fmt::Debug for RwLock<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sys/hermit/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ pub struct RWLock {
state: UnsafeCell<State>,
}

pub type MovableRWLock = Box<RWLock>;

enum State {
Unlocked,
Reading(usize),
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sys/sgx/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ pub struct RWLock {
writer: SpinMutex<WaitVariable<bool>>,
}

pub type MovableRWLock = Box<RWLock>;

// Check at compile time that RWLock size matches C definition (see test_c_rwlock_initializer below)
//
// # Safety
Expand Down
11 changes: 5 additions & 6 deletions library/std/src/sys/unix/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ use crate::str;
use crate::sys::cvt;
use crate::sys::fd;
use crate::sys::memchr;
use crate::sys::rwlock::{RWLockReadGuard, StaticRWLock};
use crate::sys_common::mutex::{StaticMutex, StaticMutexGuard};
use crate::sys_common::rwlock::{StaticRWLock, StaticRWLockReadGuard};
use crate::vec;

use libc::{c_char, c_int, c_void};
Expand Down Expand Up @@ -490,8 +489,8 @@ pub unsafe fn environ() -> *mut *const *const c_char {

static ENV_LOCK: StaticRWLock = StaticRWLock::new();

pub fn env_read_lock() -> RWLockReadGuard {
ENV_LOCK.read_with_guard()
pub fn env_read_lock() -> StaticRWLockReadGuard {
ENV_LOCK.read()
}

/// Returns a vector of (variable, value) byte-vector pairs for all the
Expand Down Expand Up @@ -551,7 +550,7 @@ pub fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {
let v = CString::new(v.as_bytes())?;

unsafe {
let _guard = ENV_LOCK.write_with_guard();
let _guard = ENV_LOCK.write();
cvt(libc::setenv(k.as_ptr(), v.as_ptr(), 1)).map(drop)
}
}
Expand All @@ -560,7 +559,7 @@ pub fn unsetenv(n: &OsStr) -> io::Result<()> {
let nbuf = CString::new(n.as_bytes())?;

unsafe {
let _guard = ENV_LOCK.write_with_guard();
let _guard = ENV_LOCK.write();
cvt(libc::unsetenv(nbuf.as_ptr())).map(drop)
}
}
Expand Down
54 changes: 2 additions & 52 deletions library/std/src/sys/unix/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ pub struct RWLock {
num_readers: AtomicUsize,
}

pub type MovableRWLock = Box<RWLock>;

unsafe impl Send for RWLock {}
unsafe impl Sync for RWLock {}

Expand Down Expand Up @@ -139,55 +141,3 @@ impl RWLock {
}
}
}

pub struct StaticRWLock(RWLock);

impl StaticRWLock {
pub const fn new() -> StaticRWLock {
StaticRWLock(RWLock::new())
}

/// Acquires shared access to the underlying lock, blocking the current
/// thread to do so.
///
/// The lock is automatically unlocked when the returned guard is dropped.
#[inline]
pub fn read_with_guard(&'static self) -> RWLockReadGuard {
// SAFETY: All methods require static references, therefore self
// cannot be moved between invocations.
unsafe {
self.0.read();
}
RWLockReadGuard(&self.0)
}

/// Acquires write access to the underlying lock, blocking the current thread
/// to do so.
///
/// The lock is automatically unlocked when the returned guard is dropped.
#[inline]
pub fn write_with_guard(&'static self) -> RWLockWriteGuard {
// SAFETY: All methods require static references, therefore self
// cannot be moved between invocations.
unsafe {
self.0.write();
}
RWLockWriteGuard(&self.0)
}
}

pub struct RWLockReadGuard(&'static RWLock);

impl Drop for RWLockReadGuard {
fn drop(&mut self) {
unsafe { self.0.read_unlock() }
}
}

pub struct RWLockWriteGuard(&'static RWLock);

impl Drop for RWLockWriteGuard {
fn drop(&mut self) {
unsafe { self.0.write_unlock() }
}
}
2 changes: 2 additions & 0 deletions library/std/src/sys/unsupported/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ pub struct RWLock {
mode: Cell<isize>,
}

pub type MovableRWLock = RWLock;

unsafe impl Send for RWLock {}
unsafe impl Sync for RWLock {} // no threads on this platform

Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sys/wasm/atomics/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ pub struct RWLock {
state: UnsafeCell<State>,
}

pub type MovableRWLock = RWLock;

enum State {
Unlocked,
Reading(usize),
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sys/windows/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ pub struct RWLock {
inner: UnsafeCell<c::SRWLOCK>,
}

pub type MovableRWLock = RWLock;

unsafe impl Send for RWLock {}
unsafe impl Sync for RWLock {}

Expand Down
Loading

0 comments on commit 578eb6d

Please sign in to comment.