Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make RwLockReadGuard covariant #96820

Merged
merged 12 commits into from
Jun 25, 2022
28 changes: 24 additions & 4 deletions library/std/src/sync/rwlock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ mod tests;
use crate::cell::UnsafeCell;
use crate::fmt;
use crate::ops::{Deref, DerefMut};
use crate::ptr::NonNull;
use crate::sync::{poison, LockResult, TryLockError, TryLockResult};
use crate::sys_common::rwlock as sys;

Expand Down Expand Up @@ -101,7 +102,12 @@ unsafe impl<T: ?Sized + Send + Sync> Sync for RwLock<T> {}
#[stable(feature = "rust1", since = "1.0.0")]
#[clippy::has_significant_drop]
pub struct RwLockReadGuard<'a, T: ?Sized + 'a> {
lock: &'a RwLock<T>,
// NB: we use a pointer instead of `&'a T` to avoid `noalias` violations, because a
// `Ref` argument doesn't hold immutability for its whole scope, only until it drops.
// `NonNull` is also covariant over `T`, just like we would have with `&T`. `NonNull`
// is preferable over `const* T` to allow for niche optimization.
data: NonNull<T>,
cuviper marked this conversation as resolved.
Show resolved Hide resolved
inner_lock: &'a sys::MovableRwLock,
}

#[stable(feature = "rust1", since = "1.0.0")]
Expand Down Expand Up @@ -509,12 +515,21 @@ impl<T> From<T> for RwLock<T> {
}

impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> {
/// Create a new instance of `RwLockReadGuard<T>` from a `RwLock<T>`.
// SAFETY: if and only if `lock.inner.read()` (or `lock.inner.try_read()`) has been
// successfully called from the same thread before instantiating this object.
unsafe fn new(lock: &'rwlock RwLock<T>) -> LockResult<RwLockReadGuard<'rwlock, T>> {
poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard { lock })
poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard {
data: NonNull::new_unchecked(lock.data.get()),
inner_lock: &lock.inner,
})
}
}

impl<'rwlock, T: ?Sized> RwLockWriteGuard<'rwlock, T> {
/// Create a new instance of `RwLockWriteGuard<T>` from a `RwLock<T>`.
// SAFETY: if and only if `lock.inner.write()` (or `lock.inner.try_write()`) has been
// successfully called from the same thread before instantiating this object.
unsafe fn new(lock: &'rwlock RwLock<T>) -> LockResult<RwLockWriteGuard<'rwlock, T>> {
poison::map_result(lock.poison.guard(), |guard| RwLockWriteGuard { lock, poison: guard })
}
Expand Down Expand Up @@ -553,7 +568,8 @@ impl<T: ?Sized> Deref for RwLockReadGuard<'_, T> {
type Target = T;

fn deref(&self) -> &T {
unsafe { &*self.lock.data.get() }
// SAFETY: the conditions of `RwLockGuard::new` were satisfied when created.
unsafe { self.data.as_ref() }
}
}

Expand All @@ -562,22 +578,25 @@ impl<T: ?Sized> Deref for RwLockWriteGuard<'_, T> {
type Target = T;

fn deref(&self) -> &T {
// SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when created.
unsafe { &*self.lock.data.get() }
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized> DerefMut for RwLockWriteGuard<'_, T> {
fn deref_mut(&mut self) -> &mut T {
// SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when created.
unsafe { &mut *self.lock.data.get() }
}
}

#[stable(feature = "rust1", since = "1.0.0")]
impl<T: ?Sized> Drop for RwLockReadGuard<'_, T> {
fn drop(&mut self) {
// SAFETY: the conditions of `RwLockReadGuard::new` were satisfied when created.
unsafe {
self.lock.inner.read_unlock();
self.inner_lock.read_unlock();
}
}
}
Expand All @@ -586,6 +605,7 @@ impl<T: ?Sized> Drop for RwLockReadGuard<'_, T> {
impl<T: ?Sized> Drop for RwLockWriteGuard<'_, T> {
fn drop(&mut self) {
self.lock.poison.done(&self.poison);
// SAFETY: the conditions of `RwLockWriteGuard::new` were satisfied when created.
unsafe {
self.lock.inner.write_unlock();
}
Expand Down
14 changes: 13 additions & 1 deletion library/std/src/sync/rwlock/tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::sync::atomic::{AtomicUsize, Ordering};
use crate::sync::mpsc::channel;
use crate::sync::{Arc, RwLock, TryLockError};
use crate::sync::{Arc, RwLock, RwLockReadGuard, TryLockError};
use crate::thread;
use rand::{self, Rng};

Expand Down Expand Up @@ -245,3 +245,15 @@ fn test_get_mut_poison() {
Ok(x) => panic!("get_mut of poisoned RwLock is Ok: {x:?}"),
}
}

#[test]
fn test_read_guard_covariance() {
fn do_stuff<'a>(_: RwLockReadGuard<'_, &'a i32>, _: &'a i32) {}
let j: i32 = 5;
let lock = RwLock::new(&j);
{
let i = 6;
do_stuff(lock.read().unwrap(), &i);
}
drop(lock);
}
14 changes: 14 additions & 0 deletions src/test/codegen/noalias-rwlockreadguard.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// compile-flags: -O -C no-prepopulate-passes -Z mutable-noalias=yes

#![crate_type = "lib"]

use std::sync::{RwLock, RwLockReadGuard};

// Make sure that `RwLockReadGuard` does not get a `noalias` attribute, because
// the `RwLock` might alias writes after it is dropped.

// CHECK-LABEL: @maybe_aliased(
// CHECK-NOT: noalias
// CHECK-SAME: %_data
#[no_mangle]
pub unsafe fn maybe_aliased(_: RwLockReadGuard<'_, i32>, _data: &RwLock<i32>) {}
7 changes: 2 additions & 5 deletions src/test/debuginfo/rwlock-read.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,8 @@
//
// cdb-command:dx r
// cdb-check:r [Type: std::sync::rwlock::RwLockReadGuard<i32>]
// cdb-check: [...] lock : [...] [Type: std::sync::rwlock::RwLock<i32> *]
//
// cdb-command:dx r.lock->data,d
// cdb-check:r.lock->data,d : 0 [Type: core::cell::UnsafeCell<i32>]
// cdb-check: [<Raw View>] [Type: core::cell::UnsafeCell<i32>]
// cdb-check: [...] data : NonNull([...]: 0) [Type: core::ptr::non_null::NonNull<i32>]
// cdb-check: [...] inner_lock : [...] [Type: std::sys_common::rwlock::MovableRwLock *]

#[allow(unused_variables)]

Expand Down