print proper error when using unsupported synchronization primitive with GenMC

Author: RalfJung

src/tools/miri/src/alloc_addresses/mod.rs

@@ -498,6 +498,8 @@ impl<'tcx> MiriMachine<'tcx> {
         // Also remember this address for future reuse.
         let thread = self.threads.active_thread();
         global_state.reuse.add_addr(rng, addr, size, align, kind, thread, || {
+            // We already excluded GenMC above. We cannot use `self.release_clock` as
+            // `self.alloc_addresses` is borrowed.
             if let Some(data_race) = self.data_race.as_vclocks_ref() {
                 data_race.release_clock(&self.threads, |clock| clock.clone())
             } else {

src/tools/miri/src/concurrency/data_race.rs

@@ -958,16 +958,20 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> {
     /// with this program point.
     ///
     /// The closure will only be invoked if data race handling is on.
-    fn release_clock<R>(&self, callback: impl FnOnce(&VClock) -> R) -> Option<R> {
+    fn release_clock<R>(
+        &self,
+        callback: impl FnOnce(&VClock) -> R,
+    ) -> InterpResult<'tcx, Option<R>> {
         let this = self.eval_context_ref();
-        // FIXME: make this a proper error instead of ICEing the interpreter.
-        assert!(
-            this.machine.data_race.as_genmc_ref().is_none(),
-            "this operation performs synchronization that is not supported in GenMC mode"
-        );
-        Some(
-            this.machine.data_race.as_vclocks_ref()?.release_clock(&this.machine.threads, callback),
-        )
+        interp_ok(match &this.machine.data_race {
+            GlobalDataRaceHandler::None => None,
+            GlobalDataRaceHandler::Genmc(_genmc_ctx) =>
+                throw_unsup_format!(
+                    "this operation performs synchronization that is not supported in GenMC mode"
+                ),
+            GlobalDataRaceHandler::Vclocks(data_race) =>
+                Some(data_race.release_clock(&this.machine.threads, callback)),
+        })
     }
 
     /// Acquire the given clock into the current thread, establishing synchronization with

src/tools/miri/src/concurrency/init_once.rs

@@ -96,10 +96,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         init_once.status = InitOnceStatus::Complete;
 
         // Each complete happens-before the end of the wait
-        if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-            data_race
-                .release_clock(&this.machine.threads, |clock| init_once.clock.clone_from(clock));
-        }
+        this.release_clock(|clock| init_once.clock.clone_from(clock))?;
 
         // Wake up everyone.
         // need to take the queue to avoid having `this` be borrowed multiple times
@@ -125,10 +122,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         init_once.status = InitOnceStatus::Uninitialized;
 
         // Each complete happens-before the end of the wait
-        if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-            data_race
-                .release_clock(&this.machine.threads, |clock| init_once.clock.clone_from(clock));
-        }
+        this.release_clock(|clock| init_once.clock.clone_from(clock))?;
 
         // Wake up one waiting thread, so they can go ahead and try to init this.
         if let Some(waiter) = init_once.waiters.pop_front() {

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

@@ -204,7 +204,7 @@ pub(super) trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
             this.mutex_enqueue_and_block(mutex_ref, Some((retval, dest)));
         } else {
             // We can have it right now!
-            this.mutex_lock(&mutex_ref);
+            this.mutex_lock(&mutex_ref)?;
             // Don't forget to write the return value.
             this.write_scalar(retval, &dest)?;
         }
@@ -338,7 +338,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     }
 
     /// Lock by setting the mutex owner and increasing the lock count.
-    fn mutex_lock(&mut self, mutex_ref: &MutexRef) {
+    fn mutex_lock(&mut self, mutex_ref: &MutexRef) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         let thread = this.active_thread();
         let mut mutex = mutex_ref.0.borrow_mut();
@@ -352,9 +352,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             mutex.owner = Some(thread);
         }
         mutex.lock_count = mutex.lock_count.strict_add(1);
-        if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-            data_race.acquire_clock(&mutex.clock, &this.machine.threads);
-        }
+        this.acquire_clock(&mutex.clock)?;
+        interp_ok(())
     }
 
     /// Try unlocking by decreasing the lock count and returning the old lock
@@ -377,11 +376,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 // The mutex is completely unlocked. Try transferring ownership
                 // to another thread.
 
-                if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-                    data_race.release_clock(&this.machine.threads, |clock| {
-                        mutex.clock.clone_from(clock)
-                    });
-                }
+                this.release_clock(|clock| mutex.clock.clone_from(clock))?;
                 let thread_id = mutex.queue.pop_front();
                 // We need to drop our mutex borrow before unblock_thread
                 // because it will be borrowed again in the unblock callback.
@@ -425,7 +420,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     assert_eq!(unblock, UnblockKind::Ready);
 
                     assert!(mutex_ref.owner().is_none());
-                    this.mutex_lock(&mutex_ref);
+                    this.mutex_lock(&mutex_ref)?;
 
                     if let Some((retval, dest)) = retval_dest {
                         this.write_scalar(retval, &dest)?;
@@ -439,17 +434,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
     /// Read-lock the lock by adding the `reader` the list of threads that own
     /// this lock.
-    fn rwlock_reader_lock(&mut self, rwlock_ref: &RwLockRef) {
+    fn rwlock_reader_lock(&mut self, rwlock_ref: &RwLockRef) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         let thread = this.active_thread();
         trace!("rwlock_reader_lock: now also held (one more time) by {:?}", thread);
         let mut rwlock = rwlock_ref.0.borrow_mut();
         assert!(!rwlock.is_write_locked(), "the lock is write locked");
         let count = rwlock.readers.entry(thread).or_insert(0);
         *count = count.strict_add(1);
-        if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-            data_race.acquire_clock(&rwlock.clock_unlocked, &this.machine.threads);
-        }
+        this.acquire_clock(&rwlock.clock_unlocked)?;
+        interp_ok(())
     }
 
     /// Try read-unlock the lock for the current threads and potentially give the lock to a new owner.
@@ -472,12 +466,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             }
             Entry::Vacant(_) => return interp_ok(false), // we did not even own this lock
         }
-        if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-            // Add this to the shared-release clock of all concurrent readers.
-            data_race.release_clock(&this.machine.threads, |clock| {
-                rwlock.clock_current_readers.join(clock)
-            });
-        }
+        // Add this to the shared-release clock of all concurrent readers.
+        this.release_clock(|clock| rwlock.clock_current_readers.join(clock))?;
 
         // The thread was a reader. If the lock is not held any more, give it to a writer.
         if rwlock.is_locked().not() {
@@ -521,7 +511,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 }
                 |this, unblock: UnblockKind| {
                     assert_eq!(unblock, UnblockKind::Ready);
-                    this.rwlock_reader_lock(&rwlock_ref);
+                    this.rwlock_reader_lock(&rwlock_ref)?;
                     this.write_scalar(retval, &dest)?;
                     interp_ok(())
                 }
@@ -531,17 +521,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
     /// Lock by setting the writer that owns the lock.
     #[inline]
-    fn rwlock_writer_lock(&mut self, rwlock_ref: &RwLockRef) {
+    fn rwlock_writer_lock(&mut self, rwlock_ref: &RwLockRef) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         let thread = this.active_thread();
         trace!("rwlock_writer_lock: now held by {:?}", thread);
 
         let mut rwlock = rwlock_ref.0.borrow_mut();
         assert!(!rwlock.is_locked(), "the rwlock is already locked");
         rwlock.writer = Some(thread);
-        if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-            data_race.acquire_clock(&rwlock.clock_unlocked, &this.machine.threads);
-        }
+        this.acquire_clock(&rwlock.clock_unlocked)?;
+        interp_ok(())
     }
 
     /// Try to unlock an rwlock held by the current thread.
@@ -559,11 +548,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             rwlock.writer = None;
             trace!("rwlock_writer_unlock: unlocked by {:?}", thread);
             // Record release clock for next lock holder.
-            if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-                data_race.release_clock(&this.machine.threads, |clock| {
-                    rwlock.clock_unlocked.clone_from(clock)
-                });
-            }
+            this.release_clock(|clock| rwlock.clock_unlocked.clone_from(clock))?;
 
             // The thread was a writer.
             //
@@ -613,7 +598,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 }
                 |this, unblock: UnblockKind| {
                     assert_eq!(unblock, UnblockKind::Ready);
-                    this.rwlock_writer_lock(&rwlock_ref);
+                    this.rwlock_writer_lock(&rwlock_ref)?;
                     this.write_scalar(retval, &dest)?;
                     interp_ok(())
                 }
@@ -663,12 +648,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     match unblock {
                         UnblockKind::Ready => {
                             // The condvar was signaled. Make sure we get the clock for that.
-                            if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-                                data_race.acquire_clock(
+                            this.acquire_clock(
                                     &condvar_ref.0.borrow().clock,
-                                    &this.machine.threads,
-                                );
-                            }
+                                )?;
                             // Try to acquire the mutex.
                             // The timeout only applies to the first wait (until the signal), not for mutex acquisition.
                             this.condvar_reacquire_mutex(mutex_ref, retval_succ, dest)
@@ -695,9 +677,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let mut condvar = condvar_ref.0.borrow_mut();
 
         // Each condvar signal happens-before the end of the condvar wake
-        if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-            data_race.release_clock(&this.machine.threads, |clock| condvar.clock.clone_from(clock));
-        }
+        this.release_clock(|clock| condvar.clock.clone_from(clock))?;
         let Some(waiter) = condvar.waiters.pop_front() else {
             return interp_ok(false);
         };
@@ -736,9 +716,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                         UnblockKind::Ready => {
                             let futex = futex_ref.0.borrow();
                             // Acquire the clock of the futex.
-                            if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-                                data_race.acquire_clock(&futex.clock, &this.machine.threads);
-                            }
+                            this.acquire_clock(&futex.clock)?;
                         },
                         UnblockKind::TimedOut => {
                             // Remove the waiter from the futex.
@@ -766,9 +744,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let mut futex = futex_ref.0.borrow_mut();
 
         // Each futex-wake happens-before the end of the futex wait
-        if let Some(data_race) = this.machine.data_race.as_vclocks_ref() {
-            data_race.release_clock(&this.machine.threads, |clock| futex.clock.clone_from(clock));
-        }
+        this.release_clock(|clock| futex.clock.clone_from(clock))?;
 
         // Remove `count` of the threads in the queue that match any of the bits in the bitset.
         // We collect all of them before unblocking because the unblock callback may access the

src/tools/miri/src/shims/unix/linux_like/epoll.rs

@@ -608,7 +608,7 @@ fn check_and_update_one_event_interest<'tcx>(
         // If we are tracking data races, remember the current clock so we can sync with it later.
         ecx.release_clock(|clock| {
             event_instance.clock.clone_from(clock);
-        });
+        })?;
         // Triggers the notification by inserting it to the ready list.
         ready_list.insert(epoll_key, event_instance);
         interp_ok(true)

src/tools/miri/src/shims/unix/linux_like/eventfd.rs

@@ -199,7 +199,7 @@ fn eventfd_write<'tcx>(
             // Future `read` calls will synchronize with this write, so update the FD clock.
             ecx.release_clock(|clock| {
                 eventfd.clock.borrow_mut().join(clock);
-            });
+            })?;
 
             // Store new counter value.
             eventfd.counter.set(new_count);

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

@@ -296,7 +296,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
             this.mutex_enqueue_and_block(mutex_ref, None);
         } else {
-            this.mutex_lock(&mutex_ref);
+            this.mutex_lock(&mutex_ref)?;
         }
 
         interp_ok(())
@@ -321,7 +321,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             // reentrancy.
             this.write_scalar(Scalar::from_bool(false), dest)?;
         } else {
-            this.mutex_lock(&mutex_ref);
+            this.mutex_lock(&mutex_ref)?;
             this.write_scalar(Scalar::from_bool(true), dest)?;
         }
 

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

@@ -498,14 +498,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     MutexKind::Normal => throw_machine_stop!(TerminationInfo::Deadlock),
                     MutexKind::ErrorCheck => this.eval_libc_i32("EDEADLK"),
                     MutexKind::Recursive => {
-                        this.mutex_lock(&mutex.mutex_ref);
+                        this.mutex_lock(&mutex.mutex_ref)?;
                         0
                     }
                 }
             }
         } else {
             // The mutex is unlocked. Let's lock it.
-            this.mutex_lock(&mutex.mutex_ref);
+            this.mutex_lock(&mutex.mutex_ref)?;
             0
         };
         this.write_scalar(Scalar::from_i32(ret), dest)?;
@@ -525,14 +525,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     MutexKind::Default | MutexKind::Normal | MutexKind::ErrorCheck =>
                         this.eval_libc_i32("EBUSY"),
                     MutexKind::Recursive => {
-                        this.mutex_lock(&mutex.mutex_ref);
+                        this.mutex_lock(&mutex.mutex_ref)?;
                         0
                     }
                 }
             }
         } else {
             // The mutex is unlocked. Let's lock it.
-            this.mutex_lock(&mutex.mutex_ref);
+            this.mutex_lock(&mutex.mutex_ref)?;
             0
         }))
     }
@@ -600,7 +600,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 dest.clone(),
             );
         } else {
-            this.rwlock_reader_lock(&rwlock.rwlock_ref);
+            this.rwlock_reader_lock(&rwlock.rwlock_ref)?;
             this.write_null(dest)?;
         }
 
@@ -615,7 +615,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         if rwlock.rwlock_ref.is_write_locked() {
             interp_ok(Scalar::from_i32(this.eval_libc_i32("EBUSY")))
         } else {
-            this.rwlock_reader_lock(&rwlock.rwlock_ref);
+            this.rwlock_reader_lock(&rwlock.rwlock_ref)?;
             interp_ok(Scalar::from_i32(0))
         }
     }
@@ -648,7 +648,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 dest.clone(),
             );
         } else {
-            this.rwlock_writer_lock(&rwlock.rwlock_ref);
+            this.rwlock_writer_lock(&rwlock.rwlock_ref)?;
             this.write_null(dest)?;
         }
 
@@ -663,7 +663,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         if rwlock.rwlock_ref.is_locked() {
             interp_ok(Scalar::from_i32(this.eval_libc_i32("EBUSY")))
         } else {
-            this.rwlock_writer_lock(&rwlock.rwlock_ref);
+            this.rwlock_writer_lock(&rwlock.rwlock_ref)?;
             interp_ok(Scalar::from_i32(0))
         }
     }

src/tools/miri/src/shims/unix/unnamed_socket.rs

@@ -259,7 +259,7 @@ fn anonsocket_write<'tcx>(
         // Remember this clock so `read` can synchronize with us.
         ecx.release_clock(|clock| {
             writebuf.clock.join(clock);
-        });
+        })?;
         // Do full write / partial write based on the space available.
         let write_size = len.min(available_space);
         let actual_write_size = ecx.write_to_host(&mut writebuf.buf, write_size, ptr)?.unwrap();