Small changes.

vakaras · vakaras · commit bd97074517c6 · 2020-05-25T00:02:54.000+02:00
diff --git a/src/shims/sync.rs b/src/shims/sync.rs
@@ -288,15 +288,12 @@ fn release_cond_mutex<'mir, 'tcx: 'mir>(
     active_thread: ThreadId,
     mutex: MutexId,
 ) -> InterpResult<'tcx> {
-    if let Some((old_owner_thread, old_locked_count)) = ecx.mutex_unlock(mutex)? {
+    if let Some(old_locked_count) = ecx.mutex_unlock(mutex, active_thread)? {
         if old_locked_count != 1 {
             throw_unsup_format!("awaiting on a lock acquired multiple times is not supported");
         }
-        if old_owner_thread != active_thread {
-            throw_ub_format!("awaiting on a mutex owned by a different thread");
-        }
     } else {
-        throw_ub_format!("awaiting on unlocked mutex");
+        throw_ub_format!("awaiting on unlocked or owned by a different thread mutex");
     }
     ecx.block_thread(active_thread)?;
     Ok(())
@@ -321,7 +318,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let this = self.eval_context_mut();
 
         let kind = this.read_scalar(kind_op)?.not_undef()?;
-        if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")?
+        if kind == this.eval_libc("PTHREAD_MUTEX_DEFAULT")?
+            || kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")?
             || kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")?
             || kind == this.eval_libc("PTHREAD_MUTEX_RECURSIVE")?
         {
@@ -380,6 +378,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 Ok(0)
             } else {
                 // Trying to acquire the same mutex again.
+                if kind == this.eval_libc("PTHREAD_MUTEX_DEFAULT")? {
+                    // FIXME: Sometimes this is actually a Deadlock.
+                    // https://github.com/rust-lang/miri/issues/1419
+                    throw_ub_format!(
+                        "trying to acquire already locked PTHREAD_MUTEX_DEFAULT (see #1419)"
+                    );
+                }
                 if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? {
                     throw_machine_stop!(TerminationInfo::Deadlock);
                 } else if kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")? {
@@ -388,7 +393,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                     this.mutex_lock(id, active_thread);
                     Ok(0)
                 } else {
-                    throw_ub_format!("called pthread_mutex_lock on an unsupported type of mutex");
+                    throw_unsup_format!(
+                        "called pthread_mutex_lock on an unsupported type of mutex"
+                    );
                 }
             }
         } else {
@@ -410,15 +417,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             if owner_thread != active_thread {
                 this.eval_libc_i32("EBUSY")
             } else {
-                if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")?
+                if kind == this.eval_libc("PTHREAD_MUTEX_DEFAULT")?
+                    || kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")?
                     || kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")?
                 {
                     this.eval_libc_i32("EBUSY")
                 } else if kind == this.eval_libc("PTHREAD_MUTEX_RECURSIVE")? {
                     this.mutex_lock(id, active_thread);
                     Ok(0)
                 } else {
-                    throw_ub_format!(
+                    throw_unsup_format!(
                         "called pthread_mutex_trylock on an unsupported type of mutex"
                     );
                 }
@@ -435,21 +443,29 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         let kind = mutex_get_kind(this, mutex_op)?.not_undef()?;
         let id = mutex_get_or_create_id(this, mutex_op)?;
+        let active_thread = this.get_active_thread()?;
 
-        if let Some((old_owner_thread, _old_locked_count)) = this.mutex_unlock(id)? {
-            if old_owner_thread != this.get_active_thread()? {
-                throw_ub_format!("called pthread_mutex_unlock on a mutex owned by another thread");
-            }
+        if let Some(_old_locked_count) = this.mutex_unlock(id, active_thread)? {
+            // The mutex was locked by the current thread.
             Ok(0)
         } else {
-            if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? {
-                throw_ub_format!("unlocked a PTHREAD_MUTEX_NORMAL mutex that was not locked");
+            // The mutex was locked by another thread or not locked at all. See
+            // the “Unlock When Not Owner” column in
+            // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_unlock.html.
+            if kind == this.eval_libc("PTHREAD_MUTEX_DEFAULT")? {
+                throw_ub_format!(
+                    "unlocked a PTHREAD_MUTEX_DEFAULT mutex that was not locked by the current thread"
+                );
+            } else if kind == this.eval_libc("PTHREAD_MUTEX_NORMAL")? {
+                throw_ub_format!(
+                    "unlocked a PTHREAD_MUTEX_NORMAL mutex that was not locked by the current thread"
+                );
             } else if kind == this.eval_libc("PTHREAD_MUTEX_ERRORCHECK")? {
                 this.eval_libc_i32("EPERM")
             } else if kind == this.eval_libc("PTHREAD_MUTEX_RECURSIVE")? {
                 this.eval_libc_i32("EPERM")
             } else {
-                throw_ub_format!("called pthread_mutex_unlock on an unsupported type of mutex");
+                throw_unsup_format!("called pthread_mutex_unlock on an unsupported type of mutex");
             }
         }
     }
@@ -505,6 +521,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let active_thread = this.get_active_thread()?;
 
         if this.rwlock_is_locked(id) {
+            // Note: this will deadlock if the lock is already locked by this
+            // thread in any way.
+            //
+            // Relevant documentation:
+            // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_wrlock.html
+            // An in depth discussion on this topic:
+            // https://github.com/rust-lang/rust/issues/53127
+            //
+            // FIXME: Detect and report the deadlock proactively. (We currently
+            // 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, active_thread)?;
         } else {
             this.rwlock_writer_lock(id, active_thread);
@@ -719,19 +747,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let clock_id = cond_get_clock_id(this, cond_op)?.to_i32()?;
         let duration = {
             let tp = this.deref_operand(abstime_op)?;
-            let mut offset = Size::from_bytes(0);
-            let layout = this.libc_ty_layout("time_t")?;
-            let seconds_place = tp.offset(offset, MemPlaceMeta::None, layout, this)?;
+            let seconds_place = this.mplace_field(tp, 0)?;
             let seconds = this.read_scalar(seconds_place.into())?;
-            offset += layout.size;
-            let layout = this.libc_ty_layout("c_long")?;
-            let nanoseconds_place = tp.offset(offset, MemPlaceMeta::None, layout, this)?;
+            let nanoseconds_place = this.mplace_field(tp, 1)?;
             let nanoseconds = this.read_scalar(nanoseconds_place.into())?;
-            let (seconds, nanoseconds) = if this.pointer_size().bytes() == 8 {
-                (seconds.to_u64()?, nanoseconds.to_u64()?.try_into().unwrap())
-            } else {
-                (seconds.to_u32()?.into(), nanoseconds.to_u32()?)
-            };
+            let (seconds, nanoseconds) = (
+                seconds.to_machine_usize(this)?,
+                nanoseconds.to_machine_usize(this)?.try_into().unwrap(),
+            );
             Duration::new(seconds, nanoseconds)
         };
 
@@ -740,7 +763,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         } else if clock_id == this.eval_libc_i32("CLOCK_MONOTONIC")? {
             Time::Monotonic(this.machine.time_anchor.checked_add(duration).unwrap())
         } else {
-            throw_unsup_format!("Unsupported clock id.");
+            throw_unsup_format!("unsupported clock id: {}", clock_id);
         };
 
         // Register the timeout callback.
diff --git a/src/sync.rs b/src/sync.rs
@@ -30,10 +30,12 @@ macro_rules! declare_id {
                 // We use 0 as a sentinel value (see the comment above) and,
                 // therefore, need to shift by one when converting from an index
                 // into a vector.
-                $name(NonZeroU32::new(u32::try_from(idx).unwrap() + 1).unwrap())
+                let shifted_idx = u32::try_from(idx).unwrap().checked_add(1).unwrap();
+                $name(NonZeroU32::new(shifted_idx).unwrap())
             }
             fn index(self) -> usize {
                 // See the comment in `Self::new`.
+                // (This cannot underflow because self is NonZeroU32.)
                 usize::try_from(self.0.get() - 1).unwrap()
             }
         }
@@ -150,11 +152,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
     ///
     /// Note: It is the caller's responsibility to check that the thread that
     /// unlocked the lock actually is the same one, which owned it.
-    fn mutex_unlock(&mut self, id: MutexId) -> InterpResult<'tcx, Option<(ThreadId, usize)>> {
+    fn mutex_unlock(
+        &mut self,
+        id: MutexId,
+        expected_owner: ThreadId,
+    ) -> InterpResult<'tcx, Option<usize>> {
         let this = self.eval_context_mut();
         let mutex = &mut this.machine.threads.sync.mutexes[id];
         if let Some(current_owner) = mutex.owner {
             // Mutex is locked.
+            if current_owner != expected_owner {
+                // Only the owner can unlock the mutex.
+                return Ok(None);
+            }
             let old_lock_count = mutex.lock_count;
             mutex.lock_count = old_lock_count
                 .checked_sub(1)
@@ -168,7 +178,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                     this.unblock_thread(new_owner)?;
                 }
             }
-            Ok(Some((current_owner, old_lock_count)))
+            Ok(Some(old_lock_count))
         } else {
             // Mutex is unlocked.
             Ok(None)
diff --git a/src/thread.rs b/src/thread.rs
@@ -159,6 +159,7 @@ impl<'mir, 'tcx> Default for Thread<'mir, 'tcx> {
     }
 }
 
+/// A specific moment in time.
 #[derive(Debug)]
 pub enum Time {
     Monotonic(Instant),
@@ -449,9 +450,9 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
         //
         // Documentation:
         // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cond_timedwait.html#
-        if let Some(sleep_time) =
-            self.timeout_callbacks.values().map(|info| info.call_time.get_wait_time()).min()
-        {
+        let potential_sleep_time =
+            self.timeout_callbacks.values().map(|info| info.call_time.get_wait_time()).min();
+        if let Some(sleep_time) = potential_sleep_time {
             if sleep_time == Duration::new(0, 0) {
                 return Ok(SchedulingAction::ExecuteTimeoutCallback);
             }
@@ -479,9 +480,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
         // We have not found a thread to execute.
         if self.threads.iter().all(|thread| thread.state == ThreadState::Terminated) {
             unreachable!("all threads terminated without the main thread terminating?!");
-        } else if let Some(sleep_time) =
-            self.timeout_callbacks.values().map(|info| info.call_time.get_wait_time()).min()
-        {
+        } else if let Some(sleep_time) = potential_sleep_time {
             // All threads are currently blocked, but we have unexecuted
             // timeout_callbacks, which may unblock some of the threads. Hence,
             // sleep until the first callback.
diff --git a/tests/compile-fail/sync/libc_pthread_mutex_normal_deadlock.rs b/tests/compile-fail/sync/libc_pthread_mutex_normal_deadlock.rs
@@ -11,6 +11,8 @@ fn main() {
         let mut mutex: libc::pthread_mutex_t = std::mem::zeroed();
         assert_eq!(libc::pthread_mutex_init(&mut mutex as *mut _, &mutexattr as *const _), 0);
         assert_eq!(libc::pthread_mutex_lock(&mut mutex as *mut _), 0);
-        libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR deadlock
+        // FIXME: The error should be deadlock. See issue
+        // https://github.com/rust-lang/miri/issues/1419.
+        libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR Undefined Behavior
     }
 }
diff --git a/tests/compile-fail/sync/libc_pthread_mutex_wrong_owner.rs b/tests/compile-fail/sync/libc_pthread_mutex_wrong_owner.rs
@@ -24,7 +24,7 @@ fn main() {
 
         let lock_copy = lock.clone();
         thread::spawn(move || {
-            assert_eq!(libc::pthread_mutex_unlock(lock_copy.0.get() as *mut _), 0); //~ ERROR: Undefined Behavior: called pthread_mutex_unlock on a mutex owned by another thread
+            assert_eq!(libc::pthread_mutex_unlock(lock_copy.0.get() as *mut _), 0); //~ ERROR: Undefined Behavior: unlocked a PTHREAD_MUTEX_DEFAULT mutex that was not locked
         })
         .join()
         .unwrap();
diff --git a/tests/run-pass/concurrency/sync.rs b/tests/run-pass/concurrency/sync.rs
@@ -91,7 +91,8 @@ fn check_conditional_variables_timed_wait_timeout() {
     let now = Instant::now();
     let (_guard, timeout) = cvar.wait_timeout(guard, Duration::from_millis(100)).unwrap();
     assert!(timeout.timed_out());
-    assert!(now.elapsed().as_millis() >= 100);
+    let elapsed_time = now.elapsed().as_millis();
+    assert!(100 <= elapsed_time && elapsed_time <= 300);
 }
 
 /// Test that signaling a conditional variable when waiting with a timeout works
@@ -243,7 +244,7 @@ fn get_cached_val() -> usize {
 fn expensive_computation() -> usize {
     let mut i = 1;
     let mut c = 1;
-    while i < 10000 {
+    while i < 1000 {
         i *= c;
         c += 1;
     }
@@ -257,7 +258,7 @@ fn check_once() {
             thread::spawn(|| {
                 thread::yield_now();
                 let val = get_cached_val();
-                assert_eq!(val, 40320);
+                assert_eq!(val, 5040);
             })
         })
         .collect();

Original file line number	Diff line number	Diff line change
`@@ -11,6 +11,8 @@ fn main() {`
`11`	`11`	`let mut mutex: libc::pthread_mutex_t = std::mem::zeroed();`
`12`	`12`	`assert_eq!(libc::pthread_mutex_init(&mut mutex as mut _, &mutexattr as const _), 0);`
`13`	`13`	`assert_eq!(libc::pthread_mutex_lock(&mut mutex as *mut _), 0);`
`14`		`- libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR deadlock`
	`14`	`+ // FIXME: The error should be deadlock. See issue`
	`15`	`+ // https://github.com/rust-lang/miri/issues/1419.`
	`16`	`+ libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR Undefined Behavior`
`15`	`17`	`}`
`16`	`18`	`}`