Review comments

diff --git a/src/helpers.rs b/src/helpers.rs
index f8bf9598c14f710a8f6249fac0d8bb7de973a322..f5094b169f9ed0e0956568dbe0b9ffda03886091 100644 (file)
--- a/src/helpers.rs
+++ b/src/helpers.rs
@@ -42,9 +42,6 @@ fn try_resolve_did<'mir, 'tcx>(tcx: TyCtxt<'tcx>, path: &[&str]) -> Option<DefId
         })
 }
 
-/// This error indicates that the value in a `timespec` C struct was invalid.
-pub struct TimespecError;
-
 pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
     /// Gets an instance for a path.
     fn resolve_path(&self, path: &[&str]) -> ty::Instance<'tcx> {
@@ -517,14 +514,13 @@ fn write_scalar_at_offset(
         this.write_scalar(value, value_place.into())
     }
 
-    /// Parse a `timespec` struct and return it as a `std::time::Duration`. The outer `Result` is
-    /// for interpreter errors encountered while reading memory, and the inner `Result` indicates
-    /// whether the value in the `timespec` struct is invalid. Some libc functions will return
-    /// `EINVAL` if the struct's value is invalid.
+    /// Parse a `timespec` struct and return it as a `std::time::Duration`. It returns `None`
+    /// if the value in the `timespec` struct is invalid. Some libc functions will return
+    /// `EINVAL` in this case.
     fn read_timespec(
         &mut self,
         timespec_ptr_op: OpTy<'tcx, Tag>,
-    ) -> InterpResult<'tcx, Result<Duration, TimespecError>> {
+    ) -> InterpResult<'tcx, Option<Duration>> {
         let this = self.eval_context_mut();
         let tp = this.deref_operand(timespec_ptr_op)?;
         let seconds_place = this.mplace_field(tp, 0)?;
@@ -537,17 +533,20 @@ fn read_timespec(
         let seconds: u64 = if let Ok(s) = seconds.try_into() {
             s
         } else {
-            return Ok(Err(TimespecError));
+            // tv_sec must be non-negative.
+            return Ok(None);
         };
         let nanoseconds: u32 = if let Ok(ns) = nanoseconds.try_into() {
             if ns >= 1_000_000_000 {
-                return Ok(Err(TimespecError));
+                // tv_nsec must not be greater than 999,999,999.
+                return Ok(None);
             }
             ns
         } else {
-            return Ok(Err(TimespecError));
+            // tv_nsec must be non-negative.
+            return Ok(None);
         };
-        Ok(Ok(Duration::new(seconds, nanoseconds)))
+        Ok(Some(Duration::new(seconds, nanoseconds)))
     }
 }
 

diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs
index 94704bff32acfd888b8a7027d6e462f21571058f..6918fb7fd7eca0316d0e3fc98afcbec56710a03a 100644 (file)
--- a/src/shims/posix/sync.rs
+++ b/src/shims/posix/sync.rs
@@ -1,7 +1,6 @@
 use std::time::SystemTime;
 
 use crate::*;
-use helpers::TimespecError;
 use stacked_borrows::Tag;
 use thread::Time;
 
@@ -700,8 +699,8 @@ fn pthread_cond_timedwait(
         // Extract the timeout.
         let clock_id = cond_get_clock_id(this, cond_op)?.to_i32()?;
         let duration = match this.read_timespec(abstime_op)? {
-            Ok(duration) => duration,
-            Err(TimespecError) => {
+            Some(duration) => duration,
+            None => {
                 let einval = this.eval_libc("EINVAL")?;
                 this.write_scalar(einval, dest)?;
                 return Ok(());

diff --git a/src/shims/time.rs b/src/shims/time.rs
index 32c1ce888ed0e922e3d4eee683525e4c3b8a78ad..9d6d6ed38daab68ff5e272cd4a555e8dcfcca459 100644 (file)
--- a/src/shims/time.rs
+++ b/src/shims/time.rs
@@ -3,7 +3,7 @@
 
 use crate::stacked_borrows::Tag;
 use crate::*;
-use helpers::{immty_from_int_checked, immty_from_uint_checked, TimespecError};
+use helpers::{immty_from_int_checked, immty_from_uint_checked};
 use thread::Time;
 
 /// Returns the time elapsed between the provided time and the unix epoch as a `Duration`.
@@ -191,14 +191,14 @@ fn nanosleep(
         this.check_no_isolation("nanosleep")?;
 
         let duration = match this.read_timespec(req_op)? {
-            Ok(duration) => duration,
-            Err(TimespecError) => {
+            Some(duration) => duration,
+            None => {
                 let einval = this.eval_libc("EINVAL")?;
                 this.set_last_error(einval)?;
                 return Ok(-1);
             }
         };
-        let timeout_time = Time::RealTime(SystemTime::now().checked_add(duration).unwrap());
+        let timeout_time = Time::Monotonic(Instant::now().checked_add(duration).unwrap());
 
         let active_thread = this.get_active_thread();
         this.block_thread(active_thread);

diff --git a/tests/run-pass/concurrency/libc_pthread_cond.rs b/tests/run-pass/concurrency/libc_pthread_cond.rs
index e1ca63f9ca4370203015778641c310d311dae90a..d4e52bb3a97b0bb538479acb926b8ac72ad9a571 100644 (file)
--- a/tests/run-pass/concurrency/libc_pthread_cond.rs
+++ b/tests/run-pass/concurrency/libc_pthread_cond.rs
@@ -38,6 +38,8 @@ fn test_timed_wait_timeout(clock_id: i32) {
         let elapsed_time = current_time.elapsed().as_millis();
         assert!(900 <= elapsed_time && elapsed_time <= 1300);
 
+        // Test that invalid nanosecond values (above 10^9 or negative) are rejected with the
+        // correct error code.
         let invalid_timeout_1 = libc::timespec { tv_sec: now.tv_sec + 1, tv_nsec: 1_000_000_000 };
         assert_eq!(
             libc::pthread_cond_timedwait(
@@ -56,6 +58,16 @@ fn test_timed_wait_timeout(clock_id: i32) {
             ),
             libc::EINVAL
         );
+        // Test that invalid second values (negative) are rejected with the correct error code.
+        let invalid_timeout_3 = libc::timespec { tv_sec: -1, tv_nsec: 0 };
+        assert_eq!(
+            libc::pthread_cond_timedwait(
+                &mut cond as *mut _,
+                &mut mutex as *mut _,
+                &invalid_timeout_3
+            ),
+            libc::EINVAL
+        );
 
         assert_eq!(libc::pthread_mutex_unlock(&mut mutex as *mut _), 0);
         assert_eq!(libc::pthread_mutex_destroy(&mut mutex as *mut _), 0);

diff --git a/tests/run-pass/time.rs b/tests/run-pass/time.rs
index ae287234d177465fef09eef4fef7fe389d5451fc..e76c8573c5162a79d39891eb05c08a771fe67430 100644 (file)
--- a/tests/run-pass/time.rs
+++ b/tests/run-pass/time.rs
@@ -8,6 +8,7 @@ fn duration_sanity(diff: Duration) {
     assert!(diff.as_millis() < 500);
 }
 
+// Thus far, only `libc::nanosleep`, is implemented, not `c::Sleep`.
 #[cfg(unix)]
 fn test_sleep() {
     let before = Instant::now();