Auto merge of #3923 - tiif:refactor, r=RalfJung

Refactor ``return_read_bytes_and_count`` and ``return_written_byte_count_or_error``

Fixes #3904

This PR
- separate the error logic from ``return_read_bytes_and_count`` and ``return_written_byte_count_or_error`` into a helper function ``set_last_error_and_return``.

Author: bors

src/helpers.rs

@@ -2,7 +2,7 @@ use std::collections::BTreeSet;
 use std::num::NonZero;
 use std::sync::Mutex;
 use std::time::Duration;
-use std::{cmp, iter};
+use std::{cmp, io, iter};
 
 use rand::RngCore;
 use rustc_apfloat::Float;
@@ -839,6 +839,17 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         self.set_last_error(self.io_error_to_errnum(err)?)
     }
 
+    /// Sets the last OS error using a `std::io::ErrorKind` and writes -1 to dest place.
+    fn set_last_error_and_return(
+        &mut self,
+        err: impl Into<io::Error>,
+        dest: &MPlaceTy<'tcx>,
+    ) -> InterpResult<'tcx> {
+        self.set_last_error(self.io_error_to_errnum(err.into())?)?;
+        self.write_int(-1, dest)?;
+        Ok(())
+    }
+
     /// Helper function that consumes an `std::io::Result<T>` and returns an
     /// `InterpResult<'tcx,T>::Ok` instead. In case the result is an error, this function returns
     /// `Ok(-1)` and sets the last OS error accordingly.

src/shims/unix/fd.rs

@@ -150,7 +150,10 @@ impl FileDescription for io::Stdin {
             helpers::isolation_abort_error("`read` from stdin")?;
         }
         let result = Read::read(&mut { self }, &mut bytes);
-        ecx.return_read_bytes_and_count(ptr, &bytes, result, dest)
+        match result {
+            Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest),
+            Err(e) => ecx.set_last_error_and_return(e, dest),
+        }
     }
 
     fn is_tty(&self, communicate_allowed: bool) -> bool {
@@ -181,7 +184,10 @@ impl FileDescription for io::Stdout {
         // the host -- there is no good in adding extra buffering
         // here.
         io::stdout().flush().unwrap();
-        ecx.return_written_byte_count_or_error(result, dest)
+        match result {
+            Ok(write_size) => ecx.return_write_success(write_size, dest),
+            Err(e) => ecx.set_last_error_and_return(e, dest),
+        }
     }
 
     fn is_tty(&self, communicate_allowed: bool) -> bool {
@@ -207,7 +213,10 @@ impl FileDescription for io::Stderr {
         // We allow writing to stderr even with isolation enabled.
         // No need to flush, stderr is not buffered.
         let result = Write::write(&mut { self }, bytes);
-        ecx.return_written_byte_count_or_error(result, dest)
+        match result {
+            Ok(write_size) => ecx.return_write_success(write_size, dest),
+            Err(e) => ecx.set_last_error_and_return(e, dest),
+        }
     }
 
     fn is_tty(&self, communicate_allowed: bool) -> bool {
@@ -234,8 +243,7 @@ impl FileDescription for NullOutput {
         ecx: &mut MiriInterpCx<'tcx>,
     ) -> InterpResult<'tcx> {
         // We just don't write anything, but report to the user that we did.
-        let result = Ok(len);
-        ecx.return_written_byte_count_or_error(result, dest)
+        ecx.return_write_success(len, dest)
     }
 }
 
@@ -655,46 +663,39 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     }
 
     /// Helper to implement `FileDescription::read`:
-    /// `result` should be the return value of some underlying `read` call that used `bytes` as its output buffer.
+    /// This is only used when `read` is successful.
+    /// `actual_read_size` should be the return value of some underlying `read` call that used
+    /// `bytes` as its output buffer.
     /// The length of `bytes` must not exceed either the host's or the target's `isize`.
-    /// If `Result` indicates success, `bytes` is written to `buf` and the size is written to `dest`.
-    /// Otherwise, `-1` is written to `dest` and the last libc error is set appropriately.
-    fn return_read_bytes_and_count(
+    /// `bytes` is written to `buf` and the size is written to `dest`.
+    fn return_read_success(
         &mut self,
         buf: Pointer,
         bytes: &[u8],
-        result: io::Result<usize>,
+        actual_read_size: usize,
         dest: &MPlaceTy<'tcx>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        match result {
-            Ok(read_bytes) => {
-                // If reading to `bytes` did not fail, we write those bytes to the buffer.
-                // Crucially, if fewer than `bytes.len()` bytes were read, only write
-                // that much into the output buffer!
-                this.write_bytes_ptr(buf, bytes[..read_bytes].iter().copied())?;
-                // The actual read size is always less than what got originally requested so this cannot fail.
-                this.write_int(u64::try_from(read_bytes).unwrap(), dest)?;
-                Ok(())
-            }
-            Err(e) => {
-                this.set_last_error_from_io_error(e)?;
-                this.write_int(-1, dest)?;
-                Ok(())
-            }
-        }
+        // If reading to `bytes` did not fail, we write those bytes to the buffer.
+        // Crucially, if fewer than `bytes.len()` bytes were read, only write
+        // that much into the output buffer!
+        this.write_bytes_ptr(buf, bytes[..actual_read_size].iter().copied())?;
+
+        // The actual read size is always less than what got originally requested so this cannot fail.
+        this.write_int(u64::try_from(actual_read_size).unwrap(), dest)?;
+        Ok(())
     }
 
-    /// This function writes the number of written bytes (given in `result`) to `dest`, or sets the
-    /// last libc error and writes -1 to dest.
-    fn return_written_byte_count_or_error(
+    /// Helper to implement `FileDescription::write`:
+    /// This function is only used when `write` is successful, and writes `actual_write_size` to `dest`
+    fn return_write_success(
         &mut self,
-        result: io::Result<usize>,
+        actual_write_size: usize,
         dest: &MPlaceTy<'tcx>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        let result = this.try_unwrap_io_result(result.map(|c| i64::try_from(c).unwrap()))?;
-        this.write_int(result, dest)?;
+        // The actual write size is always less than what got originally requested so this cannot fail.
+        this.write_int(u64::try_from(actual_write_size).unwrap(), dest)?;
         Ok(())
     }
 }

src/shims/unix/fs.rs

@@ -41,7 +41,10 @@ impl FileDescription for FileHandle {
         assert!(communicate_allowed, "isolation should have prevented even opening a file");
         let mut bytes = vec![0; len];
         let result = (&mut &self.file).read(&mut bytes);
-        ecx.return_read_bytes_and_count(ptr, &bytes, result, dest)
+        match result {
+            Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest),
+            Err(e) => ecx.set_last_error_and_return(e, dest),
+        }
     }
 
     fn write<'tcx>(
@@ -56,7 +59,10 @@ impl FileDescription for FileHandle {
         assert!(communicate_allowed, "isolation should have prevented even opening a file");
         let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?;
         let result = (&mut &self.file).write(bytes);
-        ecx.return_written_byte_count_or_error(result, dest)
+        match result {
+            Ok(write_size) => ecx.return_write_success(write_size, dest),
+            Err(e) => ecx.set_last_error_and_return(e, dest),
+        }
     }
 
     fn pread<'tcx>(
@@ -84,7 +90,10 @@ impl FileDescription for FileHandle {
             res
         };
         let result = f();
-        ecx.return_read_bytes_and_count(ptr, &bytes, result, dest)
+        match result {
+            Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest),
+            Err(e) => ecx.set_last_error_and_return(e, dest),
+        }
     }
 
     fn pwrite<'tcx>(
@@ -112,7 +121,10 @@ impl FileDescription for FileHandle {
             res
         };
         let result = f();
-        ecx.return_written_byte_count_or_error(result, dest)
+        match result {
+            Ok(write_size) => ecx.return_write_success(write_size, dest),
+            Err(e) => ecx.set_last_error_and_return(e, dest),
+        }
     }
 
     fn seek<'tcx>(

src/shims/unix/linux/eventfd.rs

@@ -1,7 +1,7 @@
 //! Linux `eventfd` implementation.
 use std::cell::{Cell, RefCell};
 use std::io;
-use std::io::{Error, ErrorKind};
+use std::io::ErrorKind;
 
 use crate::concurrency::VClock;
 use crate::shims::unix::fd::FileDescriptionRef;
@@ -66,9 +66,7 @@ impl FileDescription for Event {
         let ty = ecx.machine.layouts.u64;
         // Check the size of slice, and return error only if the size of the slice < 8.
         if len < ty.size.bytes_usize() {
-            ecx.set_last_error_from_io_error(Error::from(ErrorKind::InvalidInput))?;
-            ecx.write_int(-1, dest)?;
-            return Ok(());
+            return ecx.set_last_error_and_return(ErrorKind::InvalidInput, dest);
         }
 
         // eventfd read at the size of u64.
@@ -78,9 +76,7 @@ impl FileDescription for Event {
         let counter = self.counter.get();
         if counter == 0 {
             if self.is_nonblock {
-                ecx.set_last_error_from_io_error(Error::from(ErrorKind::WouldBlock))?;
-                ecx.write_int(-1, dest)?;
-                return Ok(());
+                return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest);
             }
 
             throw_unsup_format!("eventfd: blocking is unsupported");
@@ -128,8 +124,7 @@ impl FileDescription for Event {
         let ty = ecx.machine.layouts.u64;
         // Check the size of slice, and return error only if the size of the slice < 8.
         if len < ty.layout.size.bytes_usize() {
-            let result = Err(Error::from(ErrorKind::InvalidInput));
-            return ecx.return_written_byte_count_or_error(result, dest);
+            return ecx.set_last_error_and_return(ErrorKind::InvalidInput, dest);
         }
 
         // Read the user supplied value from the pointer.
@@ -138,8 +133,7 @@ impl FileDescription for Event {
 
         // u64::MAX as input is invalid because the maximum value of counter is u64::MAX - 1.
         if num == u64::MAX {
-            let result = Err(Error::from(ErrorKind::InvalidInput));
-            return ecx.return_written_byte_count_or_error(result, dest);
+            return ecx.set_last_error_and_return(ErrorKind::InvalidInput, dest);
         }
         // If the addition does not let the counter to exceed the maximum value, update the counter.
         // Else, block.
@@ -153,8 +147,7 @@ impl FileDescription for Event {
             }
             None | Some(u64::MAX) =>
                 if self.is_nonblock {
-                    let result = Err(Error::from(ErrorKind::WouldBlock));
-                    return ecx.return_written_byte_count_or_error(result, dest);
+                    return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest);
                 } else {
                     throw_unsup_format!("eventfd: blocking is unsupported");
                 },

src/shims/unix/unnamed_socket.rs

@@ -5,7 +5,7 @@
 use std::cell::{Cell, OnceCell, RefCell};
 use std::collections::VecDeque;
 use std::io;
-use std::io::{Error, ErrorKind, Read};
+use std::io::{ErrorKind, Read};
 
 use rustc_target::abi::Size;
 
@@ -138,8 +138,7 @@ impl FileDescription for AnonSocket {
 
         // Always succeed on read size 0.
         if len == 0 {
-            let result = Ok(0);
-            return ecx.return_read_bytes_and_count(ptr, &bytes, result, dest);
+            return ecx.return_read_success(ptr, &bytes, 0, dest);
         }
 
         let Some(readbuf) = &self.readbuf else {
@@ -152,17 +151,15 @@ impl FileDescription for AnonSocket {
             if self.peer_fd().upgrade().is_none() {
                 // Socketpair with no peer and empty buffer.
                 // 0 bytes successfully read indicates end-of-file.
-                let result = Ok(0);
-                return ecx.return_read_bytes_and_count(ptr, &bytes, result, dest);
+                return ecx.return_read_success(ptr, &bytes, 0, dest);
             } else {
                 if self.is_nonblock {
                     // Non-blocking socketpair with writer and empty buffer.
                     // https://linux.die.net/man/2/read
                     // EAGAIN or EWOULDBLOCK can be returned for socket,
                     // POSIX.1-2001 allows either error to be returned for this case.
                     // Since there is no ErrorKind for EAGAIN, WouldBlock is used.
-                    let result = Err(Error::from(ErrorKind::WouldBlock));
-                    return ecx.return_read_bytes_and_count(ptr, &bytes, result, dest);
+                    return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest);
                 } else {
                     // Blocking socketpair with writer and empty buffer.
                     // FIXME: blocking is currently not supported
@@ -194,8 +191,7 @@ impl FileDescription for AnonSocket {
             ecx.check_and_update_readiness(&peer_fd)?;
         }
 
-        let result = Ok(actual_read_size);
-        ecx.return_read_bytes_and_count(ptr, &bytes, result, dest)
+        ecx.return_read_success(ptr, &bytes, actual_read_size, dest)
     }
 
     fn write<'tcx>(
@@ -210,16 +206,14 @@ impl FileDescription for AnonSocket {
         // Always succeed on write size 0.
         // ("If count is zero and fd refers to a file other than a regular file, the results are not specified.")
         if len == 0 {
-            let result = Ok(0);
-            return ecx.return_written_byte_count_or_error(result, dest);
+            return ecx.return_write_success(0, dest);
         }
 
         // We are writing to our peer's readbuf.
         let Some(peer_fd) = self.peer_fd().upgrade() else {
             // If the upgrade from Weak to Rc fails, it indicates that all read ends have been
             // closed.
-            let result = Err(Error::from(ErrorKind::BrokenPipe));
-            return ecx.return_written_byte_count_or_error(result, dest);
+            return ecx.set_last_error_and_return(ErrorKind::BrokenPipe, dest);
         };
 
         let Some(writebuf) = &peer_fd.downcast::<AnonSocket>().unwrap().readbuf else {
@@ -233,8 +227,7 @@ impl FileDescription for AnonSocket {
         if available_space == 0 {
             if self.is_nonblock {
                 // Non-blocking socketpair with a full buffer.
-                let result = Err(Error::from(ErrorKind::WouldBlock));
-                return ecx.return_written_byte_count_or_error(result, dest);
+                return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest);
             } else {
                 // Blocking socketpair with a full buffer.
                 throw_unsup_format!("socketpair write: blocking isn't supported yet");
@@ -256,8 +249,7 @@ impl FileDescription for AnonSocket {
         // The kernel does this even if the fd was already readable before, so we follow suit.
         ecx.check_and_update_readiness(&peer_fd)?;
 
-        let result = Ok(actual_write_size);
-        ecx.return_written_byte_count_or_error(result, dest)
+        ecx.return_write_success(actual_write_size, dest)
     }
 }