From df6c27db4e0f6060339ea1289ce87b8cbb3aa729 Mon Sep 17 00:00:00 2001 From: tiif Date: Tue, 1 Oct 2024 03:00:14 +0800 Subject: [PATCH 01/16] Refactor return_read_bytes_and_count and return_written_byte_count_or_error --- src/tools/miri/src/helpers.rs | 13 +++- src/tools/miri/src/shims/unix/fd.rs | 65 ++++++++++--------- src/tools/miri/src/shims/unix/fs.rs | 20 ++++-- .../miri/src/shims/unix/linux/eventfd.rs | 19 ++---- .../miri/src/shims/unix/unnamed_socket.rs | 26 +++----- 5 files changed, 76 insertions(+), 67 deletions(-) diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index 0cd6ba69aac9d..5324e4bec5b22 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/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, + 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` 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. diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 6bd753d0d6bee..f64d0ed1467bc 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/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, + 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, + 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(()) } } diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index 92514b67beab1..2fa1729ad9c8a 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/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>( diff --git a/src/tools/miri/src/shims/unix/linux/eventfd.rs b/src/tools/miri/src/shims/unix/linux/eventfd.rs index ab7652ca721ca..2e485d2f4e165 100644 --- a/src/tools/miri/src/shims/unix/linux/eventfd.rs +++ b/src/tools/miri/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"); }, diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index d1bfa563387dd..56327d1461aed 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/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,8 +151,7 @@ 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. @@ -161,8 +159,7 @@ impl FileDescription for AnonSocket { // 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::().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) } } From 88c74529c1210bdf06e30b70aa51721218512090 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 1 Oct 2024 08:41:54 +0200 Subject: [PATCH 02/16] move io error handling helpers to their own file --- src/tools/miri/src/helpers.rs | 185 +------------------------- src/tools/miri/src/lib.rs | 1 + src/tools/miri/src/shims/io_error.rs | 190 +++++++++++++++++++++++++++ src/tools/miri/src/shims/mod.rs | 1 + 4 files changed, 193 insertions(+), 184 deletions(-) create mode 100644 src/tools/miri/src/shims/io_error.rs diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index 5324e4bec5b22..da0581ea86254 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/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, io, iter}; +use std::{cmp, iter}; use rand::RngCore; use rustc_apfloat::Float; @@ -31,65 +31,6 @@ pub enum AccessKind { Write, } -// This mapping should match `decode_error_kind` in -// . -const UNIX_IO_ERROR_TABLE: &[(&str, std::io::ErrorKind)] = { - use std::io::ErrorKind::*; - &[ - ("E2BIG", ArgumentListTooLong), - ("EADDRINUSE", AddrInUse), - ("EADDRNOTAVAIL", AddrNotAvailable), - ("EBUSY", ResourceBusy), - ("ECONNABORTED", ConnectionAborted), - ("ECONNREFUSED", ConnectionRefused), - ("ECONNRESET", ConnectionReset), - ("EDEADLK", Deadlock), - ("EDQUOT", FilesystemQuotaExceeded), - ("EEXIST", AlreadyExists), - ("EFBIG", FileTooLarge), - ("EHOSTUNREACH", HostUnreachable), - ("EINTR", Interrupted), - ("EINVAL", InvalidInput), - ("EISDIR", IsADirectory), - ("ELOOP", FilesystemLoop), - ("ENOENT", NotFound), - ("ENOMEM", OutOfMemory), - ("ENOSPC", StorageFull), - ("ENOSYS", Unsupported), - ("EMLINK", TooManyLinks), - ("ENAMETOOLONG", InvalidFilename), - ("ENETDOWN", NetworkDown), - ("ENETUNREACH", NetworkUnreachable), - ("ENOTCONN", NotConnected), - ("ENOTDIR", NotADirectory), - ("ENOTEMPTY", DirectoryNotEmpty), - ("EPIPE", BrokenPipe), - ("EROFS", ReadOnlyFilesystem), - ("ESPIPE", NotSeekable), - ("ESTALE", StaleNetworkFileHandle), - ("ETIMEDOUT", TimedOut), - ("ETXTBSY", ExecutableFileBusy), - ("EXDEV", CrossesDevices), - // The following have two valid options. We have both for the forwards mapping; only the - // first one will be used for the backwards mapping. - ("EPERM", PermissionDenied), - ("EACCES", PermissionDenied), - ("EWOULDBLOCK", WouldBlock), - ("EAGAIN", WouldBlock), - ] -}; -// This mapping should match `decode_error_kind` in -// . -const WINDOWS_IO_ERROR_TABLE: &[(&str, std::io::ErrorKind)] = { - use std::io::ErrorKind::*; - // FIXME: this is still incomplete. - &[ - ("ERROR_ACCESS_DENIED", PermissionDenied), - ("ERROR_FILE_NOT_FOUND", NotFound), - ("ERROR_INVALID_PARAMETER", InvalidInput), - ] -}; - /// Gets an instance for a path. /// /// A `None` namespace indicates we are looking for a module. @@ -745,130 +686,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { self.eval_context_ref().tcx.sess.target.families.iter().any(|f| f == "unix") } - /// Get last error variable as a place, lazily allocating thread-local storage for it if - /// necessary. - fn last_error_place(&mut self) -> InterpResult<'tcx, MPlaceTy<'tcx>> { - let this = self.eval_context_mut(); - if let Some(errno_place) = this.active_thread_ref().last_error.as_ref() { - Ok(errno_place.clone()) - } else { - // Allocate new place, set initial value to 0. - let errno_layout = this.machine.layouts.u32; - let errno_place = this.allocate(errno_layout, MiriMemoryKind::Machine.into())?; - this.write_scalar(Scalar::from_u32(0), &errno_place)?; - this.active_thread_mut().last_error = Some(errno_place.clone()); - Ok(errno_place) - } - } - - /// Sets the last error variable. - fn set_last_error(&mut self, scalar: Scalar) -> InterpResult<'tcx> { - let this = self.eval_context_mut(); - let errno_place = this.last_error_place()?; - this.write_scalar(scalar, &errno_place) - } - - /// Gets the last error variable. - fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar> { - let this = self.eval_context_mut(); - let errno_place = this.last_error_place()?; - this.read_scalar(&errno_place) - } - - /// This function tries to produce the most similar OS error from the `std::io::ErrorKind` - /// as a platform-specific errnum. - fn io_error_to_errnum(&self, err: std::io::Error) -> InterpResult<'tcx, Scalar> { - let this = self.eval_context_ref(); - let target = &this.tcx.sess.target; - if target.families.iter().any(|f| f == "unix") { - for &(name, kind) in UNIX_IO_ERROR_TABLE { - if err.kind() == kind { - return Ok(this.eval_libc(name)); - } - } - throw_unsup_format!("unsupported io error: {err}") - } else if target.families.iter().any(|f| f == "windows") { - for &(name, kind) in WINDOWS_IO_ERROR_TABLE { - if err.kind() == kind { - return Ok(this.eval_windows("c", name)); - } - } - throw_unsup_format!("unsupported io error: {err}"); - } else { - throw_unsup_format!( - "converting io::Error into errnum is unsupported for OS {}", - target.os - ) - } - } - - /// The inverse of `io_error_to_errnum`. - #[allow(clippy::needless_return)] - fn try_errnum_to_io_error( - &self, - errnum: Scalar, - ) -> InterpResult<'tcx, Option> { - let this = self.eval_context_ref(); - let target = &this.tcx.sess.target; - if target.families.iter().any(|f| f == "unix") { - let errnum = errnum.to_i32()?; - for &(name, kind) in UNIX_IO_ERROR_TABLE { - if errnum == this.eval_libc_i32(name) { - return Ok(Some(kind)); - } - } - return Ok(None); - } else if target.families.iter().any(|f| f == "windows") { - let errnum = errnum.to_u32()?; - for &(name, kind) in WINDOWS_IO_ERROR_TABLE { - if errnum == this.eval_windows("c", name).to_u32()? { - return Ok(Some(kind)); - } - } - return Ok(None); - } else { - throw_unsup_format!( - "converting errnum into io::Error is unsupported for OS {}", - target.os - ) - } - } - - /// Sets the last OS error using a `std::io::ErrorKind`. - fn set_last_error_from_io_error(&mut self, err: std::io::Error) -> InterpResult<'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, - 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` 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. - /// - /// This function uses `T: From` instead of `i32` directly because some IO related - /// functions return different integer types (like `read`, that returns an `i64`). - fn try_unwrap_io_result>( - &mut self, - result: std::io::Result, - ) -> InterpResult<'tcx, T> { - match result { - Ok(ok) => Ok(ok), - Err(e) => { - self.eval_context_mut().set_last_error_from_io_error(e)?; - Ok((-1).into()) - } - } - } - /// Dereference a pointer operand to a place using `layout` instead of the pointer's declared type fn deref_pointer_as( &self, diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index 78e7bf704552d..c4adf38299c81 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -150,6 +150,7 @@ pub use crate::range_map::RangeMap; pub use crate::shims::EmulateItemResult; pub use crate::shims::env::{EnvVars, EvalContextExt as _}; pub use crate::shims::foreign_items::{DynSym, EvalContextExt as _}; +pub use crate::shims::io_error::EvalContextExt as _; pub use crate::shims::os_str::EvalContextExt as _; pub use crate::shims::panic::{CatchUnwindData, EvalContextExt as _}; pub use crate::shims::time::EvalContextExt as _; diff --git a/src/tools/miri/src/shims/io_error.rs b/src/tools/miri/src/shims/io_error.rs new file mode 100644 index 0000000000000..8c727f4c894ed --- /dev/null +++ b/src/tools/miri/src/shims/io_error.rs @@ -0,0 +1,190 @@ +use std::io; + +use crate::*; + +// This mapping should match `decode_error_kind` in +// . +const UNIX_IO_ERROR_TABLE: &[(&str, std::io::ErrorKind)] = { + use std::io::ErrorKind::*; + &[ + ("E2BIG", ArgumentListTooLong), + ("EADDRINUSE", AddrInUse), + ("EADDRNOTAVAIL", AddrNotAvailable), + ("EBUSY", ResourceBusy), + ("ECONNABORTED", ConnectionAborted), + ("ECONNREFUSED", ConnectionRefused), + ("ECONNRESET", ConnectionReset), + ("EDEADLK", Deadlock), + ("EDQUOT", FilesystemQuotaExceeded), + ("EEXIST", AlreadyExists), + ("EFBIG", FileTooLarge), + ("EHOSTUNREACH", HostUnreachable), + ("EINTR", Interrupted), + ("EINVAL", InvalidInput), + ("EISDIR", IsADirectory), + ("ELOOP", FilesystemLoop), + ("ENOENT", NotFound), + ("ENOMEM", OutOfMemory), + ("ENOSPC", StorageFull), + ("ENOSYS", Unsupported), + ("EMLINK", TooManyLinks), + ("ENAMETOOLONG", InvalidFilename), + ("ENETDOWN", NetworkDown), + ("ENETUNREACH", NetworkUnreachable), + ("ENOTCONN", NotConnected), + ("ENOTDIR", NotADirectory), + ("ENOTEMPTY", DirectoryNotEmpty), + ("EPIPE", BrokenPipe), + ("EROFS", ReadOnlyFilesystem), + ("ESPIPE", NotSeekable), + ("ESTALE", StaleNetworkFileHandle), + ("ETIMEDOUT", TimedOut), + ("ETXTBSY", ExecutableFileBusy), + ("EXDEV", CrossesDevices), + // The following have two valid options. We have both for the forwards mapping; only the + // first one will be used for the backwards mapping. + ("EPERM", PermissionDenied), + ("EACCES", PermissionDenied), + ("EWOULDBLOCK", WouldBlock), + ("EAGAIN", WouldBlock), + ] +}; +// This mapping should match `decode_error_kind` in +// . +const WINDOWS_IO_ERROR_TABLE: &[(&str, std::io::ErrorKind)] = { + use std::io::ErrorKind::*; + // FIXME: this is still incomplete. + &[ + ("ERROR_ACCESS_DENIED", PermissionDenied), + ("ERROR_FILE_NOT_FOUND", NotFound), + ("ERROR_INVALID_PARAMETER", InvalidInput), + ] +}; + +impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} +pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { + /// Get last error variable as a place, lazily allocating thread-local storage for it if + /// necessary. + fn last_error_place(&mut self) -> InterpResult<'tcx, MPlaceTy<'tcx>> { + let this = self.eval_context_mut(); + if let Some(errno_place) = this.active_thread_ref().last_error.as_ref() { + Ok(errno_place.clone()) + } else { + // Allocate new place, set initial value to 0. + let errno_layout = this.machine.layouts.u32; + let errno_place = this.allocate(errno_layout, MiriMemoryKind::Machine.into())?; + this.write_scalar(Scalar::from_u32(0), &errno_place)?; + this.active_thread_mut().last_error = Some(errno_place.clone()); + Ok(errno_place) + } + } + + /// Sets the last error variable. + fn set_last_error(&mut self, scalar: Scalar) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + let errno_place = this.last_error_place()?; + this.write_scalar(scalar, &errno_place) + } + + /// Gets the last error variable. + fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar> { + let this = self.eval_context_mut(); + let errno_place = this.last_error_place()?; + this.read_scalar(&errno_place) + } + + /// This function tries to produce the most similar OS error from the `std::io::ErrorKind` + /// as a platform-specific errnum. + fn io_error_to_errnum(&self, err: std::io::Error) -> InterpResult<'tcx, Scalar> { + let this = self.eval_context_ref(); + let target = &this.tcx.sess.target; + if target.families.iter().any(|f| f == "unix") { + for &(name, kind) in UNIX_IO_ERROR_TABLE { + if err.kind() == kind { + return Ok(this.eval_libc(name)); + } + } + throw_unsup_format!("unsupported io error: {err}") + } else if target.families.iter().any(|f| f == "windows") { + for &(name, kind) in WINDOWS_IO_ERROR_TABLE { + if err.kind() == kind { + return Ok(this.eval_windows("c", name)); + } + } + throw_unsup_format!("unsupported io error: {err}"); + } else { + throw_unsup_format!( + "converting io::Error into errnum is unsupported for OS {}", + target.os + ) + } + } + + /// The inverse of `io_error_to_errnum`. + #[allow(clippy::needless_return)] + fn try_errnum_to_io_error( + &self, + errnum: Scalar, + ) -> InterpResult<'tcx, Option> { + let this = self.eval_context_ref(); + let target = &this.tcx.sess.target; + if target.families.iter().any(|f| f == "unix") { + let errnum = errnum.to_i32()?; + for &(name, kind) in UNIX_IO_ERROR_TABLE { + if errnum == this.eval_libc_i32(name) { + return Ok(Some(kind)); + } + } + return Ok(None); + } else if target.families.iter().any(|f| f == "windows") { + let errnum = errnum.to_u32()?; + for &(name, kind) in WINDOWS_IO_ERROR_TABLE { + if errnum == this.eval_windows("c", name).to_u32()? { + return Ok(Some(kind)); + } + } + return Ok(None); + } else { + throw_unsup_format!( + "converting errnum into io::Error is unsupported for OS {}", + target.os + ) + } + } + + /// Sets the last OS error using a `std::io::ErrorKind`. + fn set_last_error_from_io_error(&mut self, err: std::io::Error) -> InterpResult<'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, + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + this.set_last_error(this.io_error_to_errnum(err.into())?)?; + this.write_int(-1, dest)?; + Ok(()) + } + + /// Helper function that consumes an `std::io::Result` 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. + /// + /// This function uses `T: From` instead of `i32` directly because some IO related + /// functions return different integer types (like `read`, that returns an `i64`). + fn try_unwrap_io_result>( + &mut self, + result: std::io::Result, + ) -> InterpResult<'tcx, T> { + match result { + Ok(ok) => Ok(ok), + Err(e) => { + self.eval_context_mut().set_last_error_from_io_error(e)?; + Ok((-1).into()) + } + } + } +} diff --git a/src/tools/miri/src/shims/mod.rs b/src/tools/miri/src/shims/mod.rs index a689ac2b3784e..b9317ac1a15fa 100644 --- a/src/tools/miri/src/shims/mod.rs +++ b/src/tools/miri/src/shims/mod.rs @@ -12,6 +12,7 @@ mod x86; pub mod env; pub mod extern_static; pub mod foreign_items; +pub mod io_error; pub mod os_str; pub mod panic; pub mod time; From 9c21fd4b93540d841e11ba4f975660666a9bfc3f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 1 Oct 2024 09:02:33 +0200 Subject: [PATCH 03/16] make set_last_error directly callable on a bunch of ways to represent errors --- src/tools/miri/src/lib.rs | 2 +- src/tools/miri/src/shims/io_error.rs | 48 +++++++++++++++---- src/tools/miri/src/shims/unix/env.rs | 15 +++--- src/tools/miri/src/shims/unix/fd.rs | 8 ++-- .../miri/src/shims/unix/foreign_items.rs | 22 ++++----- src/tools/miri/src/shims/unix/fs.rs | 48 ++++++++----------- src/tools/miri/src/shims/unix/linux/epoll.rs | 6 +-- src/tools/miri/src/shims/unix/linux/sync.rs | 9 ++-- src/tools/miri/src/shims/windows/env.rs | 8 ++-- .../miri/src/shims/windows/foreign_items.rs | 2 +- 10 files changed, 88 insertions(+), 80 deletions(-) diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index c4adf38299c81..330147c8f1cf8 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -150,7 +150,7 @@ pub use crate::range_map::RangeMap; pub use crate::shims::EmulateItemResult; pub use crate::shims::env::{EnvVars, EvalContextExt as _}; pub use crate::shims::foreign_items::{DynSym, EvalContextExt as _}; -pub use crate::shims::io_error::EvalContextExt as _; +pub use crate::shims::io_error::{EvalContextExt as _, LibcError}; pub use crate::shims::os_str::EvalContextExt as _; pub use crate::shims::panic::{CatchUnwindData, EvalContextExt as _}; pub use crate::shims::time::EvalContextExt as _; diff --git a/src/tools/miri/src/shims/io_error.rs b/src/tools/miri/src/shims/io_error.rs index 8c727f4c894ed..61b943b599d93 100644 --- a/src/tools/miri/src/shims/io_error.rs +++ b/src/tools/miri/src/shims/io_error.rs @@ -2,6 +2,34 @@ use std::io; use crate::*; +/// A representation of an IO error: either a libc error name, +/// or a host error. +#[derive(Debug)] +pub enum IoError { + LibcError(&'static str), + HostError(io::Error), + Raw(Scalar), +} +pub use self::IoError::*; + +impl From for IoError { + fn from(value: io::Error) -> Self { + IoError::HostError(value) + } +} + +impl From for IoError { + fn from(value: io::ErrorKind) -> Self { + IoError::HostError(value.into()) + } +} + +impl From for IoError { + fn from(value: Scalar) -> Self { + IoError::Raw(value) + } +} + // This mapping should match `decode_error_kind` in // . const UNIX_IO_ERROR_TABLE: &[(&str, std::io::ErrorKind)] = { @@ -80,10 +108,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } /// Sets the last error variable. - fn set_last_error(&mut self, scalar: Scalar) -> InterpResult<'tcx> { + fn set_last_error(&mut self, err: impl Into) -> InterpResult<'tcx> { let this = self.eval_context_mut(); + let errno = match err.into() { + HostError(err) => this.io_error_to_errnum(err)?, + LibcError(name) => this.eval_libc(name), + Raw(val) => val, + }; let errno_place = this.last_error_place()?; - this.write_scalar(scalar, &errno_place) + this.write_scalar(errno, &errno_place) } /// Gets the last error variable. @@ -152,19 +185,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } } - /// Sets the last OS error using a `std::io::ErrorKind`. - fn set_last_error_from_io_error(&mut self, err: std::io::Error) -> InterpResult<'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, + err: impl Into, dest: &MPlaceTy<'tcx>, ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - this.set_last_error(this.io_error_to_errnum(err.into())?)?; + this.set_last_error(err)?; this.write_int(-1, dest)?; Ok(()) } @@ -182,7 +210,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { match result { Ok(ok) => Ok(ok), Err(e) => { - self.eval_context_mut().set_last_error_from_io_error(e)?; + self.eval_context_mut().set_last_error(e)?; Ok((-1).into()) } } diff --git a/src/tools/miri/src/shims/unix/env.rs b/src/tools/miri/src/shims/unix/env.rs index 184a6c238b392..923de15d2dafa 100644 --- a/src/tools/miri/src/shims/unix/env.rs +++ b/src/tools/miri/src/shims/unix/env.rs @@ -177,8 +177,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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)?; + this.set_last_error(LibcError("EINVAL"))?; Ok(Scalar::from_i32(-1)) } } @@ -203,8 +202,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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)?; + this.set_last_error(LibcError("EINVAL"))?; Ok(Scalar::from_i32(-1)) } } @@ -218,7 +216,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`getcwd`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?; + this.set_last_error(ErrorKind::PermissionDenied)?; return Ok(Pointer::null()); } @@ -228,10 +226,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if this.write_path_to_c_str(&cwd, buf, size)?.0 { return Ok(buf); } - let erange = this.eval_libc("ERANGE"); - this.set_last_error(erange)?; + this.set_last_error(LibcError("ERANGE"))?; } - Err(e) => this.set_last_error_from_io_error(e)?, + Err(e) => this.set_last_error(e)?, } Ok(Pointer::null()) @@ -245,7 +242,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`chdir`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?; + this.set_last_error(ErrorKind::PermissionDenied)?; return Ok(Scalar::from_i32(-1)); } diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index f64d0ed1467bc..fe634de122c46 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -524,7 +524,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // 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())?; + this.set_last_error(ErrorKind::PermissionDenied)?; return Ok(Scalar::from_i32(-1)); } @@ -606,8 +606,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { None => fd.read(&fd, communicate, buf, count, dest, this)?, Some(offset) => { let Ok(offset) = u64::try_from(offset) else { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; this.write_int(-1, dest)?; return Ok(()); }; @@ -651,8 +650,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { None => fd.write(&fd, communicate, buf, count, dest, this)?, Some(offset) => { let Ok(offset) = u64::try_from(offset) else { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; this.write_int(-1, dest)?; return Ok(()); }; diff --git a/src/tools/miri/src/shims/unix/foreign_items.rs b/src/tools/miri/src/shims/unix/foreign_items.rs index 528d068ea92c0..14af4245148e2 100644 --- a/src/tools/miri/src/shims/unix/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/foreign_items.rs @@ -355,8 +355,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // FreeBSD: https://man.freebsd.org/cgi/man.cgi?query=reallocarray match this.compute_size_in_bytes(Size::from_bytes(size), nmemb) { None => { - let einval = this.eval_libc("ENOMEM"); - this.set_last_error(einval)?; + let enmem = this.eval_libc("ENOMEM"); + this.set_last_error(enmem)?; this.write_null(dest)?; } Some(len) => { @@ -646,13 +646,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let chunk_size = CpuAffinityMask::chunk_size(this); if this.ptr_is_null(mask)? { - let einval = this.eval_libc("EFAULT"); - this.set_last_error(einval)?; + let efault = this.eval_libc("EFAULT"); + this.set_last_error(efault)?; this.write_int(-1, dest)?; } else if cpusetsize == 0 || cpusetsize.checked_rem(chunk_size).unwrap() != 0 { // we only copy whole chunks of size_of::() - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; this.write_int(-1, dest)?; } else if let Some(cpuset) = this.machine.thread_cpu_affinity.get(&thread_id) { let cpuset = cpuset.clone(); @@ -662,8 +661,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_null(dest)?; } else { // The thread whose ID is pid could not be found - let einval = this.eval_libc("ESRCH"); - this.set_last_error(einval)?; + let esrch = this.eval_libc("ESRCH"); + this.set_last_error(esrch)?; this.write_int(-1, dest)?; } } @@ -689,8 +688,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; if this.ptr_is_null(mask)? { - let einval = this.eval_libc("EFAULT"); - this.set_last_error(einval)?; + let efault = this.eval_libc("EFAULT"); + this.set_last_error(efault)?; this.write_int(-1, dest)?; } else { // NOTE: cpusetsize might be smaller than `CpuAffinityMask::CPU_MASK_BYTES`. @@ -707,8 +706,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } None => { // The intersection between the mask and the available CPUs was empty. - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; this.write_int(-1, dest)?; } } diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index 2fa1729ad9c8a..80e45d8439819 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -534,8 +534,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let o_tmpfile = this.eval_libc_i32("O_TMPFILE"); if flag & o_tmpfile == o_tmpfile { // if the flag contains `O_TMPFILE` then we return a graceful error - let eopnotsupp = this.eval_libc("EOPNOTSUPP"); - this.set_last_error(eopnotsupp)?; + this.set_last_error(LibcError("EOPNOTSUPP"))?; return Ok(Scalar::from_i32(-1)); } } @@ -572,7 +571,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`open`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?; + this.set_last_error(ErrorKind::PermissionDenied)?; return Ok(Scalar::from_i32(-1)); } @@ -591,8 +590,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let seek_from = if whence == this.eval_libc_i32("SEEK_SET") { if offset < 0 { // Negative offsets return `EINVAL`. - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; return Ok(Scalar::from_i64(-1)); } else { SeekFrom::Start(u64::try_from(offset).unwrap()) @@ -602,8 +600,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } else if whence == this.eval_libc_i32("SEEK_END") { SeekFrom::End(i64::try_from(offset).unwrap()) } else { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; return Ok(Scalar::from_i64(-1)); }; @@ -627,7 +624,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`unlink`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?; + this.set_last_error(ErrorKind::PermissionDenied)?; return Ok(Scalar::from_i32(-1)); } @@ -658,7 +655,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`symlink`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?; + this.set_last_error(ErrorKind::PermissionDenied)?; return Ok(Scalar::from_i32(-1)); } @@ -959,7 +956,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`rename`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?; + this.set_last_error(ErrorKind::PermissionDenied)?; return Ok(Scalar::from_i32(-1)); } @@ -983,7 +980,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`mkdir`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?; + this.set_last_error(ErrorKind::PermissionDenied)?; return Ok(Scalar::from_i32(-1)); } @@ -1011,7 +1008,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`rmdir`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?; + this.set_last_error(ErrorKind::PermissionDenied)?; return Ok(Scalar::from_i32(-1)); } @@ -1045,7 +1042,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(Scalar::from_target_usize(id, this)) } Err(e) => { - this.set_last_error_from_io_error(e)?; + this.set_last_error(e)?; Ok(Scalar::null_ptr(this)) } } @@ -1130,7 +1127,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { None } Some(Err(e)) => { - this.set_last_error_from_io_error(e)?; + this.set_last_error(e)?; None } }; @@ -1314,15 +1311,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(Scalar::from_i32(result)) } else { drop(fd); - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; Ok(Scalar::from_i32(-1)) } } else { drop(fd); // The file is not writable - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; Ok(Scalar::from_i32(-1)) } } @@ -1400,16 +1395,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let flags = this.read_scalar(flags_op)?.to_i32()?; if offset < 0 || nbytes < 0 { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; return Ok(Scalar::from_i32(-1)); } let allowed_flags = this.eval_libc_i32("SYNC_FILE_RANGE_WAIT_BEFORE") | this.eval_libc_i32("SYNC_FILE_RANGE_WRITE") | this.eval_libc_i32("SYNC_FILE_RANGE_WAIT_AFTER"); if flags & allowed_flags != flags { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; return Ok(Scalar::from_i32(-1)); } @@ -1471,7 +1464,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(path_bytes.len().try_into().unwrap()) } Err(e) => { - this.set_last_error_from_io_error(e)?; + this.set_last_error(e)?; Ok(-1) } } @@ -1551,7 +1544,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(Scalar::from_maybe_pointer(dest, this)) } Err(e) => { - this.set_last_error_from_io_error(e)?; + this.set_last_error(e)?; Ok(Scalar::from_target_usize(0, this)) } } @@ -1603,8 +1596,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // If we don't find the suffix, it is an error. if last_six_char_bytes != suffix_bytes { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; return Ok(Scalar::from_i32(-1)); } @@ -1670,7 +1662,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { _ => { // "On error, -1 is returned, and errno is set to // indicate the error" - this.set_last_error_from_io_error(e)?; + this.set_last_error(e)?; return Ok(Scalar::from_i32(-1)); } }, @@ -1749,7 +1741,7 @@ impl FileMetadata { let metadata = match metadata { Ok(metadata) => metadata, Err(e) => { - ecx.set_last_error_from_io_error(e)?; + ecx.set_last_error(e)?; return Ok(None); } }; diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 675a404ae117c..ff0709dd5ac1c 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -261,8 +261,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Throw EINVAL if epfd and fd have the same value. if epfd_value == fd { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; return Ok(Scalar::from_i32(-1)); } @@ -443,8 +442,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let timeout = this.read_scalar(timeout)?.to_i32()?; if epfd_value <= 0 || maxevents <= 0 { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; this.write_int(-1, dest)?; return Ok(()); } diff --git a/src/tools/miri/src/shims/unix/linux/sync.rs b/src/tools/miri/src/shims/unix/linux/sync.rs index bd21203907419..1d1764b3a1a11 100644 --- a/src/tools/miri/src/shims/unix/linux/sync.rs +++ b/src/tools/miri/src/shims/unix/linux/sync.rs @@ -75,8 +75,7 @@ pub fn futex<'tcx>( }; if bitset == 0 { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; this.write_scalar(Scalar::from_target_isize(-1, this), dest)?; return Ok(()); } @@ -88,8 +87,7 @@ pub fn futex<'tcx>( let duration = match this.read_timespec(&timeout)? { Some(duration) => duration, None => { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; this.write_scalar(Scalar::from_target_isize(-1, this), dest)?; return Ok(()); } @@ -198,8 +196,7 @@ pub fn futex<'tcx>( u32::MAX }; if bitset == 0 { - let einval = this.eval_libc("EINVAL"); - this.set_last_error(einval)?; + this.set_last_error(LibcError("EINVAL"))?; this.write_scalar(Scalar::from_target_isize(-1, this), dest)?; return Ok(()); } diff --git a/src/tools/miri/src/shims/windows/env.rs b/src/tools/miri/src/shims/windows/env.rs index 9707482b1e2c2..cf99ac758c643 100644 --- a/src/tools/miri/src/shims/windows/env.rs +++ b/src/tools/miri/src/shims/windows/env.rs @@ -150,7 +150,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`GetCurrentDirectoryW`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?; + this.set_last_error(ErrorKind::PermissionDenied)?; return Ok(Scalar::from_u32(0)); } @@ -163,7 +163,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_path_to_wide_str(&cwd, buf, size)?, ))); } - Err(e) => this.set_last_error_from_io_error(e)?, + Err(e) => this.set_last_error(e)?, } Ok(Scalar::from_u32(0)) } @@ -182,7 +182,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`SetCurrentDirectoryW`", reject_with)?; - this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?; + this.set_last_error(ErrorKind::PermissionDenied)?; return Ok(this.eval_windows("c", "FALSE")); } @@ -190,7 +190,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { match env::set_current_dir(path) { Ok(()) => Ok(this.eval_windows("c", "TRUE")), Err(e) => { - this.set_last_error_from_io_error(e)?; + this.set_last_error(e)?; Ok(this.eval_windows("c", "FALSE")) } } diff --git a/src/tools/miri/src/shims/windows/foreign_items.rs b/src/tools/miri/src/shims/windows/foreign_items.rs index 22634c509bed0..c51726b05dc5a 100644 --- a/src/tools/miri/src/shims/windows/foreign_items.rs +++ b/src/tools/miri/src/shims/windows/foreign_items.rs @@ -227,7 +227,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let filename = this.read_path_from_wide_str(filename)?; let result = match win_absolute(&filename)? { Err(err) => { - this.set_last_error_from_io_error(err)?; + this.set_last_error(err)?; Scalar::from_u32(0) // return zero upon failure } Ok(abs_filename) => { From 4f4e1d42b59e96141745ace47ed7a7c96211bf33 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 1 Oct 2024 09:08:10 +0200 Subject: [PATCH 04/16] add set_last_error_and_return_i32 helper and use it in a few places --- src/tools/miri/src/shims/io_error.rs | 34 ++++++++++++++++++---------- src/tools/miri/src/shims/unix/env.rs | 10 +++----- src/tools/miri/src/shims/unix/fd.rs | 11 +++------ 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/src/tools/miri/src/shims/io_error.rs b/src/tools/miri/src/shims/io_error.rs index 61b943b599d93..aa13c405145cc 100644 --- a/src/tools/miri/src/shims/io_error.rs +++ b/src/tools/miri/src/shims/io_error.rs @@ -119,6 +119,28 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(errno, &errno_place) } + /// Sets the last OS error and writes -1 to dest place. + fn set_last_error_and_return( + &mut self, + err: impl Into, + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + this.set_last_error(err)?; + this.write_int(-1, dest)?; + Ok(()) + } + + /// Sets the last OS error and return `-1` as a `i32`-typed Scalar + fn set_last_error_and_return_i32( + &mut self, + err: impl Into, + ) -> InterpResult<'tcx, Scalar> { + let this = self.eval_context_mut(); + this.set_last_error(err)?; + Ok(Scalar::from_i32(-1)) + } + /// Gets the last error variable. fn get_last_error(&mut self) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); @@ -185,18 +207,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } } - /// 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, - dest: &MPlaceTy<'tcx>, - ) -> InterpResult<'tcx> { - let this = self.eval_context_mut(); - this.set_last_error(err)?; - this.write_int(-1, dest)?; - Ok(()) - } - /// Helper function that consumes an `std::io::Result` 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. diff --git a/src/tools/miri/src/shims/unix/env.rs b/src/tools/miri/src/shims/unix/env.rs index 923de15d2dafa..4a9e2313aadf1 100644 --- a/src/tools/miri/src/shims/unix/env.rs +++ b/src/tools/miri/src/shims/unix/env.rs @@ -177,8 +177,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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. - this.set_last_error(LibcError("EINVAL"))?; - Ok(Scalar::from_i32(-1)) + this.set_last_error_and_return_i32(LibcError("EINVAL")) } } @@ -202,8 +201,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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. - this.set_last_error(LibcError("EINVAL"))?; - Ok(Scalar::from_i32(-1)) + this.set_last_error_and_return_i32(LibcError("EINVAL")) } } @@ -242,9 +240,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`chdir`", reject_with)?; - this.set_last_error(ErrorKind::PermissionDenied)?; - - return Ok(Scalar::from_i32(-1)); + return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied); } let result = env::set_current_dir(path).map(|()| 0); diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index fe634de122c46..41a819fb31f8c 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -524,8 +524,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // 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(ErrorKind::PermissionDenied)?; - return Ok(Scalar::from_i32(-1)); + return this.set_last_error_and_return_i32(ErrorKind::PermissionDenied); } this.ffullsync_fd(fd_num) @@ -606,9 +605,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { None => fd.read(&fd, communicate, buf, count, dest, this)?, Some(offset) => { let Ok(offset) = u64::try_from(offset) else { - this.set_last_error(LibcError("EINVAL"))?; - this.write_int(-1, dest)?; - return Ok(()); + return this.set_last_error_and_return(LibcError("EINVAL"), dest); }; fd.pread(communicate, offset, buf, count, dest, this)? } @@ -650,9 +647,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { None => fd.write(&fd, communicate, buf, count, dest, this)?, Some(offset) => { let Ok(offset) = u64::try_from(offset) else { - this.set_last_error(LibcError("EINVAL"))?; - this.write_int(-1, dest)?; - return Ok(()); + return this.set_last_error_and_return(LibcError("EINVAL"), dest); }; fd.pwrite(communicate, buf, count, offset, dest, this)? } From 3e089b0e16db2779de0c5ca7d54a462d78354fb7 Mon Sep 17 00:00:00 2001 From: Frank Rehwinkel Date: Wed, 2 Oct 2024 07:44:01 -0400 Subject: [PATCH 05/16] epoll: add data_race test This test demonstrates the need to synchronize the clock of the thread waking up from an epoll_wait from the thread that issued the epoll awake event. --- .../fail-dep/libc/libc-epoll-blocking.rs | 79 +++++++++++++++++++ .../fail-dep/libc/libc-epoll-blocking.stderr | 29 +++++++ 2 files changed, 108 insertions(+) create mode 100644 src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs create mode 100644 src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr diff --git a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs new file mode 100644 index 0000000000000..fd9d1875daf83 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs @@ -0,0 +1,79 @@ +//@only-target: linux +// test_epoll_race depends on a deterministic schedule. +//@compile-flags: -Zmiri-preemption-rate=0 + +use std::convert::TryInto; +use std::thread; + +fn main() { + test_epoll_race(); +} + +// Using `as` cast since `EPOLLET` wraps around +const EPOLL_IN_OUT_ET: u32 = (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET) as _; + +#[track_caller] +fn check_epoll_wait( + epfd: i32, + expected_notifications: &[(u32, u64)], + timeout: i32, +) { + let epoll_event = libc::epoll_event { events: 0, u64: 0 }; + let mut array: [libc::epoll_event; N] = [epoll_event; N]; + let maxsize = N; + let array_ptr = array.as_mut_ptr(); + let res = unsafe { libc::epoll_wait(epfd, array_ptr, maxsize.try_into().unwrap(), timeout) }; + if res < 0 { + panic!("epoll_wait failed: {}", std::io::Error::last_os_error()); + } + assert_eq!( + res, + expected_notifications.len().try_into().unwrap(), + "got wrong number of notifications" + ); + let slice = unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; + for (return_event, expected_event) in slice.iter().zip(expected_notifications.iter()) { + let event = return_event.events; + let data = return_event.u64; + assert_eq!(event, expected_event.0, "got wrong events"); + assert_eq!(data, expected_event.1, "got wrong data"); + } +} + +// This test shows a data_race before epoll had vector clocks added. +fn test_epoll_race() { + // Create an epoll instance. + let epfd = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd, -1); + + // Create an eventfd instance. + let flags = libc::EFD_NONBLOCK | libc::EFD_CLOEXEC; + let fd = unsafe { libc::eventfd(0, flags) }; + + // Register eventfd with the epoll instance. + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fd as u64 }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; + assert_eq!(res, 0); + + static mut VAL: u8 = 0; + let thread1 = thread::spawn(move || { + // Write to the static mut variable. + unsafe { VAL = 1 }; + // Write to the eventfd instance. + let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); + let res = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8) }; + // read returns number of bytes that have been read, which is always 8. + assert_eq!(res, 8); + }); + thread::yield_now(); + // epoll_wait for the event to happen. + let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); + let expected_value = u64::try_from(fd).unwrap(); + check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)], -1); + // Read from the static mut variable. + #[allow(static_mut_refs)] + unsafe { + assert_eq!(VAL, 1) //~ ERROR: Data race detected + }; + thread1.join().unwrap(); +} diff --git a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr new file mode 100644 index 0000000000000..27974981cea7a --- /dev/null +++ b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr @@ -0,0 +1,29 @@ +error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `unnamed-ID` and (2) retag read of type `u8` on thread `main` at ALLOC. (2) just happened here + --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC + | +LL | assert_eq!(VAL, 1) + | ^^^^^^^^^^^^^^^^^^ Data race detected between (1) non-atomic write on thread `unnamed-ID` and (2) retag read of type `u8` on thread `main` at ALLOC. (2) just happened here + | +help: and (1) occurred earlier here + --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC + | +LL | unsafe { VAL = 1 }; + | ^^^^^^^ + = help: retags occur on all (re)borrows and as well as when references are copied or moved + = help: retags permit optimizations that insert speculative reads or writes + = help: therefore from the perspective of data races, a retag has the same implications as a read or write + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE (of the first span): + = note: inside `test_epoll_race` at RUSTLIB/core/src/macros/mod.rs:LL:CC +note: inside `main` + --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC + | +LL | test_epoll_race(); + | ^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + From 1b622f46728a51b6582b02b84407e5f64262479b Mon Sep 17 00:00:00 2001 From: Frank Rehwinkel Date: Wed, 2 Oct 2024 07:57:36 -0400 Subject: [PATCH 06/16] epoll: remove unnecessary instructions A couple of instructions were left over from an earlier rebase it would seem. They don't impact the logic but the ready_list type is about to change in the next commit. Rather than modify one of these lines in the commit that changes ready_list, only to have these lines removed later on, remove them now. They don't impact the tests results. --- src/tools/miri/src/shims/unix/linux/epoll.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index ff0709dd5ac1c..13c44334d5844 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -207,9 +207,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ); } - let mut epoll_instance = Epoll::default(); - epoll_instance.ready_list = Rc::new(RefCell::new(BTreeMap::new())); - let fd = this.machine.fds.insert_new(Epoll::default()); Ok(Scalar::from_i32(fd)) } From 86bb1373aa94aec8d9d4c8fb692cd352eca01d90 Mon Sep 17 00:00:00 2001 From: Frank Rehwinkel Date: Wed, 2 Oct 2024 08:04:25 -0400 Subject: [PATCH 07/16] epoll: add vector clock to the epoll ready_list This adds a VClock to the epoll implementation's ready_list and has this VClock synced from the thread that updates an event in the ready_list and then has the VClocks of any threads being made runnable again, out of the calls to epoll_wait, synced from it. --- src/tools/miri/src/shims/unix/linux/epoll.rs | 37 ++++++--- .../fail-dep/libc/libc-epoll-blocking.rs | 79 ------------------- .../fail-dep/libc/libc-epoll-blocking.stderr | 29 ------- .../pass-dep/libc/libc-epoll-blocking.rs | 41 +++++++++- 4 files changed, 67 insertions(+), 119 deletions(-) delete mode 100644 src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs delete mode 100644 src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 13c44334d5844..88fab8aa010ab 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -4,6 +4,7 @@ use std::io; use std::rc::{Rc, Weak}; use std::time::Duration; +use crate::concurrency::VClock; use crate::shims::unix::fd::{FdId, FileDescriptionRef, WeakFileDescriptionRef}; use crate::shims::unix::*; use crate::*; @@ -19,7 +20,7 @@ struct Epoll { /// and file descriptor value. // This is an Rc because EpollInterest need to hold a reference to update // it. - ready_list: Rc>>, + ready_list: Rc, /// A list of thread ids blocked on this epoll instance. thread_id: RefCell>, } @@ -63,7 +64,7 @@ pub struct EpollEventInterest { /// data: u64, /// Ready list of the epoll instance under which this EpollEventInterest is registered. - ready_list: Rc>>, + ready_list: Rc, /// The epoll file description that this EpollEventInterest is registered under. weak_epfd: WeakFileDescriptionRef, } @@ -88,6 +89,12 @@ pub struct EpollReadyEvents { pub epollerr: bool, } +#[derive(Debug, Default)] +struct ReadyList { + mapping: RefCell>, + clock: RefCell, +} + impl EpollReadyEvents { pub fn new() -> Self { EpollReadyEvents { @@ -127,7 +134,7 @@ impl EpollReadyEvents { } impl Epoll { - fn get_ready_list(&self) -> Rc>> { + fn get_ready_list(&self) -> Rc { Rc::clone(&self.ready_list) } } @@ -374,7 +381,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { drop(epoll_interest); // Remove related epoll_interest from ready list. - ready_list.borrow_mut().remove(&epoll_key); + ready_list.mapping.borrow_mut().remove(&epoll_key); // Remove dangling EpollEventInterest from its global table. // .unwrap() below should succeed because the file description id must have registered @@ -469,7 +476,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { .downcast::() .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; let binding = epoll_file_description.get_ready_list(); - ready_list_empty = binding.borrow_mut().is_empty(); + ready_list_empty = binding.mapping.borrow_mut().is_empty(); thread_ids = epoll_file_description.thread_id.borrow_mut(); } if timeout == 0 || !ready_list_empty { @@ -558,9 +565,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // holds a strong ref to epoll_interest. let epfd = epoll_interest.borrow().weak_epfd.upgrade().unwrap(); // FIXME: We can randomly pick a thread to unblock. - if let Some(thread_id) = - epfd.downcast::().unwrap().thread_id.borrow_mut().pop() - { + + let epoll = epfd.downcast::().unwrap(); + + // Synchronize running thread to the epoll ready list. + if let Some(clock) = &this.release_clock() { + epoll.ready_list.clock.borrow_mut().join(clock); + } + + if let Some(thread_id) = epoll.thread_id.borrow_mut().pop() { waiter.push(thread_id); }; } @@ -614,7 +627,7 @@ fn check_and_update_one_event_interest<'tcx>( // insert an epoll_return to the ready list. if flags != 0 { let epoll_key = (id, epoll_event_interest.fd_num); - let ready_list = &mut epoll_event_interest.ready_list.borrow_mut(); + let ready_list = &mut epoll_event_interest.ready_list.mapping.borrow_mut(); let event_instance = EpollEventInstance::new(flags, epoll_event_interest.data); // Triggers the notification by inserting it to the ready list. ready_list.insert(epoll_key, event_instance); @@ -641,7 +654,11 @@ fn blocking_epoll_callback<'tcx>( .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; let ready_list = epoll_file_description.get_ready_list(); - let mut ready_list = ready_list.borrow_mut(); + + // Synchronize waking thread from the epoll ready list. + ecx.acquire_clock(&ready_list.clock.borrow()); + + let mut ready_list = ready_list.mapping.borrow_mut(); let mut num_of_events: i32 = 0; let mut array_iter = ecx.project_array_fields(events)?; diff --git a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs deleted file mode 100644 index fd9d1875daf83..0000000000000 --- a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs +++ /dev/null @@ -1,79 +0,0 @@ -//@only-target: linux -// test_epoll_race depends on a deterministic schedule. -//@compile-flags: -Zmiri-preemption-rate=0 - -use std::convert::TryInto; -use std::thread; - -fn main() { - test_epoll_race(); -} - -// Using `as` cast since `EPOLLET` wraps around -const EPOLL_IN_OUT_ET: u32 = (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET) as _; - -#[track_caller] -fn check_epoll_wait( - epfd: i32, - expected_notifications: &[(u32, u64)], - timeout: i32, -) { - let epoll_event = libc::epoll_event { events: 0, u64: 0 }; - let mut array: [libc::epoll_event; N] = [epoll_event; N]; - let maxsize = N; - let array_ptr = array.as_mut_ptr(); - let res = unsafe { libc::epoll_wait(epfd, array_ptr, maxsize.try_into().unwrap(), timeout) }; - if res < 0 { - panic!("epoll_wait failed: {}", std::io::Error::last_os_error()); - } - assert_eq!( - res, - expected_notifications.len().try_into().unwrap(), - "got wrong number of notifications" - ); - let slice = unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; - for (return_event, expected_event) in slice.iter().zip(expected_notifications.iter()) { - let event = return_event.events; - let data = return_event.u64; - assert_eq!(event, expected_event.0, "got wrong events"); - assert_eq!(data, expected_event.1, "got wrong data"); - } -} - -// This test shows a data_race before epoll had vector clocks added. -fn test_epoll_race() { - // Create an epoll instance. - let epfd = unsafe { libc::epoll_create1(0) }; - assert_ne!(epfd, -1); - - // Create an eventfd instance. - let flags = libc::EFD_NONBLOCK | libc::EFD_CLOEXEC; - let fd = unsafe { libc::eventfd(0, flags) }; - - // Register eventfd with the epoll instance. - let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fd as u64 }; - let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; - assert_eq!(res, 0); - - static mut VAL: u8 = 0; - let thread1 = thread::spawn(move || { - // Write to the static mut variable. - unsafe { VAL = 1 }; - // Write to the eventfd instance. - let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); - let res = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8) }; - // read returns number of bytes that have been read, which is always 8. - assert_eq!(res, 8); - }); - thread::yield_now(); - // epoll_wait for the event to happen. - let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); - let expected_value = u64::try_from(fd).unwrap(); - check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)], -1); - // Read from the static mut variable. - #[allow(static_mut_refs)] - unsafe { - assert_eq!(VAL, 1) //~ ERROR: Data race detected - }; - thread1.join().unwrap(); -} diff --git a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr deleted file mode 100644 index 27974981cea7a..0000000000000 --- a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr +++ /dev/null @@ -1,29 +0,0 @@ -error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `unnamed-ID` and (2) retag read of type `u8` on thread `main` at ALLOC. (2) just happened here - --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC - | -LL | assert_eq!(VAL, 1) - | ^^^^^^^^^^^^^^^^^^ Data race detected between (1) non-atomic write on thread `unnamed-ID` and (2) retag read of type `u8` on thread `main` at ALLOC. (2) just happened here - | -help: and (1) occurred earlier here - --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC - | -LL | unsafe { VAL = 1 }; - | ^^^^^^^ - = help: retags occur on all (re)borrows and as well as when references are copied or moved - = help: retags permit optimizations that insert speculative reads or writes - = help: therefore from the perspective of data races, a retag has the same implications as a read or write - = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior - = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE (of the first span): - = note: inside `test_epoll_race` at RUSTLIB/core/src/macros/mod.rs:LL:CC -note: inside `main` - --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC - | -LL | test_epoll_race(); - | ^^^^^^^^^^^^^^^^^ - = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info) - -note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace - -error: aborting due to 1 previous error - diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs index eb38529ae57df..d7675a40163c5 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs @@ -1,5 +1,5 @@ //@only-target: linux -// test_epoll_block_then_unblock depends on a deterministic schedule. +// test_epoll_block_then_unblock and test_epoll_race depend on a deterministic schedule. //@compile-flags: -Zmiri-preemption-rate=0 use std::convert::TryInto; @@ -12,6 +12,7 @@ fn main() { test_epoll_block_without_notification(); test_epoll_block_then_unblock(); test_notification_after_timeout(); + test_epoll_race(); } // Using `as` cast since `EPOLLET` wraps around @@ -137,3 +138,41 @@ fn test_notification_after_timeout() { let expected_value = fds[0] as u64; check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], 10); } + +// This test shows a data_race before epoll had vector clocks added. +fn test_epoll_race() { + // Create an epoll instance. + let epfd = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd, -1); + + // Create an eventfd instance. + let flags = libc::EFD_NONBLOCK | libc::EFD_CLOEXEC; + let fd = unsafe { libc::eventfd(0, flags) }; + + // Register eventfd with the epoll instance. + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fd as u64 }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; + assert_eq!(res, 0); + + static mut VAL: u8 = 0; + let thread1 = thread::spawn(move || { + // Write to the static mut variable. + unsafe { VAL = 1 }; + // Write to the eventfd instance. + let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); + let res = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8) }; + // read returns number of bytes that have been read, which is always 8. + assert_eq!(res, 8); + }); + thread::yield_now(); + // epoll_wait for the event to happen. + let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); + let expected_value = u64::try_from(fd).unwrap(); + check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)], -1); + // Read from the static mut variable. + #[allow(static_mut_refs)] + unsafe { + assert_eq!(VAL, 1) + }; + thread1.join().unwrap(); +} From 81202c8b13c6ff64c14fd740757ba3e283adfec7 Mon Sep 17 00:00:00 2001 From: Frank Rehwinkel Date: Wed, 2 Oct 2024 08:13:11 -0400 Subject: [PATCH 08/16] epoll: remove extraneous clone of ready_list A simplification that doesn't impact the epoll implementation's logic. It is not necessary to clone the ready_list before reading its `is_empty` state. This avoids the clone step but more importantly avoids the invisible drop step of the clone. --- src/tools/miri/src/shims/unix/linux/epoll.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 88fab8aa010ab..c4bd38c47e54d 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -475,8 +475,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let epoll_file_description = epfd .downcast::() .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; - let binding = epoll_file_description.get_ready_list(); - ready_list_empty = binding.mapping.borrow_mut().is_empty(); + ready_list_empty = epoll_file_description.ready_list.mapping.borrow().is_empty(); thread_ids = epoll_file_description.thread_id.borrow_mut(); } if timeout == 0 || !ready_list_empty { From be819d415952f877019e6a0fea269e2aaccc552b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 3 Oct 2024 07:47:41 +0200 Subject: [PATCH 09/16] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index b05c409f82319..fe0421b2075a5 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -76ed7a1fa40c3f54d3fd3f834e12bf9c932d0146 +ad9c494835e746fb7c8a26eeed0ad90e4e834058 From 32d49680d6088623f8d709a90247f65b4b81cfd6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 3 Oct 2024 07:59:28 +0200 Subject: [PATCH 10/16] fmt --- src/tools/miri/src/concurrency/init_once.rs | 3 ++- src/tools/miri/src/concurrency/sync.rs | 9 ++++--- src/tools/miri/src/helpers.rs | 24 ++++++++++--------- src/tools/miri/src/shims/time.rs | 3 ++- src/tools/miri/src/shims/unix/fd.rs | 3 ++- .../data_race/mixed_size_read_read_write.rs | 2 +- .../fail/data_race/mixed_size_read_write.rs | 2 +- 7 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/tools/miri/src/concurrency/init_once.rs b/src/tools/miri/src/concurrency/init_once.rs index 8985135f4e892..7a9b12bbe82c9 100644 --- a/src/tools/miri/src/concurrency/init_once.rs +++ b/src/tools/miri/src/concurrency/init_once.rs @@ -39,7 +39,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { |ecx| &mut ecx.machine.sync.init_onces, |_| interp_ok(Default::default()), )? - .ok_or_else(|| err_ub_format!("init_once has invalid ID")).into() + .ok_or_else(|| err_ub_format!("init_once has invalid ID")) + .into() } #[inline] diff --git a/src/tools/miri/src/concurrency/sync.rs b/src/tools/miri/src/concurrency/sync.rs index 3b57af641b589..5627ccdbbea27 100644 --- a/src/tools/miri/src/concurrency/sync.rs +++ b/src/tools/miri/src/concurrency/sync.rs @@ -307,7 +307,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { |ecx| &mut ecx.machine.sync.mutexes, |ecx| initialize_data(ecx).map(|data| Mutex { data, ..Default::default() }), )? - .ok_or_else(|| err_ub_format!("mutex has invalid ID")).into() + .ok_or_else(|| err_ub_format!("mutex has invalid ID")) + .into() } /// Retrieve the additional data stored for a mutex. @@ -334,7 +335,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { |ecx| &mut ecx.machine.sync.rwlocks, |ecx| initialize_data(ecx).map(|data| RwLock { data, ..Default::default() }), )? - .ok_or_else(|| err_ub_format!("rwlock has invalid ID")).into() + .ok_or_else(|| err_ub_format!("rwlock has invalid ID")) + .into() } /// Retrieve the additional data stored for a rwlock. @@ -375,7 +377,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { |ecx| &mut ecx.machine.sync.condvars, |ecx| initialize_data(ecx).map(|data| Condvar { data, ..Default::default() }), )? - .ok_or_else(|| err_ub_format!("condvar has invalid ID")).into() + .ok_or_else(|| err_ub_format!("condvar has invalid ID")) + .into() } /// Retrieve the additional data stored for a condvar. diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index b12a1939e2580..013bfe03aafa7 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/src/helpers.rs @@ -752,17 +752,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let nanoseconds_scalar = this.read_scalar(&nanoseconds_place)?; let nanoseconds = nanoseconds_scalar.to_target_isize(this)?; - interp_ok(try { - // tv_sec must be non-negative. - let seconds: u64 = seconds.try_into().ok()?; - // tv_nsec must be non-negative. - let nanoseconds: u32 = nanoseconds.try_into().ok()?; - if nanoseconds >= 1_000_000_000 { - // tv_nsec must not be greater than 999,999,999. - None? - } - Duration::new(seconds, nanoseconds) - }) + interp_ok( + try { + // tv_sec must be non-negative. + let seconds: u64 = seconds.try_into().ok()?; + // tv_nsec must be non-negative. + let nanoseconds: u32 = nanoseconds.try_into().ok()?; + if nanoseconds >= 1_000_000_000 { + // tv_nsec must not be greater than 999,999,999. + None? + } + Duration::new(seconds, nanoseconds) + }, + ) } /// Read bytes from a byte slice. diff --git a/src/tools/miri/src/shims/time.rs b/src/tools/miri/src/shims/time.rs index 21c5421f10941..12c7679608ded 100644 --- a/src/tools/miri/src/shims/time.rs +++ b/src/tools/miri/src/shims/time.rs @@ -11,7 +11,8 @@ use crate::*; /// Returns the time elapsed between the provided time and the unix epoch as a `Duration`. pub fn system_time_to_duration<'tcx>(time: &SystemTime) -> InterpResult<'tcx, Duration> { time.duration_since(SystemTime::UNIX_EPOCH) - .map_err(|_| err_unsup_format!("times before the Unix epoch are not supported")).into() + .map_err(|_| err_unsup_format!("times before the Unix epoch are not supported")) + .into() } impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index cf8e629cea1b1..f2d3115f98507 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -517,7 +517,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let start = this.read_scalar(&args[2])?.to_i32()?; match this.machine.fds.get(fd_num) { - Some(fd) => interp_ok(Scalar::from_i32(this.machine.fds.insert_with_min_num(fd, start))), + Some(fd) => + interp_ok(Scalar::from_i32(this.machine.fds.insert_with_min_num(fd, start))), None => interp_ok(Scalar::from_i32(this.fd_not_found()?)), } } else if this.tcx.sess.target.os == "macos" && cmd == this.eval_libc_i32("F_FULLFSYNC") { diff --git a/src/tools/miri/tests/fail/data_race/mixed_size_read_read_write.rs b/src/tools/miri/tests/fail/data_race/mixed_size_read_read_write.rs index ece5bd31274de..e76654806bb1e 100644 --- a/src/tools/miri/tests/fail/data_race/mixed_size_read_read_write.rs +++ b/src/tools/miri/tests/fail/data_race/mixed_size_read_read_write.rs @@ -4,7 +4,7 @@ // Two variants: the atomic store matches the size of the first or second atomic load. //@revisions: match_first_load match_second_load -use std::sync::atomic::{AtomicU16, AtomicU8, Ordering}; +use std::sync::atomic::{AtomicU8, AtomicU16, Ordering}; use std::thread; fn convert(a: &AtomicU16) -> &[AtomicU8; 2] { diff --git a/src/tools/miri/tests/fail/data_race/mixed_size_read_write.rs b/src/tools/miri/tests/fail/data_race/mixed_size_read_write.rs index acc12427b3fdc..53016bab78045 100644 --- a/src/tools/miri/tests/fail/data_race/mixed_size_read_write.rs +++ b/src/tools/miri/tests/fail/data_race/mixed_size_read_write.rs @@ -4,7 +4,7 @@ // Two revisions, depending on which access goes first. //@revisions: read_write write_read -use std::sync::atomic::{AtomicU16, AtomicU8, Ordering}; +use std::sync::atomic::{AtomicU8, AtomicU16, Ordering}; use std::thread; fn convert(a: &AtomicU16) -> &[AtomicU8; 2] { From 9b35886adfe5da4163e95508f586dc071eec4d0f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 3 Oct 2024 08:19:07 +0200 Subject: [PATCH 11/16] make sure we also detect mixed-size races that begin at different addresses --- .../mixed_size_write_write.fst.stderr | 22 +++++++++++++++++++ .../fail/data_race/mixed_size_write_write.rs | 4 +++- .../mixed_size_write_write.snd.stderr | 22 +++++++++++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 src/tools/miri/tests/fail/data_race/mixed_size_write_write.fst.stderr create mode 100644 src/tools/miri/tests/fail/data_race/mixed_size_write_write.snd.stderr diff --git a/src/tools/miri/tests/fail/data_race/mixed_size_write_write.fst.stderr b/src/tools/miri/tests/fail/data_race/mixed_size_write_write.fst.stderr new file mode 100644 index 0000000000000..a353910dcc998 --- /dev/null +++ b/src/tools/miri/tests/fail/data_race/mixed_size_write_write.fst.stderr @@ -0,0 +1,22 @@ +error: Undefined Behavior: Race condition detected between (1) 2-byte atomic store on thread `unnamed-ID` and (2) 1-byte atomic store on thread `unnamed-ID` at ALLOC. (2) just happened here + --> tests/fail/data_race/mixed_size_write_write.rs:LL:CC + | +LL | a8[idx].store(1, Ordering::SeqCst); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Race condition detected between (1) 2-byte atomic store on thread `unnamed-ID` and (2) 1-byte atomic store on thread `unnamed-ID` at ALLOC. (2) just happened here + | +help: and (1) occurred earlier here + --> tests/fail/data_race/mixed_size_write_write.rs:LL:CC + | +LL | a16.store(1, Ordering::SeqCst); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: overlapping unsynchronized atomic accesses must use the same access size + = help: see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE (of the first span) on thread `unnamed-ID`: + = note: inside closure at tests/fail/data_race/mixed_size_write_write.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/fail/data_race/mixed_size_write_write.rs b/src/tools/miri/tests/fail/data_race/mixed_size_write_write.rs index 89afda2fff5c2..545e354a0372c 100644 --- a/src/tools/miri/tests/fail/data_race/mixed_size_write_write.rs +++ b/src/tools/miri/tests/fail/data_race/mixed_size_write_write.rs @@ -1,6 +1,7 @@ //@compile-flags: -Zmiri-preemption-rate=0.0 -Zmiri-disable-weak-memory-emulation // Avoid accidental synchronization via address reuse inside `thread::spawn`. //@compile-flags: -Zmiri-address-reuse-cross-thread-rate=0 +//@revisions: fst snd use std::sync::atomic::{AtomicU8, AtomicU16, Ordering}; use std::thread; @@ -21,7 +22,8 @@ fn main() { a16.store(1, Ordering::SeqCst); }); s.spawn(|| { - a8[0].store(1, Ordering::SeqCst); + let idx = if cfg!(fst) { 0 } else { 1 }; + a8[idx].store(1, Ordering::SeqCst); //~^ ERROR: Race condition detected between (1) 2-byte atomic store on thread `unnamed-1` and (2) 1-byte atomic store on thread `unnamed-2` }); }); diff --git a/src/tools/miri/tests/fail/data_race/mixed_size_write_write.snd.stderr b/src/tools/miri/tests/fail/data_race/mixed_size_write_write.snd.stderr new file mode 100644 index 0000000000000..3b9c0491502a1 --- /dev/null +++ b/src/tools/miri/tests/fail/data_race/mixed_size_write_write.snd.stderr @@ -0,0 +1,22 @@ +error: Undefined Behavior: Race condition detected between (1) 2-byte atomic store on thread `unnamed-ID` and (2) 1-byte atomic store on thread `unnamed-ID` at ALLOC+0x1. (2) just happened here + --> tests/fail/data_race/mixed_size_write_write.rs:LL:CC + | +LL | a8[idx].store(1, Ordering::SeqCst); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Race condition detected between (1) 2-byte atomic store on thread `unnamed-ID` and (2) 1-byte atomic store on thread `unnamed-ID` at ALLOC+0x1. (2) just happened here + | +help: and (1) occurred earlier here + --> tests/fail/data_race/mixed_size_write_write.rs:LL:CC + | +LL | a16.store(1, Ordering::SeqCst); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = help: overlapping unsynchronized atomic accesses must use the same access size + = help: see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE (of the first span) on thread `unnamed-ID`: + = note: inside closure at tests/fail/data_race/mixed_size_write_write.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + From d19bd66a4e88116af18a1f5a6ab4ffe2f5bd1956 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Fri, 4 Oct 2024 05:00:12 +0000 Subject: [PATCH 12/16] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index fe0421b2075a5..eb4dfcf57cf00 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -ad9c494835e746fb7c8a26eeed0ad90e4e834058 +7067e4aee45c18cfa1c6af3bf79bd097684fb294 From dc88a6a788ebbcc2c342b2cfb584f48b2b78b43f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 4 Oct 2024 10:12:05 +0200 Subject: [PATCH 13/16] clippy --- src/tools/miri/cargo-miri/src/phases.rs | 1 - src/tools/miri/src/concurrency/vector_clock.rs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tools/miri/cargo-miri/src/phases.rs b/src/tools/miri/cargo-miri/src/phases.rs index 52bc8e1a3b6d7..f1f76fd338cea 100644 --- a/src/tools/miri/cargo-miri/src/phases.rs +++ b/src/tools/miri/cargo-miri/src/phases.rs @@ -668,7 +668,6 @@ pub fn phase_runner(mut binary_args: impl Iterator, phase: Runner RunnerPhase::Rustdoc => { cmd.stdin(std::process::Stdio::piped()); // the warning is wrong, we have a `wait` inside the `scope` closure. - #[expect(clippy::zombie_processes)] let mut child = cmd.spawn().expect("failed to spawn process"); let child_stdin = child.stdin.take().unwrap(); // Write stdin in a background thread, as it may block. diff --git a/src/tools/miri/src/concurrency/vector_clock.rs b/src/tools/miri/src/concurrency/vector_clock.rs index f9025e06c684e..345726634299b 100644 --- a/src/tools/miri/src/concurrency/vector_clock.rs +++ b/src/tools/miri/src/concurrency/vector_clock.rs @@ -151,7 +151,7 @@ impl VClock { /// Load the internal timestamp slice in the vector clock #[inline] pub(super) fn as_slice(&self) -> &[VTimestamp] { - debug_assert!(!self.0.last().is_some_and(|t| t.time() == 0)); + debug_assert!(self.0.last().is_none_or(|t| t.time() != 0)); self.0.as_slice() } From d00b754721a1cfb4feaa1e06d65c836f856e3c4e Mon Sep 17 00:00:00 2001 From: Tobias Decking Date: Tue, 17 Sep 2024 15:16:55 +0200 Subject: [PATCH 14/16] Implement LLVM x86 gfni intrinsics --- src/tools/miri/src/shims/x86/gfni.rs | 196 +++++++ src/tools/miri/src/shims/x86/mod.rs | 8 + .../pass/shims/x86/intrinsics-x86-gfni.rs | 518 ++++++++++++++++++ 3 files changed, 722 insertions(+) create mode 100644 src/tools/miri/src/shims/x86/gfni.rs create mode 100644 src/tools/miri/tests/pass/shims/x86/intrinsics-x86-gfni.rs diff --git a/src/tools/miri/src/shims/x86/gfni.rs b/src/tools/miri/src/shims/x86/gfni.rs new file mode 100644 index 0000000000000..c91b8c835f2e9 --- /dev/null +++ b/src/tools/miri/src/shims/x86/gfni.rs @@ -0,0 +1,196 @@ +use rustc_span::Symbol; +use rustc_target::spec::abi::Abi; + +use crate::*; + +impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} +pub(super) trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { + fn emulate_x86_gfni_intrinsic( + &mut self, + link_name: Symbol, + abi: Abi, + args: &[OpTy<'tcx>], + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx, EmulateItemResult> { + let this = self.eval_context_mut(); + + // Prefix should have already been checked. + let unprefixed_name = link_name.as_str().strip_prefix("llvm.x86.").unwrap(); + + this.expect_target_feature_for_intrinsic(link_name, "gfni")?; + if unprefixed_name.ends_with(".256") { + this.expect_target_feature_for_intrinsic(link_name, "avx")?; + } else if unprefixed_name.ends_with(".512") { + this.expect_target_feature_for_intrinsic(link_name, "avx512f")?; + } + + match unprefixed_name { + // Used to implement the `_mm{, 256, 512}_gf2p8affine_epi64_epi8` functions. + // See `affine_transform` for details. + // https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=gf2p8affine_ + "vgf2p8affineqb.128" | "vgf2p8affineqb.256" | "vgf2p8affineqb.512" => { + let [left, right, imm8] = + this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; + affine_transform(this, left, right, imm8, dest, /* inverse */ false)?; + } + // Used to implement the `_mm{, 256, 512}_gf2p8affineinv_epi64_epi8` functions. + // See `affine_transform` for details. + // https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=gf2p8affineinv + "vgf2p8affineinvqb.128" | "vgf2p8affineinvqb.256" | "vgf2p8affineinvqb.512" => { + let [left, right, imm8] = + this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; + affine_transform(this, left, right, imm8, dest, /* inverse */ true)?; + } + // Used to implement the `_mm{, 256, 512}_gf2p8mul_epi8` functions. + // Multiplies packed 8-bit integers in `left` and `right` in the finite field GF(2^8) + // and store the results in `dst`. The field GF(2^8) is represented in + // polynomial representation with the reduction polynomial x^8 + x^4 + x^3 + x + 1. + // https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=gf2p8mul + "vgf2p8mulb.128" | "vgf2p8mulb.256" | "vgf2p8mulb.512" => { + let [left, right] = + this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; + + let (left, left_len) = this.project_to_simd(left)?; + let (right, right_len) = this.project_to_simd(right)?; + let (dest, dest_len) = this.project_to_simd(dest)?; + + assert_eq!(left_len, right_len); + assert_eq!(dest_len, right_len); + + for i in 0..dest_len { + let left = this.read_scalar(&this.project_index(&left, i)?)?.to_u8()?; + let right = this.read_scalar(&this.project_index(&right, i)?)?.to_u8()?; + let dest = this.project_index(&dest, i)?; + this.write_scalar(Scalar::from_u8(gf2p8_mul(left, right)), &dest)?; + } + } + _ => return interp_ok(EmulateItemResult::NotSupported), + } + interp_ok(EmulateItemResult::NeedsReturn) + } +} + +/// Calculates the affine transformation `right * left + imm8` inside the finite field GF(2^8). +/// `right` is an 8x8 bit matrix, `left` and `imm8` are bit vectors. +/// If `inverse` is set, then the inverse transformation with respect to the reduction polynomial +/// x^8 + x^4 + x^3 + x + 1 is performed instead. +fn affine_transform<'tcx>( + this: &mut MiriInterpCx<'tcx>, + left: &OpTy<'tcx>, + right: &OpTy<'tcx>, + imm8: &OpTy<'tcx>, + dest: &MPlaceTy<'tcx>, + inverse: bool, +) -> InterpResult<'tcx, ()> { + let (left, left_len) = this.project_to_simd(left)?; + let (right, right_len) = this.project_to_simd(right)?; + let (dest, dest_len) = this.project_to_simd(dest)?; + + assert_eq!(dest_len, right_len); + assert_eq!(dest_len, left_len); + + let imm8 = this.read_scalar(imm8)?.to_u8()?; + + // Each 8x8 bit matrix gets multiplied with eight bit vectors. + // Therefore, the iteration is done in chunks of eight. + for i in (0..dest_len).step_by(8) { + // Get the bit matrix. + let mut matrix = [0u8; 8]; + for j in 0..8 { + matrix[usize::try_from(j).unwrap()] = + this.read_scalar(&this.project_index(&right, i.wrapping_add(j))?)?.to_u8()?; + } + + // Multiply the matrix with the vector and perform the addition. + for j in 0..8 { + let index = i.wrapping_add(j); + let left = this.read_scalar(&this.project_index(&left, index)?)?.to_u8()?; + let left = if inverse { TABLE[usize::from(left)] } else { left }; + + let mut res = 0; + + // Do the matrix multiplication. + for bit in 0u8..8 { + let mut b = matrix[usize::from(bit)] & left; + + // Calculate the parity bit. + b = (b & 0b1111) ^ (b >> 4); + b = (b & 0b11) ^ (b >> 2); + b = (b & 0b1) ^ (b >> 1); + + res |= b << 7u8.wrapping_sub(bit); + } + + // Perform the addition. + res ^= imm8; + + let dest = this.project_index(&dest, index)?; + this.write_scalar(Scalar::from_u8(res), &dest)?; + } + } + + interp_ok(()) +} + +/// A lookup table for computing the inverse byte for the inverse affine transformation. +// This is a evaluated at compile time. Trait based conversion is not available. +/// See for the +/// definition of `gf_inv` which was used for the creation of this table. +#[allow(clippy::cast_possible_truncation)] +static TABLE: [u8; 256] = { + let mut array = [0; 256]; + + let mut i = 1; + while i < 256 { + let mut x = i as u8; + let mut y = gf2p8_mul(x, x); + x = y; + let mut j = 2; + while j < 8 { + x = gf2p8_mul(x, x); + y = gf2p8_mul(x, y); + j += 1; + } + array[i] = y; + i += 1; + } + + array +}; + +/// Multiplies packed 8-bit integers in `left` and `right` in the finite field GF(2^8) +/// and store the results in `dst`. The field GF(2^8) is represented in +/// polynomial representation with the reduction polynomial x^8 + x^4 + x^3 + x + 1. +/// See for details. +// This is a const function. Trait based conversion is not available. +#[allow(clippy::cast_possible_truncation)] +const fn gf2p8_mul(left: u8, right: u8) -> u8 { + // This implementation is based on the `gf2p8mul_byte` definition found inside the Intel intrinsics guide. + // See https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=gf2p8mul + // for more information. + + const POLYNOMIAL: u32 = 0x11b; + + let left = left as u32; + let right = right as u32; + + let mut result = 0u32; + + let mut i = 0u32; + while i < 8 { + if left & (1 << i) != 0 { + result ^= right << i; + } + i = i.wrapping_add(1); + } + + let mut i = 14u32; + while i >= 8 { + if result & (1 << i) != 0 { + result ^= POLYNOMIAL << i.wrapping_sub(8); + } + i = i.wrapping_sub(1); + } + + result as u8 +} diff --git a/src/tools/miri/src/shims/x86/mod.rs b/src/tools/miri/src/shims/x86/mod.rs index 19c678cb7faca..9339d301aeecd 100644 --- a/src/tools/miri/src/shims/x86/mod.rs +++ b/src/tools/miri/src/shims/x86/mod.rs @@ -15,6 +15,7 @@ mod aesni; mod avx; mod avx2; mod bmi; +mod gfni; mod sha; mod sse; mod sse2; @@ -106,6 +107,13 @@ pub(super) trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this, link_name, abi, args, dest, ); } + // The GFNI extension does not get its own namespace. + // Check for instruction names instead. + name if name.starts_with("vgf2p8affine") || name.starts_with("vgf2p8mulb") => { + return gfni::EvalContextExt::emulate_x86_gfni_intrinsic( + this, link_name, abi, args, dest, + ); + } name if name.starts_with("sha") => { return sha::EvalContextExt::emulate_x86_sha_intrinsic( this, link_name, abi, args, dest, diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-gfni.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-gfni.rs new file mode 100644 index 0000000000000..a629e2acfe998 --- /dev/null +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-gfni.rs @@ -0,0 +1,518 @@ +// We're testing x86 target specific features +//@only-target: x86_64 i686 +//@compile-flags: -C target-feature=+gfni,+avx512f + +// The constants in the tests below are just bit patterns. They should not +// be interpreted as integers; signedness does not make sense for them, but +// __mXXXi happens to be defined in terms of signed integers. +#![allow(overflowing_literals)] +#![feature(avx512_target_feature)] +#![feature(stdarch_x86_avx512)] + +#[cfg(target_arch = "x86")] +use std::arch::x86::*; +#[cfg(target_arch = "x86_64")] +use std::arch::x86_64::*; +use std::hint::black_box; +use std::mem::{size_of, transmute}; + +const IDENTITY_BYTE: i32 = 0; +const CONSTANT_BYTE: i32 = 0x63; + +fn main() { + // Mostly copied from library/stdarch/crates/core_arch/src/x86/gfni.rs + + assert!(is_x86_feature_detected!("avx512f")); + assert!(is_x86_feature_detected!("gfni")); + + unsafe { + let byte_mul_test_data = generate_byte_mul_test_data(); + let affine_mul_test_data_identity = generate_affine_mul_test_data(IDENTITY_BYTE as u8); + let affine_mul_test_data_constant = generate_affine_mul_test_data(CONSTANT_BYTE as u8); + let inv_tests_data = generate_inv_tests_data(); + + test_mm512_gf2p8mul_epi8(&byte_mul_test_data); + test_mm256_gf2p8mul_epi8(&byte_mul_test_data); + test_mm_gf2p8mul_epi8(&byte_mul_test_data); + test_mm512_gf2p8affine_epi64_epi8(&byte_mul_test_data, &affine_mul_test_data_identity); + test_mm256_gf2p8affine_epi64_epi8(&byte_mul_test_data, &affine_mul_test_data_identity); + test_mm_gf2p8affine_epi64_epi8(&byte_mul_test_data, &affine_mul_test_data_identity); + test_mm512_gf2p8affineinv_epi64_epi8(&inv_tests_data, &affine_mul_test_data_constant); + test_mm256_gf2p8affineinv_epi64_epi8(&inv_tests_data, &affine_mul_test_data_constant); + test_mm_gf2p8affineinv_epi64_epi8(&inv_tests_data, &affine_mul_test_data_constant); + } +} + +#[target_feature(enable = "gfni,avx512f")] +unsafe fn test_mm512_gf2p8mul_epi8( + byte_mul_test_data: &([u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES]), +) { + let (left, right, expected) = byte_mul_test_data; + + for i in 0..NUM_TEST_WORDS_512 { + let left = load_m512i_word(left, i); + let right = load_m512i_word(right, i); + let expected = load_m512i_word(expected, i); + let result = _mm512_gf2p8mul_epi8(left, right); + assert_eq_m512i(result, expected); + } +} + +#[target_feature(enable = "gfni,avx")] +unsafe fn test_mm256_gf2p8mul_epi8( + byte_mul_test_data: &([u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES]), +) { + let (left, right, expected) = byte_mul_test_data; + + for i in 0..NUM_TEST_WORDS_256 { + let left = load_m256i_word(left, i); + let right = load_m256i_word(right, i); + let expected = load_m256i_word(expected, i); + let result = _mm256_gf2p8mul_epi8(left, right); + assert_eq_m256i(result, expected); + } +} + +#[target_feature(enable = "gfni")] +unsafe fn test_mm_gf2p8mul_epi8( + byte_mul_test_data: &([u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES]), +) { + let (left, right, expected) = byte_mul_test_data; + + for i in 0..NUM_TEST_WORDS_128 { + let left = load_m128i_word(left, i); + let right = load_m128i_word(right, i); + let expected = load_m128i_word(expected, i); + let result = _mm_gf2p8mul_epi8(left, right); + assert_eq_m128i(result, expected); + } +} + +#[target_feature(enable = "gfni,avx512f")] +unsafe fn test_mm512_gf2p8affine_epi64_epi8( + byte_mul_test_data: &([u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES]), + affine_mul_test_data_identity: &( + [u64; NUM_TEST_WORDS_64], + [u8; NUM_TEST_ENTRIES], + [u8; NUM_TEST_ENTRIES], + ), +) { + let identity: i64 = 0x01_02_04_08_10_20_40_80; + let constant: i64 = 0; + let identity = _mm512_set1_epi64(identity); + let constant = _mm512_set1_epi64(constant); + let constant_reference = _mm512_set1_epi8(CONSTANT_BYTE as i8); + + let (bytes, more_bytes, _) = byte_mul_test_data; + let (matrices, vectors, references) = affine_mul_test_data_identity; + + for i in 0..NUM_TEST_WORDS_512 { + let data = load_m512i_word(bytes, i); + let result = _mm512_gf2p8affine_epi64_epi8::(data, identity); + assert_eq_m512i(result, data); + let result = _mm512_gf2p8affine_epi64_epi8::(data, constant); + assert_eq_m512i(result, constant_reference); + let data = load_m512i_word(more_bytes, i); + let result = _mm512_gf2p8affine_epi64_epi8::(data, identity); + assert_eq_m512i(result, data); + let result = _mm512_gf2p8affine_epi64_epi8::(data, constant); + assert_eq_m512i(result, constant_reference); + + let matrix = load_m512i_word(matrices, i); + let vector = load_m512i_word(vectors, i); + let reference = load_m512i_word(references, i); + + let result = _mm512_gf2p8affine_epi64_epi8::(vector, matrix); + assert_eq_m512i(result, reference); + } +} + +#[target_feature(enable = "gfni,avx")] +unsafe fn test_mm256_gf2p8affine_epi64_epi8( + byte_mul_test_data: &([u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES]), + affine_mul_test_data_identity: &( + [u64; NUM_TEST_WORDS_64], + [u8; NUM_TEST_ENTRIES], + [u8; NUM_TEST_ENTRIES], + ), +) { + let identity: i64 = 0x01_02_04_08_10_20_40_80; + let constant: i64 = 0; + let identity = _mm256_set1_epi64x(identity); + let constant = _mm256_set1_epi64x(constant); + let constant_reference = _mm256_set1_epi8(CONSTANT_BYTE as i8); + + let (bytes, more_bytes, _) = byte_mul_test_data; + let (matrices, vectors, references) = affine_mul_test_data_identity; + + for i in 0..NUM_TEST_WORDS_256 { + let data = load_m256i_word(bytes, i); + let result = _mm256_gf2p8affine_epi64_epi8::(data, identity); + assert_eq_m256i(result, data); + let result = _mm256_gf2p8affine_epi64_epi8::(data, constant); + assert_eq_m256i(result, constant_reference); + let data = load_m256i_word(more_bytes, i); + let result = _mm256_gf2p8affine_epi64_epi8::(data, identity); + assert_eq_m256i(result, data); + let result = _mm256_gf2p8affine_epi64_epi8::(data, constant); + assert_eq_m256i(result, constant_reference); + + let matrix = load_m256i_word(matrices, i); + let vector = load_m256i_word(vectors, i); + let reference = load_m256i_word(references, i); + + let result = _mm256_gf2p8affine_epi64_epi8::(vector, matrix); + assert_eq_m256i(result, reference); + } +} + +#[target_feature(enable = "gfni")] +unsafe fn test_mm_gf2p8affine_epi64_epi8( + byte_mul_test_data: &([u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES]), + affine_mul_test_data_identity: &( + [u64; NUM_TEST_WORDS_64], + [u8; NUM_TEST_ENTRIES], + [u8; NUM_TEST_ENTRIES], + ), +) { + let identity: i64 = 0x01_02_04_08_10_20_40_80; + let constant: i64 = 0; + let identity = _mm_set1_epi64x(identity); + let constant = _mm_set1_epi64x(constant); + let constant_reference = _mm_set1_epi8(CONSTANT_BYTE as i8); + + let (bytes, more_bytes, _) = byte_mul_test_data; + let (matrices, vectors, references) = affine_mul_test_data_identity; + + for i in 0..NUM_TEST_WORDS_128 { + let data = load_m128i_word(bytes, i); + let result = _mm_gf2p8affine_epi64_epi8::(data, identity); + assert_eq_m128i(result, data); + let result = _mm_gf2p8affine_epi64_epi8::(data, constant); + assert_eq_m128i(result, constant_reference); + let data = load_m128i_word(more_bytes, i); + let result = _mm_gf2p8affine_epi64_epi8::(data, identity); + assert_eq_m128i(result, data); + let result = _mm_gf2p8affine_epi64_epi8::(data, constant); + assert_eq_m128i(result, constant_reference); + + let matrix = load_m128i_word(matrices, i); + let vector = load_m128i_word(vectors, i); + let reference = load_m128i_word(references, i); + + let result = _mm_gf2p8affine_epi64_epi8::(vector, matrix); + assert_eq_m128i(result, reference); + } +} + +#[target_feature(enable = "gfni,avx512f")] +unsafe fn test_mm512_gf2p8affineinv_epi64_epi8( + inv_tests_data: &([u8; NUM_BYTES], [u8; NUM_BYTES]), + affine_mul_test_data_constant: &( + [u64; NUM_TEST_WORDS_64], + [u8; NUM_TEST_ENTRIES], + [u8; NUM_TEST_ENTRIES], + ), +) { + let identity: i64 = 0x01_02_04_08_10_20_40_80; + let identity = _mm512_set1_epi64(identity); + + // validate inversion + let (inputs, results) = inv_tests_data; + + for i in 0..NUM_BYTES_WORDS_512 { + let input = load_m512i_word(inputs, i); + let reference = load_m512i_word(results, i); + let result = _mm512_gf2p8affineinv_epi64_epi8::(input, identity); + let remultiplied = _mm512_gf2p8mul_epi8(result, input); + assert_eq_m512i(remultiplied, reference); + } + + // validate subsequent affine operation + let (matrices, vectors, _affine_expected) = affine_mul_test_data_constant; + + for i in 0..NUM_TEST_WORDS_512 { + let vector = load_m512i_word(vectors, i); + let matrix = load_m512i_word(matrices, i); + + let inv_vec = _mm512_gf2p8affineinv_epi64_epi8::(vector, identity); + let reference = _mm512_gf2p8affine_epi64_epi8::(inv_vec, matrix); + let result = _mm512_gf2p8affineinv_epi64_epi8::(vector, matrix); + assert_eq_m512i(result, reference); + } + + // validate everything by virtue of checking against the AES SBox + const AES_S_BOX_MATRIX: i64 = 0xF1_E3_C7_8F_1F_3E_7C_F8; + let sbox_matrix = _mm512_set1_epi64(AES_S_BOX_MATRIX); + + for i in 0..NUM_BYTES_WORDS_512 { + let reference = load_m512i_word(&AES_S_BOX, i); + let input = load_m512i_word(inputs, i); + let result = _mm512_gf2p8affineinv_epi64_epi8::(input, sbox_matrix); + assert_eq_m512i(result, reference); + } +} + +#[target_feature(enable = "gfni,avx")] +unsafe fn test_mm256_gf2p8affineinv_epi64_epi8( + inv_tests_data: &([u8; NUM_BYTES], [u8; NUM_BYTES]), + affine_mul_test_data_constant: &( + [u64; NUM_TEST_WORDS_64], + [u8; NUM_TEST_ENTRIES], + [u8; NUM_TEST_ENTRIES], + ), +) { + let identity: i64 = 0x01_02_04_08_10_20_40_80; + let identity = _mm256_set1_epi64x(identity); + + // validate inversion + let (inputs, results) = inv_tests_data; + + for i in 0..NUM_BYTES_WORDS_256 { + let input = load_m256i_word(inputs, i); + let reference = load_m256i_word(results, i); + let result = _mm256_gf2p8affineinv_epi64_epi8::(input, identity); + let remultiplied = _mm256_gf2p8mul_epi8(result, input); + assert_eq_m256i(remultiplied, reference); + } + + // validate subsequent affine operation + let (matrices, vectors, _affine_expected) = affine_mul_test_data_constant; + + for i in 0..NUM_TEST_WORDS_256 { + let vector = load_m256i_word(vectors, i); + let matrix = load_m256i_word(matrices, i); + + let inv_vec = _mm256_gf2p8affineinv_epi64_epi8::(vector, identity); + let reference = _mm256_gf2p8affine_epi64_epi8::(inv_vec, matrix); + let result = _mm256_gf2p8affineinv_epi64_epi8::(vector, matrix); + assert_eq_m256i(result, reference); + } + + // validate everything by virtue of checking against the AES SBox + const AES_S_BOX_MATRIX: i64 = 0xF1_E3_C7_8F_1F_3E_7C_F8; + let sbox_matrix = _mm256_set1_epi64x(AES_S_BOX_MATRIX); + + for i in 0..NUM_BYTES_WORDS_256 { + let reference = load_m256i_word(&AES_S_BOX, i); + let input = load_m256i_word(inputs, i); + let result = _mm256_gf2p8affineinv_epi64_epi8::(input, sbox_matrix); + assert_eq_m256i(result, reference); + } +} + +#[target_feature(enable = "gfni")] +unsafe fn test_mm_gf2p8affineinv_epi64_epi8( + inv_tests_data: &([u8; NUM_BYTES], [u8; NUM_BYTES]), + affine_mul_test_data_constant: &( + [u64; NUM_TEST_WORDS_64], + [u8; NUM_TEST_ENTRIES], + [u8; NUM_TEST_ENTRIES], + ), +) { + let identity: i64 = 0x01_02_04_08_10_20_40_80; + let identity = _mm_set1_epi64x(identity); + + // validate inversion + let (inputs, results) = inv_tests_data; + + for i in 0..NUM_BYTES_WORDS_128 { + let input = load_m128i_word(inputs, i); + let reference = load_m128i_word(results, i); + let result = _mm_gf2p8affineinv_epi64_epi8::(input, identity); + let remultiplied = _mm_gf2p8mul_epi8(result, input); + assert_eq_m128i(remultiplied, reference); + } + + // validate subsequent affine operation + let (matrices, vectors, _affine_expected) = affine_mul_test_data_constant; + + for i in 0..NUM_TEST_WORDS_128 { + let vector = load_m128i_word(vectors, i); + let matrix = load_m128i_word(matrices, i); + + let inv_vec = _mm_gf2p8affineinv_epi64_epi8::(vector, identity); + let reference = _mm_gf2p8affine_epi64_epi8::(inv_vec, matrix); + let result = _mm_gf2p8affineinv_epi64_epi8::(vector, matrix); + assert_eq_m128i(result, reference); + } + + // validate everything by virtue of checking against the AES SBox + const AES_S_BOX_MATRIX: i64 = 0xF1_E3_C7_8F_1F_3E_7C_F8; + let sbox_matrix = _mm_set1_epi64x(AES_S_BOX_MATRIX); + + for i in 0..NUM_BYTES_WORDS_128 { + let reference = load_m128i_word(&AES_S_BOX, i); + let input = load_m128i_word(inputs, i); + let result = _mm_gf2p8affineinv_epi64_epi8::(input, sbox_matrix); + assert_eq_m128i(result, reference); + } +} + +/* Various utilities for processing SIMD values. */ + +#[target_feature(enable = "sse2")] +unsafe fn load_m128i_word(data: &[T], word_index: usize) -> __m128i { + let byte_offset = word_index * 16 / size_of::(); + let pointer = data.as_ptr().add(byte_offset) as *const __m128i; + _mm_loadu_si128(black_box(pointer)) +} + +#[target_feature(enable = "avx")] +unsafe fn load_m256i_word(data: &[T], word_index: usize) -> __m256i { + let byte_offset = word_index * 32 / size_of::(); + let pointer = data.as_ptr().add(byte_offset) as *const __m256i; + _mm256_loadu_si256(black_box(pointer)) +} + +#[target_feature(enable = "avx512f")] +unsafe fn load_m512i_word(data: &[T], word_index: usize) -> __m512i { + let byte_offset = word_index * 64 / size_of::(); + let pointer = data.as_ptr().add(byte_offset) as *const i32; + _mm512_loadu_si512(black_box(pointer)) +} + +#[track_caller] +#[target_feature(enable = "sse2")] +unsafe fn assert_eq_m128i(a: __m128i, b: __m128i) { + assert_eq!(transmute::<_, [u64; 2]>(a), transmute::<_, [u64; 2]>(b)) +} + +#[track_caller] +#[target_feature(enable = "avx")] +unsafe fn assert_eq_m256i(a: __m256i, b: __m256i) { + assert_eq!(transmute::<_, [u64; 4]>(a), transmute::<_, [u64; 4]>(b)) +} + +#[track_caller] +#[target_feature(enable = "avx512f")] +unsafe fn assert_eq_m512i(a: __m512i, b: __m512i) { + assert_eq!(transmute::<_, [u64; 8]>(a), transmute::<_, [u64; 8]>(b)) +} + +/* Software implementation of the hardware intrinsics. */ + +fn mulbyte(left: u8, right: u8) -> u8 { + // this implementation follows the description in + // https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm512_gf2p8mul_epi8 + const REDUCTION_POLYNOMIAL: u16 = 0x11b; + let left: u16 = left.into(); + let right: u16 = right.into(); + let mut carryless_product: u16 = 0; + + // Carryless multiplication + for i in 0..8 { + if ((left >> i) & 0x01) != 0 { + carryless_product ^= right << i; + } + } + + // reduction, adding in "0" where appropriate to clear out high bits + // note that REDUCTION_POLYNOMIAL is zero in this context + for i in (8..=14).rev() { + if ((carryless_product >> i) & 0x01) != 0 { + carryless_product ^= REDUCTION_POLYNOMIAL << (i - 8); + } + } + + carryless_product as u8 +} + +/// Calculates the bitwise XOR of all bits inside a byte. +fn parity(input: u8) -> u8 { + let mut accumulator = 0; + for i in 0..8 { + accumulator ^= (input >> i) & 0x01; + } + accumulator +} + +/// Calculates `matrix * x + b` inside the finite field GF(2). +fn mat_vec_multiply_affine(matrix: u64, x: u8, b: u8) -> u8 { + // this implementation follows the description in + // https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm_gf2p8affine_epi64_epi8 + let mut accumulator = 0; + + for bit in 0..8 { + accumulator |= parity(x & matrix.to_le_bytes()[bit]) << (7 - bit); + } + + accumulator ^ b +} + +/* Test data generation. */ + +const NUM_TEST_WORDS_512: usize = 4; +const NUM_TEST_WORDS_256: usize = NUM_TEST_WORDS_512 * 2; +const NUM_TEST_WORDS_128: usize = NUM_TEST_WORDS_256 * 2; +const NUM_TEST_ENTRIES: usize = NUM_TEST_WORDS_512 * 64; +const NUM_TEST_WORDS_64: usize = NUM_TEST_WORDS_128 * 2; +const NUM_BYTES: usize = 256; +const NUM_BYTES_WORDS_128: usize = NUM_BYTES / 16; +const NUM_BYTES_WORDS_256: usize = NUM_BYTES_WORDS_128 / 2; +const NUM_BYTES_WORDS_512: usize = NUM_BYTES_WORDS_256 / 2; + +fn generate_affine_mul_test_data( + immediate: u8, +) -> ([u64; NUM_TEST_WORDS_64], [u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES]) { + let mut left: [u64; NUM_TEST_WORDS_64] = [0; NUM_TEST_WORDS_64]; + let mut right: [u8; NUM_TEST_ENTRIES] = [0; NUM_TEST_ENTRIES]; + let mut result: [u8; NUM_TEST_ENTRIES] = [0; NUM_TEST_ENTRIES]; + + for i in 0..NUM_TEST_WORDS_64 { + left[i] = (i as u64) * 103 * 101; + for j in 0..8 { + let j64 = j as u64; + right[i * 8 + j] = ((left[i] + j64) % 256) as u8; + result[i * 8 + j] = mat_vec_multiply_affine(left[i], right[i * 8 + j], immediate); + } + } + + (left, right, result) +} + +fn generate_inv_tests_data() -> ([u8; NUM_BYTES], [u8; NUM_BYTES]) { + let mut input: [u8; NUM_BYTES] = [0; NUM_BYTES]; + let mut result: [u8; NUM_BYTES] = [0; NUM_BYTES]; + + for i in 0..NUM_BYTES { + input[i] = (i % 256) as u8; + result[i] = if i == 0 { 0 } else { 1 }; + } + + (input, result) +} + +const AES_S_BOX: [u8; NUM_BYTES] = [ + 0x63, 0x7c, 0x77, 0x7b, 0xf2, 0x6b, 0x6f, 0xc5, 0x30, 0x01, 0x67, 0x2b, 0xfe, 0xd7, 0xab, 0x76, + 0xca, 0x82, 0xc9, 0x7d, 0xfa, 0x59, 0x47, 0xf0, 0xad, 0xd4, 0xa2, 0xaf, 0x9c, 0xa4, 0x72, 0xc0, + 0xb7, 0xfd, 0x93, 0x26, 0x36, 0x3f, 0xf7, 0xcc, 0x34, 0xa5, 0xe5, 0xf1, 0x71, 0xd8, 0x31, 0x15, + 0x04, 0xc7, 0x23, 0xc3, 0x18, 0x96, 0x05, 0x9a, 0x07, 0x12, 0x80, 0xe2, 0xeb, 0x27, 0xb2, 0x75, + 0x09, 0x83, 0x2c, 0x1a, 0x1b, 0x6e, 0x5a, 0xa0, 0x52, 0x3b, 0xd6, 0xb3, 0x29, 0xe3, 0x2f, 0x84, + 0x53, 0xd1, 0x00, 0xed, 0x20, 0xfc, 0xb1, 0x5b, 0x6a, 0xcb, 0xbe, 0x39, 0x4a, 0x4c, 0x58, 0xcf, + 0xd0, 0xef, 0xaa, 0xfb, 0x43, 0x4d, 0x33, 0x85, 0x45, 0xf9, 0x02, 0x7f, 0x50, 0x3c, 0x9f, 0xa8, + 0x51, 0xa3, 0x40, 0x8f, 0x92, 0x9d, 0x38, 0xf5, 0xbc, 0xb6, 0xda, 0x21, 0x10, 0xff, 0xf3, 0xd2, + 0xcd, 0x0c, 0x13, 0xec, 0x5f, 0x97, 0x44, 0x17, 0xc4, 0xa7, 0x7e, 0x3d, 0x64, 0x5d, 0x19, 0x73, + 0x60, 0x81, 0x4f, 0xdc, 0x22, 0x2a, 0x90, 0x88, 0x46, 0xee, 0xb8, 0x14, 0xde, 0x5e, 0x0b, 0xdb, + 0xe0, 0x32, 0x3a, 0x0a, 0x49, 0x06, 0x24, 0x5c, 0xc2, 0xd3, 0xac, 0x62, 0x91, 0x95, 0xe4, 0x79, + 0xe7, 0xc8, 0x37, 0x6d, 0x8d, 0xd5, 0x4e, 0xa9, 0x6c, 0x56, 0xf4, 0xea, 0x65, 0x7a, 0xae, 0x08, + 0xba, 0x78, 0x25, 0x2e, 0x1c, 0xa6, 0xb4, 0xc6, 0xe8, 0xdd, 0x74, 0x1f, 0x4b, 0xbd, 0x8b, 0x8a, + 0x70, 0x3e, 0xb5, 0x66, 0x48, 0x03, 0xf6, 0x0e, 0x61, 0x35, 0x57, 0xb9, 0x86, 0xc1, 0x1d, 0x9e, + 0xe1, 0xf8, 0x98, 0x11, 0x69, 0xd9, 0x8e, 0x94, 0x9b, 0x1e, 0x87, 0xe9, 0xce, 0x55, 0x28, 0xdf, + 0x8c, 0xa1, 0x89, 0x0d, 0xbf, 0xe6, 0x42, 0x68, 0x41, 0x99, 0x2d, 0x0f, 0xb0, 0x54, 0xbb, 0x16, +]; + +fn generate_byte_mul_test_data() +-> ([u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES], [u8; NUM_TEST_ENTRIES]) { + let mut left: [u8; NUM_TEST_ENTRIES] = [0; NUM_TEST_ENTRIES]; + let mut right: [u8; NUM_TEST_ENTRIES] = [0; NUM_TEST_ENTRIES]; + let mut result: [u8; NUM_TEST_ENTRIES] = [0; NUM_TEST_ENTRIES]; + + for i in 0..NUM_TEST_ENTRIES { + left[i] = (i % 256) as u8; + right[i] = left[i].wrapping_mul(101); + result[i] = mulbyte(left[i], right[i]); + } + + (left, right, result) +} From f46e1624cbee1558cccf10f0b392ea22c4165e45 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Fri, 4 Oct 2024 20:11:26 +0200 Subject: [PATCH 15/16] Prefer refutable slice patterns over len check + index op --- .../src/borrow_tracker/tree_borrows/tree.rs | 5 ++-- src/tools/miri/src/shims/unix/fd.rs | 14 ++++----- src/tools/miri/src/shims/unix/fs.rs | 8 ++--- .../src/shims/unix/linux/foreign_items.rs | 10 +++---- src/tools/miri/src/shims/unix/linux/sync.rs | 30 +++++++++---------- 5 files changed, 34 insertions(+), 33 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs index 410f4a58ac530..15cefab1a68e7 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -859,14 +859,15 @@ impl Tree { ) -> Option { let node = self.nodes.get(idx).unwrap(); + let [child_idx] = node.children[..] else { return None }; + // We never want to replace the root node, as it is also kept in `root_ptr_tags`. - if node.children.len() != 1 || live.contains(&node.tag) || node.parent.is_none() { + if live.contains(&node.tag) || node.parent.is_none() { return None; } // Since protected nodes are never GC'd (see `borrow_tracker::FrameExtra::visit_provenance`), // we know that `node` is not protected because otherwise `live` would // have contained `node.tag`. - let child_idx = node.children[0]; let child = self.nodes.get(child_idx).unwrap(); // Check that for that one child, `can_be_replaced_by_child` holds for the permission // on all locations. diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index f2d3115f98507..34e29760da783 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -481,14 +481,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - if args.len() < 2 { + let [fd_num, cmd, ..] = args else { throw_ub_format!( "incorrect number of arguments for fcntl: got {}, expected at least 2", args.len() ); - } - let fd_num = this.read_scalar(&args[0])?.to_i32()?; - let cmd = this.read_scalar(&args[1])?.to_i32()?; + }; + let fd_num = this.read_scalar(fd_num)?.to_i32()?; + let cmd = this.read_scalar(cmd)?.to_i32()?; // We only support getting the flags for a descriptor. if cmd == this.eval_libc_i32("F_GETFD") { @@ -508,13 +508,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // because exec() isn't supported. The F_DUPFD and F_DUPFD_CLOEXEC commands only // differ in whether the FD_CLOEXEC flag is pre-set on the new file descriptor, // thus they can share the same implementation here. - if args.len() < 3 { + let [_, _, start, ..] = args else { throw_ub_format!( "incorrect number of arguments for fcntl with cmd=`F_DUPFD`/`F_DUPFD_CLOEXEC`: got {}, expected at least 3", args.len() ); - } - let start = this.read_scalar(&args[2])?.to_i32()?; + }; + let start = this.read_scalar(start)?.to_i32()?; match this.machine.fds.get(fd_num) { Some(fd) => diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index 2df178c4385af..6c9a2beac2d2a 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -433,18 +433,18 @@ fn maybe_sync_file( impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn open(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> { - if args.len() < 2 { + let [path_raw, flag, ..] = args else { throw_ub_format!( "incorrect number of arguments for `open`: got {}, expected at least 2", args.len() ); - } + }; let this = self.eval_context_mut(); - let path_raw = this.read_pointer(&args[0])?; + let path_raw = this.read_pointer(path_raw)?; let path = this.read_path_from_c_str(path_raw)?; - let flag = this.read_scalar(&args[1])?.to_i32()?; + let flag = this.read_scalar(flag)?.to_i32()?; let mut options = OpenOptions::new(); diff --git a/src/tools/miri/src/shims/unix/linux/foreign_items.rs b/src/tools/miri/src/shims/unix/linux/foreign_items.rs index 9726dac7e516b..4b5f3b6c81bae 100644 --- a/src/tools/miri/src/shims/unix/linux/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/linux/foreign_items.rs @@ -122,19 +122,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { id if id == sys_getrandom => { // Used by getrandom 0.1 // The first argument is the syscall id, so skip over it. - if args.len() < 4 { + let [_, ptr, len, flags, ..] = args else { throw_ub_format!( "incorrect number of arguments for `getrandom` syscall: got {}, expected at least 4", args.len() ); - } + }; - let ptr = this.read_pointer(&args[1])?; - let len = this.read_target_usize(&args[2])?; + let ptr = this.read_pointer(ptr)?; + let len = this.read_target_usize(len)?; // The only supported flags are GRND_RANDOM and GRND_NONBLOCK, // neither of which have any effect on our current PRNG. // See for a discussion of argument sizes. - let _flags = this.read_scalar(&args[3])?.to_i32()?; + let _flags = this.read_scalar(flags)?.to_i32()?; this.gen_random(ptr, len)?; this.write_scalar(Scalar::from_target_usize(len, this), dest)?; diff --git a/src/tools/miri/src/shims/unix/linux/sync.rs b/src/tools/miri/src/shims/unix/linux/sync.rs index 404479e76e39b..5833ec64fc68f 100644 --- a/src/tools/miri/src/shims/unix/linux/sync.rs +++ b/src/tools/miri/src/shims/unix/linux/sync.rs @@ -15,19 +15,19 @@ pub fn futex<'tcx>( // may or may not be left out from the `syscall()` call. // Therefore we don't use `check_arg_count` here, but only check for the // number of arguments to fall within a range. - if args.len() < 3 { + let [addr, op, val, ..] = args else { throw_ub_format!( "incorrect number of arguments for `futex` syscall: got {}, expected at least 3", args.len() ); - } + }; // The first three arguments (after the syscall number itself) are the same to all futex operations: // (int *addr, int op, int val). // We checked above that these definitely exist. - let addr = this.read_pointer(&args[0])?; - let op = this.read_scalar(&args[1])?.to_i32()?; - let val = this.read_scalar(&args[2])?.to_i32()?; + let addr = this.read_pointer(addr)?; + let op = this.read_scalar(op)?.to_i32()?; + let val = this.read_scalar(val)?.to_i32()?; // This is a vararg function so we have to bring our own type for this pointer. let addr = this.ptr_to_mplace(addr, this.machine.layouts.i32); @@ -55,15 +55,15 @@ pub fn futex<'tcx>( let wait_bitset = op & !futex_realtime == futex_wait_bitset; let bitset = if wait_bitset { - if args.len() < 6 { + let [_, _, _, timeout, uaddr2, bitset, ..] = args else { throw_ub_format!( "incorrect number of arguments for `futex` syscall with `op=FUTEX_WAIT_BITSET`: got {}, expected at least 6", args.len() ); - } - let _timeout = this.read_pointer(&args[3])?; - let _uaddr2 = this.read_pointer(&args[4])?; - this.read_scalar(&args[5])?.to_u32()? + }; + let _timeout = this.read_pointer(timeout)?; + let _uaddr2 = this.read_pointer(uaddr2)?; + this.read_scalar(bitset)?.to_u32()? } else { if args.len() < 4 { throw_ub_format!( @@ -183,15 +183,15 @@ pub fn futex<'tcx>( // Same as FUTEX_WAKE, but allows you to specify a bitset to select which threads to wake up. op if op == futex_wake || op == futex_wake_bitset => { let bitset = if op == futex_wake_bitset { - if args.len() < 6 { + let [_, _, _, timeout, uaddr2, bitset, ..] = args else { throw_ub_format!( "incorrect number of arguments for `futex` syscall with `op=FUTEX_WAKE_BITSET`: got {}, expected at least 6", args.len() ); - } - let _timeout = this.read_pointer(&args[3])?; - let _uaddr2 = this.read_pointer(&args[4])?; - this.read_scalar(&args[5])?.to_u32()? + }; + let _timeout = this.read_pointer(timeout)?; + let _uaddr2 = this.read_pointer(uaddr2)?; + this.read_scalar(bitset)?.to_u32()? } else { u32::MAX }; From 155380523c1fe0381255556d2b0588f9a0fdcacd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 5 Oct 2024 10:40:38 +0200 Subject: [PATCH 16/16] pthread mutex: better error in reentrant-locking-UB, also test PTHREAD_MUTEX_INITIALIZER --- src/tools/miri/src/shims/unix/sync.rs | 4 +++- ...ck.rs => libc_pthread_mutex_NULL_reentrant.rs} | 4 ++-- ...r => libc_pthread_mutex_NULL_reentrant.stderr} | 8 ++++---- ...rs => libc_pthread_mutex_default_reentrant.rs} | 9 +++++++-- ...> libc_pthread_mutex_default_reentrant.stderr} | 8 ++++---- ....rs => libc_pthread_mutex_normal_reentrant.rs} | 2 ++ ...=> libc_pthread_mutex_normal_reentrant.stderr} | 4 ++-- .../libc_pthread_mutex_staticinit_reentrant.rs | 12 ++++++++++++ ...libc_pthread_mutex_staticinit_reentrant.stderr | 15 +++++++++++++++ 9 files changed, 51 insertions(+), 15 deletions(-) rename src/tools/miri/tests/fail-dep/concurrency/{libc_pthread_mutex_NULL_deadlock.rs => libc_pthread_mutex_NULL_reentrant.rs} (72%) rename src/tools/miri/tests/fail-dep/concurrency/{libc_pthread_mutex_NULL_deadlock.stderr => libc_pthread_mutex_NULL_reentrant.stderr} (68%) rename src/tools/miri/tests/fail-dep/concurrency/{libc_pthread_mutex_default_deadlock.rs => libc_pthread_mutex_default_reentrant.rs} (52%) rename src/tools/miri/tests/fail-dep/concurrency/{libc_pthread_mutex_default_deadlock.stderr => libc_pthread_mutex_default_reentrant.stderr} (68%) rename src/tools/miri/tests/fail-dep/concurrency/{libc_pthread_mutex_normal_deadlock.rs => libc_pthread_mutex_normal_reentrant.rs} (81%) rename src/tools/miri/tests/fail-dep/concurrency/{libc_pthread_mutex_normal_deadlock.stderr => libc_pthread_mutex_normal_reentrant.stderr} (79%) create mode 100644 src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_staticinit_reentrant.rs create mode 100644 src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_staticinit_reentrant.stderr diff --git a/src/tools/miri/src/shims/unix/sync.rs b/src/tools/miri/src/shims/unix/sync.rs index 017291f81a22e..b05f340861e78 100644 --- a/src/tools/miri/src/shims/unix/sync.rs +++ b/src/tools/miri/src/shims/unix/sync.rs @@ -483,7 +483,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // Trying to acquire the same mutex again. match kind { MutexKind::Default => - throw_ub_format!("trying to acquire already locked default mutex"), + throw_ub_format!( + "trying to acquire default mutex already locked by the current thread" + ), MutexKind::Normal => throw_machine_stop!(TerminationInfo::Deadlock), MutexKind::ErrorCheck => this.eval_libc_i32("EDEADLK"), MutexKind::Recursive => { diff --git a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_deadlock.rs b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_reentrant.rs similarity index 72% rename from src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_deadlock.rs rename to src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_reentrant.rs index a79abe65328e0..f2df8bdca12bd 100644 --- a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_deadlock.rs +++ b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_reentrant.rs @@ -1,12 +1,12 @@ //@ignore-target: windows # No pthreads on Windows // -// Check that if we pass NULL attribute, then we get the default mutex type. +// Check that if we pass NULL attribute, then reentrant locking is UB. fn main() { unsafe { let mut mutex: libc::pthread_mutex_t = std::mem::zeroed(); assert_eq!(libc::pthread_mutex_init(&mut mutex as *mut _, std::ptr::null() as *const _), 0); assert_eq!(libc::pthread_mutex_lock(&mut mutex as *mut _), 0); - libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR: Undefined Behavior: trying to acquire already locked default mutex + libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR: already locked by the current thread } } diff --git a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_deadlock.stderr b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_reentrant.stderr similarity index 68% rename from src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_deadlock.stderr rename to src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_reentrant.stderr index e9961ed413d0c..9455e70437629 100644 --- a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_deadlock.stderr +++ b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_NULL_reentrant.stderr @@ -1,13 +1,13 @@ -error: Undefined Behavior: trying to acquire already locked default mutex - --> tests/fail-dep/concurrency/libc_pthread_mutex_NULL_deadlock.rs:LL:CC +error: Undefined Behavior: trying to acquire default mutex already locked by the current thread + --> tests/fail-dep/concurrency/libc_pthread_mutex_NULL_reentrant.rs:LL:CC | LL | libc::pthread_mutex_lock(&mut mutex as *mut _); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to acquire already locked default mutex + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to acquire default mutex already locked by the current thread | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information = note: BACKTRACE: - = note: inside `main` at tests/fail-dep/concurrency/libc_pthread_mutex_NULL_deadlock.rs:LL:CC + = note: inside `main` at tests/fail-dep/concurrency/libc_pthread_mutex_NULL_reentrant.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_deadlock.rs b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_reentrant.rs similarity index 52% rename from src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_deadlock.rs rename to src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_reentrant.rs index d9293f938b6aa..d2d0ffff07a92 100644 --- a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_deadlock.rs +++ b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_reentrant.rs @@ -1,6 +1,11 @@ //@ignore-target: windows # No pthreads on Windows // -// Check that if we do not set the mutex type, it is the default. +// Check that if we do not set the mutex type, it is UB to do reentrant locking. glibc apparently +// actually exploits this, see +// : +// one must actively call pthread_mutexattr_settype to disable lock elision. This means a call to +// pthread_mutexattr_settype(PTHREAD_MUTEX_NORMAL) makes a difference even if +// PTHREAD_MUTEX_NORMAL == PTHREAD_MUTEX_DEFAULT! fn main() { unsafe { @@ -9,6 +14,6 @@ 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: Undefined Behavior: trying to acquire already locked default mutex + libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR: already locked by the current thread } } diff --git a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_deadlock.stderr b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_reentrant.stderr similarity index 68% rename from src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_deadlock.stderr rename to src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_reentrant.stderr index a57d10753d90d..a9ffbde1b65cf 100644 --- a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_deadlock.stderr +++ b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_default_reentrant.stderr @@ -1,13 +1,13 @@ -error: Undefined Behavior: trying to acquire already locked default mutex - --> tests/fail-dep/concurrency/libc_pthread_mutex_default_deadlock.rs:LL:CC +error: Undefined Behavior: trying to acquire default mutex already locked by the current thread + --> tests/fail-dep/concurrency/libc_pthread_mutex_default_reentrant.rs:LL:CC | LL | libc::pthread_mutex_lock(&mut mutex as *mut _); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to acquire already locked default mutex + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to acquire default mutex already locked by the current thread | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information = note: BACKTRACE: - = note: inside `main` at tests/fail-dep/concurrency/libc_pthread_mutex_default_deadlock.rs:LL:CC + = note: inside `main` at tests/fail-dep/concurrency/libc_pthread_mutex_default_reentrant.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_deadlock.rs b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_reentrant.rs similarity index 81% rename from src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_deadlock.rs rename to src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_reentrant.rs index b38582482b88b..9a88639edf795 100644 --- a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_deadlock.rs +++ b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_reentrant.rs @@ -10,6 +10,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); + // A "normal" mutex properly tries to acquire the lock even if its is already held + // by the current thread -- and then we deadlock. libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR: deadlock: the evaluated program deadlocked } } diff --git a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_deadlock.stderr b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_reentrant.stderr similarity index 79% rename from src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_deadlock.stderr rename to src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_reentrant.stderr index 4337475963e4a..f20b26297e274 100644 --- a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_deadlock.stderr +++ b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_normal_reentrant.stderr @@ -1,11 +1,11 @@ error: deadlock: the evaluated program deadlocked - --> tests/fail-dep/concurrency/libc_pthread_mutex_normal_deadlock.rs:LL:CC + --> tests/fail-dep/concurrency/libc_pthread_mutex_normal_reentrant.rs:LL:CC | LL | libc::pthread_mutex_lock(&mut mutex as *mut _); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program deadlocked | = note: BACKTRACE: - = note: inside `main` at tests/fail-dep/concurrency/libc_pthread_mutex_normal_deadlock.rs:LL:CC + = note: inside `main` at tests/fail-dep/concurrency/libc_pthread_mutex_normal_reentrant.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace diff --git a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_staticinit_reentrant.rs b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_staticinit_reentrant.rs new file mode 100644 index 0000000000000..bd8aef787e6fb --- /dev/null +++ b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_staticinit_reentrant.rs @@ -0,0 +1,12 @@ +//@ignore-target: windows # No pthreads on Windows +// +// Check that if we use PTHREAD_MUTEX_INITIALIZER, then reentrant locking is UB. +// glibc apparently actually exploits this so we better catch it! + +fn main() { + unsafe { + let mut mutex: libc::pthread_mutex_t = libc::PTHREAD_MUTEX_INITIALIZER; + assert_eq!(libc::pthread_mutex_lock(&mut mutex as *mut _), 0); + libc::pthread_mutex_lock(&mut mutex as *mut _); //~ ERROR: already locked by the current thread + } +} diff --git a/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_staticinit_reentrant.stderr b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_staticinit_reentrant.stderr new file mode 100644 index 0000000000000..984bb07b72895 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/concurrency/libc_pthread_mutex_staticinit_reentrant.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: trying to acquire default mutex already locked by the current thread + --> tests/fail-dep/concurrency/libc_pthread_mutex_staticinit_reentrant.rs:LL:CC + | +LL | libc::pthread_mutex_lock(&mut mutex as *mut _); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to acquire default mutex already locked by the current thread + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at tests/fail-dep/concurrency/libc_pthread_mutex_staticinit_reentrant.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error +