From 7cefa8f9959565acc6e3606cfa6aa1376c9f862f Mon Sep 17 00:00:00 2001 From: Robin Raymond Date: Sat, 7 May 2022 17:15:03 +0000 Subject: [PATCH 01/11] Make RwLockReadGuard covariant --- library/std/src/sync/rwlock.rs | 9 +++++---- library/std/src/sync/rwlock/tests.rs | 14 +++++++++++++- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index 9517e7e1f0325..efd2771fa1ce4 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -101,7 +101,8 @@ unsafe impl Sync for RwLock {} #[stable(feature = "rust1", since = "1.0.0")] #[clippy::has_significant_drop] pub struct RwLockReadGuard<'a, T: ?Sized + 'a> { - lock: &'a RwLock, + inner_lock: &'a sys::MovableRwLock, + data: &'a T, } #[stable(feature = "rust1", since = "1.0.0")] @@ -510,7 +511,7 @@ impl From for RwLock { impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> { unsafe fn new(lock: &'rwlock RwLock) -> LockResult> { - poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard { lock }) + poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard { inner_lock: &lock.inner, data: &*lock.data.get() }) } } @@ -553,7 +554,7 @@ impl Deref for RwLockReadGuard<'_, T> { type Target = T; fn deref(&self) -> &T { - unsafe { &*self.lock.data.get() } + self.data } } @@ -577,7 +578,7 @@ impl DerefMut for RwLockWriteGuard<'_, T> { impl Drop for RwLockReadGuard<'_, T> { fn drop(&mut self) { unsafe { - self.lock.inner.read_unlock(); + self.inner_lock.read_unlock(); } } } diff --git a/library/std/src/sync/rwlock/tests.rs b/library/std/src/sync/rwlock/tests.rs index 53aa2b1e38a91..1ffa13f83218c 100644 --- a/library/std/src/sync/rwlock/tests.rs +++ b/library/std/src/sync/rwlock/tests.rs @@ -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, TryLockError, RwLockReadGuard}; use crate::thread; use rand::{self, Rng}; @@ -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); +} From 0b6e6e3d6355dbd233eb9997316f79928ddfc0a6 Mon Sep 17 00:00:00 2001 From: Robin Raymond Date: Sun, 19 Jun 2022 09:22:32 +0200 Subject: [PATCH 02/11] Formatting --- library/std/src/sync/rwlock.rs | 5 ++++- library/std/src/sync/rwlock/tests.rs | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index efd2771fa1ce4..d5414f40266e9 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -511,7 +511,10 @@ impl From for RwLock { impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> { unsafe fn new(lock: &'rwlock RwLock) -> LockResult> { - poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard { inner_lock: &lock.inner, data: &*lock.data.get() }) + poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard { + inner_lock: &lock.inner, + data: &*lock.data.get() + }) } } diff --git a/library/std/src/sync/rwlock/tests.rs b/library/std/src/sync/rwlock/tests.rs index 1ffa13f83218c..08255c985f5eb 100644 --- a/library/std/src/sync/rwlock/tests.rs +++ b/library/std/src/sync/rwlock/tests.rs @@ -1,6 +1,6 @@ use crate::sync::atomic::{AtomicUsize, Ordering}; use crate::sync::mpsc::channel; -use crate::sync::{Arc, RwLock, TryLockError, RwLockReadGuard}; +use crate::sync::{Arc, RwLock, RwLockReadGuard, TryLockError}; use crate::thread; use rand::{self, Rng}; From 391f80070549852777428d83298be16f88cb4f38 Mon Sep 17 00:00:00 2001 From: Robin Raymond Date: Sat, 7 May 2022 20:00:46 +0200 Subject: [PATCH 03/11] More formatting --- library/std/src/sync/rwlock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index d5414f40266e9..22207b266fa65 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -513,7 +513,7 @@ impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> { unsafe fn new(lock: &'rwlock RwLock) -> LockResult> { poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard { inner_lock: &lock.inner, - data: &*lock.data.get() + data: &*lock.data.get(), }) } } From cf1238e799b18f38a9c328b6700434e38f98cec7 Mon Sep 17 00:00:00 2001 From: Robin Raymond Date: Fri, 13 May 2022 07:18:57 +0000 Subject: [PATCH 04/11] Address comments --- library/std/src/sync/rwlock.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index 22207b266fa65..ac3c3b509cde6 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -101,8 +101,8 @@ unsafe impl Sync for RwLock {} #[stable(feature = "rust1", since = "1.0.0")] #[clippy::has_significant_drop] pub struct RwLockReadGuard<'a, T: ?Sized + 'a> { + data: *const T, inner_lock: &'a sys::MovableRwLock, - data: &'a T, } #[stable(feature = "rust1", since = "1.0.0")] @@ -513,7 +513,7 @@ impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> { unsafe fn new(lock: &'rwlock RwLock) -> LockResult> { poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard { inner_lock: &lock.inner, - data: &*lock.data.get(), + data: lock.data.get(), }) } } @@ -557,7 +557,7 @@ impl Deref for RwLockReadGuard<'_, T> { type Target = T; fn deref(&self) -> &T { - self.data + unsafe { &*self.data } } } From 08650fbb506c44fbc95d91abfb7cd95a56bbff30 Mon Sep 17 00:00:00 2001 From: Robin Raymond Date: Sat, 14 May 2022 07:29:02 +0000 Subject: [PATCH 05/11] *const to NonNull plus documentation --- library/std/src/sync/rwlock.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index ac3c3b509cde6..81693a7f5f5cd 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -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; @@ -101,7 +102,7 @@ unsafe impl Sync for RwLock {} #[stable(feature = "rust1", since = "1.0.0")] #[clippy::has_significant_drop] pub struct RwLockReadGuard<'a, T: ?Sized + 'a> { - data: *const T, + data: NonNull, inner_lock: &'a sys::MovableRwLock, } @@ -510,15 +511,23 @@ impl From for RwLock { } impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> { + /// Create a new instance of `RwLockReadGuard` from a `RwLock`. + /// + /// It is safe to call this function if and only if `lock.inner.read()` (or + /// `lock.inner.try_read()`) has been successfully called before instantiating this object. unsafe fn new(lock: &'rwlock RwLock) -> LockResult> { poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard { + data: NonNull::new_unchecked(lock.data.get()), inner_lock: &lock.inner, - data: lock.data.get(), }) } } impl<'rwlock, T: ?Sized> RwLockWriteGuard<'rwlock, T> { + /// Create a new instance of `RwLockReadGuard` from a `RwLock`. + /// + /// It is safe to call this function if and only if `lock.inner.write()` (or + /// `lock.inner.try_write()`) has been successfully called before instantiating this object. unsafe fn new(lock: &'rwlock RwLock) -> LockResult> { poison::map_result(lock.poison.guard(), |guard| RwLockWriteGuard { lock, poison: guard }) } @@ -557,7 +566,7 @@ impl Deref for RwLockReadGuard<'_, T> { type Target = T; fn deref(&self) -> &T { - unsafe { &*self.data } + unsafe { self.data.as_ref() } } } From 0157593c744d778824d888980d736b190ff18eaa Mon Sep 17 00:00:00 2001 From: Robin Raymond Date: Sat, 14 May 2022 07:34:11 +0000 Subject: [PATCH 06/11] Documentation typo --- library/std/src/sync/rwlock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index 81693a7f5f5cd..00c34b96b788f 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -524,7 +524,7 @@ impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> { } impl<'rwlock, T: ?Sized> RwLockWriteGuard<'rwlock, T> { - /// Create a new instance of `RwLockReadGuard` from a `RwLock`. + /// Create a new instance of `RwLockWriteGuard` from a `RwLock`. /// /// It is safe to call this function if and only if `lock.inner.write()` (or /// `lock.inner.try_write()`) has been successfully called before instantiating this object. From fa1656e8aec9b64718cb456f810b40cea4b465b1 Mon Sep 17 00:00:00 2001 From: Robin Raymond Date: Thu, 26 May 2022 06:38:23 +0000 Subject: [PATCH 07/11] Add safety comments --- library/std/src/sync/rwlock.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index 00c34b96b788f..02d221fe6c6b9 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -512,9 +512,8 @@ impl From for RwLock { impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> { /// Create a new instance of `RwLockReadGuard` from a `RwLock`. - /// - /// It is safe to call this function if and only if `lock.inner.read()` (or - /// `lock.inner.try_read()`) has been successfully called before instantiating this object. + // 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) -> LockResult> { poison::map_result(lock.poison.borrow(), |()| RwLockReadGuard { data: NonNull::new_unchecked(lock.data.get()), @@ -525,9 +524,8 @@ impl<'rwlock, T: ?Sized> RwLockReadGuard<'rwlock, T> { impl<'rwlock, T: ?Sized> RwLockWriteGuard<'rwlock, T> { /// Create a new instance of `RwLockWriteGuard` from a `RwLock`. - /// - /// It is safe to call this function if and only if `lock.inner.write()` (or - /// `lock.inner.try_write()`) has been successfully called before instantiating this object. + // 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) -> LockResult> { poison::map_result(lock.poison.guard(), |guard| RwLockWriteGuard { lock, poison: guard }) } @@ -566,6 +564,7 @@ impl Deref for RwLockReadGuard<'_, T> { type Target = T; fn deref(&self) -> &T { + // SAFETY: the conditions of `RwLockGuard::new` were satisfied when created. unsafe { self.data.as_ref() } } } @@ -575,6 +574,7 @@ impl 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() } } } @@ -582,6 +582,7 @@ impl Deref for RwLockWriteGuard<'_, T> { #[stable(feature = "rust1", since = "1.0.0")] impl 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() } } } @@ -589,6 +590,7 @@ impl DerefMut for RwLockWriteGuard<'_, T> { #[stable(feature = "rust1", since = "1.0.0")] impl Drop for RwLockReadGuard<'_, T> { fn drop(&mut self) { + // SAFETY: the conditions of `RwLockReadGuard::new` were satisfied when created. unsafe { self.inner_lock.read_unlock(); } @@ -599,6 +601,7 @@ impl Drop for RwLockReadGuard<'_, T> { impl 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(); } From 09d937ed5f26888154c499fa97f8c45c691c9a31 Mon Sep 17 00:00:00 2001 From: Robin Raymond Date: Thu, 16 Jun 2022 05:35:44 +0000 Subject: [PATCH 08/11] Add comment explaining why we use NonNull --- library/std/src/sync/rwlock.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/std/src/sync/rwlock.rs b/library/std/src/sync/rwlock.rs index 02d221fe6c6b9..51e9c73d41a74 100644 --- a/library/std/src/sync/rwlock.rs +++ b/library/std/src/sync/rwlock.rs @@ -102,6 +102,10 @@ unsafe impl Sync for RwLock {} #[stable(feature = "rust1", since = "1.0.0")] #[clippy::has_significant_drop] pub struct RwLockReadGuard<'a, T: ?Sized + 'a> { + // 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, inner_lock: &'a sys::MovableRwLock, } From cb20e252aded0a8b66cc7164b42a16883925b4b5 Mon Sep 17 00:00:00 2001 From: Robin Raymond Date: Sun, 19 Jun 2022 10:36:26 +0000 Subject: [PATCH 09/11] Add test to verify noalias is not being added --- src/test/codegen/noalias-rwlockreadguard.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 src/test/codegen/noalias-rwlockreadguard.rs diff --git a/src/test/codegen/noalias-rwlockreadguard.rs b/src/test/codegen/noalias-rwlockreadguard.rs new file mode 100644 index 0000000000000..48e4d46b5894a --- /dev/null +++ b/src/test/codegen/noalias-rwlockreadguard.rs @@ -0,0 +1,14 @@ +// compile-flags: -O -C no-prepopulate-passes -Z mutable-noalias=yes + +#![crate_type = "lib"] + +use std::cell::UnsafeCell; +use std::sync::RwLockReadGuard; + +// Make sure that `RwLockReadGuard` does not get a `noalias` attribute, because +// the `UnsafeCell` might alias writes after it is dropped. + +// CHECK-LABEL: @maybe_aliased( +// CHECK-NOT: noalias +#[no_mangle] +pub unsafe fn maybe_aliased(_: RwLockReadGuard<'_, i32>, _data: &UnsafeCell) {} From 43c6f9c6919ec377dff35ecfdd6d87b66c692920 Mon Sep 17 00:00:00 2001 From: Robin Raymond Date: Sun, 19 Jun 2022 11:07:35 +0000 Subject: [PATCH 10/11] Make sure we don't match noalias in later lines --- src/test/codegen/noalias-rwlockreadguard.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/codegen/noalias-rwlockreadguard.rs b/src/test/codegen/noalias-rwlockreadguard.rs index 48e4d46b5894a..f9a2cc759eb57 100644 --- a/src/test/codegen/noalias-rwlockreadguard.rs +++ b/src/test/codegen/noalias-rwlockreadguard.rs @@ -10,5 +10,6 @@ use std::sync::RwLockReadGuard; // CHECK-LABEL: @maybe_aliased( // CHECK-NOT: noalias +// CHECK-SAME: %_data #[no_mangle] pub unsafe fn maybe_aliased(_: RwLockReadGuard<'_, i32>, _data: &UnsafeCell) {} From 048a80140b82471b671e7ae17f6dce2bb604f473 Mon Sep 17 00:00:00 2001 From: Robin Raymond Date: Mon, 20 Jun 2022 04:44:25 +0000 Subject: [PATCH 11/11] UnsafeCell -> RwLock --- src/test/codegen/noalias-rwlockreadguard.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/test/codegen/noalias-rwlockreadguard.rs b/src/test/codegen/noalias-rwlockreadguard.rs index f9a2cc759eb57..7f7b46c85a8b0 100644 --- a/src/test/codegen/noalias-rwlockreadguard.rs +++ b/src/test/codegen/noalias-rwlockreadguard.rs @@ -2,14 +2,13 @@ #![crate_type = "lib"] -use std::cell::UnsafeCell; -use std::sync::RwLockReadGuard; +use std::sync::{RwLock, RwLockReadGuard}; // Make sure that `RwLockReadGuard` does not get a `noalias` attribute, because -// the `UnsafeCell` might alias writes after it is dropped. +// 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: &UnsafeCell) {} +pub unsafe fn maybe_aliased(_: RwLockReadGuard<'_, i32>, _data: &RwLock) {}