Auto merge of #103150 - joboet:remove_lock_wrappers, r=m-ou-se

Remove lock wrappers in `sys_common`

This moves the lazy allocation to `sys` (SGX and UNIX). While this leads to a bit more verbosity, it will simplify future improvements by making room in `sys_common` for platform-independent implementations.

This also removes the condvar check on SGX as it is not necessary for soundness and will be removed anyway once mutex has been made movable.

For simplicity's sake, `libunwind` also uses lazy allocation now on SGX. This will require an update to the C definitions before merging this (CC `@raoulstrackx).`

r? `@m-ou-se`

4 files changed, 74 insertions, 73 deletions

diff --git a/library/std/src/sys/sgx/condvar.rs b/library/std/src/sys/sgx/condvar.rs
index 36534e0eff3..aa1174664ae 100644
--- a/library/std/src/sys/sgx/condvar.rs
+++ b/library/std/src/sys/sgx/condvar.rs
@@ -4,42 +4,43 @@ use crate::time::Duration;
 
 use super::waitqueue::{SpinMutex, WaitQueue, WaitVariable};
 
+/// FIXME: `UnsafeList` is not movable.
+struct AllocatedCondvar(SpinMutex<WaitVariable<()>>);
+
 pub struct Condvar {
-    inner: SpinMutex<WaitVariable<()>>,
+    inner: LazyBox<AllocatedCondvar>,
 }
 
-pub(crate) type MovableCondvar = LazyBox<Condvar>;
-
-impl LazyInit for Condvar {
+impl LazyInit for AllocatedCondvar {
     fn init() -> Box<Self> {
-        Box::new(Self::new())
+        Box::new(AllocatedCondvar(SpinMutex::new(WaitVariable::new(()))))
     }
 }
 
 impl Condvar {
     pub const fn new() -> Condvar {
-        Condvar { inner: SpinMutex::new(WaitVariable::new(())) }
+        Condvar { inner: LazyBox::new() }
     }
 
     #[inline]
-    pub unsafe fn notify_one(&self) {
-        let _ = WaitQueue::notify_one(self.inner.lock());
+    pub fn notify_one(&self) {
+        let _ = WaitQueue::notify_one(self.inner.0.lock());
     }
 
     #[inline]
-    pub unsafe fn notify_all(&self) {
-        let _ = WaitQueue::notify_all(self.inner.lock());
+    pub fn notify_all(&self) {
+        let _ = WaitQueue::notify_all(self.inner.0.lock());
     }
 
     pub unsafe fn wait(&self, mutex: &Mutex) {
-        let guard = self.inner.lock();
+        let guard = self.inner.0.lock();
         WaitQueue::wait(guard, || unsafe { mutex.unlock() });
-        unsafe { mutex.lock() }
+        mutex.lock()
     }
 
     pub unsafe fn wait_timeout(&self, mutex: &Mutex, dur: Duration) -> bool {
-        let success = WaitQueue::wait_timeout(&self.inner, dur, || unsafe { mutex.unlock() });
-        unsafe { mutex.lock() };
+        let success = WaitQueue::wait_timeout(&self.inner.0, dur, || unsafe { mutex.unlock() });
+        mutex.lock();
         success
     }
 }
diff --git a/library/std/src/sys/sgx/mutex.rs b/library/std/src/sys/sgx/mutex.rs
index aa747d56b0d..0dbf020ebe0 100644
--- a/library/std/src/sys/sgx/mutex.rs
+++ b/library/std/src/sys/sgx/mutex.rs
@@ -1,28 +1,28 @@
 use super::waitqueue::{try_lock_or_false, SpinMutex, WaitQueue, WaitVariable};
 use crate::sys_common::lazy_box::{LazyBox, LazyInit};
 
+/// FIXME: `UnsafeList` is not movable.
+struct AllocatedMutex(SpinMutex<WaitVariable<bool>>);
+
 pub struct Mutex {
-    inner: SpinMutex<WaitVariable<bool>>,
+    inner: LazyBox<AllocatedMutex>,
 }
 
-// not movable: see UnsafeList implementation
-pub(crate) type MovableMutex = LazyBox<Mutex>;
-
-impl LazyInit for Mutex {
+impl LazyInit for AllocatedMutex {
     fn init() -> Box<Self> {
-        Box::new(Self::new())
+        Box::new(AllocatedMutex(SpinMutex::new(WaitVariable::new(false))))
     }
 }
 
 // Implementation according to “Operating Systems: Three Easy Pieces”, chapter 28
 impl Mutex {
     pub const fn new() -> Mutex {
-        Mutex { inner: SpinMutex::new(WaitVariable::new(false)) }
+        Mutex { inner: LazyBox::new() }
     }
 
     #[inline]
-    pub unsafe fn lock(&self) {
-        let mut guard = self.inner.lock();
+    pub fn lock(&self) {
+        let mut guard = self.inner.0.lock();
         if *guard.lock_var() {
             // Another thread has the lock, wait
             WaitQueue::wait(guard, || {})
@@ -35,7 +35,7 @@ impl Mutex {
 
     #[inline]
     pub unsafe fn unlock(&self) {
-        let guard = self.inner.lock();
+        let guard = self.inner.0.lock();
         if let Err(mut guard) = WaitQueue::notify_one(guard) {
             // No other waiters, unlock
             *guard.lock_var_mut() = false;
@@ -45,8 +45,8 @@ impl Mutex {
     }
 
     #[inline]
-    pub unsafe fn try_lock(&self) -> bool {
-        let mut guard = try_lock_or_false!(self.inner);
+    pub fn try_lock(&self) -> bool {
+        let mut guard = try_lock_or_false!(self.inner.0);
         if *guard.lock_var() {
             // Another thread has the lock
             false
diff --git a/library/std/src/sys/sgx/rwlock.rs b/library/std/src/sys/sgx/rwlock.rs
index a97fb9ab026..d89de18ca5f 100644
--- a/library/std/src/sys/sgx/rwlock.rs
+++ b/library/std/src/sys/sgx/rwlock.rs
@@ -7,42 +7,45 @@ use crate::sys_common::lazy_box::{LazyBox, LazyInit};
 use super::waitqueue::{
     try_lock_or_false, NotifiedTcs, SpinMutex, SpinMutexGuard, WaitQueue, WaitVariable,
 };
-use crate::mem;
+use crate::alloc::Layout;
 
-pub struct RwLock {
+struct AllocatedRwLock {
     readers: SpinMutex<WaitVariable<Option<NonZeroUsize>>>,
     writer: SpinMutex<WaitVariable<bool>>,
 }
 
-pub(crate) type MovableRwLock = LazyBox<RwLock>;
+pub struct RwLock {
+    inner: LazyBox<AllocatedRwLock>,
+}
 
-impl LazyInit for RwLock {
+impl LazyInit for AllocatedRwLock {
     fn init() -> Box<Self> {
-        Box::new(Self::new())
+        Box::new(AllocatedRwLock {
+            readers: SpinMutex::new(WaitVariable::new(None)),
+            writer: SpinMutex::new(WaitVariable::new(false)),
+        })
     }
 }
 
-// Check at compile time that RwLock size matches C definition (see test_c_rwlock_initializer below)
-//
-// # Safety
-// Never called, as it is a compile time check.
-#[allow(dead_code)]
-unsafe fn rw_lock_size_assert(r: RwLock) {
-    unsafe { mem::transmute::<RwLock, [u8; 144]>(r) };
-}
+// Check at compile time that RwLock's size and alignment matches the C definition
+// in libunwind (see also `test_c_rwlock_initializer` in `tests`).
+const _: () = {
+    let rust = Layout::new::<RwLock>();
+    let c = Layout::new::<*mut ()>();
+    assert!(rust.size() == c.size());
+    assert!(rust.align() == c.align());
+};
 
 impl RwLock {
     pub const fn new() -> RwLock {
-        RwLock {
-            readers: SpinMutex::new(WaitVariable::new(None)),
-            writer: SpinMutex::new(WaitVariable::new(false)),
-        }
+        RwLock { inner: LazyBox::new() }
     }
 
     #[inline]
-    pub unsafe fn read(&self) {
-        let mut rguard = self.readers.lock();
-        let wguard = self.writer.lock();
+    pub fn read(&self) {
+        let lock = &*self.inner;
+        let mut rguard = lock.readers.lock();
+        let wguard = lock.writer.lock();
         if *wguard.lock_var() || !wguard.queue_empty() {
             // Another thread has or is waiting for the write lock, wait
             drop(wguard);
@@ -57,8 +60,9 @@ impl RwLock {
 
     #[inline]
     pub unsafe fn try_read(&self) -> bool {
-        let mut rguard = try_lock_or_false!(self.readers);
-        let wguard = try_lock_or_false!(self.writer);
+        let lock = &*self.inner;
+        let mut rguard = try_lock_or_false!(lock.readers);
+        let wguard = try_lock_or_false!(lock.writer);
         if *wguard.lock_var() || !wguard.queue_empty() {
             // Another thread has or is waiting for the write lock
             false
@@ -71,9 +75,10 @@ impl RwLock {
     }
 
     #[inline]
-    pub unsafe fn write(&self) {
-        let rguard = self.readers.lock();
-        let mut wguard = self.writer.lock();
+    pub fn write(&self) {
+        let lock = &*self.inner;
+        let rguard = lock.readers.lock();
+        let mut wguard = lock.writer.lock();
         if *wguard.lock_var() || rguard.lock_var().is_some() {
             // Another thread has the lock, wait
             drop(rguard);
@@ -86,9 +91,10 @@ impl RwLock {
     }
 
     #[inline]
-    pub unsafe fn try_write(&self) -> bool {
-        let rguard = try_lock_or_false!(self.readers);
-        let mut wguard = try_lock_or_false!(self.writer);
+    pub fn try_write(&self) -> bool {
+        let lock = &*self.inner;
+        let rguard = try_lock_or_false!(lock.readers);
+        let mut wguard = try_lock_or_false!(lock.writer);
         if *wguard.lock_var() || rguard.lock_var().is_some() {
             // Another thread has the lock
             false
@@ -122,8 +128,9 @@ impl RwLock {
 
     #[inline]
     pub unsafe fn read_unlock(&self) {
-        let rguard = self.readers.lock();
-        let wguard = self.writer.lock();
+        let lock = &*self.inner;
+        let rguard = lock.readers.lock();
+        let wguard = lock.writer.lock();
         unsafe { self.__read_unlock(rguard, wguard) };
     }
 
@@ -158,8 +165,9 @@ impl RwLock {
 
     #[inline]
     pub unsafe fn write_unlock(&self) {
-        let rguard = self.readers.lock();
-        let wguard = self.writer.lock();
+        let lock = &*self.inner;
+        let rguard = lock.readers.lock();
+        let wguard = lock.writer.lock();
         unsafe { self.__write_unlock(rguard, wguard) };
     }
 
@@ -167,8 +175,9 @@ impl RwLock {
     #[inline]
     #[cfg_attr(test, allow(dead_code))]
     unsafe fn unlock(&self) {
-        let rguard = self.readers.lock();
-        let wguard = self.writer.lock();
+        let lock = &*self.inner;
+        let rguard = lock.readers.lock();
+        let wguard = lock.writer.lock();
         if *wguard.lock_var() == true {
             unsafe { self.__write_unlock(rguard, wguard) };
         } else {
@@ -201,6 +210,7 @@ pub unsafe extern "C" fn __rust_rwlock_wrlock(p: *mut RwLock) -> i32 {
     unsafe { (*p).write() };
     return 0;
 }
+
 #[cfg(not(test))]
 #[no_mangle]
 pub unsafe extern "C" fn __rust_rwlock_unlock(p: *mut RwLock) -> i32 {
diff --git a/library/std/src/sys/sgx/rwlock/tests.rs b/library/std/src/sys/sgx/rwlock/tests.rs
index 4799961154a..5fd6670afd4 100644
--- a/library/std/src/sys/sgx/rwlock/tests.rs
+++ b/library/std/src/sys/sgx/rwlock/tests.rs
@@ -1,22 +1,12 @@
 use super::*;
+use crate::ptr;
 
 // Verify that the byte pattern libunwind uses to initialize an RwLock is
 // equivalent to the value of RwLock::new(). If the value changes,
 // `src/UnwindRustSgx.h` in libunwind needs to be changed too.
 #[test]
 fn test_c_rwlock_initializer() {
-    #[rustfmt::skip]
-    const C_RWLOCK_INIT: &[u8] = &[
-        /* 0x00 */ 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
-        /* 0x10 */ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x2, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
-        /* 0x20 */ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
-        /* 0x30 */ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
-        /* 0x40 */ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
-        /* 0x50 */ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
-        /* 0x60 */ 0x2, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
-        /* 0x70 */ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
-        /* 0x80 */ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
-    ];
+    const C_RWLOCK_INIT: *mut () = ptr::null_mut();
 
     // For the test to work, we need the padding/unused bytes in RwLock to be
     // initialized as 0. In practice, this is the case with statics.
@@ -26,6 +16,6 @@ fn test_c_rwlock_initializer() {
         // If the assertion fails, that not necessarily an issue with the value
         // of C_RWLOCK_INIT. It might just be an issue with the way padding
         // bytes are initialized in the test code.
-        assert_eq!(&crate::mem::transmute_copy::<_, [u8; 144]>(&RUST_RWLOCK_INIT), C_RWLOCK_INIT);
+        assert_eq!(crate::mem::transmute_copy::<_, *mut ()>(&RUST_RWLOCK_INIT), C_RWLOCK_INIT);
     };
 }