Auto merge of #142556 - RalfJung:miri-sync, r=RalfJung

Miri subtree update

r? `@ghost`

Author: bors

src/tools/miri/rust-version

@@ -1 +1 @@
-c6768de2d63de7a41124a0fb8fc78f9e26111c01
+0cbc0764380630780a275c437260e4d4d5f28c92

src/tools/miri/src/concurrency/sync.rs

@@ -99,7 +99,7 @@ pub enum BlockReason {
     /// Blocked on a condition variable.
     Condvar(CondvarId),
     /// Blocked on a reader-writer lock.
-    RwLock(RwLockId),
+    RwLock,
     /// Blocked on a Futex variable.
     Futex,
     /// Blocked on an InitOnce.

src/tools/miri/src/concurrency/thread.rs

@@ -85,7 +85,7 @@ impl fmt::Display for TerminationInfo {
             DataRace { involves_non_atomic, ptr, op1, op2, .. } =>
                 write!(
                     f,
-                    "{} detected between (1) {} on {} and (2) {} on {} at {ptr:?}. (2) just happened here",
+                    "{} detected between (1) {} on {} and (2) {} on {} at {ptr:?}",
                     if *involves_non_atomic { "Data race" } else { "Race condition" },
                     op1.action,
                     op1.thread_info,
@@ -224,7 +224,7 @@ pub fn report_error<'tcx>(
     use InterpErrorKind::*;
     use UndefinedBehaviorInfo::*;
 
-    let mut msg = vec![];
+    let mut labels = vec![];
 
     let (title, helps) = if let MachineStop(info) = e.kind() {
         let info = info.downcast_ref::<TerminationInfo>().expect("invalid MachineStop payload");
@@ -237,7 +237,10 @@ pub fn report_error<'tcx>(
                 Some("unsupported operation"),
             StackedBorrowsUb { .. } | TreeBorrowsUb { .. } | DataRace { .. } =>
                 Some("Undefined Behavior"),
-            Deadlock => Some("deadlock"),
+            Deadlock => {
+                labels.push(format!("this thread got stuck here"));
+                None
+            }
             GenmcStuckExecution => {
                 // This case should only happen in GenMC mode. We treat it like a normal program exit.
                 assert!(ecx.machine.data_race.as_genmc_ref().is_some());
@@ -259,7 +262,7 @@ pub fn report_error<'tcx>(
                 ]
             }
             StackedBorrowsUb { help, history, .. } => {
-                msg.extend(help.clone());
+                labels.extend(help.clone());
                 let mut helps = vec![
                     note!("this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental"),
                     note!("see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information"),
@@ -297,6 +300,7 @@ pub fn report_error<'tcx>(
             Int2PtrWithStrictProvenance =>
                 vec![note!("use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead")],
             DataRace { op1, extra, retag_explain, .. } => {
+                labels.push(format!("(2) just happened here"));
                 let mut helps = vec![note_span!(op1.span, "and (1) occurred earlier here")];
                 if let Some(extra) = extra {
                     helps.push(note!("{extra}"));
@@ -426,12 +430,20 @@ pub fn report_error<'tcx>(
         _ => {}
     }
 
-    msg.insert(0, format_interp_error(ecx.tcx.dcx(), e));
+    let mut primary_msg = String::new();
+    if let Some(title) = title {
+        write!(primary_msg, "{title}: ").unwrap();
+    }
+    write!(primary_msg, "{}", format_interp_error(ecx.tcx.dcx(), e)).unwrap();
+
+    if labels.is_empty() {
+        labels.push(format!("{} occurred here", title.unwrap_or("error")));
+    }
 
     report_msg(
         DiagLevel::Error,
-        if let Some(title) = title { format!("{title}: {}", msg[0]) } else { msg[0].clone() },
-        msg,
+        primary_msg,
+        labels,
         vec![],
         helps,
         &stacktrace,
@@ -449,8 +461,8 @@ pub fn report_error<'tcx>(
                 any_pruned |= was_pruned;
                 report_msg(
                     DiagLevel::Error,
-                    format!("deadlock: the evaluated program deadlocked"),
-                    vec![format!("the evaluated program deadlocked")],
+                    format!("the evaluated program deadlocked"),
+                    vec![format!("this thread got stuck here")],
                     vec![],
                     vec![],
                     &stacktrace,
@@ -611,7 +623,7 @@ impl<'tcx> MiriMachine<'tcx> {
         let stacktrace = Frame::generate_stacktrace_from_stack(self.threads.active_thread_stack());
         let (stacktrace, _was_pruned) = prune_stacktrace(stacktrace, self);
 
-        let (title, diag_level) = match &e {
+        let (label, diag_level) = match &e {
             RejectedIsolatedOp(_) =>
                 ("operation rejected by isolation".to_string(), DiagLevel::Warning),
             Int2Ptr { .. } => ("integer-to-pointer cast".to_string(), DiagLevel::Warning),
@@ -626,10 +638,10 @@ impl<'tcx> MiriMachine<'tcx> {
             | FreedAlloc(..)
             | ProgressReport { .. }
             | WeakMemoryOutdatedLoad { .. } =>
-                ("tracking was triggered".to_string(), DiagLevel::Note),
+                ("tracking was triggered here".to_string(), DiagLevel::Note),
         };
 
-        let msg = match &e {
+        let title = match &e {
             CreatedPointerTag(tag, None, _) => format!("created base tag {tag:?}"),
             CreatedPointerTag(tag, Some(perm), None) =>
                 format!("created {tag:?} with {perm} derived from unknown tag"),
@@ -735,7 +747,7 @@ impl<'tcx> MiriMachine<'tcx> {
         report_msg(
             diag_level,
             title,
-            vec![msg],
+            vec![label],
             notes,
             helps,
             &stacktrace,

src/tools/miri/src/diagnostics.rs

@@ -292,11 +292,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 let b = this.read_scalar(b)?.to_f32()?;
                 let c = this.read_scalar(c)?.to_f32()?;
                 let fuse: bool = this.machine.float_nondet && this.machine.rng.get_mut().random();
-                let res = if fuse {
-                    a.mul_add(b, c).value
-                } else {
-                    ((a * b).value + c).value
-                };
+                let res = if fuse { a.mul_add(b, c).value } else { ((a * b).value + c).value };
                 let res = this.adjust_nan(res, &[a, b, c]);
                 this.write_scalar(res, dest)?;
             }
@@ -306,11 +302,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 let b = this.read_scalar(b)?.to_f64()?;
                 let c = this.read_scalar(c)?.to_f64()?;
                 let fuse: bool = this.machine.float_nondet && this.machine.rng.get_mut().random();
-                let res = if fuse {
-                    a.mul_add(b, c).value
-                } else {
-                    ((a * b).value + c).value
-                };
+                let res = if fuse { a.mul_add(b, c).value } else { ((a * b).value + c).value };
                 let res = this.adjust_nan(res, &[a, b, c]);
                 this.write_scalar(res, dest)?;
             }

src/tools/miri/src/intrinsics/mod.rs

@@ -124,7 +124,7 @@ pub use crate::concurrency::data_race::{
 };
 pub use crate::concurrency::init_once::{EvalContextExt as _, InitOnceId};
 pub use crate::concurrency::sync::{
-    CondvarId, EvalContextExt as _, MutexRef, RwLockId, SynchronizationObjects,
+    CondvarId, EvalContextExt as _, MutexRef, RwLockRef, SynchronizationObjects,
 };
 pub use crate::concurrency::thread::{
     BlockReason, DynUnblockCallback, EvalContextExt as _, StackEmptyCallback, ThreadId,

src/tools/miri/src/lib.rs

@@ -289,15 +289,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         };
         let mutex_ref = mutex_ref.clone();
 
-        if this.mutex_is_locked(&mutex_ref) {
-            if this.mutex_get_owner(&mutex_ref) == this.active_thread() {
+        if let Some(owner) = mutex_ref.owner() {
+            if owner == this.active_thread() {
                 // Matching the current macOS implementation: abort on reentrant locking.
                 throw_machine_stop!(TerminationInfo::Abort(
                     "attempted to lock an os_unfair_lock that is already locked by the current thread".to_owned()
                 ));
             }
 
-            this.mutex_enqueue_and_block(&mutex_ref, None);
+            this.mutex_enqueue_and_block(mutex_ref, None);
         } else {
             this.mutex_lock(&mutex_ref);
         }
@@ -319,7 +319,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         };
         let mutex_ref = mutex_ref.clone();
 
-        if this.mutex_is_locked(&mutex_ref) {
+        if mutex_ref.owner().is_some() {
             // Contrary to the blocking lock function, this does not check for
             // reentrancy.
             this.write_scalar(Scalar::from_bool(false), dest)?;
@@ -350,9 +350,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             ));
         }
 
-        // If the lock is not locked by anyone now, it went quer.
+        // If the lock is not locked by anyone now, it went quiet.
         // Reset to zero so that it can be moved and initialized again for the next phase.
-        if !this.mutex_is_locked(&mutex_ref) {
+        if mutex_ref.owner().is_none() {
             let lock_place = this.deref_pointer_as(lock_op, this.machine.layouts.u32)?;
             this.write_scalar_atomic(Scalar::from_u32(0), &lock_place, AtomicWriteOrd::Relaxed)?;
         }
@@ -371,9 +371,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         };
         let mutex_ref = mutex_ref.clone();
 
-        if !this.mutex_is_locked(&mutex_ref)
-            || this.mutex_get_owner(&mutex_ref) != this.active_thread()
-        {
+        if mutex_ref.owner().is_none_or(|o| o != this.active_thread()) {
             throw_machine_stop!(TerminationInfo::Abort(
                 "called os_unfair_lock_assert_owner on an os_unfair_lock not owned by the current thread".to_owned()
             ));
@@ -393,17 +391,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         };
         let mutex_ref = mutex_ref.clone();
 
-        if this.mutex_is_locked(&mutex_ref)
-            && this.mutex_get_owner(&mutex_ref) == this.active_thread()
-        {
+        if mutex_ref.owner().is_some_and(|o| o == this.active_thread()) {
             throw_machine_stop!(TerminationInfo::Abort(
                 "called os_unfair_lock_assert_not_owner on an os_unfair_lock owned by the current thread".to_owned()
             ));
         }
 
-        // If the lock is not locked by anyone now, it went quer.
+        // If the lock is not locked by anyone now, it went quiet.
         // Reset to zero so that it can be moved and initialized again for the next phase.
-        if !this.mutex_is_locked(&mutex_ref) {
+        if mutex_ref.owner().is_none() {
             let lock_place = this.deref_pointer_as(lock_op, this.machine.layouts.u32)?;
             this.write_scalar_atomic(Scalar::from_u32(0), &lock_place, AtomicWriteOrd::Relaxed)?;
         }

src/tools/miri/src/shims/unix/macos/sync.rs

@@ -229,9 +229,9 @@ fn mutex_kind_from_static_initializer<'tcx>(
 // We store some data directly inside the type, ignoring the platform layout:
 // - init: u32
 
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Clone)]
 struct PthreadRwLock {
-    id: RwLockId,
+    rwlock_ref: RwLockRef,
 }
 
 fn rwlock_init_offset<'tcx>(ecx: &MiriInterpCx<'tcx>) -> InterpResult<'tcx, Size> {
@@ -278,8 +278,8 @@ where
             )? {
                 throw_unsup_format!("unsupported static initializer used for `pthread_rwlock_t`");
             }
-            let id = ecx.machine.sync.rwlock_create();
-            interp_ok(PthreadRwLock { id })
+            let rwlock_ref = ecx.machine.sync.rwlock_create();
+            interp_ok(PthreadRwLock { rwlock_ref })
         },
     )
 }
@@ -504,11 +504,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         let mutex = mutex_get_data(this, mutex_op)?.clone();
 
-        let ret = if this.mutex_is_locked(&mutex.mutex_ref) {
-            let owner_thread = this.mutex_get_owner(&mutex.mutex_ref);
+        let ret = if let Some(owner_thread) = mutex.mutex_ref.owner() {
             if owner_thread != this.active_thread() {
                 this.mutex_enqueue_and_block(
-                    &mutex.mutex_ref,
+                    mutex.mutex_ref,
                     Some((Scalar::from_i32(0), dest.clone())),
                 );
                 return interp_ok(());
@@ -541,8 +540,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         let mutex = mutex_get_data(this, mutex_op)?.clone();
 
-        interp_ok(Scalar::from_i32(if this.mutex_is_locked(&mutex.mutex_ref) {
-            let owner_thread = this.mutex_get_owner(&mutex.mutex_ref);
+        interp_ok(Scalar::from_i32(if let Some(owner_thread) = mutex.mutex_ref.owner() {
             if owner_thread != this.active_thread() {
                 this.eval_libc_i32("EBUSY")
             } else {
@@ -596,7 +594,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         // since we make the field uninit below.
         let mutex = mutex_get_data(this, mutex_op)?.clone();
 
-        if this.mutex_is_locked(&mutex.mutex_ref) {
+        if mutex.mutex_ref.owner().is_some() {
             throw_ub_format!("destroyed a locked mutex");
         }
 
@@ -616,12 +614,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
-        let id = rwlock_get_data(this, rwlock_op)?.id;
+        let rwlock = rwlock_get_data(this, rwlock_op)?.clone();
 
-        if this.rwlock_is_write_locked(id) {
-            this.rwlock_enqueue_and_block_reader(id, Scalar::from_i32(0), dest.clone());
+        if rwlock.rwlock_ref.is_write_locked() {
+            this.rwlock_enqueue_and_block_reader(
+                rwlock.rwlock_ref,
+                Scalar::from_i32(0),
+                dest.clone(),
+            );
         } else {
-            this.rwlock_reader_lock(id);
+            this.rwlock_reader_lock(&rwlock.rwlock_ref);
             this.write_null(dest)?;
         }
 
@@ -631,12 +633,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn pthread_rwlock_tryrdlock(&mut self, rwlock_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
-        let id = rwlock_get_data(this, rwlock_op)?.id;
+        let rwlock = rwlock_get_data(this, rwlock_op)?.clone();
 
-        if this.rwlock_is_write_locked(id) {
+        if rwlock.rwlock_ref.is_write_locked() {
             interp_ok(Scalar::from_i32(this.eval_libc_i32("EBUSY")))
         } else {
-            this.rwlock_reader_lock(id);
+            this.rwlock_reader_lock(&rwlock.rwlock_ref);
             interp_ok(Scalar::from_i32(0))
         }
     }
@@ -648,9 +650,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
-        let id = rwlock_get_data(this, rwlock_op)?.id;
+        let rwlock = rwlock_get_data(this, rwlock_op)?.clone();
 
-        if this.rwlock_is_locked(id) {
+        if rwlock.rwlock_ref.is_locked() {
             // Note: this will deadlock if the lock is already locked by this
             // thread in any way.
             //
@@ -663,9 +665,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             // report the deadlock only when no thread can continue execution,
             // but we could detect that this lock is already locked and report
             // an error.)
-            this.rwlock_enqueue_and_block_writer(id, Scalar::from_i32(0), dest.clone());
+            this.rwlock_enqueue_and_block_writer(
+                rwlock.rwlock_ref,
+                Scalar::from_i32(0),
+                dest.clone(),
+            );
         } else {
-            this.rwlock_writer_lock(id);
+            this.rwlock_writer_lock(&rwlock.rwlock_ref);
             this.write_null(dest)?;
         }
 
@@ -675,22 +681,24 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     fn pthread_rwlock_trywrlock(&mut self, rwlock_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
-        let id = rwlock_get_data(this, rwlock_op)?.id;
+        let rwlock = rwlock_get_data(this, rwlock_op)?.clone();
 
-        if this.rwlock_is_locked(id) {
+        if rwlock.rwlock_ref.is_locked() {
             interp_ok(Scalar::from_i32(this.eval_libc_i32("EBUSY")))
         } else {
-            this.rwlock_writer_lock(id);
+            this.rwlock_writer_lock(&rwlock.rwlock_ref);
             interp_ok(Scalar::from_i32(0))
         }
     }
 
     fn pthread_rwlock_unlock(&mut self, rwlock_op: &OpTy<'tcx>) -> InterpResult<'tcx, ()> {
         let this = self.eval_context_mut();
 
-        let id = rwlock_get_data(this, rwlock_op)?.id;
+        let rwlock = rwlock_get_data(this, rwlock_op)?.clone();
 
-        if this.rwlock_reader_unlock(id)? || this.rwlock_writer_unlock(id)? {
+        if this.rwlock_reader_unlock(&rwlock.rwlock_ref)?
+            || this.rwlock_writer_unlock(&rwlock.rwlock_ref)?
+        {
             interp_ok(())
         } else {
             throw_ub_format!("unlocked an rwlock that was not locked by the active thread");
@@ -702,9 +710,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         // Reading the field also has the side-effect that we detect double-`destroy`
         // since we make the field uninit below.
-        let id = rwlock_get_data(this, rwlock_op)?.id;
+        let rwlock = rwlock_get_data(this, rwlock_op)?.clone();
 
-        if this.rwlock_is_locked(id) {
+        if rwlock.rwlock_ref.is_locked() {
             throw_ub_format!("destroyed a locked rwlock");
         }