From 95a51384820eb3d599618b4f74e5edf375885f40 Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Sat, 26 Oct 2024 16:35:56 +0200 Subject: [PATCH 01/44] Add benchmark showing effectivity of subtree skipping --- src/tools/miri/bench-cargo-miri/string-replace/Cargo.lock | 7 +++++++ src/tools/miri/bench-cargo-miri/string-replace/Cargo.toml | 8 ++++++++ src/tools/miri/bench-cargo-miri/string-replace/data.json | 1 + .../miri/bench-cargo-miri/string-replace/src/main.rs | 7 +++++++ 4 files changed, 23 insertions(+) create mode 100644 src/tools/miri/bench-cargo-miri/string-replace/Cargo.lock create mode 100644 src/tools/miri/bench-cargo-miri/string-replace/Cargo.toml create mode 100644 src/tools/miri/bench-cargo-miri/string-replace/data.json create mode 100644 src/tools/miri/bench-cargo-miri/string-replace/src/main.rs diff --git a/src/tools/miri/bench-cargo-miri/string-replace/Cargo.lock b/src/tools/miri/bench-cargo-miri/string-replace/Cargo.lock new file mode 100644 index 0000000000000..443115c126502 --- /dev/null +++ b/src/tools/miri/bench-cargo-miri/string-replace/Cargo.lock @@ -0,0 +1,7 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 3 + +[[package]] +name = "string-replace" +version = "0.1.0" diff --git a/src/tools/miri/bench-cargo-miri/string-replace/Cargo.toml b/src/tools/miri/bench-cargo-miri/string-replace/Cargo.toml new file mode 100644 index 0000000000000..f0785cd693ef9 --- /dev/null +++ b/src/tools/miri/bench-cargo-miri/string-replace/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "string-replace" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/src/tools/miri/bench-cargo-miri/string-replace/data.json b/src/tools/miri/bench-cargo-miri/string-replace/data.json new file mode 100644 index 0000000000000..7e074cd6954a7 --- /dev/null +++ b/src/tools/miri/bench-cargo-miri/string-replace/data.json @@ -0,0 +1 @@ +[{"_id":"6724e6fc58417687afba2b85","index":0,"guid":"5c1bd108-2ee2-40bd-bce8-895c206409df","isActive":true,"balance":"$2,927.88","picture":"http://placehold.it/32x32","age":40,"eyeColor":"green","name":"Wynn Bradshaw","gender":"male","company":"ROTODYNE","email":"wynnbradshaw@rotodyne.com","phone":"+1 (904) 559-3130","address":"287 Bergen Avenue, Sperryville, Alaska, 5392","about":"Adipisicing fugiat aute adipisicing qui esse cillum. Lorem consequat consectetur voluptate id pariatur nostrud incididunt aliquip incididunt laboris aliqua. Magna nulla adipisicing cupidatat ea velit aliquip magna duis duis sunt ipsum. Cillum labore mollit fugiat tempor dolor sit.\r\n","registered":"2017-01-26T01:28:10 -01:00","latitude":46.089504,"longitude":51.763723,"greeting":"Hello, Wynn Bradshaw! You have 6 unread messages.","favoriteFruit":"banana"},{"_id":"6724e6fce8619d86c0389ccf","index":1,"guid":"ced7fbb7-3b1a-419b-9fc7-d47582bbb3ea","isActive":true,"balance":"$1,856.38","picture":"http://placehold.it/32x32","age":21,"eyeColor":"brown","name":"Olsen Larsen","gender":"male","company":"VERBUS","email":"olsenlarsen@verbus.com","phone":"+1 (936) 480-3749","address":"370 Losee Terrace, Churchill, Maine, 4040","about":"Consequat Lorem in laboris fugiat veniam tempor eiusmod eu incididunt enim do et qui. Sit commodo eu excepteur cillum ex tempor commodo ex ex laboris esse. Aute aute nulla dolore dolor do. Irure esse proident nostrud non. Incididunt velit reprehenderit incididunt laboris do. Consequat nulla est id ex veniam tempor. Sit Lorem magna cillum aliquip irure quis sit minim anim.\r\n","registered":"2016-07-12T02:08:39 -02:00","latitude":-12.843628,"longitude":-124.143829,"greeting":"Hello, Olsen Larsen! You have 1 unread messages.","favoriteFruit":"apple"},{"_id":"6724e6fc01b471965ea560cf","index":2,"guid":"21fde9a3-13ba-46be-baed-fb503f668b9e","isActive":false,"balance":"$2,025.88","picture":"http://placehold.it/32x32","age":29,"eyeColor":"green","name":"Ramirez Kinney","gender":"male","company":"QUAREX","email":"ramirezkinney@quarex.com","phone":"+1 (852) 447-2930","address":"986 Cornelia Street, Oberlin, Texas, 362","about":"Minim ea proident quis eiusmod aliquip duis excepteur velit minim aute cupidatat. Esse qui ex aliquip laborum id reprehenderit. Anim dolore commodo deserunt laborum nulla duis. Sint quis anim mollit fugiat sit incididunt reprehenderit occaecat aliqua dolor. Ullamco ipsum eiusmod incididunt proident qui exercitation adipisicing voluptate elit aliquip. Tempor duis aute incididunt adipisicing.\r\n","registered":"2016-02-23T05:34:14 -01:00","latitude":-56.21645,"longitude":44.048129,"greeting":"Hello, Ramirez Kinney! You have 9 unread messages.","favoriteFruit":"banana"},{"_id":"6724e6fc3ea8e4182b9e170f","index":3,"guid":"46b20637-eecc-40db-87d7-03da9bcd1cea","isActive":true,"balance":"$3,399.31","picture":"http://placehold.it/32x32","age":39,"eyeColor":"brown","name":"Hansen Kaufman","gender":"male","company":"EVENTAGE","email":"hansenkaufman@eventage.com","phone":"+1 (827) 483-2303","address":"916 Brighton Court, Sunbury, New Mexico, 3804","about":"Nisi in voluptate aute ullamco ipsum proident fugiat veniam anim reprehenderit. In ad irure dolor labore culpa incididunt veniam mollit Lorem deserunt cupidatat incididunt. Aliquip aliquip proident ut culpa.\r\n","registered":"2023-10-18T07:03:48 -02:00","latitude":-40.239135,"longitude":49.802049,"greeting":"Hello, Hansen Kaufman! You have 10 unread messages.","favoriteFruit":"apple"},{"_id":"6724e6fc721f83a10cf2aa37","index":4,"guid":"3d23743b-1e82-474e-8f7a-855fa46170d1","isActive":false,"balance":"$1,967.87","picture":"http://placehold.it/32x32","age":35,"eyeColor":"green","name":"Imelda Stephens","gender":"female","company":"OHMNET","email":"imeldastephens@ohmnet.com","phone":"+1 (893) 523-2400","address":"391 Wilson Street, Glidden, Kansas, 7226","about":"Officia sunt magna adipisicing id exercitation deserunt deserunt aliquip excepteur Lorem enim fugiat. Nulla culpa ut cupidatat excepteur do deserunt labore id eu laboris ullamco adipisicing ad. Et non nisi adipisicing minim aliquip ea ut qui adipisicing do laboris ex dolore duis.\r\n","registered":"2020-10-20T07:03:38 -02:00","latitude":0.348698,"longitude":-157.961956,"greeting":"Hello, Imelda Stephens! You have 2 unread messages.","favoriteFruit":"strawberry"},{"_id":"6724e6fc7ad7274b9f4c406c","index":5,"guid":"626292b1-ae84-4887-9e29-78e548cd24e6","isActive":true,"balance":"$1,577.44","picture":"http://placehold.it/32x32","age":40,"eyeColor":"brown","name":"Lynne Jarvis","gender":"female","company":"CORECOM","email":"lynnejarvis@corecom.com","phone":"+1 (899) 556-3876","address":"465 National Drive, Davenport, Palau, 9786","about":"Aliquip elit dolore sint quis do laboris exercitation elit aliqua eiusmod. Excepteur ad aliqua eiusmod incididunt tempor laboris officia consectetur sit. Cupidatat voluptate deserunt ut consectetur qui laborum duis elit incididunt occaecat laborum. Mollit aute velit officia amet aute minim fugiat sit laborum Lorem deserunt in. Exercitation eu sunt nulla adipisicing quis ea aute est. Lorem ea cillum ad labore quis minim et est laboris deserunt proident. Amet ut tempor laborum occaecat exercitation ullamco laborum adipisicing fugiat ea voluptate quis fugiat.\r\n","registered":"2018-11-03T03:53:15 -01:00","latitude":89.827087,"longitude":-136.882799,"greeting":"Hello, Lynne Jarvis! You have 3 unread messages.","favoriteFruit":"strawberry"},{"_id":"6724e6fcef1a1db2cf170762","index":6,"guid":"b8777c06-b90f-49a4-8737-96712fc504a3","isActive":false,"balance":"$2,285.03","picture":"http://placehold.it/32x32","age":37,"eyeColor":"green","name":"Price Bolton","gender":"male","company":"IMANT","email":"pricebolton@imant.com","phone":"+1 (825) 424-2873","address":"237 Aberdeen Street, Sattley, Montana, 2918","about":"Non cillum irure fugiat consequat ad ex. Magna magna tempor excepteur irure quis. Duis in laboris ipsum adipisicing culpa magna reprehenderit nisi incididunt est veniam quis. Labore culpa ut culpa veniam est est consectetur ipsum ex esse.\r\n","registered":"2014-04-26T01:20:19 -02:00","latitude":70.349258,"longitude":126.810102,"greeting":"Hello, Price Bolton! You have 10 unread messages.","favoriteFruit":"banana"},{"_id":"6724e6fc8bcb952208c159f9","index":7,"guid":"a4e6c6c8-3fe3-42de-ae28-79c16956d309","isActive":false,"balance":"$1,298.07","picture":"http://placehold.it/32x32","age":28,"eyeColor":"blue","name":"Gretchen Wynn","gender":"female","company":"TERASCAPE","email":"gretchenwynn@terascape.com","phone":"+1 (882) 447-2895","address":"973 Suydam Place, Shindler, Nebraska, 8094","about":"Anim mollit labore magna proident ipsum culpa enim deserunt dolore sunt veniam fugiat. Ad fugiat cupidatat nisi commodo dolore duis commodo nostrud est. Enim proident ullamco non adipisicing magna consequat mollit ad reprehenderit laboris. Ex quis duis anim id non commodo amet sunt est magna officia.\r\n","registered":"2021-08-13T08:51:32 -02:00","latitude":14.551848,"longitude":-27.142242,"greeting":"Hello, Gretchen Wynn! You have 7 unread messages.","favoriteFruit":"apple"},{"_id":"6724e6fcc8243c2dfa47f5d4","index":8,"guid":"27df20d5-c1d8-419b-ad38-bdd1e6094775","isActive":true,"balance":"$3,005.40","picture":"http://placehold.it/32x32","age":33,"eyeColor":"blue","name":"Chen Travis","gender":"male","company":"MEMORA","email":"chentravis@memora.com","phone":"+1 (980) 500-2406","address":"182 Dahlgreen Place, Baker, South Carolina, 9817","about":"Ad nisi consequat aliquip eiusmod aute pariatur est sint magna. Ad magna anim esse qui Lorem nulla veniam dolore eiusmod. Cillum consequat sit aliqua est proident exercitation eiusmod irure. Minim eu laboris ad incididunt enim sunt. Sunt in excepteur aute non tempor irure mollit laboris. Eu et duis ullamco dolor sint occaecat officia culpa ipsum anim anim eu veniam aliquip. Exercitation ipsum dolor sint cillum duis incididunt minim quis irure enim reprehenderit do do incididunt.\r\n","registered":"2019-09-01T02:57:37 -02:00","latitude":25.442301,"longitude":48.381036,"greeting":"Hello, Chen Travis! You have 1 unread messages.","favoriteFruit":"banana"}] \ No newline at end of file diff --git a/src/tools/miri/bench-cargo-miri/string-replace/src/main.rs b/src/tools/miri/bench-cargo-miri/string-replace/src/main.rs new file mode 100644 index 0000000000000..73bf4a850e487 --- /dev/null +++ b/src/tools/miri/bench-cargo-miri/string-replace/src/main.rs @@ -0,0 +1,7 @@ +const TCB_INFO_JSON: &str = include_str!("../data.json"); + +fn main() { + let tcb_json = TCB_INFO_JSON; + let bad_tcb_json = tcb_json.replace("female", "male"); + std::hint::black_box(bad_tcb_json); +} From 440080407219f4ad413fcc4819323d0ba920c440 Mon Sep 17 00:00:00 2001 From: tiif Date: Mon, 18 Nov 2024 02:17:36 +0800 Subject: [PATCH 02/44] Refactor AnonSocket::read/write --- .../miri/src/shims/unix/unnamed_socket.rs | 124 +++++++++++------- 1 file changed, 80 insertions(+), 44 deletions(-) diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index 8ccce7c198679..36575f4b5fb0a 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -146,8 +146,7 @@ impl FileDescription for AnonSocket { // corresponding ErrorKind variant. throw_unsup_format!("reading from the write end of a pipe"); }; - let mut readbuf = readbuf.borrow_mut(); - if readbuf.buf.is_empty() { + if readbuf.borrow().buf.is_empty() { if self.peer_fd().upgrade().is_none() { // Socketpair with no peer and empty buffer. // 0 bytes successfully read indicates end-of-file. @@ -167,31 +166,8 @@ impl FileDescription for AnonSocket { } } } - - // Synchronize with all previous writes to this buffer. - // FIXME: this over-synchronizes; a more precise approach would be to - // only sync with the writes whose data we will read. - ecx.acquire_clock(&readbuf.clock); - - // Do full read / partial read based on the space available. - // Conveniently, `read` exists on `VecDeque` and has exactly the desired behavior. - let actual_read_size = readbuf.buf.read(&mut bytes).unwrap(); - - // Need to drop before others can access the readbuf again. - drop(readbuf); - - // A notification should be provided for the peer file description even when it can - // only write 1 byte. This implementation is not compliant with the actual Linux kernel - // implementation. For optimization reasons, the kernel will only mark the file description - // as "writable" when it can write more than a certain number of bytes. Since we - // don't know what that *certain number* is, we will provide a notification every time - // a read is successful. This might result in our epoll emulation providing more - // notifications than the real system. - if let Some(peer_fd) = self.peer_fd().upgrade() { - ecx.check_and_update_readiness(&peer_fd)?; - } - - ecx.return_read_success(ptr, &bytes, actual_read_size, dest) + // TODO: We might need to decide what to do if peer_fd is closed when read is blocked. + anonsocket_read(self, self.peer_fd().upgrade(), &mut bytes, ptr, dest, ecx) } fn write<'tcx>( @@ -221,9 +197,8 @@ impl FileDescription for AnonSocket { // corresponding ErrorKind variant. throw_unsup_format!("writing to the reading end of a pipe"); }; - let mut writebuf = writebuf.borrow_mut(); - let data_size = writebuf.buf.len(); - let available_space = MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(data_size); + let available_space = + MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(writebuf.borrow().buf.len()); if available_space == 0 { if self.is_nonblock { // Non-blocking socketpair with a full buffer. @@ -233,24 +208,85 @@ impl FileDescription for AnonSocket { throw_unsup_format!("socketpair/pipe/pipe2 write: blocking isn't supported yet"); } } - // Remember this clock so `read` can synchronize with us. - ecx.release_clock(|clock| { - writebuf.clock.join(clock); - }); - // Do full write / partial write based on the space available. - let actual_write_size = len.min(available_space); - let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; - writebuf.buf.extend(&bytes[..actual_write_size]); + anonsocket_write(available_space, &peer_fd, ptr, len, dest, ecx) + } +} - // Need to stop accessing peer_fd so that it can be notified. - drop(writebuf); +/// Write to AnonSocket based on the space available and return the written byte size. +fn anonsocket_write<'tcx>( + available_space: usize, + peer_fd: &FileDescriptionRef, + ptr: Pointer, + len: usize, + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, +) -> InterpResult<'tcx> { + let Some(writebuf) = &peer_fd.downcast::().unwrap().readbuf else { + // FIXME: This should return EBADF, but there's no nice way to do that as there's no + // corresponding ErrorKind variant. + throw_unsup_format!("writing to the reading end of a pipe") + }; + let mut writebuf = writebuf.borrow_mut(); + + // Remember this clock so `read` can synchronize with us. + ecx.release_clock(|clock| { + writebuf.clock.join(clock); + }); + // Do full write / partial write based on the space available. + let actual_write_size = len.min(available_space); + let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; + writebuf.buf.extend(&bytes[..actual_write_size]); + + // Need to stop accessing peer_fd so that it can be notified. + drop(writebuf); + + // Notification should be provided for peer fd as it became readable. + // The kernel does this even if the fd was already readable before, so we follow suit. + ecx.check_and_update_readiness(peer_fd)?; + + ecx.return_write_success(actual_write_size, dest) +} - // Notification should be provided for peer fd as it became readable. - // The kernel does this even if the fd was already readable before, so we follow suit. +/// Read from AnonSocket and return the number of bytes read. +fn anonsocket_read<'tcx>( + anonsocket: &AnonSocket, + peer_fd: Option, + bytes: &mut [u8], + ptr: Pointer, + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, +) -> InterpResult<'tcx> { + let Some(readbuf) = &anonsocket.readbuf else { + // FIXME: This should return EBADF, but there's no nice way to do that as there's no + // corresponding ErrorKind variant. + throw_unsup_format!("reading from the write end of a pipe") + }; + let mut readbuf = readbuf.borrow_mut(); + + // Synchronize with all previous writes to this buffer. + // FIXME: this over-synchronizes; a more precise approach would be to + // only sync with the writes whose data we will read. + ecx.acquire_clock(&readbuf.clock); + + // Do full read / partial read based on the space available. + // Conveniently, `read` exists on `VecDeque` and has exactly the desired behavior. + let actual_read_size = readbuf.buf.read(bytes).unwrap(); + + // Need to drop before others can access the readbuf again. + drop(readbuf); + + // A notification should be provided for the peer file description even when it can + // only write 1 byte. This implementation is not compliant with the actual Linux kernel + // implementation. For optimization reasons, the kernel will only mark the file description + // as "writable" when it can write more than a certain number of bytes. Since we + // don't know what that *certain number* is, we will provide a notification every time + // a read is successful. This might result in our epoll emulation providing more + // notifications than the real system. + if let Some(peer_fd) = peer_fd { ecx.check_and_update_readiness(&peer_fd)?; - - ecx.return_write_success(actual_write_size, dest) } + + ecx.return_read_success(ptr, bytes, actual_read_size, dest) } impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} From 6af006e988a8a522fbfac8dfdb6540fe001ebdf4 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 21 Nov 2024 11:44:30 -0800 Subject: [PATCH 03/44] Fill out Windows error mapping table --- src/tools/miri/src/shims/io_error.rs | 63 ++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/src/tools/miri/src/shims/io_error.rs b/src/tools/miri/src/shims/io_error.rs index 0cbb4850b7fd9..c29469f1eddb2 100644 --- a/src/tools/miri/src/shims/io_error.rs +++ b/src/tools/miri/src/shims/io_error.rs @@ -82,11 +82,68 @@ const UNIX_IO_ERROR_TABLE: &[(&str, std::io::ErrorKind)] = { // . 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), + ("WSAEADDRINUSE", AddrInUse), + ("WSAEADDRNOTAVAIL", AddrNotAvailable), + ("ERROR_ALREADY_EXISTS", AlreadyExists), + ("ERROR_FILE_EXISTS", AlreadyExists), + ("ERROR_NO_DATA", BrokenPipe), + ("WSAECONNABORTED", ConnectionAborted), + ("WSAECONNREFUSED", ConnectionRefused), + ("WSAECONNRESET", ConnectionReset), + ("ERROR_NOT_SAME_DEVICE", CrossesDevices), + ("ERROR_POSSIBLE_DEADLOCK", Deadlock), + ("ERROR_DIR_NOT_EMPTY", DirectoryNotEmpty), + ("ERROR_CANT_RESOLVE_FILENAME", FilesystemLoop), + ("ERROR_DISK_QUOTA_EXCEEDED", FilesystemQuotaExceeded), + ("WSAEDQUOT", FilesystemQuotaExceeded), + ("ERROR_FILE_TOO_LARGE", FileTooLarge), + ("ERROR_HOST_UNREACHABLE", HostUnreachable), + ("WSAEHOSTUNREACH", HostUnreachable), + ("ERROR_INVALID_NAME", InvalidFilename), + ("ERROR_BAD_PATHNAME", InvalidFilename), + ("ERROR_FILENAME_EXCED_RANGE", InvalidFilename), ("ERROR_INVALID_PARAMETER", InvalidInput), + ("WSAEINVAL", InvalidInput), + ("ERROR_DIRECTORY_NOT_SUPPORTED", IsADirectory), + ("WSAENETDOWN", NetworkDown), + ("ERROR_NETWORK_UNREACHABLE", NetworkUnreachable), + ("WSAENETUNREACH", NetworkUnreachable), + ("ERROR_DIRECTORY", NotADirectory), + ("WSAENOTCONN", NotConnected), + ("ERROR_FILE_NOT_FOUND", NotFound), + ("ERROR_PATH_NOT_FOUND", NotFound), + ("ERROR_INVALID_DRIVE", NotFound), + ("ERROR_BAD_NETPATH", NotFound), + ("ERROR_BAD_NET_NAME", NotFound), + ("ERROR_SEEK_ON_DEVICE", NotSeekable), + ("ERROR_NOT_ENOUGH_MEMORY", OutOfMemory), + ("ERROR_OUTOFMEMORY", OutOfMemory), + ("ERROR_ACCESS_DENIED", PermissionDenied), + ("WSAEACCES", PermissionDenied), + ("ERROR_WRITE_PROTECT", ReadOnlyFilesystem), + ("ERROR_BUSY", ResourceBusy), + ("ERROR_DISK_FULL", StorageFull), + ("ERROR_HANDLE_DISK_FULL", StorageFull), + ("WAIT_TIMEOUT", TimedOut), + ("WSAETIMEDOUT", TimedOut), + ("ERROR_DRIVER_CANCEL_TIMEOUT", TimedOut), + ("ERROR_OPERATION_ABORTED", TimedOut), + ("ERROR_SERVICE_REQUEST_TIMEOUT", TimedOut), + ("ERROR_COUNTER_TIMEOUT", TimedOut), + ("ERROR_TIMEOUT", TimedOut), + ("ERROR_RESOURCE_CALL_TIMED_OUT", TimedOut), + ("ERROR_CTX_MODEM_RESPONSE_TIMEOUT", TimedOut), + ("ERROR_CTX_CLIENT_QUERY_TIMEOUT", TimedOut), + ("FRS_ERR_SYSVOL_POPULATE_TIMEOUT", TimedOut), + ("ERROR_DS_TIMELIMIT_EXCEEDED", TimedOut), + ("DNS_ERROR_RECORD_TIMED_OUT", TimedOut), + ("ERROR_IPSEC_IKE_TIMED_OUT", TimedOut), + ("ERROR_RUNLEVEL_SWITCH_TIMEOUT", TimedOut), + ("ERROR_RUNLEVEL_SWITCH_AGENT_TIMEOUT", TimedOut), + ("ERROR_TOO_MANY_LINKS", TooManyLinks), + ("ERROR_CALL_NOT_IMPLEMENTED", Unsupported), + ("WSAEWOULDBLOCK", WouldBlock), ] }; From 7d38c51555cf4c77858f399846b6ca20d7d29ede Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 21 Nov 2024 22:27:43 +0100 Subject: [PATCH 04/44] eventfd: comment tweaks --- src/tools/miri/src/shims/unix/linux/eventfd.rs | 18 +++++++++++------- .../fail-dep/libc/eventfd_block_read_twice.rs | 2 +- .../fail-dep/libc/eventfd_block_write_twice.rs | 2 +- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/tools/miri/src/shims/unix/linux/eventfd.rs b/src/tools/miri/src/shims/unix/linux/eventfd.rs index 63b7d37b13e1f..2ec3d792d9421 100644 --- a/src/tools/miri/src/shims/unix/linux/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux/eventfd.rs @@ -211,10 +211,10 @@ fn eventfd_write<'tcx>( eventfd.clock.borrow_mut().join(clock); }); - // When this function is called, the addition is guaranteed to not exceed u64::MAX - 1. + // Store new counter value. eventfd.counter.set(new_count); - // When any of the event happened, we check and update the status of all supported event + // The state changed; we check and update the status of all supported event // types for current file description. ecx.check_and_update_readiness(&eventfd_ref)?; @@ -228,10 +228,11 @@ fn eventfd_write<'tcx>( ecx.unblock_thread(thread_id, BlockReason::Eventfd)?; } - // Return how many bytes we wrote. + // Return how many bytes we consumed from the user-provided buffer. return ecx.write_int(buf_place.layout.size.bytes(), dest); } None | Some(u64::MAX) => { + // We can't update the state, so we have to block. if eventfd.is_nonblock { return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest); } @@ -251,6 +252,7 @@ fn eventfd_write<'tcx>( weak_eventfd: WeakFileDescriptionRef, } @unblock = |this| { + // When we get unblocked, try again. eventfd_write(num, buf_place, &dest, weak_eventfd, this) } ), @@ -276,9 +278,10 @@ fn eventfd_read<'tcx>( // an eventfd file description. let eventfd = eventfd_ref.downcast::().unwrap(); - // Block when counter == 0. + // Set counter to 0, get old value. let counter = eventfd.counter.replace(0); + // Block when counter == 0. if counter == 0 { if eventfd.is_nonblock { return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest); @@ -297,6 +300,7 @@ fn eventfd_read<'tcx>( weak_eventfd: WeakFileDescriptionRef, } @unblock = |this| { + // When we get unblocked, try again. eventfd_read(buf_place, &dest, weak_eventfd, this) } ), @@ -305,10 +309,10 @@ fn eventfd_read<'tcx>( // Synchronize with all prior `write` calls to this FD. ecx.acquire_clock(&eventfd.clock.borrow()); - // Give old counter value to userspace, and set counter value to 0. + // Return old counter value into user-space buffer. ecx.write_int(counter, &buf_place)?; - // When any of the events happened, we check and update the status of all supported event + // The state changed; we check and update the status of all supported event // types for current file description. ecx.check_and_update_readiness(&eventfd_ref)?; @@ -322,7 +326,7 @@ fn eventfd_read<'tcx>( ecx.unblock_thread(thread_id, BlockReason::Eventfd)?; } - // Tell userspace how many bytes we read. + // Tell userspace how many bytes we put into the buffer. return ecx.write_int(buf_place.layout.size.bytes(), dest); } interp_ok(()) diff --git a/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs b/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs index 65d29b2c6bad1..111ca86a9b6a7 100644 --- a/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs +++ b/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs @@ -14,7 +14,7 @@ use std::thread; // 2. Thread 2 blocks. // 3. Thread 3 unblocks both thread 1 and thread 2. // 4. Thread 1 reads. -// 5. Thread 2's `read` deadlocked. +// 5. Thread 2's `read` can neber complete -> deadlocked. fn main() { // eventfd write will block when EFD_NONBLOCK flag is clear diff --git a/src/tools/miri/tests/fail-dep/libc/eventfd_block_write_twice.rs b/src/tools/miri/tests/fail-dep/libc/eventfd_block_write_twice.rs index f9d34d2fb58a2..2212d6c7eb5b2 100644 --- a/src/tools/miri/tests/fail-dep/libc/eventfd_block_write_twice.rs +++ b/src/tools/miri/tests/fail-dep/libc/eventfd_block_write_twice.rs @@ -14,7 +14,7 @@ use std::thread; // 2. Thread 2 blocks. // 3. Thread 3 unblocks both thread 1 and thread 2. // 4. Thread 1 writes u64::MAX. -// 5. Thread 2's `write` deadlocked. +// 5. Thread 2's `write` can never complete -> deadlocked. fn main() { // eventfd write will block when EFD_NONBLOCK flag is clear // and the addition caused counter to exceed u64::MAX - 1. From 0a7e63a529c0c97e100f040fd34217e4b9d8143d Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 21 Nov 2024 13:36:42 -0800 Subject: [PATCH 05/44] Add comment about multiple errors to one ErrorKind --- src/tools/miri/src/shims/io_error.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/tools/miri/src/shims/io_error.rs b/src/tools/miri/src/shims/io_error.rs index c29469f1eddb2..f67787e83f6d3 100644 --- a/src/tools/miri/src/shims/io_error.rs +++ b/src/tools/miri/src/shims/io_error.rs @@ -82,6 +82,10 @@ const UNIX_IO_ERROR_TABLE: &[(&str, std::io::ErrorKind)] = { // . const WINDOWS_IO_ERROR_TABLE: &[(&str, std::io::ErrorKind)] = { use std::io::ErrorKind::*; + // It's common for multiple error codes to map to the same io::ErrorKind. We have all for the + // forwards mapping; only the first one will be used for the backwards mapping. + // Slightly arbitrarily, we prefer non-WSA and the most generic sounding variant for backwards + // mapping. &[ ("WSAEADDRINUSE", AddrInUse), ("WSAEADDRNOTAVAIL", AddrNotAvailable), From f77817a701c291b94fe6da25c4865bc50a8fcb1d Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Fri, 22 Nov 2024 05:20:39 +0000 Subject: [PATCH 06/44] 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 effed0cd180d1..e5812ae260303 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -2d0ea7956c45de6e421fd579e2ded27be405dec6 +5d3c6ee9b34989595d2a72b79e61ca37e949d757 From 33f4d2ee61cedc12dd8119894c15e169b075cc46 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 21 Nov 2024 22:28:50 +0100 Subject: [PATCH 07/44] epoll: fix comment typo --- src/tools/miri/src/shims/unix/linux/epoll.rs | 2 +- src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs | 2 +- 2 files changed, 2 insertions(+), 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 de108665e9f8c..b20b12528db95 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -539,7 +539,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { )?; if is_updated { // Edge-triggered notification only notify one thread even if there are - // multiple threads block on the same epfd. + // multiple threads blocked on the same epfd. // This unwrap can never fail because if the current epoll instance were // closed, the upgrade of weak_epoll_interest diff --git a/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs b/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs index 111ca86a9b6a7..a9bb4c7eba8ba 100644 --- a/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs +++ b/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs @@ -14,7 +14,7 @@ use std::thread; // 2. Thread 2 blocks. // 3. Thread 3 unblocks both thread 1 and thread 2. // 4. Thread 1 reads. -// 5. Thread 2's `read` can neber complete -> deadlocked. +// 5. Thread 2's `read` can never complete -> deadlocked. fn main() { // eventfd write will block when EFD_NONBLOCK flag is clear From 7dcba03d28ee69764e5a9492d1f1f2518e36834f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 22 Nov 2024 07:37:38 +0100 Subject: [PATCH 08/44] disable solaris on CI for now --- src/tools/miri/ci/ci.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/ci/ci.sh b/src/tools/miri/ci/ci.sh index 0356d7ecf1015..9b020d029a684 100755 --- a/src/tools/miri/ci/ci.sh +++ b/src/tools/miri/ci/ci.sh @@ -153,7 +153,7 @@ case $HOST_TARGET in TEST_TARGET=x86_64-unknown-freebsd run_tests_minimal $BASIC $UNIX time hashmap random threadname pthread fs libc-pipe TEST_TARGET=i686-unknown-freebsd run_tests_minimal $BASIC $UNIX time hashmap random threadname pthread fs libc-pipe TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe - TEST_TARGET=x86_64-pc-solaris run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe + # TEST_TARGET=x86_64-pc-solaris run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe TEST_TARGET=aarch64-linux-android run_tests_minimal $BASIC $UNIX time hashmap random sync threadname pthread TEST_TARGET=wasm32-wasip2 run_tests_minimal $BASIC wasm TEST_TARGET=wasm32-unknown-unknown run_tests_minimal no_std empty_main wasm # this target doesn't really have std From 004edb27cf319b3d844f3bce397aa1079235a390 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Fri, 22 Nov 2024 08:08:34 +0000 Subject: [PATCH 09/44] sysconf interception fix for solarish systems. also adding the `_SC_PAGESIZE` alias `_SC_PAGE_SIZE` supported by Linux, macOS and FreeBSD. close #4050 --- src/tools/miri/ci/ci.sh | 2 +- .../src/shims/unix/android/foreign_items.rs | 9 +++ .../miri/src/shims/unix/foreign_items.rs | 58 +++++++++---------- .../src/shims/unix/freebsd/foreign_items.rs | 9 +++ .../src/shims/unix/linux/foreign_items.rs | 9 +++ .../src/shims/unix/macos/foreign_items.rs | 8 +++ .../src/shims/unix/solarish/foreign_items.rs | 8 +++ .../miri/tests/pass-dep/libc/libc-sysconf.rs | 17 ++++++ 8 files changed, 90 insertions(+), 30 deletions(-) create mode 100644 src/tools/miri/tests/pass-dep/libc/libc-sysconf.rs diff --git a/src/tools/miri/ci/ci.sh b/src/tools/miri/ci/ci.sh index 9b020d029a684..0356d7ecf1015 100755 --- a/src/tools/miri/ci/ci.sh +++ b/src/tools/miri/ci/ci.sh @@ -153,7 +153,7 @@ case $HOST_TARGET in TEST_TARGET=x86_64-unknown-freebsd run_tests_minimal $BASIC $UNIX time hashmap random threadname pthread fs libc-pipe TEST_TARGET=i686-unknown-freebsd run_tests_minimal $BASIC $UNIX time hashmap random threadname pthread fs libc-pipe TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe - # TEST_TARGET=x86_64-pc-solaris run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe + TEST_TARGET=x86_64-pc-solaris run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe TEST_TARGET=aarch64-linux-android run_tests_minimal $BASIC $UNIX time hashmap random sync threadname pthread TEST_TARGET=wasm32-wasip2 run_tests_minimal $BASIC wasm TEST_TARGET=wasm32-unknown-unknown run_tests_minimal no_std empty_main wasm # this target doesn't really have std diff --git a/src/tools/miri/src/shims/unix/android/foreign_items.rs b/src/tools/miri/src/shims/unix/android/foreign_items.rs index 80ad40e1624f7..6c5a61437aff8 100644 --- a/src/tools/miri/src/shims/unix/android/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/android/foreign_items.rs @@ -2,6 +2,7 @@ use rustc_abi::ExternAbi; use rustc_span::Symbol; use crate::shims::unix::android::thread::prctl; +use crate::shims::unix::foreign_items::EvalContextExt as _; use crate::shims::unix::linux::syscall::syscall; use crate::*; @@ -20,6 +21,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx, EmulateItemResult> { let this = self.eval_context_mut(); match link_name.as_str() { + // Querying system information + "sysconf" => { + let [val] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.sysconf(val)?; + this.write_scalar(result, dest)?; + } + // Miscellaneous "__errno" => { let [] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; diff --git a/src/tools/miri/src/shims/unix/foreign_items.rs b/src/tools/miri/src/shims/unix/foreign_items.rs index 55202a0814922..a50ac1be77ea1 100644 --- a/src/tools/miri/src/shims/unix/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/foreign_items.rs @@ -39,6 +39,35 @@ pub fn is_dyn_sym(name: &str, target_os: &str) -> bool { impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { + // Querying system information + fn sysconf(&mut self, val: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { + let this = self.eval_context_mut(); + + let name = this.read_scalar(val)?.to_i32()?; + // FIXME: Which of these are POSIX, and which are GNU/Linux? + // At least the names seem to all also exist on macOS. + let sysconfs: &[(&str, fn(&MiriInterpCx<'_>) -> Scalar)] = &[ + ("_SC_PAGESIZE", |this| Scalar::from_int(this.machine.page_size, this.pointer_size())), + ("_SC_PAGE_SIZE", |this| Scalar::from_int(this.machine.page_size, this.pointer_size())), + ("_SC_NPROCESSORS_CONF", |this| { + Scalar::from_int(this.machine.num_cpus, this.pointer_size()) + }), + ("_SC_NPROCESSORS_ONLN", |this| { + Scalar::from_int(this.machine.num_cpus, this.pointer_size()) + }), + // 512 seems to be a reasonable default. The value is not critical, in + // the sense that getpwuid_r takes and checks the buffer length. + ("_SC_GETPW_R_SIZE_MAX", |this| Scalar::from_int(512, this.pointer_size())), + ]; + for &(sysconf_name, value) in sysconfs { + let sysconf_name = this.eval_libc_i32(sysconf_name); + if sysconf_name == name { + return interp_ok(value(this)); + } + } + throw_unsup_format!("unimplemented sysconf name: {}", name) + } + fn emulate_foreign_item_inner( &mut self, link_name: Symbol, @@ -393,35 +422,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } } - // Querying system information - "sysconf" => { - let [name] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let name = this.read_scalar(name)?.to_i32()?; - // FIXME: Which of these are POSIX, and which are GNU/Linux? - // At least the names seem to all also exist on macOS. - let sysconfs: &[(&str, fn(&MiriInterpCx<'_>) -> Scalar)] = &[ - ("_SC_PAGESIZE", |this| Scalar::from_int(this.machine.page_size, this.pointer_size())), - ("_SC_NPROCESSORS_CONF", |this| Scalar::from_int(this.machine.num_cpus, this.pointer_size())), - ("_SC_NPROCESSORS_ONLN", |this| Scalar::from_int(this.machine.num_cpus, this.pointer_size())), - // 512 seems to be a reasonable default. The value is not critical, in - // the sense that getpwuid_r takes and checks the buffer length. - ("_SC_GETPW_R_SIZE_MAX", |this| Scalar::from_int(512, this.pointer_size())) - ]; - let mut result = None; - for &(sysconf_name, value) in sysconfs { - let sysconf_name = this.eval_libc_i32(sysconf_name); - if sysconf_name == name { - result = Some(value(this)); - break; - } - } - if let Some(result) = result { - this.write_scalar(result, dest)?; - } else { - throw_unsup_format!("unimplemented sysconf name: {}", name) - } - } - // Thread-local storage "pthread_key_create" => { let [key, dtor] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; diff --git a/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs b/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs index 1346d8de7eaa2..ddea9ecc294dd 100644 --- a/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs @@ -1,6 +1,7 @@ use rustc_abi::ExternAbi; use rustc_span::Symbol; +use crate::shims::unix::foreign_items::EvalContextExt as _; use crate::shims::unix::*; use crate::*; @@ -75,6 +76,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(result, dest)?; } + // Querying system information + "sysconf" => { + let [val] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.sysconf(val)?; + this.write_scalar(result, dest)?; + } + // Miscellaneous "__error" => { let [] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; 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 85f0d6e13307f..75b0afcac0039 100644 --- a/src/tools/miri/src/shims/unix/linux/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/linux/foreign_items.rs @@ -6,6 +6,7 @@ use self::shims::unix::linux::eventfd::EvalContextExt as _; use self::shims::unix::linux::mem::EvalContextExt as _; use self::shims::unix::linux::syscall::syscall; use crate::machine::{SIGRTMAX, SIGRTMIN}; +use crate::shims::unix::foreign_items::EvalContextExt as _; use crate::shims::unix::*; use crate::*; @@ -124,6 +125,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(result, dest)?; } + // Querying system information + "sysconf" => { + let [val] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.sysconf(val)?; + this.write_scalar(result, dest)?; + } + // Dynamically invoked syscalls "syscall" => { syscall(this, link_name, abi, args, dest)?; diff --git a/src/tools/miri/src/shims/unix/macos/foreign_items.rs b/src/tools/miri/src/shims/unix/macos/foreign_items.rs index 003025916cd42..7f67a2cab369a 100644 --- a/src/tools/miri/src/shims/unix/macos/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/macos/foreign_items.rs @@ -2,6 +2,7 @@ use rustc_abi::ExternAbi; use rustc_span::Symbol; use super::sync::EvalContextExt as _; +use crate::shims::unix::foreign_items::EvalContextExt as _; use crate::shims::unix::*; use crate::*; @@ -167,6 +168,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(stack_size, dest)?; } + "sysconf" => { + let [val] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.sysconf(val)?; + this.write_scalar(result, dest)?; + } + // Threading "pthread_setname_np" => { let [name] = diff --git a/src/tools/miri/src/shims/unix/solarish/foreign_items.rs b/src/tools/miri/src/shims/unix/solarish/foreign_items.rs index 526b64cff695a..2eeab38e35cd9 100644 --- a/src/tools/miri/src/shims/unix/solarish/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/solarish/foreign_items.rs @@ -1,6 +1,7 @@ use rustc_abi::ExternAbi; use rustc_span::Symbol; +use crate::shims::unix::foreign_items::EvalContextExt as _; use crate::shims::unix::*; use crate::*; @@ -112,6 +113,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_null(dest)?; } + "sysconf" | "__sysconf_xpg7" => { + let [val] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.sysconf(val)?; + this.write_scalar(result, dest)?; + } + _ => return interp_ok(EmulateItemResult::NotSupported), } interp_ok(EmulateItemResult::NeedsReturn) diff --git a/src/tools/miri/tests/pass-dep/libc/libc-sysconf.rs b/src/tools/miri/tests/pass-dep/libc/libc-sysconf.rs new file mode 100644 index 0000000000000..34d5b1e38a631 --- /dev/null +++ b/src/tools/miri/tests/pass-dep/libc/libc-sysconf.rs @@ -0,0 +1,17 @@ +//@ignore-target: windows # Supported only on unixes + +fn test_sysconfbasic() { + unsafe { + let ncpus = libc::sysconf(libc::_SC_NPROCESSORS_CONF); + assert!(ncpus >= 1); + let psz = libc::sysconf(libc::_SC_PAGESIZE); + assert!(psz % 4096 == 0); + // note that in reality it can return -1 (no hard limit) on some platforms. + let gwmax = libc::sysconf(libc::_SC_GETPW_R_SIZE_MAX); + assert!(gwmax >= 512); + } +} + +fn main() { + test_sysconfbasic(); +} From bb55f5239a237f0b37973d49a357b4443446d5f0 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sat, 23 Nov 2024 11:55:26 +0000 Subject: [PATCH 10/44] follow-up on #4052, making a miri evaluation context fn for strerror_r. --- .../src/shims/unix/android/foreign_items.rs | 9 ---- .../miri/src/shims/unix/foreign_items.rs | 54 +++++++++++++------ .../src/shims/unix/freebsd/foreign_items.rs | 9 ---- .../src/shims/unix/linux/foreign_items.rs | 14 +++-- .../src/shims/unix/macos/foreign_items.rs | 8 --- .../src/shims/unix/solarish/foreign_items.rs | 2 +- .../tests/pass-dep/libc/libc-strerror_r.rs | 16 ++++++ 7 files changed, 62 insertions(+), 50 deletions(-) create mode 100644 src/tools/miri/tests/pass-dep/libc/libc-strerror_r.rs diff --git a/src/tools/miri/src/shims/unix/android/foreign_items.rs b/src/tools/miri/src/shims/unix/android/foreign_items.rs index 6c5a61437aff8..80ad40e1624f7 100644 --- a/src/tools/miri/src/shims/unix/android/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/android/foreign_items.rs @@ -2,7 +2,6 @@ use rustc_abi::ExternAbi; use rustc_span::Symbol; use crate::shims::unix::android::thread::prctl; -use crate::shims::unix::foreign_items::EvalContextExt as _; use crate::shims::unix::linux::syscall::syscall; use crate::*; @@ -21,14 +20,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx, EmulateItemResult> { let this = self.eval_context_mut(); match link_name.as_str() { - // Querying system information - "sysconf" => { - let [val] = - this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let result = this.sysconf(val)?; - this.write_scalar(result, dest)?; - } - // Miscellaneous "__errno" => { let [] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; diff --git a/src/tools/miri/src/shims/unix/foreign_items.rs b/src/tools/miri/src/shims/unix/foreign_items.rs index a50ac1be77ea1..5594bd4e7906a 100644 --- a/src/tools/miri/src/shims/unix/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/foreign_items.rs @@ -68,6 +68,30 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_unsup_format!("unimplemented sysconf name: {}", name) } + fn strerror_r( + &mut self, + errnum: &OpTy<'tcx>, + buf: &OpTy<'tcx>, + buflen: &OpTy<'tcx>, + ) -> InterpResult<'tcx, Scalar> { + let this = self.eval_context_mut(); + + let errnum = this.read_scalar(errnum)?; + let buf = this.read_pointer(buf)?; + let buflen = this.read_target_usize(buflen)?; + let error = this.try_errnum_to_io_error(errnum)?; + let formatted = match error { + Some(err) => format!("{err}"), + None => format!(""), + }; + let (complete, _) = this.write_os_str_to_c_str(OsStr::new(&formatted), buf, buflen)?; + if complete { + interp_ok(Scalar::from_i32(0)) + } else { + interp_ok(Scalar::from_i32(this.eval_libc_i32("ERANGE"))) + } + } + fn emulate_foreign_item_inner( &mut self, link_name: Symbol, @@ -113,6 +137,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(result, dest)?; } + "sysconf" => { + let [val] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.sysconf(val)?; + this.write_scalar(result, dest)?; + } + // File descriptors "read" => { let [fd, buf, count] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; @@ -724,21 +755,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // We do not support forking, so there is nothing to do here. this.write_null(dest)?; } - "strerror_r" | "__xpg_strerror_r" => { - let [errnum, buf, buflen] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let errnum = this.read_scalar(errnum)?; - let buf = this.read_pointer(buf)?; - let buflen = this.read_target_usize(buflen)?; - - let error = this.try_errnum_to_io_error(errnum)?; - let formatted = match error { - Some(err) => format!("{err}"), - None => format!(""), - }; - let (complete, _) = this.write_os_str_to_c_str(OsStr::new(&formatted), buf, buflen)?; - let ret = if complete { 0 } else { this.eval_libc_i32("ERANGE") }; - this.write_int(ret, dest)?; - } "getentropy" => { // This function is non-standard but exists with the same signature and behavior on // Linux, macOS, FreeBSD and Solaris/Illumos. @@ -766,6 +782,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_null(dest)?; } } + + "strerror_r" => { + let [errnum, buf, buflen] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.strerror_r(errnum, buf, buflen)?; + this.write_scalar(result, dest)?; + } + "getrandom" => { // This function is non-standard but exists with the same signature and behavior on // Linux, FreeBSD and Solaris/Illumos. diff --git a/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs b/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs index ddea9ecc294dd..1346d8de7eaa2 100644 --- a/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs @@ -1,7 +1,6 @@ use rustc_abi::ExternAbi; use rustc_span::Symbol; -use crate::shims::unix::foreign_items::EvalContextExt as _; use crate::shims::unix::*; use crate::*; @@ -76,14 +75,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(result, dest)?; } - // Querying system information - "sysconf" => { - let [val] = - this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let result = this.sysconf(val)?; - this.write_scalar(result, dest)?; - } - // Miscellaneous "__error" => { let [] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; 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 75b0afcac0039..810e8d3340adb 100644 --- a/src/tools/miri/src/shims/unix/linux/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/linux/foreign_items.rs @@ -125,14 +125,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(result, dest)?; } - // Querying system information - "sysconf" => { - let [val] = - this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let result = this.sysconf(val)?; - this.write_scalar(result, dest)?; - } - // Dynamically invoked syscalls "syscall" => { syscall(this, link_name, abi, args, dest)?; @@ -152,6 +144,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let ptr = this.mremap(old_address, old_size, new_size, flags)?; this.write_scalar(ptr, dest)?; } + "__xpg_strerror_r" => { + let [errnum, buf, buflen] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.strerror_r(errnum, buf, buflen)?; + this.write_scalar(result, dest)?; + } "__errno_location" => { let [] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; let errno_place = this.last_error_place()?; diff --git a/src/tools/miri/src/shims/unix/macos/foreign_items.rs b/src/tools/miri/src/shims/unix/macos/foreign_items.rs index 7f67a2cab369a..003025916cd42 100644 --- a/src/tools/miri/src/shims/unix/macos/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/macos/foreign_items.rs @@ -2,7 +2,6 @@ use rustc_abi::ExternAbi; use rustc_span::Symbol; use super::sync::EvalContextExt as _; -use crate::shims::unix::foreign_items::EvalContextExt as _; use crate::shims::unix::*; use crate::*; @@ -168,13 +167,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(stack_size, dest)?; } - "sysconf" => { - let [val] = - this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let result = this.sysconf(val)?; - this.write_scalar(result, dest)?; - } - // Threading "pthread_setname_np" => { let [name] = diff --git a/src/tools/miri/src/shims/unix/solarish/foreign_items.rs b/src/tools/miri/src/shims/unix/solarish/foreign_items.rs index 2eeab38e35cd9..1bbd25617e556 100644 --- a/src/tools/miri/src/shims/unix/solarish/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/solarish/foreign_items.rs @@ -113,7 +113,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_null(dest)?; } - "sysconf" | "__sysconf_xpg7" => { + "__sysconf_xpg7" => { let [val] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; let result = this.sysconf(val)?; diff --git a/src/tools/miri/tests/pass-dep/libc/libc-strerror_r.rs b/src/tools/miri/tests/pass-dep/libc/libc-strerror_r.rs new file mode 100644 index 0000000000000..09885ce839d0f --- /dev/null +++ b/src/tools/miri/tests/pass-dep/libc/libc-strerror_r.rs @@ -0,0 +1,16 @@ +//@ignore-target: windows # Supported only on unixes + +fn main() { + unsafe { + let mut buf = vec![0u8; 32]; + assert_eq!(libc::strerror_r(libc::EPERM, buf.as_mut_ptr().cast(), buf.len()), 0); + let mut buf2 = vec![0u8; 64]; + assert_eq!(libc::strerror_r(-1i32, buf2.as_mut_ptr().cast(), buf2.len()), 0); + // This buffer is deliberately too small so this triggers ERANGE. + let mut buf3 = vec![0u8; 2]; + assert_eq!( + libc::strerror_r(libc::E2BIG, buf3.as_mut_ptr().cast(), buf3.len()), + libc::ERANGE + ); + } +} From 1deb8f9ec1b8a08b76c5bf4ca8b63dd1702e644b Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sat, 23 Nov 2024 09:53:45 +0000 Subject: [PATCH 11/44] sysconf: add _SC_OPEN_MAX --- src/tools/miri/src/shims/unix/foreign_items.rs | 5 +++++ src/tools/miri/tests/pass-dep/libc/libc-sysconf.rs | 2 ++ 2 files changed, 7 insertions(+) diff --git a/src/tools/miri/src/shims/unix/foreign_items.rs b/src/tools/miri/src/shims/unix/foreign_items.rs index 5594bd4e7906a..88ec32808b1c7 100644 --- a/src/tools/miri/src/shims/unix/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/foreign_items.rs @@ -58,6 +58,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // 512 seems to be a reasonable default. The value is not critical, in // the sense that getpwuid_r takes and checks the buffer length. ("_SC_GETPW_R_SIZE_MAX", |this| Scalar::from_int(512, this.pointer_size())), + // Miri doesn't have a fixed limit on FDs, but we may be limited in terms of how + // many *host* FDs we can open. Just use some arbitrary, pretty big value; + // this can be adjusted if it causes problems. + // The spec imposes a minimum of `_POSIX_OPEN_MAX` (20). + ("_SC_OPEN_MAX", |this| Scalar::from_int(2_i32.pow(16), this.pointer_size())), ]; for &(sysconf_name, value) in sysconfs { let sysconf_name = this.eval_libc_i32(sysconf_name); diff --git a/src/tools/miri/tests/pass-dep/libc/libc-sysconf.rs b/src/tools/miri/tests/pass-dep/libc/libc-sysconf.rs index 34d5b1e38a631..b832b3033b76e 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-sysconf.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-sysconf.rs @@ -9,6 +9,8 @@ fn test_sysconfbasic() { // note that in reality it can return -1 (no hard limit) on some platforms. let gwmax = libc::sysconf(libc::_SC_GETPW_R_SIZE_MAX); assert!(gwmax >= 512); + let omax = libc::sysconf(libc::_SC_OPEN_MAX); + assert_eq!(omax, 65536); } } From 57c66159dae0e89930f75a0f60aa578bc7a86111 Mon Sep 17 00:00:00 2001 From: Yoh Deadfall Date: Wed, 6 Nov 2024 21:54:18 +0300 Subject: [PATCH 12/44] Added epoll and eventfd for Android --- src/tools/miri/ci/ci.sh | 2 +- .../src/shims/unix/android/foreign_items.rs | 27 +++++++++++++++++++ .../miri/src/shims/unix/linux/eventfd.rs | 3 --- .../fail-dep/libc/libc-epoll-data-race.rs | 1 + .../pass-dep/libc/libc-epoll-blocking.rs | 1 + .../pass-dep/libc/libc-epoll-no-blocking.rs | 1 + .../miri/tests/pass-dep/libc/libc-eventfd.rs | 1 + 7 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/tools/miri/ci/ci.sh b/src/tools/miri/ci/ci.sh index 0356d7ecf1015..1e5a2847c6ff8 100755 --- a/src/tools/miri/ci/ci.sh +++ b/src/tools/miri/ci/ci.sh @@ -154,7 +154,7 @@ case $HOST_TARGET in TEST_TARGET=i686-unknown-freebsd run_tests_minimal $BASIC $UNIX time hashmap random threadname pthread fs libc-pipe TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe TEST_TARGET=x86_64-pc-solaris run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe - TEST_TARGET=aarch64-linux-android run_tests_minimal $BASIC $UNIX time hashmap random sync threadname pthread + TEST_TARGET=aarch64-linux-android run_tests_minimal $BASIC $UNIX time hashmap random sync threadname pthread epoll eventfd TEST_TARGET=wasm32-wasip2 run_tests_minimal $BASIC wasm TEST_TARGET=wasm32-unknown-unknown run_tests_minimal no_std empty_main wasm # this target doesn't really have std TEST_TARGET=thumbv7em-none-eabihf run_tests_minimal no_std diff --git a/src/tools/miri/src/shims/unix/android/foreign_items.rs b/src/tools/miri/src/shims/unix/android/foreign_items.rs index 80ad40e1624f7..4a005093b2799 100644 --- a/src/tools/miri/src/shims/unix/android/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/android/foreign_items.rs @@ -1,6 +1,8 @@ use rustc_abi::ExternAbi; use rustc_span::Symbol; +use self::shims::unix::linux::epoll::EvalContextExt as _; +use self::shims::unix::linux::eventfd::EvalContextExt as _; use crate::shims::unix::android::thread::prctl; use crate::shims::unix::linux::syscall::syscall; use crate::*; @@ -20,6 +22,31 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx, EmulateItemResult> { let this = self.eval_context_mut(); match link_name.as_str() { + // epoll, eventfd + "epoll_create1" => { + let [flag] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.epoll_create1(flag)?; + this.write_scalar(result, dest)?; + } + "epoll_ctl" => { + let [epfd, op, fd, event] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.epoll_ctl(epfd, op, fd, event)?; + this.write_scalar(result, dest)?; + } + "epoll_wait" => { + let [epfd, events, maxevents, timeout] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + this.epoll_wait(epfd, events, maxevents, timeout, dest)?; + } + "eventfd" => { + let [val, flag] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.eventfd(val, flag)?; + this.write_scalar(result, dest)?; + } + // Miscellaneous "__errno" => { let [] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; diff --git a/src/tools/miri/src/shims/unix/linux/eventfd.rs b/src/tools/miri/src/shims/unix/linux/eventfd.rs index 2ec3d792d9421..6aee9c1d86b01 100644 --- a/src/tools/miri/src/shims/unix/linux/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux/eventfd.rs @@ -144,9 +144,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn eventfd(&mut self, val: &OpTy<'tcx>, flags: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - // eventfd is Linux specific. - this.assert_target_os("linux", "eventfd"); - let val = this.read_scalar(val)?.to_u32()?; let mut flags = this.read_scalar(flags)?.to_i32()?; diff --git a/src/tools/miri/tests/fail-dep/libc/libc-epoll-data-race.rs b/src/tools/miri/tests/fail-dep/libc/libc-epoll-data-race.rs index 398bc92b39252..0794867118c63 100644 --- a/src/tools/miri/tests/fail-dep/libc/libc-epoll-data-race.rs +++ b/src/tools/miri/tests/fail-dep/libc/libc-epoll-data-race.rs @@ -3,6 +3,7 @@ //! and therefore still report a data race for things that need to see the second event //! to be considered synchronized. //@only-target: linux +//@only-target: android // ensure deterministic schedule //@compile-flags: -Zmiri-preemption-rate=0 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 9bcc776e28126..d45dc5af48969 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,4 +1,5 @@ //@only-target: linux +//@only-target: android // test_epoll_block_then_unblock and test_epoll_race depend on a deterministic schedule. //@compile-flags: -Zmiri-preemption-rate=0 diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs index 288c1d41f3077..59141cd5dcc12 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs @@ -1,4 +1,5 @@ //@only-target: linux +//@only-target: android use std::convert::TryInto; diff --git a/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs b/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs index dd9c0eb0b54d6..767ab1065c248 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs @@ -1,4 +1,5 @@ //@only-target: linux +//@only-target: android // test_race, test_blocking_read and test_blocking_write depend on a deterministic schedule. //@compile-flags: -Zmiri-preemption-rate=0 From 64f42a4107a6d095fd7ea4fb726f7a73ba235ada Mon Sep 17 00:00:00 2001 From: Yoh Deadfall Date: Mon, 11 Nov 2024 15:38:10 +0300 Subject: [PATCH 13/44] Fixed test target Co-authored-by: Ralf Jung --- src/tools/miri/tests/fail-dep/libc/libc-epoll-data-race.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tools/miri/tests/fail-dep/libc/libc-epoll-data-race.rs b/src/tools/miri/tests/fail-dep/libc/libc-epoll-data-race.rs index 0794867118c63..7bef687e33952 100644 --- a/src/tools/miri/tests/fail-dep/libc/libc-epoll-data-race.rs +++ b/src/tools/miri/tests/fail-dep/libc/libc-epoll-data-race.rs @@ -2,8 +2,7 @@ //! and we only read one of them, we do not synchronize with the other events //! and therefore still report a data race for things that need to see the second event //! to be considered synchronized. -//@only-target: linux -//@only-target: android +//@only-target: linux android // ensure deterministic schedule //@compile-flags: -Zmiri-preemption-rate=0 From 13aa5fbd7866a975a7f56b42aa111bb16dac4b1d Mon Sep 17 00:00:00 2001 From: Yoh Deadfall Date: Mon, 11 Nov 2024 18:20:26 +0300 Subject: [PATCH 14/44] Added linux_like module --- src/tools/miri/src/shims/unix/android/foreign_items.rs | 6 +++--- src/tools/miri/src/shims/unix/fd.rs | 2 +- src/tools/miri/src/shims/unix/linux/foreign_items.rs | 6 +++--- src/tools/miri/src/shims/unix/linux/mod.rs | 4 ---- .../miri/src/shims/unix/{linux => linux_like}/epoll.rs | 0 .../miri/src/shims/unix/{linux => linux_like}/eventfd.rs | 2 +- src/tools/miri/src/shims/unix/linux_like/mod.rs | 4 ++++ src/tools/miri/src/shims/unix/{linux => linux_like}/sync.rs | 0 .../miri/src/shims/unix/{linux => linux_like}/syscall.rs | 4 ++-- src/tools/miri/src/shims/unix/mod.rs | 3 ++- src/tools/miri/src/shims/unix/unnamed_socket.rs | 2 +- 11 files changed, 17 insertions(+), 16 deletions(-) rename src/tools/miri/src/shims/unix/{linux => linux_like}/epoll.rs (100%) rename src/tools/miri/src/shims/unix/{linux => linux_like}/eventfd.rs (99%) create mode 100644 src/tools/miri/src/shims/unix/linux_like/mod.rs rename src/tools/miri/src/shims/unix/{linux => linux_like}/sync.rs (100%) rename src/tools/miri/src/shims/unix/{linux => linux_like}/syscall.rs (95%) diff --git a/src/tools/miri/src/shims/unix/android/foreign_items.rs b/src/tools/miri/src/shims/unix/android/foreign_items.rs index 4a005093b2799..f9003885450c8 100644 --- a/src/tools/miri/src/shims/unix/android/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/android/foreign_items.rs @@ -1,10 +1,10 @@ use rustc_abi::ExternAbi; use rustc_span::Symbol; -use self::shims::unix::linux::epoll::EvalContextExt as _; -use self::shims::unix::linux::eventfd::EvalContextExt as _; use crate::shims::unix::android::thread::prctl; -use crate::shims::unix::linux::syscall::syscall; +use crate::shims::unix::linux_like::epoll::EvalContextExt as _; +use crate::shims::unix::linux_like::eventfd::EvalContextExt as _; +use crate::shims::unix::linux_like::syscall::syscall; use crate::*; pub fn is_dyn_sym(_name: &str) -> bool { diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 27bdd508f7742..4e80630e67443 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -10,7 +10,7 @@ use std::rc::{Rc, Weak}; use rustc_abi::Size; use crate::helpers::check_min_arg_count; -use crate::shims::unix::linux::epoll::EpollReadyEvents; +use crate::shims::unix::linux_like::epoll::EpollReadyEvents; use crate::shims::unix::*; use crate::*; 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 810e8d3340adb..bc3619090c087 100644 --- a/src/tools/miri/src/shims/unix/linux/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/linux/foreign_items.rs @@ -1,10 +1,10 @@ use rustc_abi::ExternAbi; use rustc_span::Symbol; -use self::shims::unix::linux::epoll::EvalContextExt as _; -use self::shims::unix::linux::eventfd::EvalContextExt as _; use self::shims::unix::linux::mem::EvalContextExt as _; -use self::shims::unix::linux::syscall::syscall; +use self::shims::unix::linux_like::epoll::EvalContextExt as _; +use self::shims::unix::linux_like::eventfd::EvalContextExt as _; +use self::shims::unix::linux_like::syscall::syscall; use crate::machine::{SIGRTMAX, SIGRTMIN}; use crate::shims::unix::foreign_items::EvalContextExt as _; use crate::shims::unix::*; diff --git a/src/tools/miri/src/shims/unix/linux/mod.rs b/src/tools/miri/src/shims/unix/linux/mod.rs index 159e5aca03105..c10dc52cb2880 100644 --- a/src/tools/miri/src/shims/unix/linux/mod.rs +++ b/src/tools/miri/src/shims/unix/linux/mod.rs @@ -1,6 +1,2 @@ -pub mod epoll; -pub mod eventfd; pub mod foreign_items; pub mod mem; -pub mod sync; -pub mod syscall; diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux_like/epoll.rs similarity index 100% rename from src/tools/miri/src/shims/unix/linux/epoll.rs rename to src/tools/miri/src/shims/unix/linux_like/epoll.rs diff --git a/src/tools/miri/src/shims/unix/linux/eventfd.rs b/src/tools/miri/src/shims/unix/linux_like/eventfd.rs similarity index 99% rename from src/tools/miri/src/shims/unix/linux/eventfd.rs rename to src/tools/miri/src/shims/unix/linux_like/eventfd.rs index 6aee9c1d86b01..61c91877946ea 100644 --- a/src/tools/miri/src/shims/unix/linux/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux_like/eventfd.rs @@ -5,7 +5,7 @@ use std::io::ErrorKind; use crate::concurrency::VClock; use crate::shims::unix::fd::{FileDescriptionRef, WeakFileDescriptionRef}; -use crate::shims::unix::linux::epoll::{EpollReadyEvents, EvalContextExt as _}; +use crate::shims::unix::linux_like::epoll::{EpollReadyEvents, EvalContextExt as _}; use crate::shims::unix::*; use crate::*; diff --git a/src/tools/miri/src/shims/unix/linux_like/mod.rs b/src/tools/miri/src/shims/unix/linux_like/mod.rs new file mode 100644 index 0000000000000..1a22539a4eea9 --- /dev/null +++ b/src/tools/miri/src/shims/unix/linux_like/mod.rs @@ -0,0 +1,4 @@ +pub mod epoll; +pub mod eventfd; +pub mod sync; +pub mod syscall; diff --git a/src/tools/miri/src/shims/unix/linux/sync.rs b/src/tools/miri/src/shims/unix/linux_like/sync.rs similarity index 100% rename from src/tools/miri/src/shims/unix/linux/sync.rs rename to src/tools/miri/src/shims/unix/linux_like/sync.rs diff --git a/src/tools/miri/src/shims/unix/linux/syscall.rs b/src/tools/miri/src/shims/unix/linux_like/syscall.rs similarity index 95% rename from src/tools/miri/src/shims/unix/linux/syscall.rs rename to src/tools/miri/src/shims/unix/linux_like/syscall.rs index 9f6935f096be4..e9a32a263263f 100644 --- a/src/tools/miri/src/shims/unix/linux/syscall.rs +++ b/src/tools/miri/src/shims/unix/linux_like/syscall.rs @@ -1,9 +1,9 @@ use rustc_abi::ExternAbi; use rustc_span::Symbol; -use self::shims::unix::linux::eventfd::EvalContextExt as _; use crate::helpers::check_min_arg_count; -use crate::shims::unix::linux::sync::futex; +use crate::shims::unix::linux_like::eventfd::EvalContextExt as _; +use crate::shims::unix::linux_like::sync::futex; use crate::*; pub fn syscall<'tcx>( diff --git a/src/tools/miri/src/shims/unix/mod.rs b/src/tools/miri/src/shims/unix/mod.rs index c8c25c636eee3..0620b57753a49 100644 --- a/src/tools/miri/src/shims/unix/mod.rs +++ b/src/tools/miri/src/shims/unix/mod.rs @@ -11,6 +11,7 @@ mod unnamed_socket; mod android; mod freebsd; mod linux; +mod linux_like; mod macos; mod solarish; @@ -18,7 +19,7 @@ mod solarish; pub use self::env::{EvalContextExt as _, UnixEnvVars}; pub use self::fd::{EvalContextExt as _, FdTable, FileDescription}; pub use self::fs::{DirTable, EvalContextExt as _}; -pub use self::linux::epoll::EpollInterestTable; +pub use self::linux_like::epoll::EpollInterestTable; pub use self::mem::EvalContextExt as _; pub use self::sync::EvalContextExt as _; pub use self::thread::{EvalContextExt as _, ThreadNameResult}; diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index 36575f4b5fb0a..232f4500dba0f 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -11,7 +11,7 @@ use rustc_abi::Size; use crate::concurrency::VClock; use crate::shims::unix::fd::{FileDescriptionRef, WeakFileDescriptionRef}; -use crate::shims::unix::linux::epoll::{EpollReadyEvents, EvalContextExt as _}; +use crate::shims::unix::linux_like::epoll::{EpollReadyEvents, EvalContextExt as _}; use crate::shims::unix::*; use crate::*; From 065d9b5dc3dd21ad0b7d954ed6934e74442843eb Mon Sep 17 00:00:00 2001 From: Yoh Deadfall Date: Mon, 11 Nov 2024 18:54:17 +0300 Subject: [PATCH 15/44] Fix the rest of the tests --- src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs | 3 +-- src/tools/miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs | 3 +-- src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) 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 d45dc5af48969..e3c42b2701cd3 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,4 @@ -//@only-target: linux -//@only-target: android +//@only-target: linux android // test_epoll_block_then_unblock and test_epoll_race depend on a deterministic schedule. //@compile-flags: -Zmiri-preemption-rate=0 diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs index 59141cd5dcc12..111e639c86416 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs @@ -1,5 +1,4 @@ -//@only-target: linux -//@only-target: android +//@only-target: linux android use std::convert::TryInto; diff --git a/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs b/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs index 767ab1065c248..538a157a43072 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs @@ -1,5 +1,4 @@ -//@only-target: linux -//@only-target: android +//@only-target: linux android // test_race, test_blocking_read and test_blocking_write depend on a deterministic schedule. //@compile-flags: -Zmiri-preemption-rate=0 From a05e53b68956f3b183d52e0ba4a46e0fce929a61 Mon Sep 17 00:00:00 2001 From: asquared31415 <34665709+asquared31415@users.noreply.github.com> Date: Mon, 25 Nov 2024 23:59:17 -0500 Subject: [PATCH 16/44] attempt to fix miri failing to create file when under weird powershell configurations --- src/tools/miri/cargo-miri/src/phases.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/tools/miri/cargo-miri/src/phases.rs b/src/tools/miri/cargo-miri/src/phases.rs index f1f76fd338cea..7ca4f414c2051 100644 --- a/src/tools/miri/cargo-miri/src/phases.rs +++ b/src/tools/miri/cargo-miri/src/phases.rs @@ -348,14 +348,17 @@ pub fn phase_rustc(mut args: impl Iterator, phase: RustcPhase) { // Create a stub .d file to stop Cargo from "rebuilding" the crate: // https://github.com/rust-lang/miri/issues/1724#issuecomment-787115693 // As we store a JSON file instead of building the crate here, an empty file is fine. - let dep_info_name = format!( - "{}/{}{}.d", - get_arg_flag_value("--out-dir").unwrap(), + let mut dep_info_name = PathBuf::from(get_arg_flag_value("--out-dir").unwrap()); + dep_info_name.push(format!( + "{}{}.d", get_arg_flag_value("--crate-name").unwrap(), get_arg_flag_value("extra-filename").unwrap_or_default(), - ); + )); if verbose > 0 { - eprintln!("[cargo-miri rustc] writing stub dep-info to `{dep_info_name}`"); + eprintln!( + "[cargo-miri rustc] writing stub dep-info to `{}`", + dep_info_name.display() + ); } File::create(dep_info_name).expect("failed to create fake .d file"); } From 109c2996461502abc339d841035c800b4b45d9e2 Mon Sep 17 00:00:00 2001 From: tiif Date: Tue, 26 Nov 2024 14:54:26 +0800 Subject: [PATCH 17/44] Simplify thread blocking tests --- .../fail-dep/libc/eventfd_block_read_twice.rs | 7 ---- .../libc/eventfd_block_write_twice.rs | 7 ---- .../libc/libc_epoll_block_two_thread.rs | 32 +++++++------------ .../libc/libc_epoll_block_two_thread.stderr | 14 ++++---- .../miri/tests/pass-dep/libc/libc-eventfd.rs | 7 ---- 5 files changed, 18 insertions(+), 49 deletions(-) diff --git a/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs b/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs index a9bb4c7eba8ba..81a96103db418 100644 --- a/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs +++ b/src/tools/miri/tests/fail-dep/libc/eventfd_block_read_twice.rs @@ -23,7 +23,6 @@ fn main() { let fd = unsafe { libc::eventfd(0, flags) }; let thread1 = thread::spawn(move || { - thread::park(); let mut buf: [u8; 8] = [0; 8]; // This read will block initially. let res: i64 = unsafe { libc::read(fd, buf.as_mut_ptr().cast(), 8).try_into().unwrap() }; @@ -33,7 +32,6 @@ fn main() { }); let thread2 = thread::spawn(move || { - thread::park(); let mut buf: [u8; 8] = [0; 8]; // This read will block initially, then get unblocked by thread3, then get blocked again // because the `read` in thread1 executes first and set the counter to 0 again. @@ -45,7 +43,6 @@ fn main() { }); let thread3 = thread::spawn(move || { - thread::park(); let sized_8_data = 1_u64.to_ne_bytes(); // Write 1 to the counter, so both thread1 and thread2 will unblock. let res: i64 = unsafe { @@ -55,10 +52,6 @@ fn main() { assert_eq!(res, 8); }); - thread1.thread().unpark(); - thread2.thread().unpark(); - thread3.thread().unpark(); - thread1.join().unwrap(); thread2.join().unwrap(); thread3.join().unwrap(); diff --git a/src/tools/miri/tests/fail-dep/libc/eventfd_block_write_twice.rs b/src/tools/miri/tests/fail-dep/libc/eventfd_block_write_twice.rs index 2212d6c7eb5b2..7a2a6f4eb1a82 100644 --- a/src/tools/miri/tests/fail-dep/libc/eventfd_block_write_twice.rs +++ b/src/tools/miri/tests/fail-dep/libc/eventfd_block_write_twice.rs @@ -28,7 +28,6 @@ fn main() { assert_eq!(res, 8); let thread1 = thread::spawn(move || { - thread::park(); let sized_8_data = (u64::MAX - 1).to_ne_bytes(); let res: i64 = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8).try_into().unwrap() @@ -38,7 +37,6 @@ fn main() { }); let thread2 = thread::spawn(move || { - thread::park(); let sized_8_data = (u64::MAX - 1).to_ne_bytes(); // Write u64::MAX - 1, so the all subsequent write will block. let res: i64 = unsafe { @@ -52,7 +50,6 @@ fn main() { }); let thread3 = thread::spawn(move || { - thread::park(); let mut buf: [u8; 8] = [0; 8]; // This will unblock both `write` in thread1 and thread2. let res: i64 = unsafe { libc::read(fd, buf.as_mut_ptr().cast(), 8).try_into().unwrap() }; @@ -61,10 +58,6 @@ fn main() { assert_eq!(counter, (u64::MAX - 1)); }); - thread1.thread().unpark(); - thread2.thread().unpark(); - thread3.thread().unpark(); - thread1.join().unwrap(); thread2.join().unwrap(); thread3.join().unwrap(); diff --git a/src/tools/miri/tests/fail-dep/libc/libc_epoll_block_two_thread.rs b/src/tools/miri/tests/fail-dep/libc/libc_epoll_block_two_thread.rs index 7f5ec477e192c..1c6c2f70c1db1 100644 --- a/src/tools/miri/tests/fail-dep/libc/libc_epoll_block_two_thread.rs +++ b/src/tools/miri/tests/fail-dep/libc/libc_epoll_block_two_thread.rs @@ -5,7 +5,6 @@ //@error-in-other-file: deadlock use std::convert::TryInto; -use std::thread; use std::thread::spawn; // Using `as` cast since `EPOLLET` wraps around @@ -41,10 +40,10 @@ fn check_epoll_wait( // Test if only one thread is unblocked if multiple threads blocked on same epfd. // Expected execution: -// 1. Thread 2 blocks. -// 2. Thread 3 blocks. -// 3. Thread 1 unblocks thread 3. -// 4. Thread 2 deadlocks. +// 1. Thread 1 blocks. +// 2. Thread 2 blocks. +// 3. Thread 3 unblocks thread 2. +// 4. Thread 1 deadlocks. fn main() { // Create an epoll instance. let epfd = unsafe { libc::epoll_create1(0) }; @@ -65,30 +64,21 @@ fn main() { let expected_value = fds[0] as u64; check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], 0); - let thread1 = spawn(move || { - thread::park(); - let data = "abcde".as_bytes().as_ptr(); - let res = unsafe { libc::write(fds[1], data as *const libc::c_void, 5) }; - assert_eq!(res, 5); - }); - let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); let expected_value = fds[0] as u64; - let thread2 = spawn(move || { - thread::park(); + let thread1 = spawn(move || { check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], -1); //~^ERROR: deadlocked }); - let thread3 = spawn(move || { - thread::park(); + let thread2 = spawn(move || { check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], -1); }); - thread2.thread().unpark(); - thread::yield_now(); - thread3.thread().unpark(); - thread::yield_now(); - thread1.thread().unpark(); + let thread3 = spawn(move || { + let data = "abcde".as_bytes().as_ptr(); + let res = unsafe { libc::write(fds[1], data as *const libc::c_void, 5) }; + assert_eq!(res, 5); + }); thread1.join().unwrap(); thread2.join().unwrap(); diff --git a/src/tools/miri/tests/fail-dep/libc/libc_epoll_block_two_thread.stderr b/src/tools/miri/tests/fail-dep/libc/libc_epoll_block_two_thread.stderr index 010dabc136449..b29794f68ddb7 100644 --- a/src/tools/miri/tests/fail-dep/libc/libc_epoll_block_two_thread.stderr +++ b/src/tools/miri/tests/fail-dep/libc/libc_epoll_block_two_thread.stderr @@ -1,3 +1,9 @@ +error: deadlock: the evaluated program deadlocked + | + = note: the evaluated program deadlocked + = note: (no span available) + = note: BACKTRACE on thread `unnamed-ID`: + error: deadlock: the evaluated program deadlocked --> RUSTLIB/std/src/sys/pal/PLATFORM/thread.rs:LL:CC | @@ -11,15 +17,9 @@ LL | let ret = unsafe { libc::pthread_join(id, ptr::null_mut()) }; note: inside `main` --> tests/fail-dep/libc/libc_epoll_block_two_thread.rs:LL:CC | -LL | thread2.join().unwrap(); +LL | thread1.join().unwrap(); | ^^^^^^^^^^^^^^ -error: deadlock: the evaluated program deadlocked - | - = note: the evaluated program deadlocked - = note: (no span available) - = note: BACKTRACE on thread `unnamed-ID`: - error: deadlock: the evaluated program deadlocked --> tests/fail-dep/libc/libc_epoll_block_two_thread.rs:LL:CC | diff --git a/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs b/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs index dd9c0eb0b54d6..dfb8f030dbe75 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-eventfd.rs @@ -197,7 +197,6 @@ fn test_two_threads_blocked_on_eventfd() { assert_eq!(res, 8); let thread1 = thread::spawn(move || { - thread::park(); let sized_8_data = 1_u64.to_ne_bytes(); let res: i64 = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8).try_into().unwrap() @@ -207,7 +206,6 @@ fn test_two_threads_blocked_on_eventfd() { }); let thread2 = thread::spawn(move || { - thread::park(); let sized_8_data = 1_u64.to_ne_bytes(); let res: i64 = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8).try_into().unwrap() @@ -217,7 +215,6 @@ fn test_two_threads_blocked_on_eventfd() { }); let thread3 = thread::spawn(move || { - thread::park(); let mut buf: [u8; 8] = [0; 8]; // This will unblock previously blocked eventfd read. let res = read_bytes(fd, &mut buf); @@ -227,10 +224,6 @@ fn test_two_threads_blocked_on_eventfd() { assert_eq!(counter, (u64::MAX - 1)); }); - thread1.thread().unpark(); - thread2.thread().unpark(); - thread3.thread().unpark(); - thread1.join().unwrap(); thread2.join().unwrap(); thread3.join().unwrap(); From 15001f36d115b2b8017c2d25211c2feec56d2752 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Tue, 12 Nov 2024 21:52:57 +0000 Subject: [PATCH 18/44] filesystem support for solarish. close #3890 --- src/tools/miri/ci/ci.sh | 4 +- .../src/shims/unix/freebsd/foreign_items.rs | 6 +- src/tools/miri/src/shims/unix/fs.rs | 66 ++++++++++++------- .../src/shims/unix/macos/foreign_items.rs | 6 +- .../src/shims/unix/solarish/foreign_items.rs | 20 ++++++ src/tools/miri/tests/pass/shims/fs.rs | 7 +- 6 files changed, 75 insertions(+), 34 deletions(-) diff --git a/src/tools/miri/ci/ci.sh b/src/tools/miri/ci/ci.sh index 1e5a2847c6ff8..8e6e31bee4302 100755 --- a/src/tools/miri/ci/ci.sh +++ b/src/tools/miri/ci/ci.sh @@ -152,8 +152,8 @@ case $HOST_TARGET in UNIX="hello panic/panic panic/unwind concurrency/simple atomic libc-mem libc-misc libc-random env num_cpus" # the things that are very similar across all Unixes, and hence easily supported there TEST_TARGET=x86_64-unknown-freebsd run_tests_minimal $BASIC $UNIX time hashmap random threadname pthread fs libc-pipe TEST_TARGET=i686-unknown-freebsd run_tests_minimal $BASIC $UNIX time hashmap random threadname pthread fs libc-pipe - TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe - TEST_TARGET=x86_64-pc-solaris run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe + TEST_TARGET=x86_64-unknown-illumos run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe fs + TEST_TARGET=x86_64-pc-solaris run_tests_minimal $BASIC $UNIX time hashmap random thread sync available-parallelism tls libc-pipe fs TEST_TARGET=aarch64-linux-android run_tests_minimal $BASIC $UNIX time hashmap random sync threadname pthread epoll eventfd TEST_TARGET=wasm32-wasip2 run_tests_minimal $BASIC wasm TEST_TARGET=wasm32-unknown-unknown run_tests_minimal no_std empty_main wasm # this target doesn't really have std diff --git a/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs b/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs index 1346d8de7eaa2..0c9bc005ece47 100644 --- a/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/freebsd/foreign_items.rs @@ -53,19 +53,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "stat" | "stat@FBSD_1.0" => { let [path, buf] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_stat(path, buf)?; + let result = this.macos_fbsd_solaris_stat(path, buf)?; this.write_scalar(result, dest)?; } "lstat" | "lstat@FBSD_1.0" => { let [path, buf] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_lstat(path, buf)?; + let result = this.macos_fbsd_solaris_lstat(path, buf)?; this.write_scalar(result, dest)?; } "fstat" | "fstat@FBSD_1.0" => { let [fd, buf] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_fstat(fd, buf)?; + let result = this.macos_fbsd_solaris_fstat(fd, buf)?; this.write_scalar(result, dest)?; } "readdir_r" | "readdir_r@FBSD_1.0" => { diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index 091def7ac65a0..070fdc94695b7 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -265,47 +265,59 @@ impl FileDescription for FileHandle { impl<'tcx> EvalContextExtPrivate<'tcx> for crate::MiriInterpCx<'tcx> {} trait EvalContextExtPrivate<'tcx>: crate::MiriInterpCxExt<'tcx> { - fn macos_stat_write_buf( + fn macos_fbsd_solaris_write_buf( &mut self, metadata: FileMetadata, buf_op: &OpTy<'tcx>, ) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); - let mode: u16 = metadata.mode.to_u16()?; - let (access_sec, access_nsec) = metadata.accessed.unwrap_or((0, 0)); let (created_sec, created_nsec) = metadata.created.unwrap_or((0, 0)); let (modified_sec, modified_nsec) = metadata.modified.unwrap_or((0, 0)); + let mode = metadata.mode.to_uint(this.libc_ty_layout("mode_t").size)?; let buf = this.deref_pointer_as(buf_op, this.libc_ty_layout("stat"))?; - this.write_int_fields_named( &[ ("st_dev", 0), - ("st_mode", mode.into()), + ("st_mode", mode.try_into().unwrap()), ("st_nlink", 0), ("st_ino", 0), ("st_uid", 0), ("st_gid", 0), ("st_rdev", 0), ("st_atime", access_sec.into()), - ("st_atime_nsec", access_nsec.into()), ("st_mtime", modified_sec.into()), - ("st_mtime_nsec", modified_nsec.into()), ("st_ctime", 0), - ("st_ctime_nsec", 0), - ("st_birthtime", created_sec.into()), - ("st_birthtime_nsec", created_nsec.into()), ("st_size", metadata.size.into()), ("st_blocks", 0), ("st_blksize", 0), - ("st_flags", 0), - ("st_gen", 0), ], &buf, )?; + if matches!(&*this.tcx.sess.target.os, "macos" | "freebsd") { + this.write_int_fields_named( + &[ + ("st_atime_nsec", access_nsec.into()), + ("st_mtime_nsec", modified_nsec.into()), + ("st_ctime_nsec", 0), + ("st_birthtime", created_sec.into()), + ("st_birthtime_nsec", created_nsec.into()), + ("st_flags", 0), + ("st_gen", 0), + ], + &buf, + )?; + } + + if matches!(&*this.tcx.sess.target.os, "solaris" | "illumos") { + // FIXME: write st_fstype field once libc is updated. + // https://github.com/rust-lang/libc/pull/4145 + //this.write_int_fields_named(&[("st_fstype", 0)], &buf)?; + } + interp_ok(0) } @@ -648,15 +660,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { interp_ok(Scalar::from_i32(this.try_unwrap_io_result(result)?)) } - fn macos_fbsd_stat( + fn macos_fbsd_solaris_stat( &mut self, path_op: &OpTy<'tcx>, buf_op: &OpTy<'tcx>, ) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd") { - panic!("`macos_fbsd_stat` should not be called on {}", this.tcx.sess.target.os); + if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd" | "solaris" | "illumos") { + panic!("`macos_fbsd_solaris_stat` should not be called on {}", this.tcx.sess.target.os); } let path_scalar = this.read_pointer(path_op)?; @@ -674,19 +686,22 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Err(err) => return this.set_last_error_and_return_i32(err), }; - interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?)) + interp_ok(Scalar::from_i32(this.macos_fbsd_solaris_write_buf(metadata, buf_op)?)) } // `lstat` is used to get symlink metadata. - fn macos_fbsd_lstat( + fn macos_fbsd_solaris_lstat( &mut self, path_op: &OpTy<'tcx>, buf_op: &OpTy<'tcx>, ) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd") { - panic!("`macos_fbsd_lstat` should not be called on {}", this.tcx.sess.target.os); + if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd" | "solaris" | "illumos") { + panic!( + "`macos_fbsd_solaris_lstat` should not be called on {}", + this.tcx.sess.target.os + ); } let path_scalar = this.read_pointer(path_op)?; @@ -703,18 +718,21 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Err(err) => return this.set_last_error_and_return_i32(err), }; - interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?)) + interp_ok(Scalar::from_i32(this.macos_fbsd_solaris_write_buf(metadata, buf_op)?)) } - fn macos_fbsd_fstat( + fn macos_fbsd_solaris_fstat( &mut self, fd_op: &OpTy<'tcx>, buf_op: &OpTy<'tcx>, ) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd") { - panic!("`macos_fbsd_fstat` should not be called on {}", this.tcx.sess.target.os); + if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd" | "solaris" | "illumos") { + panic!( + "`macos_fbsd_solaris_fstat` should not be called on {}", + this.tcx.sess.target.os + ); } let fd = this.read_scalar(fd_op)?.to_i32()?; @@ -730,7 +748,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(metadata) => metadata, Err(err) => return this.set_last_error_and_return_i32(err), }; - interp_ok(Scalar::from_i32(this.macos_stat_write_buf(metadata, buf_op)?)) + interp_ok(Scalar::from_i32(this.macos_fbsd_solaris_write_buf(metadata, buf_op)?)) } fn linux_statx( diff --git a/src/tools/miri/src/shims/unix/macos/foreign_items.rs b/src/tools/miri/src/shims/unix/macos/foreign_items.rs index 003025916cd42..103bea0862086 100644 --- a/src/tools/miri/src/shims/unix/macos/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/macos/foreign_items.rs @@ -40,19 +40,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "stat" | "stat64" | "stat$INODE64" => { let [path, buf] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_stat(path, buf)?; + let result = this.macos_fbsd_solaris_stat(path, buf)?; this.write_scalar(result, dest)?; } "lstat" | "lstat64" | "lstat$INODE64" => { let [path, buf] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_lstat(path, buf)?; + let result = this.macos_fbsd_solaris_lstat(path, buf)?; this.write_scalar(result, dest)?; } "fstat" | "fstat64" | "fstat$INODE64" => { let [fd, buf] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; - let result = this.macos_fbsd_fstat(fd, buf)?; + let result = this.macos_fbsd_solaris_fstat(fd, buf)?; this.write_scalar(result, dest)?; } "opendir$INODE64" => { diff --git a/src/tools/miri/src/shims/unix/solarish/foreign_items.rs b/src/tools/miri/src/shims/unix/solarish/foreign_items.rs index 1bbd25617e556..e452917036840 100644 --- a/src/tools/miri/src/shims/unix/solarish/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/solarish/foreign_items.rs @@ -57,6 +57,26 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(res, dest)?; } + // File related shims + "stat" | "stat64" => { + let [path, buf] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.macos_fbsd_solaris_stat(path, buf)?; + this.write_scalar(result, dest)?; + } + "lstat" | "lstat64" => { + let [path, buf] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.macos_fbsd_solaris_lstat(path, buf)?; + this.write_scalar(result, dest)?; + } + "fstat" | "fstat64" => { + let [fd, buf] = + this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; + let result = this.macos_fbsd_solaris_fstat(fd, buf)?; + this.write_scalar(result, dest)?; + } + // Miscellaneous "___errno" => { let [] = this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?; diff --git a/src/tools/miri/tests/pass/shims/fs.rs b/src/tools/miri/tests/pass/shims/fs.rs index 81151f4ac4730..3e514d95ee9c7 100644 --- a/src/tools/miri/tests/pass/shims/fs.rs +++ b/src/tools/miri/tests/pass/shims/fs.rs @@ -27,8 +27,11 @@ fn main() { test_file_sync(); test_errors(); test_rename(); - test_directory(); - test_canonicalize(); + // solarish needs to support readdir/readdir64 for these tests. + if cfg!(not(any(target_os = "solaris", target_os = "illumos"))) { + test_directory(); + test_canonicalize(); + } test_from_raw_os_error(); #[cfg(unix)] test_pread_pwrite(); From 79b4f25c344bb6083af18d61dcfc0df68aec1ea9 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Thu, 28 Nov 2024 04:57:22 +0000 Subject: [PATCH 19/44] 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 e5812ae260303..e193c786effdd 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -5d3c6ee9b34989595d2a72b79e61ca37e949d757 +eddb717281a9031f645d88dd3b8323a7e25632cc From 2039a9fac08ae8dd214826e254c3a7d300cb0360 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Thu, 28 Nov 2024 05:06:38 +0000 Subject: [PATCH 20/44] fmt --- src/tools/miri/src/shims/windows/foreign_items.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/src/shims/windows/foreign_items.rs b/src/tools/miri/src/shims/windows/foreign_items.rs index a1fad6f9af4b7..d6a180451d7ad 100644 --- a/src/tools/miri/src/shims/windows/foreign_items.rs +++ b/src/tools/miri/src/shims/windows/foreign_items.rs @@ -383,7 +383,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_int(1, dest)?; } "TlsFree" => { - let [key] = this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; + let [key] = + this.check_shim(abi, ExternAbi::System { unwind: false }, link_name, args)?; let key = u128::from(this.read_scalar(key)?.to_u32()?); this.machine.tls.delete_tls_key(key)?; From 8e2d72bac8e29cda5b1457720dcdc66def70d9d4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 28 Nov 2024 08:38:48 +0100 Subject: [PATCH 21/44] silence clippy --- src/tools/miri/src/bin/miri.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tools/miri/src/bin/miri.rs b/src/tools/miri/src/bin/miri.rs index c61c62c73dad2..15f05ba909c0e 100644 --- a/src/tools/miri/src/bin/miri.rs +++ b/src/tools/miri/src/bin/miri.rs @@ -3,6 +3,7 @@ clippy::manual_range_contains, clippy::useless_format, clippy::field_reassign_with_default, + clippy::needless_lifetimes, rustc::diagnostic_outside_of_impl, rustc::untranslatable_diagnostic )] From c2cb99378a3d44ce3a5d8c158647e6da54049a0a Mon Sep 17 00:00:00 2001 From: klensy Date: Thu, 28 Nov 2024 14:02:03 +0300 Subject: [PATCH 22/44] remove ctrlc, unused --- src/tools/miri/Cargo.lock | 29 ----------------------------- src/tools/miri/Cargo.toml | 1 - 2 files changed, 30 deletions(-) diff --git a/src/tools/miri/Cargo.lock b/src/tools/miri/Cargo.lock index 05d1c2d1eb3e8..01e32229f2574 100644 --- a/src/tools/miri/Cargo.lock +++ b/src/tools/miri/Cargo.lock @@ -150,12 +150,6 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" -[[package]] -name = "cfg_aliases" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fd16c4719339c4530435d38e511904438d07cce7950afa3718a84ac36c10e89e" - [[package]] name = "chrono" version = "0.4.38" @@ -286,16 +280,6 @@ dependencies = [ "typenum", ] -[[package]] -name = "ctrlc" -version = "3.4.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "672465ae37dc1bc6380a6547a8883d5dd397b0f1faaad4f265726cc7042a5345" -dependencies = [ - "nix", - "windows-sys 0.52.0", -] - [[package]] name = "directories" version = "5.0.1" @@ -554,7 +538,6 @@ dependencies = [ "chrono", "chrono-tz", "colored", - "ctrlc", "directories", "getrandom", "jemalloc-sys", @@ -571,18 +554,6 @@ dependencies = [ "windows-sys 0.52.0", ] -[[package]] -name = "nix" -version = "0.28.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ab2156c4fce2f8df6c499cc1c763e4394b7482525bf2a9701c9d79d215f519e4" -dependencies = [ - "bitflags", - "cfg-if", - "cfg_aliases", - "libc", -] - [[package]] name = "num-traits" version = "0.2.19" diff --git a/src/tools/miri/Cargo.toml b/src/tools/miri/Cargo.toml index cb02914fd93bf..d1cd3640346da 100644 --- a/src/tools/miri/Cargo.toml +++ b/src/tools/miri/Cargo.toml @@ -23,7 +23,6 @@ rand = "0.8" smallvec = { version = "1.7", features = ["drain_filter"] } aes = { version = "0.8.3", features = ["hazmat"] } measureme = "11" -ctrlc = "3.2.5" chrono = { version = "0.4.38", default-features = false } chrono-tz = "0.10" directories = "5" From 374397f1bbfe3d3f13b3fc4d0c4728ac5cfdfcac Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 28 Nov 2024 16:35:51 -0800 Subject: [PATCH 23/44] Move FdTable to public location, fix up imports --- src/tools/miri/src/shims/files.rs | 455 ++++++++++++++++++ src/tools/miri/src/shims/mod.rs | 4 +- src/tools/miri/src/shims/unix/fd.rs | 447 +---------------- src/tools/miri/src/shims/unix/fs.rs | 4 +- .../miri/src/shims/unix/linux_like/epoll.rs | 3 +- .../miri/src/shims/unix/linux_like/eventfd.rs | 3 +- src/tools/miri/src/shims/unix/mod.rs | 2 +- .../miri/src/shims/unix/unnamed_socket.rs | 5 +- 8 files changed, 467 insertions(+), 456 deletions(-) create mode 100644 src/tools/miri/src/shims/files.rs diff --git a/src/tools/miri/src/shims/files.rs b/src/tools/miri/src/shims/files.rs new file mode 100644 index 0000000000000..197ed726a1b73 --- /dev/null +++ b/src/tools/miri/src/shims/files.rs @@ -0,0 +1,455 @@ +use std::any::Any; +use std::collections::BTreeMap; +use std::io; +use std::io::{IsTerminal, Read, SeekFrom, Write}; +use std::ops::Deref; +use std::rc::{Rc, Weak}; + +use rustc_abi::Size; + +use crate::*; + +#[derive(Debug, Clone, Copy, Eq, PartialEq)] +pub(crate) enum FlockOp { + SharedLock { nonblocking: bool }, + ExclusiveLock { nonblocking: bool }, + Unlock, +} + +/// Represents an open file description. +pub trait FileDescription: std::fmt::Debug + Any { + fn name(&self) -> &'static str; + + /// Reads as much as possible into the given buffer `ptr`. + /// `len` indicates how many bytes we should try to read. + /// `dest` is where the return value should be stored: number of bytes read, or `-1` in case of error. + fn read<'tcx>( + &self, + _self_ref: &FileDescriptionRef, + _communicate_allowed: bool, + _ptr: Pointer, + _len: usize, + _dest: &MPlaceTy<'tcx>, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + throw_unsup_format!("cannot read from {}", self.name()); + } + + /// Writes as much as possible from the given buffer `ptr`. + /// `len` indicates how many bytes we should try to write. + /// `dest` is where the return value should be stored: number of bytes written, or `-1` in case of error. + fn write<'tcx>( + &self, + _self_ref: &FileDescriptionRef, + _communicate_allowed: bool, + _ptr: Pointer, + _len: usize, + _dest: &MPlaceTy<'tcx>, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + throw_unsup_format!("cannot write to {}", self.name()); + } + + /// Reads as much as possible into the given buffer `ptr` from a given offset. + /// `len` indicates how many bytes we should try to read. + /// `dest` is where the return value should be stored: number of bytes read, or `-1` in case of error. + fn pread<'tcx>( + &self, + _communicate_allowed: bool, + _offset: u64, + _ptr: Pointer, + _len: usize, + _dest: &MPlaceTy<'tcx>, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + throw_unsup_format!("cannot pread from {}", self.name()); + } + + /// Writes as much as possible from the given buffer `ptr` starting at a given offset. + /// `ptr` is the pointer to the user supplied read buffer. + /// `len` indicates how many bytes we should try to write. + /// `dest` is where the return value should be stored: number of bytes written, or `-1` in case of error. + fn pwrite<'tcx>( + &self, + _communicate_allowed: bool, + _ptr: Pointer, + _len: usize, + _offset: u64, + _dest: &MPlaceTy<'tcx>, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + throw_unsup_format!("cannot pwrite to {}", self.name()); + } + + /// Seeks to the given offset (which can be relative to the beginning, end, or current position). + /// Returns the new position from the start of the stream. + fn seek<'tcx>( + &self, + _communicate_allowed: bool, + _offset: SeekFrom, + ) -> InterpResult<'tcx, io::Result> { + throw_unsup_format!("cannot seek on {}", self.name()); + } + + fn close<'tcx>( + self: Box, + _communicate_allowed: bool, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result<()>> { + throw_unsup_format!("cannot close {}", self.name()); + } + + fn flock<'tcx>( + &self, + _communicate_allowed: bool, + _op: FlockOp, + ) -> InterpResult<'tcx, io::Result<()>> { + throw_unsup_format!("cannot flock {}", self.name()); + } + + fn is_tty(&self, _communicate_allowed: bool) -> bool { + // Most FDs are not tty's and the consequence of a wrong `false` are minor, + // so we use a default impl here. + false + } + + /// Check the readiness of file description. + fn get_epoll_ready_events<'tcx>( + &self, + ) -> InterpResult<'tcx, crate::shims::unix::linux::epoll::EpollReadyEvents> { + throw_unsup_format!("{}: epoll does not support this file description", self.name()); + } +} + +impl dyn FileDescription { + #[inline(always)] + pub fn downcast(&self) -> Option<&T> { + (self as &dyn Any).downcast_ref() + } +} + +impl FileDescription for io::Stdin { + fn name(&self) -> &'static str { + "stdin" + } + + fn read<'tcx>( + &self, + _self_ref: &FileDescriptionRef, + communicate_allowed: bool, + ptr: Pointer, + len: usize, + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + let mut bytes = vec![0; len]; + if !communicate_allowed { + // We want isolation mode to be deterministic, so we have to disallow all reads, even stdin. + helpers::isolation_abort_error("`read` from stdin")?; + } + let result = Read::read(&mut { self }, &mut bytes); + 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 { + communicate_allowed && self.is_terminal() + } +} + +impl FileDescription for io::Stdout { + fn name(&self) -> &'static str { + "stdout" + } + + fn write<'tcx>( + &self, + _self_ref: &FileDescriptionRef, + _communicate_allowed: bool, + ptr: Pointer, + len: usize, + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; + // We allow writing to stderr even with isolation enabled. + let result = Write::write(&mut { self }, bytes); + // Stdout is buffered, flush to make sure it appears on the + // screen. This is the write() syscall of the interpreted + // program, we want it to correspond to a write() syscall on + // the host -- there is no good in adding extra buffering + // here. + io::stdout().flush().unwrap(); + 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 { + communicate_allowed && self.is_terminal() + } +} + +impl FileDescription for io::Stderr { + fn name(&self) -> &'static str { + "stderr" + } + + fn write<'tcx>( + &self, + _self_ref: &FileDescriptionRef, + _communicate_allowed: bool, + ptr: Pointer, + len: usize, + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; + // We allow writing to stderr even with isolation enabled. + // No need to flush, stderr is not buffered. + let result = Write::write(&mut { self }, bytes); + 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 { + communicate_allowed && self.is_terminal() + } +} + +/// Like /dev/null +#[derive(Debug)] +pub struct NullOutput; + +impl FileDescription for NullOutput { + fn name(&self) -> &'static str { + "stderr and stdout" + } + + fn write<'tcx>( + &self, + _self_ref: &FileDescriptionRef, + _communicate_allowed: bool, + _ptr: Pointer, + len: usize, + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + // We just don't write anything, but report to the user that we did. + ecx.return_write_success(len, dest) + } +} + +/// Structure contains both the file description and its unique identifier. +#[derive(Clone, Debug)] +pub struct FileDescWithId { + id: FdId, + file_description: Box, +} + +#[derive(Clone, Debug)] +pub struct FileDescriptionRef(Rc>); + +impl Deref for FileDescriptionRef { + type Target = dyn FileDescription; + + fn deref(&self) -> &Self::Target { + &*self.0.file_description + } +} + +impl FileDescriptionRef { + fn new(fd: impl FileDescription, id: FdId) -> Self { + FileDescriptionRef(Rc::new(FileDescWithId { id, file_description: Box::new(fd) })) + } + + pub fn close<'tcx>( + self, + communicate_allowed: bool, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result<()>> { + // Destroy this `Rc` using `into_inner` so we can call `close` instead of + // implicitly running the destructor of the file description. + let id = self.get_id(); + match Rc::into_inner(self.0) { + Some(fd) => { + // Remove entry from the global epoll_event_interest table. + ecx.machine.epoll_interests.remove(id); + + fd.file_description.close(communicate_allowed, ecx) + } + None => interp_ok(Ok(())), + } + } + + pub fn downgrade(&self) -> WeakFileDescriptionRef { + WeakFileDescriptionRef { weak_ref: Rc::downgrade(&self.0) } + } + + pub fn get_id(&self) -> FdId { + self.0.id + } +} + +/// Holds a weak reference to the actual file description. +#[derive(Clone, Debug, Default)] +pub struct WeakFileDescriptionRef { + weak_ref: Weak>, +} + +impl WeakFileDescriptionRef { + pub fn upgrade(&self) -> Option { + if let Some(file_desc_with_id) = self.weak_ref.upgrade() { + return Some(FileDescriptionRef(file_desc_with_id)); + } + None + } +} + +impl VisitProvenance for WeakFileDescriptionRef { + fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { + // A weak reference can never be the only reference to some pointer or place. + // Since the actual file description is tracked by strong ref somewhere, + // it is ok to make this a NOP operation. + } +} + +/// A unique id for file descriptions. While we could use the address, considering that +/// is definitely unique, the address would expose interpreter internal state when used +/// for sorting things. So instead we generate a unique id per file description that stays +/// the same even if a file descriptor is duplicated and gets a new integer file descriptor. +#[derive(Debug, Copy, Clone, Default, Eq, PartialEq, Ord, PartialOrd)] +pub struct FdId(usize); + +/// The file descriptor table +#[derive(Debug)] +pub struct FdTable { + pub fds: BTreeMap, + /// Unique identifier for file description, used to differentiate between various file description. + next_file_description_id: FdId, +} + +impl VisitProvenance for FdTable { + fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { + // All our FileDescription instances do not have any tags. + } +} + +impl FdTable { + fn new() -> Self { + FdTable { fds: BTreeMap::new(), next_file_description_id: FdId(0) } + } + pub(crate) fn init(mute_stdout_stderr: bool) -> FdTable { + let mut fds = FdTable::new(); + fds.insert_new(io::stdin()); + if mute_stdout_stderr { + assert_eq!(fds.insert_new(NullOutput), 1); + assert_eq!(fds.insert_new(NullOutput), 2); + } else { + assert_eq!(fds.insert_new(io::stdout()), 1); + assert_eq!(fds.insert_new(io::stderr()), 2); + } + fds + } + + pub fn new_ref(&mut self, fd: impl FileDescription) -> FileDescriptionRef { + let file_handle = FileDescriptionRef::new(fd, self.next_file_description_id); + self.next_file_description_id = FdId(self.next_file_description_id.0.strict_add(1)); + file_handle + } + + /// Insert a new file description to the FdTable. + pub fn insert_new(&mut self, fd: impl FileDescription) -> i32 { + let fd_ref = self.new_ref(fd); + self.insert(fd_ref) + } + + pub fn insert(&mut self, fd_ref: FileDescriptionRef) -> i32 { + self.insert_with_min_num(fd_ref, 0) + } + + /// Insert a file description, giving it a file descriptor that is at least `min_fd_num`. + pub fn insert_with_min_num(&mut self, file_handle: FileDescriptionRef, min_fd_num: i32) -> i32 { + // Find the lowest unused FD, starting from min_fd. If the first such unused FD is in + // between used FDs, the find_map combinator will return it. If the first such unused FD + // is after all other used FDs, the find_map combinator will return None, and we will use + // the FD following the greatest FD thus far. + let candidate_new_fd = + self.fds.range(min_fd_num..).zip(min_fd_num..).find_map(|((fd_num, _fd), counter)| { + if *fd_num != counter { + // There was a gap in the fds stored, return the first unused one + // (note that this relies on BTreeMap iterating in key order) + Some(counter) + } else { + // This fd is used, keep going + None + } + }); + let new_fd_num = candidate_new_fd.unwrap_or_else(|| { + // find_map ran out of BTreeMap entries before finding a free fd, use one plus the + // maximum fd in the map + self.fds.last_key_value().map(|(fd_num, _)| fd_num.strict_add(1)).unwrap_or(min_fd_num) + }); + + self.fds.try_insert(new_fd_num, file_handle).unwrap(); + new_fd_num + } + + pub fn get(&self, fd_num: i32) -> Option { + let fd = self.fds.get(&fd_num)?; + Some(fd.clone()) + } + + pub fn remove(&mut self, fd_num: i32) -> Option { + self.fds.remove(&fd_num) + } + + pub fn is_fd_num(&self, fd_num: i32) -> bool { + self.fds.contains_key(&fd_num) + } +} + +impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} +pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { + /// Helper to implement `FileDescription::read`: + /// 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`. + /// `bytes` is written to `buf` and the size is written to `dest`. + fn return_read_success( + &mut self, + buf: Pointer, + bytes: &[u8], + actual_read_size: usize, + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + // 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)?; + interp_ok(()) + } + + /// 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, + actual_write_size: usize, + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + // 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)?; + interp_ok(()) + } +} diff --git a/src/tools/miri/src/shims/mod.rs b/src/tools/miri/src/shims/mod.rs index b9317ac1a15fa..61681edcf762c 100644 --- a/src/tools/miri/src/shims/mod.rs +++ b/src/tools/miri/src/shims/mod.rs @@ -2,6 +2,7 @@ mod alloc; mod backtrace; +mod files; #[cfg(unix)] mod native_lib; mod unix; @@ -18,7 +19,8 @@ pub mod panic; pub mod time; pub mod tls; -pub use self::unix::{DirTable, EpollInterestTable, FdTable}; +pub use self::files::FdTable; +pub use self::unix::{DirTable, EpollInterestTable}; /// What needs to be done after emulating an item (a shim or an intrinsic) is done. pub enum EmulateItemResult { diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 4e80630e67443..a9a24dd295831 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -1,422 +1,16 @@ //! General management of file descriptors, and support for //! standard file descriptors (stdin/stdout/stderr). -use std::any::Any; -use std::collections::BTreeMap; -use std::io::{self, ErrorKind, IsTerminal, Read, SeekFrom, Write}; -use std::ops::Deref; -use std::rc::{Rc, Weak}; +use std::io::ErrorKind; use rustc_abi::Size; use crate::helpers::check_min_arg_count; +use crate::shims::files::FlockOp; use crate::shims::unix::linux_like::epoll::EpollReadyEvents; use crate::shims::unix::*; use crate::*; -#[derive(Debug, Clone, Copy, Eq, PartialEq)] -pub(crate) enum FlockOp { - SharedLock { nonblocking: bool }, - ExclusiveLock { nonblocking: bool }, - Unlock, -} - -/// Represents an open file description. -pub trait FileDescription: std::fmt::Debug + Any { - fn name(&self) -> &'static str; - - /// Reads as much as possible into the given buffer `ptr`. - /// `len` indicates how many bytes we should try to read. - /// `dest` is where the return value should be stored: number of bytes read, or `-1` in case of error. - fn read<'tcx>( - &self, - _self_ref: &FileDescriptionRef, - _communicate_allowed: bool, - _ptr: Pointer, - _len: usize, - _dest: &MPlaceTy<'tcx>, - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { - throw_unsup_format!("cannot read from {}", self.name()); - } - - /// Writes as much as possible from the given buffer `ptr`. - /// `len` indicates how many bytes we should try to write. - /// `dest` is where the return value should be stored: number of bytes written, or `-1` in case of error. - fn write<'tcx>( - &self, - _self_ref: &FileDescriptionRef, - _communicate_allowed: bool, - _ptr: Pointer, - _len: usize, - _dest: &MPlaceTy<'tcx>, - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { - throw_unsup_format!("cannot write to {}", self.name()); - } - - /// Reads as much as possible into the given buffer `ptr` from a given offset. - /// `len` indicates how many bytes we should try to read. - /// `dest` is where the return value should be stored: number of bytes read, or `-1` in case of error. - fn pread<'tcx>( - &self, - _communicate_allowed: bool, - _offset: u64, - _ptr: Pointer, - _len: usize, - _dest: &MPlaceTy<'tcx>, - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { - throw_unsup_format!("cannot pread from {}", self.name()); - } - - /// Writes as much as possible from the given buffer `ptr` starting at a given offset. - /// `ptr` is the pointer to the user supplied read buffer. - /// `len` indicates how many bytes we should try to write. - /// `dest` is where the return value should be stored: number of bytes written, or `-1` in case of error. - fn pwrite<'tcx>( - &self, - _communicate_allowed: bool, - _ptr: Pointer, - _len: usize, - _offset: u64, - _dest: &MPlaceTy<'tcx>, - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { - throw_unsup_format!("cannot pwrite to {}", self.name()); - } - - /// Seeks to the given offset (which can be relative to the beginning, end, or current position). - /// Returns the new position from the start of the stream. - fn seek<'tcx>( - &self, - _communicate_allowed: bool, - _offset: SeekFrom, - ) -> InterpResult<'tcx, io::Result> { - throw_unsup_format!("cannot seek on {}", self.name()); - } - - fn close<'tcx>( - self: Box, - _communicate_allowed: bool, - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result<()>> { - throw_unsup_format!("cannot close {}", self.name()); - } - - fn flock<'tcx>( - &self, - _communicate_allowed: bool, - _op: FlockOp, - ) -> InterpResult<'tcx, io::Result<()>> { - throw_unsup_format!("cannot flock {}", self.name()); - } - - fn is_tty(&self, _communicate_allowed: bool) -> bool { - // Most FDs are not tty's and the consequence of a wrong `false` are minor, - // so we use a default impl here. - false - } - - /// Check the readiness of file description. - fn get_epoll_ready_events<'tcx>(&self) -> InterpResult<'tcx, EpollReadyEvents> { - throw_unsup_format!("{}: epoll does not support this file description", self.name()); - } -} - -impl dyn FileDescription { - #[inline(always)] - pub fn downcast(&self) -> Option<&T> { - (self as &dyn Any).downcast_ref() - } -} - -impl FileDescription for io::Stdin { - fn name(&self) -> &'static str { - "stdin" - } - - fn read<'tcx>( - &self, - _self_ref: &FileDescriptionRef, - communicate_allowed: bool, - ptr: Pointer, - len: usize, - dest: &MPlaceTy<'tcx>, - ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { - let mut bytes = vec![0; len]; - if !communicate_allowed { - // We want isolation mode to be deterministic, so we have to disallow all reads, even stdin. - helpers::isolation_abort_error("`read` from stdin")?; - } - let result = Read::read(&mut { self }, &mut bytes); - 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 { - communicate_allowed && self.is_terminal() - } -} - -impl FileDescription for io::Stdout { - fn name(&self) -> &'static str { - "stdout" - } - - fn write<'tcx>( - &self, - _self_ref: &FileDescriptionRef, - _communicate_allowed: bool, - ptr: Pointer, - len: usize, - dest: &MPlaceTy<'tcx>, - ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { - let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; - // We allow writing to stderr even with isolation enabled. - let result = Write::write(&mut { self }, bytes); - // Stdout is buffered, flush to make sure it appears on the - // screen. This is the write() syscall of the interpreted - // program, we want it to correspond to a write() syscall on - // the host -- there is no good in adding extra buffering - // here. - io::stdout().flush().unwrap(); - 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 { - communicate_allowed && self.is_terminal() - } -} - -impl FileDescription for io::Stderr { - fn name(&self) -> &'static str { - "stderr" - } - - fn write<'tcx>( - &self, - _self_ref: &FileDescriptionRef, - _communicate_allowed: bool, - ptr: Pointer, - len: usize, - dest: &MPlaceTy<'tcx>, - ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { - let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; - // We allow writing to stderr even with isolation enabled. - // No need to flush, stderr is not buffered. - let result = Write::write(&mut { self }, bytes); - 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 { - communicate_allowed && self.is_terminal() - } -} - -/// Like /dev/null -#[derive(Debug)] -pub struct NullOutput; - -impl FileDescription for NullOutput { - fn name(&self) -> &'static str { - "stderr and stdout" - } - - fn write<'tcx>( - &self, - _self_ref: &FileDescriptionRef, - _communicate_allowed: bool, - _ptr: Pointer, - len: usize, - dest: &MPlaceTy<'tcx>, - ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { - // We just don't write anything, but report to the user that we did. - ecx.return_write_success(len, dest) - } -} - -/// Structure contains both the file description and its unique identifier. -#[derive(Clone, Debug)] -pub struct FileDescWithId { - id: FdId, - file_description: Box, -} - -#[derive(Clone, Debug)] -pub struct FileDescriptionRef(Rc>); - -impl Deref for FileDescriptionRef { - type Target = dyn FileDescription; - - fn deref(&self) -> &Self::Target { - &*self.0.file_description - } -} - -impl FileDescriptionRef { - fn new(fd: impl FileDescription, id: FdId) -> Self { - FileDescriptionRef(Rc::new(FileDescWithId { id, file_description: Box::new(fd) })) - } - - pub fn close<'tcx>( - self, - communicate_allowed: bool, - ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result<()>> { - // Destroy this `Rc` using `into_inner` so we can call `close` instead of - // implicitly running the destructor of the file description. - let id = self.get_id(); - match Rc::into_inner(self.0) { - Some(fd) => { - // Remove entry from the global epoll_event_interest table. - ecx.machine.epoll_interests.remove(id); - - fd.file_description.close(communicate_allowed, ecx) - } - None => interp_ok(Ok(())), - } - } - - pub fn downgrade(&self) -> WeakFileDescriptionRef { - WeakFileDescriptionRef { weak_ref: Rc::downgrade(&self.0) } - } - - pub fn get_id(&self) -> FdId { - self.0.id - } -} - -/// Holds a weak reference to the actual file description. -#[derive(Clone, Debug, Default)] -pub struct WeakFileDescriptionRef { - weak_ref: Weak>, -} - -impl WeakFileDescriptionRef { - pub fn upgrade(&self) -> Option { - if let Some(file_desc_with_id) = self.weak_ref.upgrade() { - return Some(FileDescriptionRef(file_desc_with_id)); - } - None - } -} - -impl VisitProvenance for WeakFileDescriptionRef { - fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { - // A weak reference can never be the only reference to some pointer or place. - // Since the actual file description is tracked by strong ref somewhere, - // it is ok to make this a NOP operation. - } -} - -/// A unique id for file descriptions. While we could use the address, considering that -/// is definitely unique, the address would expose interpreter internal state when used -/// for sorting things. So instead we generate a unique id per file description that stays -/// the same even if a file descriptor is duplicated and gets a new integer file descriptor. -#[derive(Debug, Copy, Clone, Default, Eq, PartialEq, Ord, PartialOrd)] -pub struct FdId(usize); - -/// The file descriptor table -#[derive(Debug)] -pub struct FdTable { - pub fds: BTreeMap, - /// Unique identifier for file description, used to differentiate between various file description. - next_file_description_id: FdId, -} - -impl VisitProvenance for FdTable { - fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { - // All our FileDescription instances do not have any tags. - } -} - -impl FdTable { - fn new() -> Self { - FdTable { fds: BTreeMap::new(), next_file_description_id: FdId(0) } - } - pub(crate) fn init(mute_stdout_stderr: bool) -> FdTable { - let mut fds = FdTable::new(); - fds.insert_new(io::stdin()); - if mute_stdout_stderr { - assert_eq!(fds.insert_new(NullOutput), 1); - assert_eq!(fds.insert_new(NullOutput), 2); - } else { - assert_eq!(fds.insert_new(io::stdout()), 1); - assert_eq!(fds.insert_new(io::stderr()), 2); - } - fds - } - - pub fn new_ref(&mut self, fd: impl FileDescription) -> FileDescriptionRef { - let file_handle = FileDescriptionRef::new(fd, self.next_file_description_id); - self.next_file_description_id = FdId(self.next_file_description_id.0.strict_add(1)); - file_handle - } - - /// Insert a new file description to the FdTable. - pub fn insert_new(&mut self, fd: impl FileDescription) -> i32 { - let fd_ref = self.new_ref(fd); - self.insert(fd_ref) - } - - pub fn insert(&mut self, fd_ref: FileDescriptionRef) -> i32 { - self.insert_with_min_num(fd_ref, 0) - } - - /// Insert a file description, giving it a file descriptor that is at least `min_fd_num`. - fn insert_with_min_num(&mut self, file_handle: FileDescriptionRef, min_fd_num: i32) -> i32 { - // Find the lowest unused FD, starting from min_fd. If the first such unused FD is in - // between used FDs, the find_map combinator will return it. If the first such unused FD - // is after all other used FDs, the find_map combinator will return None, and we will use - // the FD following the greatest FD thus far. - let candidate_new_fd = - self.fds.range(min_fd_num..).zip(min_fd_num..).find_map(|((fd_num, _fd), counter)| { - if *fd_num != counter { - // There was a gap in the fds stored, return the first unused one - // (note that this relies on BTreeMap iterating in key order) - Some(counter) - } else { - // This fd is used, keep going - None - } - }); - let new_fd_num = candidate_new_fd.unwrap_or_else(|| { - // find_map ran out of BTreeMap entries before finding a free fd, use one plus the - // maximum fd in the map - self.fds.last_key_value().map(|(fd_num, _)| fd_num.strict_add(1)).unwrap_or(min_fd_num) - }); - - self.fds.try_insert(new_fd_num, file_handle).unwrap(); - new_fd_num - } - - pub fn get(&self, fd_num: i32) -> Option { - let fd = self.fds.get(&fd_num)?; - Some(fd.clone()) - } - - pub fn remove(&mut self, fd_num: i32) -> Option { - self.fds.remove(&fd_num) - } - - pub fn is_fd_num(&self, fd_num: i32) -> bool { - self.fds.contains_key(&fd_num) - } -} - impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn dup(&mut self, old_fd_num: i32) -> InterpResult<'tcx, Scalar> { @@ -647,41 +241,4 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; interp_ok(()) } - - /// Helper to implement `FileDescription::read`: - /// 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`. - /// `bytes` is written to `buf` and the size is written to `dest`. - fn return_read_success( - &mut self, - buf: Pointer, - bytes: &[u8], - actual_read_size: usize, - dest: &MPlaceTy<'tcx>, - ) -> InterpResult<'tcx> { - let this = self.eval_context_mut(); - // 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)?; - interp_ok(()) - } - - /// 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, - actual_write_size: usize, - dest: &MPlaceTy<'tcx>, - ) -> InterpResult<'tcx> { - let this = self.eval_context_mut(); - // 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)?; - interp_ok(()) - } } diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index 070fdc94695b7..f363627da93a9 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -11,12 +11,10 @@ use std::time::SystemTime; use rustc_abi::Size; use rustc_data_structures::fx::FxHashMap; -use self::fd::FlockOp; use self::shims::time::system_time_to_duration; use crate::helpers::check_min_arg_count; +use crate::shims::files::{EvalContextExt as _, FileDescription, FileDescriptionRef, FlockOp}; use crate::shims::os_str::bytes_to_os_str; -use crate::shims::unix::fd::FileDescriptionRef; -use crate::shims::unix::*; use crate::*; #[derive(Debug)] diff --git a/src/tools/miri/src/shims/unix/linux_like/epoll.rs b/src/tools/miri/src/shims/unix/linux_like/epoll.rs index b20b12528db95..d0317132c6c75 100644 --- a/src/tools/miri/src/shims/unix/linux_like/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux_like/epoll.rs @@ -5,8 +5,7 @@ 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::shims::files::{FdId, FileDescription, FileDescriptionRef, WeakFileDescriptionRef}; use crate::*; /// An `Epoll` file descriptor connects file handles and epoll events diff --git a/src/tools/miri/src/shims/unix/linux_like/eventfd.rs b/src/tools/miri/src/shims/unix/linux_like/eventfd.rs index 61c91877946ea..5b51b5a0f0978 100644 --- a/src/tools/miri/src/shims/unix/linux_like/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux_like/eventfd.rs @@ -4,9 +4,8 @@ use std::io; use std::io::ErrorKind; use crate::concurrency::VClock; -use crate::shims::unix::fd::{FileDescriptionRef, WeakFileDescriptionRef}; +use crate::shims::files::{FileDescription, FileDescriptionRef, WeakFileDescriptionRef}; use crate::shims::unix::linux_like::epoll::{EpollReadyEvents, EvalContextExt as _}; -use crate::shims::unix::*; use crate::*; /// Maximum value that the eventfd counter can hold. diff --git a/src/tools/miri/src/shims/unix/mod.rs b/src/tools/miri/src/shims/unix/mod.rs index 0620b57753a49..fea5bc959c6d0 100644 --- a/src/tools/miri/src/shims/unix/mod.rs +++ b/src/tools/miri/src/shims/unix/mod.rs @@ -17,7 +17,7 @@ mod solarish; // All the Unix-specific extension traits pub use self::env::{EvalContextExt as _, UnixEnvVars}; -pub use self::fd::{EvalContextExt as _, FdTable, FileDescription}; +pub use self::fd::EvalContextExt as _; pub use self::fs::{DirTable, EvalContextExt as _}; pub use self::linux_like::epoll::EpollInterestTable; pub use self::mem::EvalContextExt as _; diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index 232f4500dba0f..5d2b767fe8b73 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -10,9 +10,10 @@ use std::io::{ErrorKind, Read}; use rustc_abi::Size; use crate::concurrency::VClock; -use crate::shims::unix::fd::{FileDescriptionRef, WeakFileDescriptionRef}; +use crate::shims::files::{ + EvalContextExt as _, FileDescription, FileDescriptionRef, WeakFileDescriptionRef, +}; use crate::shims::unix::linux_like::epoll::{EpollReadyEvents, EvalContextExt as _}; -use crate::shims::unix::*; use crate::*; /// The maximum capacity of the socketpair buffer in bytes. From 035777d47744faebde5de8d0406f64eb9f10f221 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 28 Nov 2024 17:06:26 -0800 Subject: [PATCH 24/44] Split unix-specific function into UnixFileDescription --- src/tools/miri/src/shims/files.rs | 54 +--------- src/tools/miri/src/shims/unix/fd.rs | 63 +++++++++++- src/tools/miri/src/shims/unix/fs.rs | 87 ++++++++-------- .../miri/src/shims/unix/linux_like/epoll.rs | 9 +- .../miri/src/shims/unix/linux_like/eventfd.rs | 29 +++--- src/tools/miri/src/shims/unix/mod.rs | 4 +- .../miri/src/shims/unix/unnamed_socket.rs | 99 ++++++++++--------- 7 files changed, 190 insertions(+), 155 deletions(-) diff --git a/src/tools/miri/src/shims/files.rs b/src/tools/miri/src/shims/files.rs index 197ed726a1b73..7f8b519e887de 100644 --- a/src/tools/miri/src/shims/files.rs +++ b/src/tools/miri/src/shims/files.rs @@ -7,15 +7,9 @@ use std::rc::{Rc, Weak}; use rustc_abi::Size; +use crate::shims::unix::UnixFileDescription; use crate::*; -#[derive(Debug, Clone, Copy, Eq, PartialEq)] -pub(crate) enum FlockOp { - SharedLock { nonblocking: bool }, - ExclusiveLock { nonblocking: bool }, - Unlock, -} - /// Represents an open file description. pub trait FileDescription: std::fmt::Debug + Any { fn name(&self) -> &'static str; @@ -50,37 +44,6 @@ pub trait FileDescription: std::fmt::Debug + Any { throw_unsup_format!("cannot write to {}", self.name()); } - /// Reads as much as possible into the given buffer `ptr` from a given offset. - /// `len` indicates how many bytes we should try to read. - /// `dest` is where the return value should be stored: number of bytes read, or `-1` in case of error. - fn pread<'tcx>( - &self, - _communicate_allowed: bool, - _offset: u64, - _ptr: Pointer, - _len: usize, - _dest: &MPlaceTy<'tcx>, - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { - throw_unsup_format!("cannot pread from {}", self.name()); - } - - /// Writes as much as possible from the given buffer `ptr` starting at a given offset. - /// `ptr` is the pointer to the user supplied read buffer. - /// `len` indicates how many bytes we should try to write. - /// `dest` is where the return value should be stored: number of bytes written, or `-1` in case of error. - fn pwrite<'tcx>( - &self, - _communicate_allowed: bool, - _ptr: Pointer, - _len: usize, - _offset: u64, - _dest: &MPlaceTy<'tcx>, - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx> { - throw_unsup_format!("cannot pwrite to {}", self.name()); - } - /// Seeks to the given offset (which can be relative to the beginning, end, or current position). /// Returns the new position from the start of the stream. fn seek<'tcx>( @@ -99,25 +62,14 @@ pub trait FileDescription: std::fmt::Debug + Any { throw_unsup_format!("cannot close {}", self.name()); } - fn flock<'tcx>( - &self, - _communicate_allowed: bool, - _op: FlockOp, - ) -> InterpResult<'tcx, io::Result<()>> { - throw_unsup_format!("cannot flock {}", self.name()); - } - fn is_tty(&self, _communicate_allowed: bool) -> bool { // Most FDs are not tty's and the consequence of a wrong `false` are minor, // so we use a default impl here. false } - /// Check the readiness of file description. - fn get_epoll_ready_events<'tcx>( - &self, - ) -> InterpResult<'tcx, crate::shims::unix::linux::epoll::EpollReadyEvents> { - throw_unsup_format!("{}: epoll does not support this file description", self.name()); + fn as_unix(&self) -> &dyn UnixFileDescription { + panic!("Not a unix file descriptor: {}", self.name()); } } diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index a9a24dd295831..e5dead1a26387 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -1,16 +1,71 @@ //! General management of file descriptors, and support for //! standard file descriptors (stdin/stdout/stderr). +use std::io; use std::io::ErrorKind; use rustc_abi::Size; use crate::helpers::check_min_arg_count; -use crate::shims::files::FlockOp; +use crate::shims::files::FileDescription; use crate::shims::unix::linux_like::epoll::EpollReadyEvents; use crate::shims::unix::*; use crate::*; +#[derive(Debug, Clone, Copy, Eq, PartialEq)] +pub(crate) enum FlockOp { + SharedLock { nonblocking: bool }, + ExclusiveLock { nonblocking: bool }, + Unlock, +} + +/// Represents unix-specific file descriptions. +pub trait UnixFileDescription: FileDescription { + /// Reads as much as possible into the given buffer `ptr` from a given offset. + /// `len` indicates how many bytes we should try to read. + /// `dest` is where the return value should be stored: number of bytes read, or `-1` in case of error. + fn pread<'tcx>( + &self, + _communicate_allowed: bool, + _offset: u64, + _ptr: Pointer, + _len: usize, + _dest: &MPlaceTy<'tcx>, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + throw_unsup_format!("cannot pread from {}", self.name()); + } + + /// Writes as much as possible from the given buffer `ptr` starting at a given offset. + /// `ptr` is the pointer to the user supplied read buffer. + /// `len` indicates how many bytes we should try to write. + /// `dest` is where the return value should be stored: number of bytes written, or `-1` in case of error. + fn pwrite<'tcx>( + &self, + _communicate_allowed: bool, + _ptr: Pointer, + _len: usize, + _offset: u64, + _dest: &MPlaceTy<'tcx>, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { + throw_unsup_format!("cannot pwrite to {}", self.name()); + } + + fn flock<'tcx>( + &self, + _communicate_allowed: bool, + _op: FlockOp, + ) -> InterpResult<'tcx, io::Result<()>> { + throw_unsup_format!("cannot flock {}", self.name()); + } + + /// Check the readiness of file description. + fn get_epoll_ready_events<'tcx>(&self) -> InterpResult<'tcx, EpollReadyEvents> { + throw_unsup_format!("{}: epoll does not support this file description", self.name()); + } +} + impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn dup(&mut self, old_fd_num: i32) -> InterpResult<'tcx, Scalar> { @@ -66,7 +121,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_unsup_format!("unsupported flags {:#x}", op); }; - let result = fd.flock(this.machine.communicate(), parsed_op)?; + let result = fd.as_unix().flock(this.machine.communicate(), parsed_op)?; drop(fd); // return `0` if flock is successful let result = result.map(|()| 0i32); @@ -196,7 +251,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let Ok(offset) = u64::try_from(offset) else { return this.set_last_error_and_return(LibcError("EINVAL"), dest); }; - fd.pread(communicate, offset, buf, count, dest, this)? + fd.as_unix().pread(communicate, offset, buf, count, dest, this)? } }; interp_ok(()) @@ -236,7 +291,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let Ok(offset) = u64::try_from(offset) else { return this.set_last_error_and_return(LibcError("EINVAL"), dest); }; - fd.pwrite(communicate, buf, count, offset, dest, this)? + fd.as_unix().pwrite(communicate, buf, count, offset, dest, this)? } }; interp_ok(()) diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index f363627da93a9..3b301b10e3463 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -13,8 +13,9 @@ use rustc_data_structures::fx::FxHashMap; use self::shims::time::system_time_to_duration; use crate::helpers::check_min_arg_count; -use crate::shims::files::{EvalContextExt as _, FileDescription, FileDescriptionRef, FlockOp}; +use crate::shims::files::{EvalContextExt as _, FileDescription, FileDescriptionRef}; use crate::shims::os_str::bytes_to_os_str; +use crate::shims::unix::fd::{FlockOp, UnixFileDescription}; use crate::*; #[derive(Debug)] @@ -64,6 +65,51 @@ impl FileDescription for FileHandle { } } + fn seek<'tcx>( + &self, + communicate_allowed: bool, + offset: SeekFrom, + ) -> InterpResult<'tcx, io::Result> { + assert!(communicate_allowed, "isolation should have prevented even opening a file"); + interp_ok((&mut &self.file).seek(offset)) + } + + fn close<'tcx>( + self: Box, + communicate_allowed: bool, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result<()>> { + assert!(communicate_allowed, "isolation should have prevented even opening a file"); + // We sync the file if it was opened in a mode different than read-only. + if self.writable { + // `File::sync_all` does the checks that are done when closing a file. We do this to + // to handle possible errors correctly. + let result = self.file.sync_all(); + // Now we actually close the file and return the result. + drop(*self); + interp_ok(result) + } else { + // We drop the file, this closes it but ignores any errors + // produced when closing it. This is done because + // `File::sync_all` cannot be done over files like + // `/dev/urandom` which are read-only. Check + // https://github.com/rust-lang/miri/issues/999#issuecomment-568920439 + // for a deeper discussion. + drop(*self); + interp_ok(Ok(())) + } + } + + fn is_tty(&self, communicate_allowed: bool) -> bool { + communicate_allowed && self.file.is_terminal() + } + + fn as_unix(&self) -> &dyn UnixFileDescription { + self + } +} + +impl UnixFileDescription for FileHandle { fn pread<'tcx>( &self, communicate_allowed: bool, @@ -126,41 +172,6 @@ impl FileDescription for FileHandle { } } - fn seek<'tcx>( - &self, - communicate_allowed: bool, - offset: SeekFrom, - ) -> InterpResult<'tcx, io::Result> { - assert!(communicate_allowed, "isolation should have prevented even opening a file"); - interp_ok((&mut &self.file).seek(offset)) - } - - fn close<'tcx>( - self: Box, - communicate_allowed: bool, - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result<()>> { - assert!(communicate_allowed, "isolation should have prevented even opening a file"); - // We sync the file if it was opened in a mode different than read-only. - if self.writable { - // `File::sync_all` does the checks that are done when closing a file. We do this to - // to handle possible errors correctly. - let result = self.file.sync_all(); - // Now we actually close the file and return the result. - drop(*self); - interp_ok(result) - } else { - // We drop the file, this closes it but ignores any errors - // produced when closing it. This is done because - // `File::sync_all` cannot be done over files like - // `/dev/urandom` which are read-only. Check - // https://github.com/rust-lang/miri/issues/999#issuecomment-568920439 - // for a deeper discussion. - drop(*self); - interp_ok(Ok(())) - } - } - fn flock<'tcx>( &self, communicate_allowed: bool, @@ -255,10 +266,6 @@ impl FileDescription for FileHandle { compile_error!("flock is supported only on UNIX and Windows hosts"); } } - - fn is_tty(&self, communicate_allowed: bool) -> bool { - communicate_allowed && self.file.is_terminal() - } } impl<'tcx> EvalContextExtPrivate<'tcx> for crate::MiriInterpCx<'tcx> {} diff --git a/src/tools/miri/src/shims/unix/linux_like/epoll.rs b/src/tools/miri/src/shims/unix/linux_like/epoll.rs index d0317132c6c75..5b240351c2058 100644 --- a/src/tools/miri/src/shims/unix/linux_like/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux_like/epoll.rs @@ -6,6 +6,7 @@ use std::time::Duration; use crate::concurrency::VClock; use crate::shims::files::{FdId, FileDescription, FileDescriptionRef, WeakFileDescriptionRef}; +use crate::shims::unix::UnixFileDescription; use crate::*; /// An `Epoll` file descriptor connects file handles and epoll events @@ -151,8 +152,14 @@ impl FileDescription for Epoll { ) -> InterpResult<'tcx, io::Result<()>> { interp_ok(Ok(())) } + + fn as_unix(&self) -> &dyn UnixFileDescription { + self + } } +impl UnixFileDescription for Epoll {} + /// The table of all EpollEventInterest. /// The BTreeMap key is the FdId of an active file description registered with /// any epoll instance. The value is a list of EpollEventInterest associated @@ -594,7 +601,7 @@ fn check_and_update_one_event_interest<'tcx>( ecx: &MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, bool> { // Get the bitmask of ready events for a file description. - let ready_events_bitmask = fd_ref.get_epoll_ready_events()?.get_event_bitmask(ecx); + let ready_events_bitmask = fd_ref.as_unix().get_epoll_ready_events()?.get_event_bitmask(ecx); let epoll_event_interest = interest.borrow(); // This checks if any of the events specified in epoll_event_interest.events // match those in ready_events. diff --git a/src/tools/miri/src/shims/unix/linux_like/eventfd.rs b/src/tools/miri/src/shims/unix/linux_like/eventfd.rs index 5b51b5a0f0978..4bbe417ea8db9 100644 --- a/src/tools/miri/src/shims/unix/linux_like/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux_like/eventfd.rs @@ -5,6 +5,7 @@ use std::io::ErrorKind; use crate::concurrency::VClock; use crate::shims::files::{FileDescription, FileDescriptionRef, WeakFileDescriptionRef}; +use crate::shims::unix::UnixFileDescription; use crate::shims::unix::linux_like::epoll::{EpollReadyEvents, EvalContextExt as _}; use crate::*; @@ -36,17 +37,6 @@ impl FileDescription for Event { "event" } - fn get_epoll_ready_events<'tcx>(&self) -> InterpResult<'tcx, EpollReadyEvents> { - // We only check the status of EPOLLIN and EPOLLOUT flags for eventfd. If other event flags - // need to be supported in the future, the check should be added here. - - interp_ok(EpollReadyEvents { - epollin: self.counter.get() != 0, - epollout: self.counter.get() != MAX_COUNTER, - ..EpollReadyEvents::new() - }) - } - fn close<'tcx>( self: Box, _communicate_allowed: bool, @@ -120,6 +110,23 @@ impl FileDescription for Event { let weak_eventfd = self_ref.downgrade(); eventfd_write(num, buf_place, dest, weak_eventfd, ecx) } + + fn as_unix(&self) -> &dyn UnixFileDescription { + self + } +} + +impl UnixFileDescription for Event { + fn get_epoll_ready_events<'tcx>(&self) -> InterpResult<'tcx, EpollReadyEvents> { + // We only check the status of EPOLLIN and EPOLLOUT flags for eventfd. If other event flags + // need to be supported in the future, the check should be added here. + + interp_ok(EpollReadyEvents { + epollin: self.counter.get() != 0, + epollout: self.counter.get() != MAX_COUNTER, + ..EpollReadyEvents::new() + }) + } } impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} diff --git a/src/tools/miri/src/shims/unix/mod.rs b/src/tools/miri/src/shims/unix/mod.rs index fea5bc959c6d0..660e4ded5cda7 100644 --- a/src/tools/miri/src/shims/unix/mod.rs +++ b/src/tools/miri/src/shims/unix/mod.rs @@ -10,14 +10,14 @@ mod unnamed_socket; mod android; mod freebsd; -mod linux; +pub mod linux; mod linux_like; mod macos; mod solarish; // All the Unix-specific extension traits pub use self::env::{EvalContextExt as _, UnixEnvVars}; -pub use self::fd::EvalContextExt as _; +pub use self::fd::{EvalContextExt as _, UnixFileDescription}; pub use self::fs::{DirTable, EvalContextExt as _}; pub use self::linux_like::epoll::EpollInterestTable; pub use self::mem::EvalContextExt as _; diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index 5d2b767fe8b73..24304c0c04be9 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -13,6 +13,7 @@ use crate::concurrency::VClock; use crate::shims::files::{ EvalContextExt as _, FileDescription, FileDescriptionRef, WeakFileDescriptionRef, }; +use crate::shims::unix::UnixFileDescription; use crate::shims::unix::linux_like::epoll::{EpollReadyEvents, EvalContextExt as _}; use crate::*; @@ -61,52 +62,6 @@ impl FileDescription for AnonSocket { "socketpair" } - fn get_epoll_ready_events<'tcx>(&self) -> InterpResult<'tcx, EpollReadyEvents> { - // We only check the status of EPOLLIN, EPOLLOUT, EPOLLHUP and EPOLLRDHUP flags. - // If other event flags need to be supported in the future, the check should be added here. - - let mut epoll_ready_events = EpollReadyEvents::new(); - - // Check if it is readable. - if let Some(readbuf) = &self.readbuf { - if !readbuf.borrow().buf.is_empty() { - epoll_ready_events.epollin = true; - } - } else { - // Without a read buffer, reading never blocks, so we are always ready. - epoll_ready_events.epollin = true; - } - - // Check if is writable. - if let Some(peer_fd) = self.peer_fd().upgrade() { - if let Some(writebuf) = &peer_fd.downcast::().unwrap().readbuf { - let data_size = writebuf.borrow().buf.len(); - let available_space = MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(data_size); - if available_space != 0 { - epoll_ready_events.epollout = true; - } - } else { - // Without a write buffer, writing never blocks. - epoll_ready_events.epollout = true; - } - } else { - // Peer FD has been closed. This always sets both the RDHUP and HUP flags - // as we do not support `shutdown` that could be used to partially close the stream. - epoll_ready_events.epollrdhup = true; - epoll_ready_events.epollhup = true; - // Since the peer is closed, even if no data is available reads will return EOF and - // writes will return EPIPE. In other words, they won't block, so we mark this as ready - // for read and write. - epoll_ready_events.epollin = true; - epoll_ready_events.epollout = true; - // If there is data lost in peer_fd, set EPOLLERR. - if self.peer_lost_data.get() { - epoll_ready_events.epollerr = true; - } - } - interp_ok(epoll_ready_events) - } - fn close<'tcx>( self: Box, _communicate_allowed: bool, @@ -211,6 +166,10 @@ impl FileDescription for AnonSocket { } anonsocket_write(available_space, &peer_fd, ptr, len, dest, ecx) } + + fn as_unix(&self) -> &dyn UnixFileDescription { + self + } } /// Write to AnonSocket based on the space available and return the written byte size. @@ -290,6 +249,54 @@ fn anonsocket_read<'tcx>( ecx.return_read_success(ptr, bytes, actual_read_size, dest) } +impl UnixFileDescription for AnonSocket { + fn get_epoll_ready_events<'tcx>(&self) -> InterpResult<'tcx, EpollReadyEvents> { + // We only check the status of EPOLLIN, EPOLLOUT, EPOLLHUP and EPOLLRDHUP flags. + // If other event flags need to be supported in the future, the check should be added here. + + let mut epoll_ready_events = EpollReadyEvents::new(); + + // Check if it is readable. + if let Some(readbuf) = &self.readbuf { + if !readbuf.borrow().buf.is_empty() { + epoll_ready_events.epollin = true; + } + } else { + // Without a read buffer, reading never blocks, so we are always ready. + epoll_ready_events.epollin = true; + } + + // Check if is writable. + if let Some(peer_fd) = self.peer_fd().upgrade() { + if let Some(writebuf) = &peer_fd.downcast::().unwrap().readbuf { + let data_size = writebuf.borrow().buf.len(); + let available_space = MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(data_size); + if available_space != 0 { + epoll_ready_events.epollout = true; + } + } else { + // Without a write buffer, writing never blocks. + epoll_ready_events.epollout = true; + } + } else { + // Peer FD has been closed. This always sets both the RDHUP and HUP flags + // as we do not support `shutdown` that could be used to partially close the stream. + epoll_ready_events.epollrdhup = true; + epoll_ready_events.epollhup = true; + // Since the peer is closed, even if no data is available reads will return EOF and + // writes will return EPIPE. In other words, they won't block, so we mark this as ready + // for read and write. + epoll_ready_events.epollin = true; + epoll_ready_events.epollout = true; + // If there is data lost in peer_fd, set EPOLLERR. + if self.peer_lost_data.get() { + epoll_ready_events.epollerr = true; + } + } + interp_ok(epoll_ready_events) + } +} + impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { /// For more information on the arguments see the socketpair manpage: From 244249e464742e5d3d0193b7ed4e3505d8204684 Mon Sep 17 00:00:00 2001 From: Rune Tynan Date: Thu, 28 Nov 2024 17:18:42 -0800 Subject: [PATCH 25/44] Make metadata a FileDescription operation --- src/tools/miri/src/shims/files.rs | 10 +++++++--- src/tools/miri/src/shims/unix/fs.rs | 18 +++++++----------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/tools/miri/src/shims/files.rs b/src/tools/miri/src/shims/files.rs index 7f8b519e887de..f673b834be249 100644 --- a/src/tools/miri/src/shims/files.rs +++ b/src/tools/miri/src/shims/files.rs @@ -1,9 +1,9 @@ use std::any::Any; use std::collections::BTreeMap; -use std::io; use std::io::{IsTerminal, Read, SeekFrom, Write}; use std::ops::Deref; use std::rc::{Rc, Weak}; +use std::{fs, io}; use rustc_abi::Size; @@ -62,6 +62,10 @@ pub trait FileDescription: std::fmt::Debug + Any { throw_unsup_format!("cannot close {}", self.name()); } + fn metadata<'tcx>(&self) -> InterpResult<'tcx, io::Result> { + throw_unsup_format!("obtaining metadata is only supported on file-backed file descriptors"); + } + fn is_tty(&self, _communicate_allowed: bool) -> bool { // Most FDs are not tty's and the consequence of a wrong `false` are minor, // so we use a default impl here. @@ -273,8 +277,8 @@ impl VisitProvenance for WeakFileDescriptionRef { /// A unique id for file descriptions. While we could use the address, considering that /// is definitely unique, the address would expose interpreter internal state when used -/// for sorting things. So instead we generate a unique id per file description that stays -/// the same even if a file descriptor is duplicated and gets a new integer file descriptor. +/// for sorting things. So instead we generate a unique id per file description is the name +/// for all `dup`licates and is never reused. #[derive(Debug, Copy, Clone, Default, Eq, PartialEq, Ord, PartialOrd)] pub struct FdId(usize); diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index 3b301b10e3463..b41a4d2246ff0 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -2,7 +2,8 @@ use std::borrow::Cow; use std::fs::{ - DirBuilder, File, FileType, OpenOptions, ReadDir, read_dir, remove_dir, remove_file, rename, + DirBuilder, File, FileType, Metadata, OpenOptions, ReadDir, read_dir, remove_dir, remove_file, + rename, }; use std::io::{self, ErrorKind, IsTerminal, Read, Seek, SeekFrom, Write}; use std::path::{Path, PathBuf}; @@ -100,6 +101,10 @@ impl FileDescription for FileHandle { } } + fn metadata<'tcx>(&self) -> InterpResult<'tcx, io::Result> { + interp_ok(self.file.metadata()) + } + fn is_tty(&self, communicate_allowed: bool) -> bool { communicate_allowed && self.file.is_terminal() } @@ -1698,16 +1703,7 @@ impl FileMetadata { return interp_ok(Err(LibcError("EBADF"))); }; - let file = &fd - .downcast::() - .ok_or_else(|| { - err_unsup_format!( - "obtaining metadata is only supported on file-backed file descriptors" - ) - })? - .file; - - let metadata = file.metadata(); + let metadata = fd.metadata()?; drop(fd); FileMetadata::from_meta(ecx, metadata) } From db8bef50736160db2c06064fcdacda3c2e6dfeb0 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Wed, 4 Dec 2024 05:02:38 +0000 Subject: [PATCH 26/44] 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 e193c786effdd..377639882c59e 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -eddb717281a9031f645d88dd3b8323a7e25632cc +3b382642aba7cffbb2f47829b24635fad87bcf5c From 6c9402012e4e442aff1194f1888f773fa93b9517 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Wed, 4 Dec 2024 05:12:09 +0000 Subject: [PATCH 27/44] fmt --- src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs b/src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs index 4b49e105562ff..fd7fc801dc280 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs @@ -48,7 +48,11 @@ fn test_readlink() { // Test that we report a proper error for a missing path. let res = unsafe { - libc::readlink(c"MIRI_MISSING_FILE_NAME".as_ptr(), small_buf.as_mut_ptr().cast(), small_buf.len()) + libc::readlink( + c"MIRI_MISSING_FILE_NAME".as_ptr(), + small_buf.as_mut_ptr().cast(), + small_buf.len(), + ) }; assert_eq!(res, -1); assert_eq!(Error::last_os_error().kind(), ErrorKind::NotFound); From 76c27a98495e5d61b32ad285e5d1a6533678cf38 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 4 Dec 2024 08:15:05 +0100 Subject: [PATCH 28/44] update lockfile --- src/tools/miri/Cargo.lock | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/tools/miri/Cargo.lock b/src/tools/miri/Cargo.lock index 01e32229f2574..363b96fdff1f6 100644 --- a/src/tools/miri/Cargo.lock +++ b/src/tools/miri/Cargo.lock @@ -1,6 +1,6 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 3 +version = 4 [[package]] name = "addr2line" @@ -403,16 +403,6 @@ version = "1.0.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "49f1f14873335454500d59611f1cf4a4b0f786f9ac11f4312a78e4cf2566695b" -[[package]] -name = "jemalloc-sys" -version = "0.5.4+5.3.0-patched" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ac6c1946e1cea1788cbfde01c993b52a10e2da07f4bac608228d1bed20bfebf2" -dependencies = [ - "cc", - "libc", -] - [[package]] name = "lazy_static" version = "1.5.0" @@ -540,7 +530,6 @@ dependencies = [ "colored", "directories", "getrandom", - "jemalloc-sys", "libc", "libffi", "libloading", @@ -550,6 +539,7 @@ dependencies = [ "rustc_version", "smallvec", "tempfile", + "tikv-jemalloc-sys", "ui_test", "windows-sys 0.52.0", ] @@ -1002,6 +992,16 @@ dependencies = [ "once_cell", ] +[[package]] +name = "tikv-jemalloc-sys" +version = "0.6.0+5.3.0-1-ge13ca993e8ccb9ba9847cc330696e02839f328f7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cd3c60906412afa9c2b5b5a48ca6a5abe5736aec9eb48ad05037a677e52e4e2d" +dependencies = [ + "cc", + "libc", +] + [[package]] name = "tracing" version = "0.1.40" From 91bd957a21bc936f2d8732c658c0f4a1737361b3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 4 Dec 2024 08:35:31 +0100 Subject: [PATCH 29/44] implement simd_relaxed_fma --- src/tools/miri/src/intrinsics/simd.rs | 17 +++- .../intrinsics/fmuladd_nondeterministic.rs | 91 +++++++++++++------ .../tests/pass/intrinsics/portable-simd.rs | 22 +++++ 3 files changed, 97 insertions(+), 33 deletions(-) diff --git a/src/tools/miri/src/intrinsics/simd.rs b/src/tools/miri/src/intrinsics/simd.rs index 075b6f35e0ee0..54bdd3f02c2c2 100644 --- a/src/tools/miri/src/intrinsics/simd.rs +++ b/src/tools/miri/src/intrinsics/simd.rs @@ -1,4 +1,5 @@ use either::Either; +use rand::Rng; use rustc_abi::{Endian, HasDataLayout}; use rustc_apfloat::{Float, Round}; use rustc_middle::ty::FloatTy; @@ -286,7 +287,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_scalar(val, &dest)?; } } - "fma" => { + "fma" | "relaxed_fma" => { let [a, b, c] = check_arg_count(args)?; let (a, a_len) = this.project_to_simd(a)?; let (b, b_len) = this.project_to_simd(b)?; @@ -303,6 +304,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let c = this.read_scalar(&this.project_index(&c, i)?)?; let dest = this.project_index(&dest, i)?; + let fuse: bool = intrinsic_name == "fma" || this.machine.rng.get_mut().gen(); + // Works for f32 and f64. // FIXME: using host floats to work around https://github.com/rust-lang/miri/issues/2468. let ty::Float(float_ty) = dest.layout.ty.kind() else { @@ -314,7 +317,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let a = a.to_f32()?; let b = b.to_f32()?; let c = c.to_f32()?; - let res = a.to_host().mul_add(b.to_host(), c.to_host()).to_soft(); + let res = if fuse { + a.to_host().mul_add(b.to_host(), c.to_host()).to_soft() + } else { + ((a * b).value + c).value + }; let res = this.adjust_nan(res, &[a, b, c]); Scalar::from(res) } @@ -322,7 +329,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let a = a.to_f64()?; let b = b.to_f64()?; let c = c.to_f64()?; - let res = a.to_host().mul_add(b.to_host(), c.to_host()).to_soft(); + let res = if fuse { + a.to_host().mul_add(b.to_host(), c.to_host()).to_soft() + } else { + ((a * b).value + c).value + }; let res = this.adjust_nan(res, &[a, b, c]); Scalar::from(res) } diff --git a/src/tools/miri/tests/pass/intrinsics/fmuladd_nondeterministic.rs b/src/tools/miri/tests/pass/intrinsics/fmuladd_nondeterministic.rs index b46cf1ddf65df..b688405c4b184 100644 --- a/src/tools/miri/tests/pass/intrinsics/fmuladd_nondeterministic.rs +++ b/src/tools/miri/tests/pass/intrinsics/fmuladd_nondeterministic.rs @@ -1,44 +1,75 @@ -#![feature(core_intrinsics)] +#![feature(core_intrinsics, portable_simd)] +use std::intrinsics::simd::simd_relaxed_fma; use std::intrinsics::{fmuladdf32, fmuladdf64}; +use std::simd::prelude::*; -fn main() { - let mut saw_zero = false; - let mut saw_nonzero = false; +fn ensure_both_happen(f: impl Fn() -> bool) -> bool { + let mut saw_true = false; + let mut saw_false = false; for _ in 0..50 { - let a = std::hint::black_box(0.1_f64); - let b = std::hint::black_box(0.2); - let c = std::hint::black_box(-a * b); - // It is unspecified whether the following operation is fused or not. The - // following evaluates to 0.0 if unfused, and nonzero (-1.66e-18) if fused. - let x = unsafe { fmuladdf64(a, b, c) }; - if x == 0.0 { - saw_zero = true; + let b = f(); + if b { + saw_true = true; } else { - saw_nonzero = true; + saw_false = true; + } + if saw_true && saw_false { + return true; } } + false +} + +fn main() { assert!( - saw_zero && saw_nonzero, + ensure_both_happen(|| { + let a = std::hint::black_box(0.1_f64); + let b = std::hint::black_box(0.2); + let c = std::hint::black_box(-a * b); + // It is unspecified whether the following operation is fused or not. The + // following evaluates to 0.0 if unfused, and nonzero (-1.66e-18) if fused. + let x = unsafe { fmuladdf64(a, b, c) }; + x == 0.0 + }), "`fmuladdf64` failed to be evaluated as both fused and unfused" ); - let mut saw_zero = false; - let mut saw_nonzero = false; - for _ in 0..50 { - let a = std::hint::black_box(0.1_f32); - let b = std::hint::black_box(0.2); - let c = std::hint::black_box(-a * b); - // It is unspecified whether the following operation is fused or not. The - // following evaluates to 0.0 if unfused, and nonzero (-8.1956386e-10) if fused. - let x = unsafe { fmuladdf32(a, b, c) }; - if x == 0.0 { - saw_zero = true; - } else { - saw_nonzero = true; - } - } assert!( - saw_zero && saw_nonzero, + ensure_both_happen(|| { + let a = std::hint::black_box(0.1_f32); + let b = std::hint::black_box(0.2); + let c = std::hint::black_box(-a * b); + // It is unspecified whether the following operation is fused or not. The + // following evaluates to 0.0 if unfused, and nonzero (-8.1956386e-10) if fused. + let x = unsafe { fmuladdf32(a, b, c) }; + x == 0.0 + }), "`fmuladdf32` failed to be evaluated as both fused and unfused" ); + + assert!( + ensure_both_happen(|| { + let a = f32x4::splat(std::hint::black_box(0.1)); + let b = f32x4::splat(std::hint::black_box(0.2)); + let c = std::hint::black_box(-a * b); + let x = unsafe { simd_relaxed_fma(a, b, c) }; + // Whether we fuse or not is a per-element decision, so sometimes these should be + // the same and sometimes not. + x[0] == x[1] + }), + "`simd_relaxed_fma` failed to be evaluated as both fused and unfused" + ); + + assert!( + ensure_both_happen(|| { + let a = f64x4::splat(std::hint::black_box(0.1)); + let b = f64x4::splat(std::hint::black_box(0.2)); + let c = std::hint::black_box(-a * b); + let x = unsafe { simd_relaxed_fma(a, b, c) }; + // Whether we fuse or not is a per-element decision, so sometimes these should be + // the same and sometimes not. + x[0] == x[1] + }), + "`simd_relaxed_fma` failed to be evaluated as both fused and unfused" + ); } diff --git a/src/tools/miri/tests/pass/intrinsics/portable-simd.rs b/src/tools/miri/tests/pass/intrinsics/portable-simd.rs index f560669dd6351..acd3502f5289d 100644 --- a/src/tools/miri/tests/pass/intrinsics/portable-simd.rs +++ b/src/tools/miri/tests/pass/intrinsics/portable-simd.rs @@ -40,6 +40,17 @@ fn simd_ops_f32() { f32x4::splat(-3.2).mul_add(b, f32x4::splat(f32::NEG_INFINITY)), f32x4::splat(f32::NEG_INFINITY) ); + + unsafe { + assert_eq!(intrinsics::simd_relaxed_fma(a, b, a), (a * b) + a); + assert_eq!(intrinsics::simd_relaxed_fma(b, b, a), (b * b) + a); + assert_eq!(intrinsics::simd_relaxed_fma(a, b, b), (a * b) + b); + assert_eq!( + intrinsics::simd_relaxed_fma(f32x4::splat(-3.2), b, f32x4::splat(f32::NEG_INFINITY)), + f32x4::splat(f32::NEG_INFINITY) + ); + } + assert_eq!((a * a).sqrt(), a); assert_eq!((b * b).sqrt(), b.abs()); @@ -94,6 +105,17 @@ fn simd_ops_f64() { f64x4::splat(-3.2).mul_add(b, f64x4::splat(f64::NEG_INFINITY)), f64x4::splat(f64::NEG_INFINITY) ); + + unsafe { + assert_eq!(intrinsics::simd_relaxed_fma(a, b, a), (a * b) + a); + assert_eq!(intrinsics::simd_relaxed_fma(b, b, a), (b * b) + a); + assert_eq!(intrinsics::simd_relaxed_fma(a, b, b), (a * b) + b); + assert_eq!( + intrinsics::simd_relaxed_fma(f64x4::splat(-3.2), b, f64x4::splat(f64::NEG_INFINITY)), + f64x4::splat(f64::NEG_INFINITY) + ); + } + assert_eq!((a * a).sqrt(), a); assert_eq!((b * b).sqrt(), b.abs()); From fceb304dfd38b3eda17bea19128488de43cfb691 Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Mon, 26 Aug 2024 22:40:17 +0200 Subject: [PATCH 30/44] Properly fix #3846 by resetting parents on lazy node creation This commit supplies a real fix, which makes retags more complicated, at the benefit of making accesses more performant. Co-authored-by: Ralf Jung --- .../tree_borrows/foreign_access_skipping.rs | 108 ++++++++++ .../src/borrow_tracker/tree_borrows/mod.rs | 10 +- .../src/borrow_tracker/tree_borrows/perms.rs | 61 +++++- .../src/borrow_tracker/tree_borrows/tree.rs | 190 ++++++++++-------- .../borrow_tracker/tree_borrows/tree/tests.rs | 27 ++- 5 files changed, 306 insertions(+), 90 deletions(-) create mode 100644 src/tools/miri/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs new file mode 100644 index 0000000000000..928b3e6baef44 --- /dev/null +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/foreign_access_skipping.rs @@ -0,0 +1,108 @@ +use super::AccessKind; +use super::tree::AccessRelatedness; + +/// To speed up tree traversals, we want to skip traversing subtrees when we know the traversal will have no effect. +/// This is often the case for foreign accesses, since usually foreign accesses happen several times in a row, but also +/// foreign accesses are idempotent. In particular, see tests `foreign_read_is_noop_after_foreign_write` and `all_transitions_idempotent`. +/// Thus, for each node we keep track of the "strongest idempotent foreign access" (SIFA), i.e. which foreign access can be skipped. +/// Note that for correctness, it is not required that this is the strongest access, just any access it is idempotent under. In particular, setting +/// it to `None` is always correct, but the point of this optimization is to have it be as strong as possible so that more accesses can be skipped. +/// This enum represents the kinds of values we store: +/// - `None` means that the node (and its subtrees) are not (guaranteed to be) idempotent under any foreign access. +/// - `Read` means that the node (and its subtrees) are idempotent under foreign reads, but not (yet / necessarily) under foreign writes. +/// - `Write` means that the node (and its subtrees) are idempotent under foreign writes. This also implies that it is idempotent under foreign +/// reads, since reads are stronger than writes (see test `foreign_read_is_noop_after_foreign_write`). In other words, this node can be skipped +/// for all foreign accesses. +/// +/// Since a traversal does not just visit a node, but instead the entire subtree, the SIFA field for a given node indicates that the access to +/// *the entire subtree* rooted at that node can be skipped. In order for this to work, we maintain the global invariant that at +/// each location, the SIFA at each child must be stronger than that at the parent. For normal reads and writes, this is easily accomplished by +/// tracking each foreign access as it occurs, so that then the next access can be skipped. This also obviously maintains the invariant, because +/// if a node undergoes a foreign access, then all its children also see this as a foreign access. However, the invariant is broken during retags, +/// because retags act across the entire allocation, but only emit a read event across a specific range. This means that for all nodes outside that +/// range, the invariant is potentially broken, since a new child with a weaker SIFA is inserted. Thus, during retags, special care is taken to +/// "manually" reset the parent's SIFA to be at least as strong as the new child's. This is accomplished with the `ensure_no_stronger_than` method. +/// +/// Note that we derive Ord and PartialOrd, so the order in which variants are listed below matters: +/// None < Read < Write. Do not change that order. See the `test_order` test. +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Default)] +pub enum IdempotentForeignAccess { + #[default] + None, + Read, + Write, +} + +impl IdempotentForeignAccess { + /// Returns true if a node where the strongest idempotent foreign access is `self` + /// can skip the access `happening_next`. Note that if this returns + /// `true`, then the entire subtree will be skipped. + pub fn can_skip_foreign_access(self, happening_next: IdempotentForeignAccess) -> bool { + debug_assert!(happening_next.is_foreign()); + // This ordering is correct. Intuitively, if the last access here was + // a foreign write, everything can be skipped, since after a foreign write, + // all further foreign accesses are idempotent + happening_next <= self + } + + /// Updates `self` to account for a foreign access. + pub fn record_new(&mut self, just_happened: IdempotentForeignAccess) { + if just_happened.is_local() { + // If the access is local, reset it. + *self = IdempotentForeignAccess::None; + } else { + // Otherwise, keep it or stengthen it. + *self = just_happened.max(*self); + } + } + + /// Returns true if this access is local. + pub fn is_local(self) -> bool { + matches!(self, IdempotentForeignAccess::None) + } + + /// Returns true if this access is foreign, i.e. not local. + pub fn is_foreign(self) -> bool { + !self.is_local() + } + + /// Constructs a foreign access from an `AccessKind` + pub fn from_foreign(acc: AccessKind) -> IdempotentForeignAccess { + match acc { + AccessKind::Read => Self::Read, + AccessKind::Write => Self::Write, + } + } + + /// Usually, tree traversals have an `AccessKind` and an `AccessRelatedness`. + /// This methods converts these into the corresponding `IdempotentForeignAccess`, to be used + /// to e.g. invoke `can_skip_foreign_access`. + pub fn from_acc_and_rel(acc: AccessKind, rel: AccessRelatedness) -> IdempotentForeignAccess { + if rel.is_foreign() { Self::from_foreign(acc) } else { Self::None } + } + + /// During retags, the SIFA needs to be weakened to account for children with weaker SIFAs being inserted. + /// Thus, this method is called from the bottom up on each parent, until it returns false, which means the + /// "children have stronger SIFAs" invariant is restored. + pub fn ensure_no_stronger_than(&mut self, strongest_allowed: IdempotentForeignAccess) -> bool { + if *self > strongest_allowed { + *self = strongest_allowed; + true + } else { + false + } + } +} + +#[cfg(test)] +mod tests { + use super::IdempotentForeignAccess; + + #[test] + fn test_order() { + // The internal logic relies on this order. + // Do not change. + assert!(IdempotentForeignAccess::None < IdempotentForeignAccess::Read); + assert!(IdempotentForeignAccess::Read < IdempotentForeignAccess::Write); + } +} diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs index 40467aa4bc1be..37fbcc7e3a40d 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs @@ -9,6 +9,7 @@ use crate::concurrency::data_race::NaReadType; use crate::*; pub mod diagnostics; +mod foreign_access_skipping; mod perms; mod tree; mod unimap; @@ -296,7 +297,14 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.machine.current_span(), )?; // Record the parent-child pair in the tree. - tree_borrows.new_child(orig_tag, new_tag, new_perm.initial_state, range, span)?; + tree_borrows.new_child( + orig_tag, + new_tag, + new_perm.initial_state, + range, + span, + new_perm.protector.is_some(), + )?; drop(tree_borrows); // Also inform the data race model (but only if any bytes are actually affected). diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs index 28f9dec7bb98a..6e157d3fcd34e 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs @@ -48,6 +48,7 @@ enum PermissionPriv { Disabled, } use self::PermissionPriv::*; +use super::foreign_access_skipping::IdempotentForeignAccess; impl PartialOrd for PermissionPriv { /// PermissionPriv is ordered by the reflexive transitive closure of @@ -87,6 +88,28 @@ impl PermissionPriv { fn compatible_with_protector(&self) -> bool { !matches!(self, ReservedIM) } + + /// See `foreign_access_skipping.rs`. Computes the SIFA of a permission. + fn strongest_idempotent_foreign_access(&self, prot: bool) -> IdempotentForeignAccess { + match self { + // A protected non-conflicted Reserved will become conflicted under a foreign read, + // and is hence not idempotent under it. + ReservedFrz { conflicted } if prot && !conflicted => IdempotentForeignAccess::None, + // Otherwise, foreign reads do not affect Reserved + ReservedFrz { .. } => IdempotentForeignAccess::Read, + // Famously, ReservedIM survives foreign writes. It is never protected. + ReservedIM if prot => unreachable!("Protected ReservedIM should not exist!"), + ReservedIM => IdempotentForeignAccess::Write, + // Active changes on any foreign access (becomes Frozen/Disabled). + Active => IdempotentForeignAccess::None, + // Frozen survives foreign reads, but not writes. + Frozen => IdempotentForeignAccess::Read, + // Disabled survives foreign reads and writes. It survives them + // even if protected, because a protected `Disabled` is not initialized + // and does therefore not trigger UB. + Disabled => IdempotentForeignAccess::Write, + } + } } /// This module controls how each permission individually reacts to an access. @@ -305,6 +328,13 @@ impl Permission { (Disabled, _) => false, } } + + /// Returns the strongest foreign action this node survives (without change), + /// where `prot` indicates if it is protected. + /// See `foreign_access_skipping` + pub fn strongest_idempotent_foreign_access(&self, prot: bool) -> IdempotentForeignAccess { + self.inner.strongest_idempotent_foreign_access(prot) + } } impl PermTransition { @@ -575,7 +605,7 @@ mod propagation_optimization_checks { impl Exhaustive for AccessRelatedness { fn exhaustive() -> Box> { use AccessRelatedness::*; - Box::new(vec![This, StrictChildAccess, AncestorAccess, DistantAccess].into_iter()) + Box::new(vec![This, StrictChildAccess, AncestorAccess, CousinAccess].into_iter()) } } @@ -634,6 +664,35 @@ mod propagation_optimization_checks { } } + #[test] + #[rustfmt::skip] + fn permission_sifa_is_correct() { + // Tests that `strongest_idempotent_foreign_access` is correct. See `foreign_access_skipping.rs`. + for perm in PermissionPriv::exhaustive() { + // Assert that adding a protector makes it less idempotent. + if perm.compatible_with_protector() { + assert!(perm.strongest_idempotent_foreign_access(true) <= perm.strongest_idempotent_foreign_access(false)); + } + for prot in bool::exhaustive() { + if prot { + precondition!(perm.compatible_with_protector()); + } + let access = perm.strongest_idempotent_foreign_access(prot); + // We now assert it is idempotent, and never causes UB. + // First, if the SIFA includes foreign reads, assert it is idempotent under foreign reads. + if access >= IdempotentForeignAccess::Read { + // We use `CousinAccess` here. We could also use `AncestorAccess`, since `transition::perform_access` treats these the same. + // The only place they are treated differently is in protector end accesses, but these are not handled here. + assert_eq!(perm, transition::perform_access(AccessKind::Read, AccessRelatedness::CousinAccess, perm, prot).unwrap()); + } + // Then, if the SIFA includes foreign writes, assert it is idempotent under foreign writes. + if access >= IdempotentForeignAccess::Write { + assert_eq!(perm, transition::perform_access(AccessKind::Write, AccessRelatedness::CousinAccess, perm, prot).unwrap()); + } + } + } + } + #[test] // Check that all transitions are consistent with the order on PermissionPriv, // i.e. Reserved -> Active -> Frozen -> Disabled 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 a551b017dfc76..6d4ec36f7b696 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -21,6 +21,7 @@ use crate::borrow_tracker::tree_borrows::Permission; use crate::borrow_tracker::tree_borrows::diagnostics::{ self, NodeDebugInfo, TbError, TransitionError, }; +use crate::borrow_tracker::tree_borrows::foreign_access_skipping::IdempotentForeignAccess; use crate::borrow_tracker::tree_borrows::perms::PermTransition; use crate::borrow_tracker::tree_borrows::unimap::{UniEntry, UniIndex, UniKeyMap, UniValMap}; use crate::borrow_tracker::{GlobalState, ProtectorKind}; @@ -45,28 +46,28 @@ pub(super) struct LocationState { initialized: bool, /// This pointer's current permission / future initial permission. permission: Permission, - /// Strongest foreign access whose effects have already been applied to - /// this node and all its children since the last child access. - /// This is `None` if the most recent access is a child access, - /// `Some(Write)` if at least one foreign write access has been applied - /// since the previous child access, and `Some(Read)` if at least one - /// foreign read and no foreign write have occurred since the last child access. - latest_foreign_access: Option, + /// See `foreign_access_skipping.rs`. + /// Stores an idempotent foreign access for this location and its children. + /// For correctness, this must not be too strong, and the recorded idempotent foreign access + /// of all children must be at least as strong as this. For performance, it should be as strong as possible. + idempotent_foreign_access: IdempotentForeignAccess, } impl LocationState { /// Constructs a new initial state. It has neither been accessed, nor been subjected /// to any foreign access yet. /// The permission is not allowed to be `Active`. - fn new_uninit(permission: Permission) -> Self { + /// `sifa` is the (strongest) idempotent foreign access, see `foreign_access_skipping.rs` + fn new_uninit(permission: Permission, sifa: IdempotentForeignAccess) -> Self { assert!(permission.is_initial() || permission.is_disabled()); - Self { permission, initialized: false, latest_foreign_access: None } + Self { permission, initialized: false, idempotent_foreign_access: sifa } } /// Constructs a new initial state. It has not yet been subjected /// to any foreign access. However, it is already marked as having been accessed. - fn new_init(permission: Permission) -> Self { - Self { permission, initialized: true, latest_foreign_access: None } + /// `sifa` is the (strongest) idempotent foreign access, see `foreign_access_skipping.rs` + fn new_init(permission: Permission, sifa: IdempotentForeignAccess) -> Self { + Self { permission, initialized: true, idempotent_foreign_access: sifa } } /// Check if the location has been initialized, i.e. if it has @@ -143,52 +144,21 @@ impl LocationState { } } - // Helper to optimize the tree traversal. - // The optimization here consists of observing thanks to the tests - // `foreign_read_is_noop_after_foreign_write` and `all_transitions_idempotent`, - // that there are actually just three possible sequences of events that can occur - // in between two child accesses that produce different results. - // - // Indeed, - // - applying any number of foreign read accesses is the same as applying - // exactly one foreign read, - // - applying any number of foreign read or write accesses is the same - // as applying exactly one foreign write. - // therefore the three sequences of events that can produce different - // outcomes are - // - an empty sequence (`self.latest_foreign_access = None`) - // - a nonempty read-only sequence (`self.latest_foreign_access = Some(Read)`) - // - a nonempty sequence with at least one write (`self.latest_foreign_access = Some(Write)`) - // - // This function not only determines if skipping the propagation right now - // is possible, it also updates the internal state to keep track of whether - // the propagation can be skipped next time. - // It is a performance loss not to call this function when a foreign access occurs. - // FIXME: This optimization is wrong, and is currently disabled (by ignoring the - // result returned here). Since we presumably want an optimization like this, - // we should add it back. See #3864 for more information. + /// Tree traversal optimizations. See `foreign_access_skipping.rs`. + /// This checks if such a foreign access can be skipped. fn skip_if_known_noop( &self, access_kind: AccessKind, rel_pos: AccessRelatedness, ) -> ContinueTraversal { if rel_pos.is_foreign() { - let new_access_noop = match (self.latest_foreign_access, access_kind) { - // Previously applied transition makes the new one a guaranteed - // noop in the two following cases: - // (1) justified by `foreign_read_is_noop_after_foreign_write` - (Some(AccessKind::Write), AccessKind::Read) => true, - // (2) justified by `all_transitions_idempotent` - (Some(old), new) if old == new => true, - // In all other cases there has been a recent enough - // child access that the effects of the new foreign access - // need to be applied to this subtree. - _ => false, - }; + let happening_now = IdempotentForeignAccess::from_foreign(access_kind); + let new_access_noop = + self.idempotent_foreign_access.can_skip_foreign_access(happening_now); if new_access_noop { - // Abort traversal if the new transition is indeed guaranteed + // Abort traversal if the new access is indeed guaranteed // to be noop. - // No need to update `self.latest_foreign_access`, + // No need to update `self.idempotent_foreign_access`, // the type of the current streak among nonempty read-only // or nonempty with at least one write has not changed. ContinueTraversal::SkipSelfAndChildren @@ -207,20 +177,17 @@ impl LocationState { } /// Records a new access, so that future access can potentially be skipped - /// by `skip_if_known_noop`. - /// The invariants for this function are closely coupled to the function above: - /// It MUST be called on child accesses, and on foreign accesses MUST be called - /// when `skip_if_know_noop` returns `Recurse`, and MUST NOT be called otherwise. - /// FIXME: This optimization is wrong, and is currently disabled (by ignoring the - /// result returned here). Since we presumably want an optimization like this, - /// we should add it back. See #3864 for more information. - #[allow(unused)] + /// by `skip_if_known_noop`. This must be called on child accesses, and otherwise + /// shoud be called on foreign accesses for increased performance. It should not be called + /// when `skip_if_known_noop` indicated skipping, since it then is a no-op. + /// See `foreign_access_skipping.rs` fn record_new_access(&mut self, access_kind: AccessKind, rel_pos: AccessRelatedness) { - if rel_pos.is_foreign() { - self.latest_foreign_access = Some(access_kind); - } else { - self.latest_foreign_access = None; - } + debug_assert!(matches!( + self.skip_if_known_noop(access_kind, rel_pos), + ContinueTraversal::Recurse + )); + self.idempotent_foreign_access + .record_new(IdempotentForeignAccess::from_acc_and_rel(access_kind, rel_pos)); } } @@ -277,6 +244,11 @@ pub(super) struct Node { /// It is only ever `Disabled` for a tree root, since the root is initialized to `Active` by /// its own separate mechanism. default_initial_perm: Permission, + /// The default initial (strongest) idempotent foreign access. + /// This participates in the invariant for `LocationState::idempotent_foreign_access` + /// in cases where there is no location state yet. See `foreign_access_skipping.rs`, + /// and `LocationState::idempotent_foreign_access` for more information + default_initial_idempotent_foreign_access: IdempotentForeignAccess, /// Some extra information useful only for debugging purposes pub debug_info: NodeDebugInfo, } @@ -430,7 +402,7 @@ where } self.stack.push(( child, - AccessRelatedness::DistantAccess, + AccessRelatedness::CousinAccess, RecursionState::BeforeChildren, )); } @@ -591,6 +563,8 @@ impl Tree { parent: None, children: SmallVec::default(), default_initial_perm: root_default_perm, + // The root may never be skipped, all accesses will be local. + default_initial_idempotent_foreign_access: IdempotentForeignAccess::None, debug_info, }); nodes @@ -601,7 +575,10 @@ impl Tree { // We also ensure that it is initialized, so that no `Active` but // not yet initialized nodes exist. Essentially, we pretend there // was a write that initialized these to `Active`. - perms.insert(root_idx, LocationState::new_init(Permission::new_active())); + perms.insert( + root_idx, + LocationState::new_init(Permission::new_active(), IdempotentForeignAccess::None), + ); RangeMap::new(size, perms) }; Self { root: root_idx, nodes, rperms, tag_mapping } @@ -617,29 +594,79 @@ impl<'tcx> Tree { default_initial_perm: Permission, reborrow_range: AllocRange, span: Span, + prot: bool, ) -> InterpResult<'tcx> { assert!(!self.tag_mapping.contains_key(&new_tag)); let idx = self.tag_mapping.insert(new_tag); let parent_idx = self.tag_mapping.get(&parent_tag).unwrap(); + let strongest_idempotent = default_initial_perm.strongest_idempotent_foreign_access(prot); // Create the node self.nodes.insert(idx, Node { tag: new_tag, parent: Some(parent_idx), children: SmallVec::default(), default_initial_perm, + default_initial_idempotent_foreign_access: strongest_idempotent, debug_info: NodeDebugInfo::new(new_tag, default_initial_perm, span), }); // Register new_tag as a child of parent_tag self.nodes.get_mut(parent_idx).unwrap().children.push(idx); // Initialize perms - let perm = LocationState::new_init(default_initial_perm); + let perm = LocationState::new_init(default_initial_perm, strongest_idempotent); for (_perms_range, perms) in self.rperms.iter_mut(reborrow_range.start, reborrow_range.size) { perms.insert(idx, perm); } + + // Inserting the new perms might have broken the SIFA invariant (see `foreign_access_skipping.rs`). + // We now weaken the recorded SIFA for our parents, until the invariant is restored. + // We could weaken them all to `LocalAccess`, but it is more efficient to compute the SIFA + // for the new permission statically, and use that. + self.update_last_accessed_after_retag(parent_idx, strongest_idempotent); + interp_ok(()) } + /// Restores the SIFA "children are stronger" invariant after a retag. + /// See `foreign_access_skipping` and `new_child`. + fn update_last_accessed_after_retag( + &mut self, + mut current: UniIndex, + strongest_allowed: IdempotentForeignAccess, + ) { + // We walk the tree upwards, until the invariant is restored + loop { + let current_node = self.nodes.get_mut(current).unwrap(); + // Call `ensure_no_stronger_than` on all SIFAs for this node: the per-location SIFA, as well + // as the default SIFA for not-yet-initialized locations. + // Record whether we did any change; if not, the invariant is restored and we can stop the traversal. + let mut any_change = false; + for (_, map) in self.rperms.iter_mut_all() { + // Check if this node has a state for this location (or range of locations). + if let Some(perm) = map.get_mut(current) { + // Update the per-location SIFA, recording if it changed. + any_change |= + perm.idempotent_foreign_access.ensure_no_stronger_than(strongest_allowed); + } + } + // Now update `default_initial_idempotent_foreign_access`, which stores the default SIFA for not-yet-initialized locations. + any_change |= current_node + .default_initial_idempotent_foreign_access + .ensure_no_stronger_than(strongest_allowed); + + if any_change { + let Some(next) = self.nodes.get(current).unwrap().parent else { + // We have arrived at the root. + break; + }; + current = next; + continue; + } else { + break; + } + } + } + /// Deallocation requires /// - a pointer that permits write accesses /// - the absence of Strong Protectors anywhere in the allocation @@ -731,13 +758,8 @@ impl<'tcx> Tree { let node_skipper = |access_kind: AccessKind, args: &NodeAppArgs<'_>| -> ContinueTraversal { let NodeAppArgs { node, perm, rel_pos } = args; - let old_state = perm - .get() - .copied() - .unwrap_or_else(|| LocationState::new_uninit(node.default_initial_perm)); - // FIXME: See #3684 - let _would_skip_if_not_for_fixme = old_state.skip_if_known_noop(access_kind, *rel_pos); - ContinueTraversal::Recurse + let old_state = perm.get().copied().unwrap_or_else(|| node.default_location_state()); + old_state.skip_if_known_noop(access_kind, *rel_pos) }; let node_app = |perms_range: Range, access_kind: AccessKind, @@ -746,10 +768,12 @@ impl<'tcx> Tree { -> Result<(), TransitionError> { let NodeAppArgs { node, mut perm, rel_pos } = args; - let old_state = perm.or_insert(LocationState::new_uninit(node.default_initial_perm)); + let old_state = perm.or_insert(node.default_location_state()); - // FIXME: See #3684 - // old_state.record_new_access(access_kind, rel_pos); + // Call this function now, which ensures it is only called when + // `skip_if_known_noop` returns `Recurse`, due to the contract of + // `traverse_this_parents_children_other`. + old_state.record_new_access(access_kind, rel_pos); let protected = global.borrow().protected_tags.contains_key(&node.tag); let transition = old_state.perform_access(access_kind, rel_pos, protected)?; @@ -976,6 +1000,15 @@ impl Tree { } } +impl Node { + pub fn default_location_state(&self) -> LocationState { + LocationState::new_uninit( + self.default_initial_perm, + self.default_initial_idempotent_foreign_access, + ) + } +} + impl VisitProvenance for Tree { fn visit_provenance(&self, visit: &mut VisitWith<'_>) { // To ensure that the root never gets removed, we visit it @@ -998,15 +1031,14 @@ pub enum AccessRelatedness { AncestorAccess, /// The accessed pointer is neither of the above. // It's a cousin/uncle/etc., something in a side branch. - // FIXME: find a better name ? - DistantAccess, + CousinAccess, } impl AccessRelatedness { /// Check that access is either Ancestor or Distant, i.e. not /// a transitive child (initial pointer included). pub fn is_foreign(self) -> bool { - matches!(self, AccessRelatedness::AncestorAccess | AccessRelatedness::DistantAccess) + matches!(self, AccessRelatedness::AncestorAccess | AccessRelatedness::CousinAccess) } /// Given the AccessRelatedness for the parent node, compute the AccessRelatedness @@ -1016,7 +1048,7 @@ impl AccessRelatedness { use AccessRelatedness::*; match self { AncestorAccess | This => AncestorAccess, - StrictChildAccess | DistantAccess => DistantAccess, + StrictChildAccess | CousinAccess => CousinAccess, } } } diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree/tests.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree/tests.rs index 5d51a72852ca9..a429940748c86 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree/tests.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree/tests.rs @@ -10,7 +10,11 @@ impl Exhaustive for LocationState { fn exhaustive() -> Box> { // We keep `latest_foreign_access` at `None` as that's just a cache. Box::new(<(Permission, bool)>::exhaustive().map(|(permission, initialized)| { - Self { permission, initialized, latest_foreign_access: None } + Self { + permission, + initialized, + idempotent_foreign_access: IdempotentForeignAccess::default(), + } })) } } @@ -260,15 +264,15 @@ mod spurious_read { match xy_rel { RelPosXY::MutuallyForeign => match self { - PtrSelector::X => (This, DistantAccess), - PtrSelector::Y => (DistantAccess, This), - PtrSelector::Other => (DistantAccess, DistantAccess), + PtrSelector::X => (This, CousinAccess), + PtrSelector::Y => (CousinAccess, This), + PtrSelector::Other => (CousinAccess, CousinAccess), }, RelPosXY::XChildY => match self { PtrSelector::X => (This, StrictChildAccess), PtrSelector::Y => (AncestorAccess, This), - PtrSelector::Other => (DistantAccess, DistantAccess), + PtrSelector::Other => (CousinAccess, CousinAccess), }, } } @@ -598,13 +602,18 @@ mod spurious_read { let source = LocStateProtPair { xy_rel: RelPosXY::MutuallyForeign, x: LocStateProt { - state: LocationState::new_init(Permission::new_frozen()), + // For the tests, the strongest idempotent foreign access does not matter, so we use `Default::default` + state: LocationState::new_init( + Permission::new_frozen(), + IdempotentForeignAccess::default(), + ), prot: true, }, y: LocStateProt { - state: LocationState::new_uninit(Permission::new_reserved( - /* freeze */ true, /* protected */ true, - )), + state: LocationState::new_uninit( + Permission::new_reserved(/* freeze */ true, /* protected */ true), + IdempotentForeignAccess::default(), + ), prot: true, }, }; From 8d28ec4b54dbfdd2b9408b18c78fc3c43faa8cf2 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 22 Nov 2024 14:21:33 +0100 Subject: [PATCH 31/44] extend some comments regarding weak memory emulation --- src/tools/miri/src/concurrency/data_race.rs | 14 ++++++++++---- src/tools/miri/src/concurrency/weak_memory.rs | 9 +++++++-- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index f86d1eb1dcbcc..e1fc22f685499 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -108,19 +108,25 @@ pub(super) struct ThreadClockSet { fence_acquire: VClock, /// The last timestamp of happens-before relations that - /// have been released by this thread by a fence. + /// have been released by this thread by a release fence. fence_release: VClock, /// Timestamps of the last SC fence performed by each - /// thread, updated when this thread performs an SC fence + /// thread, updated when this thread performs an SC fence. + /// This is never acquired into the thread's clock, it + /// just limits which old writes can be seen in weak memory emulation. pub(super) fence_seqcst: VClock, /// Timestamps of the last SC write performed by each - /// thread, updated when this thread performs an SC fence + /// thread, updated when this thread performs an SC fence. + /// This is never acquired into the thread's clock, it + /// just limits which old writes can be seen in weak memory emulation. pub(super) write_seqcst: VClock, /// Timestamps of the last SC fence performed by each - /// thread, updated when this thread performs an SC read + /// thread, updated when this thread performs an SC read. + /// This is never acquired into the thread's clock, it + /// just limits which old writes can be seen in weak memory emulation. pub(super) read_seqcst: VClock, } diff --git a/src/tools/miri/src/concurrency/weak_memory.rs b/src/tools/miri/src/concurrency/weak_memory.rs index c610f1999f7d7..7dac171b147de 100644 --- a/src/tools/miri/src/concurrency/weak_memory.rs +++ b/src/tools/miri/src/concurrency/weak_memory.rs @@ -15,6 +15,10 @@ //! Specifically, if an SC load reads from an atomic store of any ordering, then a later SC load cannot read from //! an earlier store in the location's modification order. This is to prevent creating a backwards S edge from the second //! load to the first, as a result of C++20's coherence-ordered before rules. +//! (This seems to rule out behaviors that were actually permitted by the RC11 model that C++20 +//! intended to copy (https://plv.mpi-sws.org/scfix/paper.pdf); a change was introduced when +//! translating the math to English. According to Viktor Vafeiadis, this difference is harmless. So +//! we stick to what the standard says, and allow fewer behaviors.) //! //! Rust follows the C++20 memory model (except for the Consume ordering and some operations not performable through C++'s //! `std::atomic` API). It is therefore possible for this implementation to generate behaviours never observable when the @@ -138,6 +142,7 @@ struct StoreElement { /// The timestamp of the storing thread when it performed the store timestamp: VTimestamp, + /// The value of this store. `None` means uninitialized. // FIXME: Currently, we cannot represent partial initialization. val: Option, @@ -356,9 +361,9 @@ impl<'tcx> StoreBuffer { false } else if is_seqcst && store_elem.load_info.borrow().sc_loaded { // The current SC load cannot read-before a store that an earlier SC load has observed. - // See https://github.com/rust-lang/miri/issues/2301#issuecomment-1222720427 + // See https://github.com/rust-lang/miri/issues/2301#issuecomment-1222720427. // Consequences of C++20 §31.4 [atomics.order] paragraph 3.1, 3.3 (coherence-ordered before) - // and 4.1 (coherence-ordered before between SC makes global total order S) + // and 4.1 (coherence-ordered before between SC makes global total order S). false } else { true From 9449cb9563d5a69a5cb9ee18437a438314e957e1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 24 Nov 2024 15:20:11 +0100 Subject: [PATCH 32/44] move GlobalState definition further up so the types are mor concentrated at the top of the file --- src/tools/miri/src/concurrency/data_race.rs | 196 ++++++++++---------- 1 file changed, 98 insertions(+), 98 deletions(-) diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index e1fc22f685499..eb1734294677b 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -262,6 +262,104 @@ enum AccessType { AtomicRmw, } +/// Per-byte vector clock metadata for data-race detection. +#[derive(Clone, PartialEq, Eq, Debug)] +struct MemoryCellClocks { + /// The vector-clock timestamp and the thread that did the last non-atomic write. We don't need + /// a full `VClock` here, it's always a single thread and nothing synchronizes, so the effective + /// clock is all-0 except for the thread that did the write. + write: (VectorIdx, VTimestamp), + + /// The type of operation that the write index represents, + /// either newly allocated memory, a non-atomic write or + /// a deallocation of memory. + write_type: NaWriteType, + + /// The vector-clock of all non-atomic reads that happened since the last non-atomic write + /// (i.e., we join together the "singleton" clocks corresponding to each read). It is reset to + /// zero on each write operation. + read: VClock, + + /// Atomic access, acquire, release sequence tracking clocks. + /// For non-atomic memory this value is set to None. + /// For atomic memory, each byte carries this information. + atomic_ops: Option>, +} + +/// Extra metadata associated with a thread. +#[derive(Debug, Clone, Default)] +struct ThreadExtraState { + /// The current vector index in use by the + /// thread currently, this is set to None + /// after the vector index has been re-used + /// and hence the value will never need to be + /// read during data-race reporting. + vector_index: Option, + + /// Thread termination vector clock, this + /// is set on thread termination and is used + /// for joining on threads since the vector_index + /// may be re-used when the join operation occurs. + termination_vector_clock: Option, +} + +/// Global data-race detection state, contains the currently +/// executing thread as well as the vector-clocks associated +/// with each of the threads. +// FIXME: it is probably better to have one large RefCell, than to have so many small ones. +#[derive(Debug, Clone)] +pub struct GlobalState { + /// Set to true once the first additional + /// thread has launched, due to the dependency + /// between before and after a thread launch. + /// Any data-races must be recorded after this + /// so concurrent execution can ignore recording + /// any data-races. + multi_threaded: Cell, + + /// A flag to mark we are currently performing + /// a data race free action (such as atomic access) + /// to suppress the race detector + ongoing_action_data_race_free: Cell, + + /// Mapping of a vector index to a known set of thread + /// clocks, this is not directly mapping from a thread id + /// since it may refer to multiple threads. + vector_clocks: RefCell>, + + /// Mapping of a given vector index to the current thread + /// that the execution is representing, this may change + /// if a vector index is re-assigned to a new thread. + vector_info: RefCell>, + + /// The mapping of a given thread to associated thread metadata. + thread_info: RefCell>, + + /// Potential vector indices that could be re-used on thread creation + /// values are inserted here on after the thread has terminated and + /// been joined with, and hence may potentially become free + /// for use as the index for a new thread. + /// Elements in this set may still require the vector index to + /// report data-races, and can only be re-used after all + /// active vector-clocks catch up with the threads timestamp. + reuse_candidates: RefCell>, + + /// The timestamp of last SC fence performed by each thread + last_sc_fence: RefCell, + + /// The timestamp of last SC write performed by each thread + last_sc_write: RefCell, + + /// Track when an outdated (weak memory) load happens. + pub track_outdated_loads: bool, +} + +impl VisitProvenance for GlobalState { + fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { + // We don't have any tags. + } +} + impl AccessType { fn description(self, ty: Option>, size: Option) -> String { let mut msg = String::new(); @@ -315,30 +413,6 @@ impl AccessType { } } -/// Per-byte vector clock metadata for data-race detection. -#[derive(Clone, PartialEq, Eq, Debug)] -struct MemoryCellClocks { - /// The vector-clock timestamp and the thread that did the last non-atomic write. We don't need - /// a full `VClock` here, it's always a single thread and nothing synchronizes, so the effective - /// clock is all-0 except for the thread that did the write. - write: (VectorIdx, VTimestamp), - - /// The type of operation that the write index represents, - /// either newly allocated memory, a non-atomic write or - /// a deallocation of memory. - write_type: NaWriteType, - - /// The vector-clock of all non-atomic reads that happened since the last non-atomic write - /// (i.e., we join together the "singleton" clocks corresponding to each read). It is reset to - /// zero on each write operation. - read: VClock, - - /// Atomic access, acquire, release sequence tracking clocks. - /// For non-atomic memory this value is set to None. - /// For atomic memory, each byte carries this information. - atomic_ops: Option>, -} - impl AtomicMemoryCellClocks { fn new(size: Size) -> Self { AtomicMemoryCellClocks { @@ -1469,80 +1543,6 @@ trait EvalContextPrivExt<'tcx>: MiriInterpCxExt<'tcx> { } } -/// Extra metadata associated with a thread. -#[derive(Debug, Clone, Default)] -struct ThreadExtraState { - /// The current vector index in use by the - /// thread currently, this is set to None - /// after the vector index has been re-used - /// and hence the value will never need to be - /// read during data-race reporting. - vector_index: Option, - - /// Thread termination vector clock, this - /// is set on thread termination and is used - /// for joining on threads since the vector_index - /// may be re-used when the join operation occurs. - termination_vector_clock: Option, -} - -/// Global data-race detection state, contains the currently -/// executing thread as well as the vector-clocks associated -/// with each of the threads. -// FIXME: it is probably better to have one large RefCell, than to have so many small ones. -#[derive(Debug, Clone)] -pub struct GlobalState { - /// Set to true once the first additional - /// thread has launched, due to the dependency - /// between before and after a thread launch. - /// Any data-races must be recorded after this - /// so concurrent execution can ignore recording - /// any data-races. - multi_threaded: Cell, - - /// A flag to mark we are currently performing - /// a data race free action (such as atomic access) - /// to suppress the race detector - ongoing_action_data_race_free: Cell, - - /// Mapping of a vector index to a known set of thread - /// clocks, this is not directly mapping from a thread id - /// since it may refer to multiple threads. - vector_clocks: RefCell>, - - /// Mapping of a given vector index to the current thread - /// that the execution is representing, this may change - /// if a vector index is re-assigned to a new thread. - vector_info: RefCell>, - - /// The mapping of a given thread to associated thread metadata. - thread_info: RefCell>, - - /// Potential vector indices that could be re-used on thread creation - /// values are inserted here on after the thread has terminated and - /// been joined with, and hence may potentially become free - /// for use as the index for a new thread. - /// Elements in this set may still require the vector index to - /// report data-races, and can only be re-used after all - /// active vector-clocks catch up with the threads timestamp. - reuse_candidates: RefCell>, - - /// The timestamp of last SC fence performed by each thread - last_sc_fence: RefCell, - - /// The timestamp of last SC write performed by each thread - last_sc_write: RefCell, - - /// Track when an outdated (weak memory) load happens. - pub track_outdated_loads: bool, -} - -impl VisitProvenance for GlobalState { - fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { - // We don't have any tags. - } -} - impl GlobalState { /// Create a new global state, setup with just thread-id=0 /// advanced to timestamp = 1. From fe856815aaa45c745ade87237c0dc5b2cf23c7d0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 24 Nov 2024 15:35:53 +0100 Subject: [PATCH 33/44] make SC fences stronger, to be correct wrt C++20 --- src/tools/miri/README.md | 7 +- src/tools/miri/src/concurrency/data_race.rs | 32 +++--- src/tools/miri/src/concurrency/weak_memory.rs | 24 +++-- .../tests/fail/should-pass/cpp20_rwc_syncs.rs | 87 ----------------- .../fail/should-pass/cpp20_rwc_syncs.stderr | 20 ---- .../tests/pass/0weak_memory_consistency.rs | 97 +++++++++++++++++-- 6 files changed, 120 insertions(+), 147 deletions(-) delete mode 100644 src/tools/miri/tests/fail/should-pass/cpp20_rwc_syncs.rs delete mode 100644 src/tools/miri/tests/fail/should-pass/cpp20_rwc_syncs.stderr diff --git a/src/tools/miri/README.md b/src/tools/miri/README.md index 9a683ae68fd71..4e30dea18ff4a 100644 --- a/src/tools/miri/README.md +++ b/src/tools/miri/README.md @@ -68,9 +68,10 @@ Further caveats that Miri users should be aware of: not support networking. System API support varies between targets; if you run on Windows it is a good idea to use `--target x86_64-unknown-linux-gnu` to get better support. -* Weak memory emulation may [produce weak behaviors](https://github.com/rust-lang/miri/issues/2301) - when `SeqCst` fences are used that are not actually permitted by the Rust memory model, and it - cannot produce all behaviors possibly observable on real hardware. +* Weak memory emulation is not complete: there are legal behaviors that Miri will never produce. + However, Miri produces many behaviors that are hard to observe on real hardware, so it can help + quite a bit in finding weak memory concurrency bugs. To be really sure about complicated atomic + code, use specialized tools such as [loom](https://github.com/tokio-rs/loom). Moreover, Miri fundamentally cannot ensure that your code is *sound*. [Soundness] is the property of never causing undefined behavior when invoked from arbitrary safe code, even in combination with diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index eb1734294677b..5cb68634b7a12 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -111,12 +111,6 @@ pub(super) struct ThreadClockSet { /// have been released by this thread by a release fence. fence_release: VClock, - /// Timestamps of the last SC fence performed by each - /// thread, updated when this thread performs an SC fence. - /// This is never acquired into the thread's clock, it - /// just limits which old writes can be seen in weak memory emulation. - pub(super) fence_seqcst: VClock, - /// Timestamps of the last SC write performed by each /// thread, updated when this thread performs an SC fence. /// This is never acquired into the thread's clock, it @@ -344,11 +338,13 @@ pub struct GlobalState { /// active vector-clocks catch up with the threads timestamp. reuse_candidates: RefCell>, - /// The timestamp of last SC fence performed by each thread + /// We make SC fences act like RMWs on a global location. + /// To implement that, they all release and acquire into this clock. last_sc_fence: RefCell, - /// The timestamp of last SC write performed by each thread - last_sc_write: RefCell, + /// The timestamp of last SC write performed by each thread. + /// Threads only update their own index here! + last_sc_write_per_thread: RefCell, /// Track when an outdated (weak memory) load happens. pub track_outdated_loads: bool, @@ -883,9 +879,17 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> { clocks.apply_release_fence(); } if atomic == AtomicFenceOrd::SeqCst { - data_race.last_sc_fence.borrow_mut().set_at_index(&clocks.clock, index); - clocks.fence_seqcst.join(&data_race.last_sc_fence.borrow()); - clocks.write_seqcst.join(&data_race.last_sc_write.borrow()); + // Behave like an RMW on the global fence location. This takes full care of + // all the SC fence requirements, including C++17 §32.4 [atomics.order] + // paragraph 6 (which would limit what future reads can see). It also rules + // out many legal behaviors, but we don't currently have a model that would + // be more precise. + let mut sc_fence_clock = data_race.last_sc_fence.borrow_mut(); + sc_fence_clock.join(&clocks.clock); + clocks.clock.join(&sc_fence_clock); + // Also establish some sort of order with the last SC write that happened, globally + // (but this is only respected by future reads). + clocks.write_seqcst.join(&data_race.last_sc_write_per_thread.borrow()); } // Increment timestamp in case of release semantics. @@ -1555,7 +1559,7 @@ impl GlobalState { thread_info: RefCell::new(IndexVec::new()), reuse_candidates: RefCell::new(FxHashSet::default()), last_sc_fence: RefCell::new(VClock::default()), - last_sc_write: RefCell::new(VClock::default()), + last_sc_write_per_thread: RefCell::new(VClock::default()), track_outdated_loads: config.track_outdated_loads, }; @@ -1857,7 +1861,7 @@ impl GlobalState { // SC ATOMIC STORE rule in the paper. pub(super) fn sc_write(&self, thread_mgr: &ThreadManager<'_>) { let (index, clocks) = self.active_thread_state(thread_mgr); - self.last_sc_write.borrow_mut().set_at_index(&clocks.clock, index); + self.last_sc_write_per_thread.borrow_mut().set_at_index(&clocks.clock, index); } // SC ATOMIC READ rule in the paper. diff --git a/src/tools/miri/src/concurrency/weak_memory.rs b/src/tools/miri/src/concurrency/weak_memory.rs index 7dac171b147de..1a3e9614f8af0 100644 --- a/src/tools/miri/src/concurrency/weak_memory.rs +++ b/src/tools/miri/src/concurrency/weak_memory.rs @@ -11,14 +11,17 @@ //! This implementation is not fully correct under the revised C++20 model and may generate behaviours C++20 //! disallows (). //! -//! A modification is made to the paper's model to partially address C++20 changes. -//! Specifically, if an SC load reads from an atomic store of any ordering, then a later SC load cannot read from -//! an earlier store in the location's modification order. This is to prevent creating a backwards S edge from the second -//! load to the first, as a result of C++20's coherence-ordered before rules. -//! (This seems to rule out behaviors that were actually permitted by the RC11 model that C++20 -//! intended to copy (https://plv.mpi-sws.org/scfix/paper.pdf); a change was introduced when -//! translating the math to English. According to Viktor Vafeiadis, this difference is harmless. So -//! we stick to what the standard says, and allow fewer behaviors.) +//! Modifications are made to the paper's model to address C++20 changes: +//! - If an SC load reads from an atomic store of any ordering, then a later SC load cannot read +//! from an earlier store in the location's modification order. This is to prevent creating a +//! backwards S edge from the second load to the first, as a result of C++20's coherence-ordered +//! before rules. (This seems to rule out behaviors that were actually permitted by the RC11 model +//! that C++20 intended to copy (); a change was +//! introduced when translating the math to English. According to Viktor Vafeiadis, this +//! difference is harmless. So we stick to what the standard says, and allow fewer behaviors.) +//! - SC fences are treated like AcqRel RMWs to a global clock, to ensure they induce enough +//! synchronization with the surrounding accesses. This rules out legal behavior, but it is really +//! hard to be more precise here. //! //! Rust follows the C++20 memory model (except for the Consume ordering and some operations not performable through C++'s //! `std::atomic` API). It is therefore possible for this implementation to generate behaviours never observable when the @@ -340,11 +343,6 @@ impl<'tcx> StoreBuffer { // then we cannot read-from anything earlier in modification order. // C++20 §6.9.2.2 [intro.races] paragraph 16 false - } else if store_elem.timestamp <= clocks.fence_seqcst[store_elem.store_index] { - // The current load, which may be sequenced-after an SC fence, cannot read-before - // the last store sequenced-before an SC fence in another thread. - // C++17 §32.4 [atomics.order] paragraph 6 - false } else if store_elem.timestamp <= clocks.write_seqcst[store_elem.store_index] && store_elem.is_seqcst { diff --git a/src/tools/miri/tests/fail/should-pass/cpp20_rwc_syncs.rs b/src/tools/miri/tests/fail/should-pass/cpp20_rwc_syncs.rs deleted file mode 100644 index cebad507ea9d2..0000000000000 --- a/src/tools/miri/tests/fail/should-pass/cpp20_rwc_syncs.rs +++ /dev/null @@ -1,87 +0,0 @@ -//@compile-flags: -Zmiri-ignore-leaks - -// https://plv.mpi-sws.org/scfix/paper.pdf -// 2.2 Second Problem: SC Fences are Too Weak -// This test should pass under the C++20 model Rust is using. -// Unfortunately, Miri's weak memory emulation only follows the C++11 model -// as we don't know how to correctly emulate C++20's revised SC semantics, -// so we have to stick to C++11 emulation from existing research. - -use std::sync::atomic::Ordering::*; -use std::sync::atomic::{AtomicUsize, fence}; -use std::thread::spawn; - -// Spins until it reads the given value -fn reads_value(loc: &AtomicUsize, val: usize) -> usize { - while loc.load(Relaxed) != val { - std::hint::spin_loop(); - } - val -} - -// We can't create static items because we need to run each test -// multiple tests -fn static_atomic(val: usize) -> &'static AtomicUsize { - let ret = Box::leak(Box::new(AtomicUsize::new(val))); - // A workaround to put the initialization value in the store buffer. - // See https://github.com/rust-lang/miri/issues/2164 - ret.load(Relaxed); - ret -} - -fn test_cpp20_rwc_syncs() { - /* - int main() { - atomic_int x = 0; - atomic_int y = 0; - - {{{ x.store(1,mo_relaxed); - ||| { r1=x.load(mo_relaxed).readsvalue(1); - fence(mo_seq_cst); - r2=y.load(mo_relaxed); } - ||| { y.store(1,mo_relaxed); - fence(mo_seq_cst); - r3=x.load(mo_relaxed); } - }}} - return 0; - } - */ - let x = static_atomic(0); - let y = static_atomic(0); - - let j1 = spawn(move || { - x.store(1, Relaxed); - }); - - let j2 = spawn(move || { - reads_value(&x, 1); - fence(SeqCst); - y.load(Relaxed) - }); - - let j3 = spawn(move || { - y.store(1, Relaxed); - fence(SeqCst); - x.load(Relaxed) - }); - - j1.join().unwrap(); - let b = j2.join().unwrap(); - let c = j3.join().unwrap(); - - // We cannot write assert_ne!() since ui_test's fail - // tests expect exit status 1, whereas panics produce 101. - // Our ui_test does not yet support overriding failure status codes. - if (b, c) == (0, 0) { - // This *should* be unreachable, but Miri will reach it. - unsafe { - std::hint::unreachable_unchecked(); //~ERROR: unreachable - } - } -} - -pub fn main() { - for _ in 0..500 { - test_cpp20_rwc_syncs(); - } -} diff --git a/src/tools/miri/tests/fail/should-pass/cpp20_rwc_syncs.stderr b/src/tools/miri/tests/fail/should-pass/cpp20_rwc_syncs.stderr deleted file mode 100644 index 5185845568e7c..0000000000000 --- a/src/tools/miri/tests/fail/should-pass/cpp20_rwc_syncs.stderr +++ /dev/null @@ -1,20 +0,0 @@ -error: Undefined Behavior: entering unreachable code - --> tests/fail/should-pass/cpp20_rwc_syncs.rs:LL:CC - | -LL | std::hint::unreachable_unchecked(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ entering unreachable code - | - = 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 `test_cpp20_rwc_syncs` at tests/fail/should-pass/cpp20_rwc_syncs.rs:LL:CC -note: inside `main` - --> tests/fail/should-pass/cpp20_rwc_syncs.rs:LL:CC - | -LL | test_cpp20_rwc_syncs(); - | ^^^^^^^^^^^^^^^^^^^^^^ - -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/0weak_memory_consistency.rs b/src/tools/miri/tests/pass/0weak_memory_consistency.rs index 10f7aed9418ab..840dd5693976c 100644 --- a/src/tools/miri/tests/pass/0weak_memory_consistency.rs +++ b/src/tools/miri/tests/pass/0weak_memory_consistency.rs @@ -22,7 +22,7 @@ // Available: https://ss265.host.cs.st-andrews.ac.uk/papers/n3132.pdf. use std::sync::atomic::Ordering::*; -use std::sync::atomic::{AtomicBool, AtomicI32, fence}; +use std::sync::atomic::{AtomicBool, AtomicI32, Ordering, fence}; use std::thread::spawn; #[derive(Copy, Clone)] @@ -45,8 +45,8 @@ fn static_atomic_bool(val: bool) -> &'static AtomicBool { } // Spins until it acquires a pre-determined value. -fn acquires_value(loc: &AtomicI32, val: i32) -> i32 { - while loc.load(Acquire) != val { +fn loads_value(loc: &AtomicI32, ord: Ordering, val: i32) -> i32 { + while loc.load(ord) != val { std::hint::spin_loop(); } val @@ -69,7 +69,7 @@ fn test_corr() { }); // | | #[rustfmt::skip] // |synchronizes-with |happens-before let j3 = spawn(move || { // | | - acquires_value(&y, 1); // <------------------+ | + loads_value(&y, Acquire, 1); // <------------+ | x.load(Relaxed) // <----------------------------------------------+ // The two reads on x are ordered by hb, so they cannot observe values // differently from the modification order. If the first read observed @@ -94,12 +94,12 @@ fn test_wrc() { }); // | | #[rustfmt::skip] // |synchronizes-with | let j2 = spawn(move || { // | | - acquires_value(&x, 1); // <------------------+ | + loads_value(&x, Acquire, 1); // <------------+ | y.store(1, Release); // ---------------------+ |happens-before }); // | | #[rustfmt::skip] // |synchronizes-with | let j3 = spawn(move || { // | | - acquires_value(&y, 1); // <------------------+ | + loads_value(&y, Acquire, 1); // <------------+ | x.load(Relaxed) // <-----------------------------------------------+ }); @@ -125,7 +125,7 @@ fn test_message_passing() { #[rustfmt::skip] // |synchronizes-with | happens-before let j2 = spawn(move || { // | | let x = x; // avoid field capturing | | - acquires_value(&y, 1); // <------------------+ | + loads_value(&y, Acquire, 1); // <------------+ | unsafe { *x.0 } // <---------------------------------------------+ }); @@ -268,9 +268,6 @@ fn test_iriw_sc_rlx() { let x = static_atomic_bool(false); let y = static_atomic_bool(false); - x.store(false, Relaxed); - y.store(false, Relaxed); - let a = spawn(move || x.store(true, Relaxed)); let b = spawn(move || y.store(true, Relaxed)); let c = spawn(move || { @@ -290,6 +287,84 @@ fn test_iriw_sc_rlx() { assert!(c || d); } +// Similar to `test_iriw_sc_rlx` but with fences instead of SC accesses. +fn test_cpp20_sc_fence_fix() { + let x = static_atomic_bool(false); + let y = static_atomic_bool(false); + + let thread1 = spawn(|| { + let a = x.load(Relaxed); + fence(SeqCst); + let b = y.load(Relaxed); + (a, b) + }); + + let thread2 = spawn(|| { + x.store(true, Relaxed); + }); + let thread3 = spawn(|| { + y.store(true, Relaxed); + }); + + let thread4 = spawn(|| { + let c = y.load(Relaxed); + fence(SeqCst); + let d = x.load(Relaxed); + (c, d) + }); + + let (a, b) = thread1.join().unwrap(); + thread2.join().unwrap(); + thread3.join().unwrap(); + let (c, d) = thread4.join().unwrap(); + let bad = a == true && b == false && c == true && d == false; + assert!(!bad); +} + +// https://plv.mpi-sws.org/scfix/paper.pdf +// 2.2 Second Problem: SC Fences are Too Weak +fn test_cpp20_rwc_syncs() { + /* + int main() { + atomic_int x = 0; + atomic_int y = 0; + {{{ x.store(1,mo_relaxed); + ||| { r1=x.load(mo_relaxed).readsvalue(1); + fence(mo_seq_cst); + r2=y.load(mo_relaxed); } + ||| { y.store(1,mo_relaxed); + fence(mo_seq_cst); + r3=x.load(mo_relaxed); } + }}} + return 0; + } + */ + let x = static_atomic(0); + let y = static_atomic(0); + + let j1 = spawn(move || { + x.store(1, Relaxed); + }); + + let j2 = spawn(move || { + loads_value(&x, Relaxed, 1); + fence(SeqCst); + y.load(Relaxed) + }); + + let j3 = spawn(move || { + y.store(1, Relaxed); + fence(SeqCst); + x.load(Relaxed) + }); + + j1.join().unwrap(); + let b = j2.join().unwrap(); + let c = j3.join().unwrap(); + + assert!((b, c) != (0, 0)); +} + pub fn main() { for _ in 0..50 { test_single_thread(); @@ -301,5 +376,7 @@ pub fn main() { test_sc_store_buffering(); test_sync_through_rmw_and_fences(); test_iriw_sc_rlx(); + test_cpp20_sc_fence_fix(); + test_cpp20_rwc_syncs(); } } From 16d549a6ce8e5fee7a85d98841e470b84566c09d Mon Sep 17 00:00:00 2001 From: tiif Date: Thu, 5 Dec 2024 23:39:29 +0800 Subject: [PATCH 34/44] Avoid passing byte slice to anonsocket_read --- src/tools/miri/src/shims/unix/unnamed_socket.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index 24304c0c04be9..40a76ea7439a2 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -90,11 +90,9 @@ impl FileDescription for AnonSocket { dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - let mut bytes = vec![0; len]; - // Always succeed on read size 0. if len == 0 { - return ecx.return_read_success(ptr, &bytes, 0, dest); + return ecx.return_read_success(ptr, &[], 0, dest); } let Some(readbuf) = &self.readbuf else { @@ -106,7 +104,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. - return ecx.return_read_success(ptr, &bytes, 0, dest); + return ecx.return_read_success(ptr, &[], 0, dest); } else { if self.is_nonblock { // Non-blocking socketpair with writer and empty buffer. @@ -123,7 +121,7 @@ impl FileDescription for AnonSocket { } } // TODO: We might need to decide what to do if peer_fd is closed when read is blocked. - anonsocket_read(self, self.peer_fd().upgrade(), &mut bytes, ptr, dest, ecx) + anonsocket_read(self, self.peer_fd().upgrade(), len, ptr, dest, ecx) } fn write<'tcx>( @@ -211,11 +209,13 @@ fn anonsocket_write<'tcx>( fn anonsocket_read<'tcx>( anonsocket: &AnonSocket, peer_fd: Option, - bytes: &mut [u8], + len: usize, ptr: Pointer, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { + let mut bytes = vec![0; len]; + let Some(readbuf) = &anonsocket.readbuf else { // FIXME: This should return EBADF, but there's no nice way to do that as there's no // corresponding ErrorKind variant. @@ -230,7 +230,7 @@ fn anonsocket_read<'tcx>( // Do full read / partial read based on the space available. // Conveniently, `read` exists on `VecDeque` and has exactly the desired behavior. - let actual_read_size = readbuf.buf.read(bytes).unwrap(); + let actual_read_size = readbuf.buf.read(&mut bytes[..]).unwrap(); // Need to drop before others can access the readbuf again. drop(readbuf); @@ -246,7 +246,7 @@ fn anonsocket_read<'tcx>( ecx.check_and_update_readiness(&peer_fd)?; } - ecx.return_read_success(ptr, bytes, actual_read_size, dest) + ecx.return_read_success(ptr, &bytes, actual_read_size, dest) } impl UnixFileDescription for AnonSocket { From cfc3924d163bb6e665705abc8bd385df36754373 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Fri, 6 Dec 2024 05:02:50 +0000 Subject: [PATCH 35/44] 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 377639882c59e..bc81038c961a9 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -3b382642aba7cffbb2f47829b24635fad87bcf5c +706141b8d9090228343340378b1d4a2b095fa1fb From a5622a2061e34486bb74414774c22a1228f6367a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 6 Dec 2024 09:57:13 +0100 Subject: [PATCH 36/44] fix SC fence logic --- src/tools/miri/src/concurrency/data_race.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/tools/miri/src/concurrency/data_race.rs b/src/tools/miri/src/concurrency/data_race.rs index 5cb68634b7a12..4cdc9348dc9f8 100644 --- a/src/tools/miri/src/concurrency/data_race.rs +++ b/src/tools/miri/src/concurrency/data_race.rs @@ -874,16 +874,14 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> { // Either Acquire | AcqRel | SeqCst clocks.apply_acquire_fence(); } - if atomic != AtomicFenceOrd::Acquire { - // Either Release | AcqRel | SeqCst - clocks.apply_release_fence(); - } if atomic == AtomicFenceOrd::SeqCst { // Behave like an RMW on the global fence location. This takes full care of // all the SC fence requirements, including C++17 §32.4 [atomics.order] // paragraph 6 (which would limit what future reads can see). It also rules // out many legal behaviors, but we don't currently have a model that would // be more precise. + // Also see the second bullet on page 10 of + // . let mut sc_fence_clock = data_race.last_sc_fence.borrow_mut(); sc_fence_clock.join(&clocks.clock); clocks.clock.join(&sc_fence_clock); @@ -891,6 +889,12 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> { // (but this is only respected by future reads). clocks.write_seqcst.join(&data_race.last_sc_write_per_thread.borrow()); } + // The release fence is last, since both of the above could alter our clock, + // which should be part of what is being released. + if atomic != AtomicFenceOrd::Acquire { + // Either Release | AcqRel | SeqCst + clocks.apply_release_fence(); + } // Increment timestamp in case of release semantics. interp_ok(atomic != AtomicFenceOrd::Acquire) From 644faf4c93e401096abd97b639a2f827ba70c17c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 6 Dec 2024 19:51:19 +0100 Subject: [PATCH 37/44] add test --- .../tests/pass/0weak_memory_consistency.rs | 42 ++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/tests/pass/0weak_memory_consistency.rs b/src/tools/miri/tests/pass/0weak_memory_consistency.rs index 840dd5693976c..27581be81824f 100644 --- a/src/tools/miri/tests/pass/0weak_memory_consistency.rs +++ b/src/tools/miri/tests/pass/0weak_memory_consistency.rs @@ -1,4 +1,4 @@ -//@compile-flags: -Zmiri-ignore-leaks -Zmiri-disable-stacked-borrows -Zmiri-provenance-gc=10000 +//@compile-flags: -Zmiri-ignore-leaks -Zmiri-disable-stacked-borrows -Zmiri-disable-validation -Zmiri-provenance-gc=10000 // This test's runtime explodes if the GC interval is set to 1 (which we do in CI), so we // override it internally back to the default frequency. @@ -365,6 +365,45 @@ fn test_cpp20_rwc_syncs() { assert!((b, c) != (0, 0)); } +/// This checks that the *last* thing the SC fence does is act like a release fence. +/// See . +fn test_sc_fence_release() { + let x = static_atomic(0); + let y = static_atomic(0); + let z = static_atomic(0); + let k = static_atomic(0); + + let j1 = spawn(move || { + x.store(1, Relaxed); + fence(SeqCst); + k.store(1, Relaxed); + }); + let j2 = spawn(move || { + y.store(1, Relaxed); + fence(SeqCst); + z.store(1, Relaxed); + }); + + let j3 = spawn(move || { + let kval = k.load(Acquire); + let yval = y.load(Relaxed); + (kval, yval) + }); + let j4 = spawn(move || { + let zval = z.load(Acquire); + let xval = x.load(Relaxed); + (zval, xval) + }); + + j1.join().unwrap(); + j2.join().unwrap(); + let (kval, yval) = j3.join().unwrap(); + let (zval, xval) = j4.join().unwrap(); + + let bad = kval == 1 && yval == 0 && zval == 1 && xval == 0; + assert!(!bad); +} + pub fn main() { for _ in 0..50 { test_single_thread(); @@ -378,5 +417,6 @@ pub fn main() { test_iriw_sc_rlx(); test_cpp20_sc_fence_fix(); test_cpp20_rwc_syncs(); + test_sc_fence_release(); } } From 016fb485fa9dd1a177b53bb436a280e387e9022b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 6 Dec 2024 19:54:22 +0100 Subject: [PATCH 38/44] remove a no-longer-needed work-around --- src/tools/miri/tests/pass/0weak_memory_consistency.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/tools/miri/tests/pass/0weak_memory_consistency.rs b/src/tools/miri/tests/pass/0weak_memory_consistency.rs index 27581be81824f..c0d0287855b27 100644 --- a/src/tools/miri/tests/pass/0weak_memory_consistency.rs +++ b/src/tools/miri/tests/pass/0weak_memory_consistency.rs @@ -34,14 +34,10 @@ unsafe impl Sync for EvilSend {} // We can't create static items because we need to run each test // multiple times fn static_atomic(val: i32) -> &'static AtomicI32 { - let ret = Box::leak(Box::new(AtomicI32::new(val))); - ret.store(val, Relaxed); // work around https://github.com/rust-lang/miri/issues/2164 - ret + Box::leak(Box::new(AtomicI32::new(val))) } fn static_atomic_bool(val: bool) -> &'static AtomicBool { - let ret = Box::leak(Box::new(AtomicBool::new(val))); - ret.store(val, Relaxed); // work around https://github.com/rust-lang/miri/issues/2164 - ret + Box::leak(Box::new(AtomicBool::new(val))) } // Spins until it acquires a pre-determined value. From 8afc3c695e22d8f816e8b3da1af671c432d5e59f Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sat, 7 Dec 2024 04:55:54 +0000 Subject: [PATCH 39/44] 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 bc81038c961a9..ee21f2136cce4 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -706141b8d9090228343340378b1d4a2b095fa1fb +75716b45105e443199ce9800c7009ddfd6d2be53 From 8f05e4c4cedd1f7db7d4615fcdfc56c16be2a07e Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sat, 7 Dec 2024 05:05:09 +0000 Subject: [PATCH 40/44] fmt --- src/tools/miri/src/machine.rs | 5 ++++- src/tools/miri/src/shims/native_lib.rs | 8 +++----- .../miri/tests/native-lib/pass/ptr_write_access.rs | 12 +++--------- 3 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 888465c5262e2..7cc22f83a24a6 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -1245,7 +1245,10 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> { /// Called on `ptr as usize` casts. /// (Actually computing the resulting `usize` doesn't need machine help, /// that's just `Scalar::try_to_int`.) - fn expose_provenance(ecx: &InterpCx<'tcx, Self>, provenance: Self::Provenance) -> InterpResult<'tcx> { + fn expose_provenance( + ecx: &InterpCx<'tcx, Self>, + provenance: Self::Provenance, + ) -> InterpResult<'tcx> { match provenance { Provenance::Concrete { alloc_id, tag } => ecx.expose_ptr(alloc_id, tag), Provenance::Wildcard => { diff --git a/src/tools/miri/src/shims/native_lib.rs b/src/tools/miri/src/shims/native_lib.rs index 4082b8eed4597..f18d023677492 100644 --- a/src/tools/miri/src/shims/native_lib.rs +++ b/src/tools/miri/src/shims/native_lib.rs @@ -4,10 +4,8 @@ use std::ops::Deref; use libffi::high::call as ffi; use libffi::low::CodePtr; use rustc_abi::{BackendRepr, HasDataLayout, Size}; -use rustc_middle::{ - mir::interpret::Pointer, - ty::{self as ty, IntTy, UintTy}, -}; +use rustc_middle::mir::interpret::Pointer; +use rustc_middle::ty::{self as ty, IntTy, UintTy}; use rustc_span::Symbol; use crate::*; @@ -177,7 +175,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.prepare_for_native_call(alloc_id, prov)?; } } - + // FIXME: In the future, we should also call `prepare_for_native_call` on all previously // exposed allocations, since C may access any of them. diff --git a/src/tools/miri/tests/native-lib/pass/ptr_write_access.rs b/src/tools/miri/tests/native-lib/pass/ptr_write_access.rs index 4045ef3cee5ce..a92e63a4da6d8 100644 --- a/src/tools/miri/tests/native-lib/pass/ptr_write_access.rs +++ b/src/tools/miri/tests/native-lib/pass/ptr_write_access.rs @@ -3,7 +3,6 @@ //@only-on-host //@compile-flags: -Zmiri-permissive-provenance - #![feature(box_as_ptr)] use std::mem::MaybeUninit; @@ -60,7 +59,7 @@ fn test_init_array() { const LEN: usize = 3; let mut array = MaybeUninit::<[i32; LEN]>::uninit(); let val = 31; - + let array = unsafe { init_array(array.as_mut_ptr().cast::(), LEN, val); array.assume_init() @@ -72,7 +71,7 @@ fn test_init_array() { fn test_init_static_inner() { #[repr(C)] struct SyncPtr { - ptr: *mut i32 + ptr: *mut i32, } unsafe impl Sync for SyncPtr {} @@ -183,17 +182,12 @@ fn test_swap_ptr_triple_dangling() { let ptr = Box::as_ptr(&b); drop(b); let z = 121; - let triple = Triple { - ptr0: &raw const x, - ptr1: ptr, - ptr2: &raw const z - }; + let triple = Triple { ptr0: &raw const x, ptr1: ptr, ptr2: &raw const z }; unsafe { swap_ptr_triple_dangling(&triple) } assert_eq!(unsafe { *triple.ptr2 }, x); } - /// Test function that directly returns its pointer argument. fn test_return_ptr() { extern "C" { From 854dcbc5d8a92132b65fcff33d46ce318a6b1944 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 7 Dec 2024 18:14:09 +0100 Subject: [PATCH 41/44] add weak memory consistency test for mixing SC accesses and fences --- .../tests/pass/0weak_memory_consistency.rs | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/tools/miri/tests/pass/0weak_memory_consistency.rs b/src/tools/miri/tests/pass/0weak_memory_consistency.rs index c0d0287855b27..ce5b8be07f0c9 100644 --- a/src/tools/miri/tests/pass/0weak_memory_consistency.rs +++ b/src/tools/miri/tests/pass/0weak_memory_consistency.rs @@ -400,6 +400,35 @@ fn test_sc_fence_release() { assert!(!bad); } +/// Test that SC fences and accesses sync correctly with each other. +fn test_sc_fence_access() { + /* + Wx1 sc + Ry0 sc + || + Wy1 rlx + SC-fence + Rx0 rlx + */ + let x = static_atomic(0); + let y = static_atomic(0); + + let j1 = spawn(move || { + x.store(1, SeqCst); + y.load(SeqCst) + }); + let j2 = spawn(move || { + y.store(1, Relaxed); + fence(SeqCst); + x.load(Relaxed) + }); + + let v1 = j1.join().unwrap(); + let v2 = j2.join().unwrap(); + let bad = v1 == 0 && v2 == 0; + assert!(!bad); +} + pub fn main() { for _ in 0..50 { test_single_thread(); @@ -414,5 +443,6 @@ pub fn main() { test_cpp20_sc_fence_fix(); test_cpp20_rwc_syncs(); test_sc_fence_release(); + test_sc_fence_access(); } } From 7ba51929131e494482cccbec8038868dc1efef21 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sun, 8 Dec 2024 04:55:19 +0000 Subject: [PATCH 42/44] 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 ee21f2136cce4..57d0b27dfd3b7 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -75716b45105e443199ce9800c7009ddfd6d2be53 +728f2daab42ba8f1b3d5caab62495798d1eabfa1 From 03c412ead4c3f61bc85a772cde5bcc1562975b09 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 8 Dec 2024 08:21:20 +0100 Subject: [PATCH 43/44] fix build --- src/tools/miri/src/shims/io_error.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/miri/src/shims/io_error.rs b/src/tools/miri/src/shims/io_error.rs index 7596db8a1989e..acf3f74a93d8b 100644 --- a/src/tools/miri/src/shims/io_error.rs +++ b/src/tools/miri/src/shims/io_error.rs @@ -99,8 +99,8 @@ const WINDOWS_IO_ERROR_TABLE: &[(&str, std::io::ErrorKind)] = { ("ERROR_POSSIBLE_DEADLOCK", Deadlock), ("ERROR_DIR_NOT_EMPTY", DirectoryNotEmpty), ("ERROR_CANT_RESOLVE_FILENAME", FilesystemLoop), - ("ERROR_DISK_QUOTA_EXCEEDED", FilesystemQuotaExceeded), - ("WSAEDQUOT", FilesystemQuotaExceeded), + ("ERROR_DISK_QUOTA_EXCEEDED", QuotaExceeded), + ("WSAEDQUOT", QuotaExceeded), ("ERROR_FILE_TOO_LARGE", FileTooLarge), ("ERROR_HOST_UNREACHABLE", HostUnreachable), ("WSAEHOSTUNREACH", HostUnreachable), From a33b4b1ca0c172125ba832144591370cc09e4360 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 8 Dec 2024 22:54:38 +0100 Subject: [PATCH 44/44] update lockfile --- Cargo.lock | 1 - 1 file changed, 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index f59dcdcf28d69..e4e080b945776 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2274,7 +2274,6 @@ dependencies = [ "chrono", "chrono-tz", "colored", - "ctrlc", "directories", "getrandom", "libc",