Auto merge of rust-lang#3776 - oli-obk:duplicator, r=oli-obk

Use Scalar consistently in foreign item emulation

Step 0 of rust-lang#3772

This just makes the code consistent. While we could also go for consistency the other way, that would only allow us to use integers in some cases where we use `Scalar` right now, because `Scalar` can also hold pointers where applicable.

There's also no danger in messing up (even though we do lose some compile-time checks), as `Scalar` knows the size of the integer stored within, so it will check that against the destination when it is written. In fact, this makes the checks much stronger compared with `write_int`, which just checks that the integer fits into the destination size.

Author: bors

src/tools/miri/src/shims/time.rs

@@ -93,7 +93,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         Ok(Scalar::from_i32(0))
     }
 
-    fn gettimeofday(&mut self, tv_op: &OpTy<'tcx>, tz_op: &OpTy<'tcx>) -> InterpResult<'tcx, i32> {
+    fn gettimeofday(
+        &mut self,
+        tv_op: &OpTy<'tcx>,
+        tz_op: &OpTy<'tcx>,
+    ) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
         this.assert_target_os_is_unix("gettimeofday");
@@ -106,7 +110,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         if !this.ptr_is_null(tz)? {
             let einval = this.eval_libc("EINVAL");
             this.set_last_error(einval)?;
-            return Ok(-1);
+            return Ok(Scalar::from_i32(-1));
         }
 
         let duration = system_time_to_duration(&SystemTime::now())?;
@@ -115,7 +119,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         this.write_int_fields(&[tv_sec.into(), tv_usec.into()], &tv)?;
 
-        Ok(0)
+        Ok(Scalar::from_i32(0))
     }
 
     // The localtime() function shall convert the time in seconds since the Epoch pointed to by
@@ -308,7 +312,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         &mut self,
         req_op: &OpTy<'tcx>,
         _rem: &OpTy<'tcx>, // Signal handlers are not supported, so rem will never be written to.
-    ) -> InterpResult<'tcx, i32> {
+    ) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
         this.assert_target_os_is_unix("nanosleep");
@@ -320,7 +324,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             None => {
                 let einval = this.eval_libc("EINVAL");
                 this.set_last_error(einval)?;
-                return Ok(-1);
+                return Ok(Scalar::from_i32(-1));
             }
         };
 
@@ -333,7 +337,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 @timeout = |_this| { Ok(()) }
             ),
         );
-        Ok(0)
+        Ok(Scalar::from_i32(0))
     }
 
     #[allow(non_snake_case)]

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

@@ -148,7 +148,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         Ok(var_ptr.unwrap_or_else(Pointer::null))
     }
 
-    fn setenv(&mut self, name_op: &OpTy<'tcx>, value_op: &OpTy<'tcx>) -> InterpResult<'tcx, i32> {
+    fn setenv(
+        &mut self,
+        name_op: &OpTy<'tcx>,
+        value_op: &OpTy<'tcx>,
+    ) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
         this.assert_target_os_is_unix("setenv");
 
@@ -169,16 +173,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 this.deallocate_ptr(var, None, MiriMemoryKind::Runtime.into())?;
             }
             this.update_environ()?;
-            Ok(0) // return zero on success
+            Ok(Scalar::from_i32(0)) // return zero on success
         } else {
             // name argument is a null pointer, points to an empty string, or points to a string containing an '=' character.
             let einval = this.eval_libc("EINVAL");
             this.set_last_error(einval)?;
-            Ok(-1)
+            Ok(Scalar::from_i32(-1))
         }
     }
 
-    fn unsetenv(&mut self, name_op: &OpTy<'tcx>) -> InterpResult<'tcx, i32> {
+    fn unsetenv(&mut self, name_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
         this.assert_target_os_is_unix("unsetenv");
 
@@ -195,12 +199,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 this.deallocate_ptr(var, None, MiriMemoryKind::Runtime.into())?;
             }
             this.update_environ()?;
-            Ok(0)
+            Ok(Scalar::from_i32(0))
         } else {
             // name argument is a null pointer, points to an empty string, or points to a string containing an '=' character.
             let einval = this.eval_libc("EINVAL");
             this.set_last_error(einval)?;
-            Ok(-1)
+            Ok(Scalar::from_i32(-1))
         }
     }
 
@@ -232,7 +236,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         Ok(Pointer::null())
     }
 
-    fn chdir(&mut self, path_op: &OpTy<'tcx>) -> InterpResult<'tcx, i32> {
+    fn chdir(&mut self, path_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
         this.assert_target_os_is_unix("chdir");
 
@@ -242,16 +246,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             this.reject_in_isolation("`chdir`", reject_with)?;
             this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?;
 
-            return Ok(-1);
+            return Ok(Scalar::from_i32(-1));
         }
 
-        match env::set_current_dir(path) {
-            Ok(()) => Ok(0),
-            Err(e) => {
-                this.set_last_error_from_io_error(e)?;
-                Ok(-1)
-            }
-        }
+        let result = env::set_current_dir(path).map(|()| 0);
+        Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
     }
 
     /// Updates the `environ` static.
@@ -270,18 +269,18 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         Ok(())
     }
 
-    fn getpid(&mut self) -> InterpResult<'tcx, i32> {
+    fn getpid(&mut self) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
         this.assert_target_os_is_unix("getpid");
 
         // The reason we need to do this wacky of a conversion is because
         // `libc::getpid` returns an i32, however, `std::process::id()` return an u32.
         // So we un-do the conversion that stdlib does and turn it back into an i32.
-        #[allow(clippy::cast_possible_wrap)]
-        Ok(this.get_pid() as i32)
+        // In `Scalar` representation, these are the same, so we don't need to anything else.
+        Ok(Scalar::from_u32(this.get_pid()))
     }
 
-    fn linux_gettid(&mut self) -> InterpResult<'tcx, i32> {
+    fn linux_gettid(&mut self) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_ref();
         this.assert_target_os("linux", "gettid");
 
@@ -290,7 +289,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         // Compute a TID for this thread, ensuring that the main thread has PID == TID.
         let tid = this.get_pid().strict_add(index);
 
-        #[allow(clippy::cast_possible_wrap)]
-        Ok(tid as i32)
+        Ok(Scalar::from_u32(tid))
     }
 }

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

@@ -310,20 +310,20 @@ impl FdTable {
 
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
 pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
-    fn dup(&mut self, old_fd: i32) -> InterpResult<'tcx, i32> {
+    fn dup(&mut self, old_fd: i32) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
         let Some(dup_fd) = this.machine.fds.dup(old_fd) else {
-            return this.fd_not_found();
+            return Ok(Scalar::from_i32(this.fd_not_found()?));
         };
-        Ok(this.machine.fds.insert_fd_with_min_fd(dup_fd, 0))
+        Ok(Scalar::from_i32(this.machine.fds.insert_fd_with_min_fd(dup_fd, 0)))
     }
 
-    fn dup2(&mut self, old_fd: i32, new_fd: i32) -> InterpResult<'tcx, i32> {
+    fn dup2(&mut self, old_fd: i32, new_fd: i32) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
         let Some(dup_fd) = this.machine.fds.dup(old_fd) else {
-            return this.fd_not_found();
+            return Ok(Scalar::from_i32(this.fd_not_found()?));
         };
         if new_fd != old_fd {
             // Close new_fd if it is previously opened.
@@ -333,7 +333,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 file_description.close(this.machine.communicate())?.ok();
             }
         }
-        Ok(new_fd)
+        Ok(Scalar::from_i32(new_fd))
     }
 
     fn flock(&mut self, fd: i32, op: i32) -> InterpResult<'tcx, Scalar> {
@@ -370,7 +370,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
     }
 
-    fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, i32> {
+    fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
         if args.len() < 2 {
@@ -388,11 +388,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             // `FD_CLOEXEC` value without checking if the flag is set for the file because `std`
             // always sets this flag when opening a file. However we still need to check that the
             // file itself is open.
-            if this.machine.fds.is_fd(fd) {
-                Ok(this.eval_libc_i32("FD_CLOEXEC"))
+            Ok(Scalar::from_i32(if this.machine.fds.is_fd(fd) {
+                this.eval_libc_i32("FD_CLOEXEC")
             } else {
-                this.fd_not_found()
-            }
+                this.fd_not_found()?
+            }))
         } else if cmd == this.eval_libc_i32("F_DUPFD")
             || cmd == this.eval_libc_i32("F_DUPFD_CLOEXEC")
         {
@@ -409,15 +409,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             let start = this.read_scalar(&args[2])?.to_i32()?;
 
             match this.machine.fds.dup(fd) {
-                Some(dup_fd) => Ok(this.machine.fds.insert_fd_with_min_fd(dup_fd, start)),
-                None => this.fd_not_found(),
+                Some(dup_fd) =>
+                    Ok(Scalar::from_i32(this.machine.fds.insert_fd_with_min_fd(dup_fd, start))),
+                None => Ok(Scalar::from_i32(this.fd_not_found()?)),
             }
         } else if this.tcx.sess.target.os == "macos" && cmd == this.eval_libc_i32("F_FULLFSYNC") {
             // Reject if isolation is enabled.
             if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
                 this.reject_in_isolation("`fcntl`", reject_with)?;
                 this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?;
-                return Ok(-1);
+                return Ok(Scalar::from_i32(-1));
             }
 
             this.ffullsync_fd(fd)
@@ -462,7 +463,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         buf: Pointer,
         count: u64,
         offset: Option<i128>,
-    ) -> InterpResult<'tcx, i64> {
+    ) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
         // Isolation check is done via `FileDescriptor` trait.
@@ -482,7 +483,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         // We temporarily dup the FD to be able to retain mutable access to `this`.
         let Some(fd) = this.machine.fds.dup(fd) else {
             trace!("read: FD not found");
-            return this.fd_not_found();
+            return Ok(Scalar::from_target_isize(this.fd_not_found()?, this));
         };
 
         trace!("read: FD mapped to {fd:?}");
@@ -496,7 +497,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 let Ok(offset) = u64::try_from(offset) else {
                     let einval = this.eval_libc("EINVAL");
                     this.set_last_error(einval)?;
-                    return Ok(-1);
+                    return Ok(Scalar::from_target_isize(-1, this));
                 };
                 fd.borrow_mut().pread(communicate, &mut bytes, offset, this)
             }
@@ -513,11 +514,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                     buf,
                     bytes[..usize::try_from(read_bytes).unwrap()].iter().copied(),
                 )?;
-                Ok(read_bytes)
+                Ok(Scalar::from_target_isize(read_bytes, this))
             }
             Err(e) => {
                 this.set_last_error_from_io_error(e)?;
-                Ok(-1)
+                Ok(Scalar::from_target_isize(-1, this))
             }
         }
     }
@@ -528,7 +529,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         buf: Pointer,
         count: u64,
         offset: Option<i128>,
-    ) -> InterpResult<'tcx, i64> {
+    ) -> InterpResult<'tcx, Scalar> {
         let this = self.eval_context_mut();
 
         // Isolation check is done via `FileDescriptor` trait.
@@ -546,7 +547,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let bytes = this.read_bytes_ptr_strip_provenance(buf, Size::from_bytes(count))?.to_owned();
         // We temporarily dup the FD to be able to retain mutable access to `this`.
         let Some(fd) = this.machine.fds.dup(fd) else {
-            return this.fd_not_found();
+            return Ok(Scalar::from_target_isize(this.fd_not_found()?, this));
         };
 
         let result = match offset {
@@ -555,15 +556,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 let Ok(offset) = u64::try_from(offset) else {
                     let einval = this.eval_libc("EINVAL");
                     this.set_last_error(einval)?;
-                    return Ok(-1);
+                    return Ok(Scalar::from_target_isize(-1, this));
                 };
                 fd.borrow_mut().pwrite(communicate, &bytes, offset, this)
             }
         };
         drop(fd);
 
         let result = result?.map(|c| i64::try_from(c).unwrap());
-        this.try_unwrap_io_result(result)
+        Ok(Scalar::from_target_isize(this.try_unwrap_io_result(result)?, this))
     }
 }