Fixes & polishes implementation of FUTEX_(UN)LOCK_PI in miri

ruihe774 · ruihe774 · commit 04ed07587db6 · 2024-10-13T02:57:16.000+08:00
diff --git a/src/tools/miri/src/shims/unix/linux/sync.rs b/src/tools/miri/src/shims/unix/linux/sync.rs
@@ -1,5 +1,7 @@
-use crate::*;
+use std::time::Duration;
+
 use crate::shims::unix::env::EvalContextExt;
+use crate::*;
 
 /// Implementation of the SYS_futex syscall.
 /// `args` is the arguments *after* the syscall number.
@@ -43,6 +45,46 @@ pub fn futex<'tcx>(
     let futex_unlock_pi = this.eval_libc_i32("FUTEX_UNLOCK_PI");
     let futex_waiters = this.eval_libc_u32("FUTEX_WAITERS");
 
+    // Ok(None) for EINVAL set, Ok(Some(None)) for no timeout (infinity), Ok(Some(Some(...))) for a timeout.
+    // Forgive me, I don't want to create an enum for this return value.
+    fn read_timeout<'tcx>(
+        this: &mut MiriInterpCx<'tcx>,
+        arg3: &OpTy<'tcx>,
+        use_realtime_clock: bool,
+        use_absolute_time: bool,
+        dest: &MPlaceTy<'tcx>,
+    ) -> InterpResult<'tcx, Option<Option<(TimeoutClock, TimeoutAnchor, Duration)>>> {
+        let timeout = this.deref_pointer_as(arg3, this.libc_ty_layout("timespec"))?;
+        interp_ok(Some(if this.ptr_is_null(timeout.ptr())? {
+            None
+        } else {
+            let duration = match this.read_timespec(&timeout)? {
+                Some(duration) => duration,
+                None => {
+                    this.set_last_error(LibcError("EINVAL"))?;
+                    this.write_scalar(Scalar::from_target_isize(-1, this), dest)?;
+                    return interp_ok(None);
+                }
+            };
+            let timeout_clock = if use_realtime_clock {
+                this.check_no_isolation(
+                    "`futex` syscall with `op=FUTEX_WAIT` and non-null timeout with `FUTEX_CLOCK_REALTIME`",
+                )?;
+                TimeoutClock::RealTime
+            } else {
+                TimeoutClock::Monotonic
+            };
+            let timeout_anchor = if use_absolute_time {
+                // FUTEX_WAIT_BITSET uses an absolute timestamp.
+                TimeoutAnchor::Absolute
+            } else {
+                // FUTEX_WAIT uses a relative timestamp.
+                TimeoutAnchor::Relative
+            };
+            Some((timeout_clock, timeout_anchor, duration))
+        }))
+    }
+
     // FUTEX_PRIVATE enables an optimization that stops it from working across processes.
     // Miri doesn't support that anyway, so we ignore that flag.
     match op & !futex_private {
@@ -54,7 +96,7 @@ pub fn futex<'tcx>(
         // This is identical to FUTEX_WAIT, except:
         //  - The timeout is absolute rather than relative.
         //  - You can specify the bitset to selecting what WAKE operations to respond to.
-        op if op & !futex_realtime == futex_wait || op & !futex_realtime == futex_wait_bitset || op == futex_lock_pi => {
+        op if op & !futex_realtime == futex_wait || op & !futex_realtime == futex_wait_bitset => {
             let wait_bitset = op & !futex_realtime == futex_wait_bitset;
 
             let bitset = if wait_bitset {
@@ -77,44 +119,25 @@ pub fn futex<'tcx>(
                 u32::MAX
             };
 
-
-            let val = this.read_scalar(&args[2])?.to_u32()?;
+            // We ensured at least 4 arguments above so these work.
+            let val = this.read_scalar(&args[2])?.to_i32()?;
+            let Some(timeout) = read_timeout(
+                this,
+                &args[3],
+                op & futex_realtime == futex_realtime,
+                wait_bitset,
+                dest,
+            )?
+            else {
+                return interp_ok(());
+            };
 
             if bitset == 0 {
                 this.set_last_error(LibcError("EINVAL"))?;
                 this.write_scalar(Scalar::from_target_isize(-1, this), dest)?;
                 return interp_ok(());
             }
 
-            let timeout = this.deref_pointer_as(&args[3], this.libc_ty_layout("timespec"))?;
-            let timeout = if this.ptr_is_null(timeout.ptr())? {
-                None
-            } else {
-                let duration = match this.read_timespec(&timeout)? {
-                    Some(duration) => duration,
-                    None => {
-                        this.set_last_error(LibcError("EINVAL"))?;
-                        this.write_scalar(Scalar::from_target_isize(-1, this), dest)?;
-                        return interp_ok(());
-                    }
-                };
-                let timeout_clock = if op & futex_realtime == futex_realtime {
-                    this.check_no_isolation(
-                        "`futex` syscall with `op=FUTEX_WAIT` and non-null timeout with `FUTEX_CLOCK_REALTIME`",
-                    )?;
-                    TimeoutClock::RealTime
-                } else {
-                    TimeoutClock::Monotonic
-                };
-                let timeout_anchor = if wait_bitset {
-                    // FUTEX_WAIT_BITSET uses an absolute timestamp.
-                    TimeoutAnchor::Absolute
-                } else {
-                    // FUTEX_WAIT uses a relative timestamp.
-                    TimeoutAnchor::Relative
-                };
-                Some((timeout_clock, timeout_anchor, duration))
-            };
             // There may be a concurrent thread changing the value of addr
             // and then invoking the FUTEX_WAKE syscall. It is critical that the
             // effects of this and the other thread are correctly observed,
@@ -158,16 +181,12 @@ pub fn futex<'tcx>(
             //
             // Thankfully, preemptions cannot happen inside a Miri shim, so we do not need to
             // do anything special to guarantee fence-load-comparison atomicity.
-            let futex_val = this.read_scalar_atomic(&addr, AtomicReadOrd::SeqCst)?.to_u32()?;
-            if if op == futex_lock_pi {
-                futex_val != 0
-            } else {
-                futex_val == val
-            } {
+            this.atomic_fence(AtomicFenceOrd::SeqCst)?;
+            // Read an `i32` through the pointer, regardless of any wrapper types.
+            // It's not uncommon for `addr` to be passed as another type than `*mut i32`, such as `*const AtomicI32`.
+            let futex_val = this.read_scalar_atomic(&addr, AtomicReadOrd::Relaxed)?.to_i32()?;
+            if val == futex_val {
                 // The value still matches, so we block the thread and make it wait for FUTEX_WAKE.
-                if op == futex_lock_pi {
-                    this.write_scalar_atomic(Scalar::from_u32(val | futex_waiters), &addr , AtomicWriteOrd::SeqCst)?;
-                }
                 this.futex_wait(
                     addr_usize,
                     bitset,
@@ -177,10 +196,6 @@ pub fn futex<'tcx>(
                     dest.clone(),
                     this.eval_libc("ETIMEDOUT"),
                 );
-            } else if op == futex_lock_pi {
-                let tid = this.linux_gettid()?;
-                this.write_scalar_atomic(tid, &addr, AtomicWriteOrd::SeqCst)?;
-                this.write_scalar(Scalar::from_target_isize(0, this), dest)?;
             } else {
                 // The futex value doesn't match the expected value, so we return failure
                 // right away without sleeping: -1 and errno set to EAGAIN.
@@ -195,8 +210,15 @@ pub fn futex<'tcx>(
         // Does not access the futex value at *addr.
         // FUTEX_WAKE_BITSET: (int *addr, int op = FUTEX_WAKE, int val, const timespect *_unused, int *_unused, unsigned int bitset)
         // Same as FUTEX_WAKE, but allows you to specify a bitset to select which threads to wake up.
-        op if op == futex_wake || op == futex_wake_bitset || op == futex_unlock_pi => {
-            let val = if op == futex_unlock_pi { 1 } else { this.read_scalar(&args[2])?.to_i32()? };
+        op if op == futex_wake || op == futex_wake_bitset => {
+            if args.len() < 3 {
+                throw_ub_format!(
+                    "incorrect number of arguments for `futex` syscall with `op=FUTEX_WAKE`: got {}, expected at least 3",
+                    args.len()
+                );
+            }
+
+            let val = this.read_scalar(&args[2])?.to_i32()?;
 
             let bitset = if op == futex_wake_bitset {
                 let [_, _, _, timeout, uaddr2, bitset, ..] = args else {
@@ -219,11 +241,7 @@ pub fn futex<'tcx>(
             // Together with the SeqCst fence in futex_wait, this makes sure that futex_wait
             // will see the latest value on addr which could be changed by our caller
             // before doing the syscall.
-            if op == futex_unlock_pi {
-                this.write_scalar_atomic(Scalar::from_u32(0), &addr, AtomicWriteOrd::SeqCst)?;
-            } else {
-                this.atomic_fence(AtomicFenceOrd::SeqCst)?;
-            }
+            this.atomic_fence(AtomicFenceOrd::SeqCst)?;
             let mut n = 0;
             #[allow(clippy::arithmetic_side_effects)]
             for _ in 0..val {
@@ -233,7 +251,79 @@ pub fn futex<'tcx>(
                     break;
                 }
             }
-            this.write_scalar(Scalar::from_target_isize(if op != futex_unlock_pi { n } else { 0 }, this), dest)?;
+            this.write_scalar(Scalar::from_target_isize(n, this), dest)?;
+        }
+        op if op == futex_lock_pi => {
+            if args.len() < 4 {
+                throw_ub_format!(
+                    "incorrect number of arguments for `futex` syscall with `op=FUTEX_LOCK_PI`: got {}, expected at least 4",
+                    args.len()
+                );
+            }
+
+            // FUTEX_LOCK_PI uses absolute CLOCK_REALTIME timestamp.
+            let Some(timeout) = read_timeout(this, &args[3], true, true, dest)? else {
+                return interp_ok(());
+            };
+
+            // The same as above. This makes modifications visible to us.
+            this.atomic_fence(AtomicFenceOrd::SeqCst)?;
+
+            // For bitand working properly, we read it as a u32.
+            let futex_val = this.read_scalar_atomic(&addr, AtomicReadOrd::Relaxed)?.to_u32()?;
+
+            if futex_val == 0 {
+                // 0 means unlocked - then lock it.
+                //
+                // The tid of the owner is store to *addr.
+                // N.B. it is not the same as posix thread id.
+                let tid = this.linux_gettid()?;
+                this.write_scalar_atomic(tid, &addr, AtomicWriteOrd::Relaxed)?;
+            } else {
+                // Other values mean locked.
+                //
+                // Mark the futex as contended.
+                this.write_scalar_atomic(
+                    Scalar::from_u32(futex_val | futex_waiters),
+                    &addr,
+                    AtomicWriteOrd::Relaxed,
+                )?;
+
+                // Put ourselves into the wait queue.
+                this.futex_wait(
+                    addr_usize,
+                    u32::MAX,
+                    timeout,
+                    Scalar::from_target_isize(0, this), // retval_succ
+                    Scalar::from_target_isize(-1, this), // retval_timeout
+                    dest.clone(),
+                    this.eval_libc("ETIMEDOUT"),
+                );
+            }
+
+            // This ensures all loads afterwards get updated value of *addr.
+            this.atomic_fence(AtomicFenceOrd::SeqCst)?;
+
+            // FUTEX_LOCK_PI returns 0 on success.
+            this.write_scalar(Scalar::from_target_isize(0, this), dest)?;
+        }
+        op if op == futex_unlock_pi => {
+            // This ensures all modifications happen before.
+            this.atomic_fence(AtomicFenceOrd::SeqCst)?;
+
+            // Clear locked state.
+            this.write_scalar_atomic(Scalar::from_u32(0), &addr, AtomicWriteOrd::Relaxed)?;
+
+            // This ensures all loads afterwards get updated value of *addr.
+            // There are no preemptions so no one can wake after we set the futex to unlocked
+            // and before we use futex_wake to wake one waiter.
+            this.atomic_fence(AtomicFenceOrd::SeqCst)?;
+
+            // Unlocking wakes zero or one waiters.
+            let _ = this.futex_wake(addr_usize, u32::MAX)?;
+
+            // FUTEX_UNLOCK_PI returns 0 on success.
+            this.write_scalar(Scalar::from_target_isize(0, this), dest)?;
         }
         op => throw_unsup_format!("Miri does not support `futex` syscall with op={}", op),
     }
