From 4abdcd7fd9767238e695eed65667756ed9bf00c5 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 19 Aug 2024 08:17:24 +0200 Subject: [PATCH 01/35] provenance_gc: fix comment --- src/tools/miri/src/provenance_gc.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/src/provenance_gc.rs b/src/tools/miri/src/provenance_gc.rs index 8edd80744ddc9..5c6edf4ddcedf 100644 --- a/src/tools/miri/src/provenance_gc.rs +++ b/src/tools/miri/src/provenance_gc.rs @@ -187,9 +187,9 @@ impl LiveAllocs<'_, '_> { impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> { fn run_provenance_gc(&mut self) { - // We collect all tags from various parts of the interpreter, but also let this = self.eval_context_mut(); + // We collect all tags and AllocId from every part of the interpreter. let mut tags = FxHashSet::default(); let mut alloc_ids = FxHashSet::default(); this.visit_provenance(&mut |id, tag| { @@ -200,6 +200,8 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> { tags.insert(tag); } }); + + // Based on this, clean up the interpreter state. self.remove_unreachable_tags(tags); self.remove_unreachable_allocs(alloc_ids); } From 4001f5933eb0f959232afce75005fc34d94803f1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 19 Aug 2024 08:25:48 +0200 Subject: [PATCH 02/35] make the cleanup functions private also move "is there a borrow tracker" check out of the loop --- src/tools/miri/src/provenance_gc.rs | 50 ++++++++++++++--------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/src/tools/miri/src/provenance_gc.rs b/src/tools/miri/src/provenance_gc.rs index 5c6edf4ddcedf..d4bed69c6707a 100644 --- a/src/tools/miri/src/provenance_gc.rs +++ b/src/tools/miri/src/provenance_gc.rs @@ -184,6 +184,29 @@ impl LiveAllocs<'_, '_> { } } +fn remove_unreachable_tags<'tcx>(this: &mut MiriInterpCx<'tcx>, tags: FxHashSet) { + // Avoid iterating all allocations if there's no borrow tracker anyway. + if this.machine.borrow_tracker.is_some() { + this.memory.alloc_map().iter(|it| { + for (_id, (_kind, alloc)) in it { + alloc.extra.borrow_tracker.as_ref().unwrap().remove_unreachable_tags(&tags); + } + }); + } +} + +fn remove_unreachable_allocs<'tcx>(this: &mut MiriInterpCx<'tcx>, allocs: FxHashSet) { + let allocs = LiveAllocs { ecx: this, collected: allocs }; + this.machine.allocation_spans.borrow_mut().retain(|id, _| allocs.is_live(*id)); + this.machine.symbolic_alignment.borrow_mut().retain(|id, _| allocs.is_live(*id)); + this.machine.alloc_addresses.borrow_mut().remove_unreachable_allocs(&allocs); + if let Some(borrow_tracker) = &this.machine.borrow_tracker { + borrow_tracker.borrow_mut().remove_unreachable_allocs(&allocs); + } + // Clean up core (non-Miri-specific) state. + this.remove_unreachable_allocs(&allocs.collected); +} + impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {} pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> { fn run_provenance_gc(&mut self) { @@ -202,30 +225,7 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> { }); // Based on this, clean up the interpreter state. - self.remove_unreachable_tags(tags); - self.remove_unreachable_allocs(alloc_ids); - } - - fn remove_unreachable_tags(&mut self, tags: FxHashSet) { - let this = self.eval_context_mut(); - this.memory.alloc_map().iter(|it| { - for (_id, (_kind, alloc)) in it { - if let Some(bt) = &alloc.extra.borrow_tracker { - bt.remove_unreachable_tags(&tags); - } - } - }); - } - - fn remove_unreachable_allocs(&mut self, allocs: FxHashSet) { - let this = self.eval_context_mut(); - let allocs = LiveAllocs { ecx: this, collected: allocs }; - this.machine.allocation_spans.borrow_mut().retain(|id, _| allocs.is_live(*id)); - this.machine.symbolic_alignment.borrow_mut().retain(|id, _| allocs.is_live(*id)); - this.machine.alloc_addresses.borrow_mut().remove_unreachable_allocs(&allocs); - if let Some(borrow_tracker) = &this.machine.borrow_tracker { - borrow_tracker.borrow_mut().remove_unreachable_allocs(&allocs); - } - this.remove_unreachable_allocs(&allocs.collected); + remove_unreachable_tags(this, tags); + remove_unreachable_allocs(this, alloc_ids); } } From e698ca1246d546532dfc966a228c54918989c1b2 Mon Sep 17 00:00:00 2001 From: sun-jacobi Date: Thu, 22 Aug 2024 16:20:18 +0200 Subject: [PATCH 03/35] fix a misleading comment in TB tests --- src/tools/miri/tests/pass/tree_borrows/tree-borrows.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/tests/pass/tree_borrows/tree-borrows.rs b/src/tools/miri/tests/pass/tree_borrows/tree-borrows.rs index adad18c1af368..c741e4de6d5b1 100644 --- a/src/tools/miri/tests/pass/tree_borrows/tree-borrows.rs +++ b/src/tools/miri/tests/pass/tree_borrows/tree-borrows.rs @@ -321,7 +321,7 @@ fn not_unpin_not_protected() { pub struct NotUnpin(#[allow(dead_code)] i32, PhantomPinned); fn inner(x: &mut NotUnpin, f: fn(&mut NotUnpin)) { - // `f` may mutate, but it may not deallocate! + // `f` is allowed to deallocate `x`. f(x) } From 3a423fc6b74dda43d771c9e77439c3220870671a Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Thu, 22 Aug 2024 16:15:57 +0200 Subject: [PATCH 04/35] Avoid extra copy by using `retain_mut` and moving the deletion into the closure --- .../src/borrow_tracker/tree_borrows/tree.rs | 45 ++++++++++--------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs index 56643c6cbe811..d51e1109ed4e2 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -754,29 +754,32 @@ impl Tree { // list of remaining children back in. let mut children_of_node = mem::take(&mut self.nodes.get_mut(*tag).unwrap().children); - // Remove all useless children, and save them for later. - // The closure needs `&self` and the loop below needs `&mut self`, so we need to `collect` - // in to a temporary list. - let to_remove: Vec<_> = - children_of_node.drain_filter(|x| self.is_useless(*x, live)).collect(); + // Remove all useless children. + children_of_node.retain_mut(|idx| { + if self.is_useless(*idx, live) { + // Note: In the rest of this comment, "this node" refers to `idx`. + // This node has no more children (if there were any, they have already been removed). + // It is also unreachable as determined by the GC, so we can remove it everywhere. + // Due to the API of UniMap we must make sure to call + // `UniValMap::remove` for the key of this node on *all* maps that used it + // (which are `self.nodes` and every range of `self.rperms`) + // before we can safely apply `UniKeyMap::remove` to truly remove + // this tag from the `tag_mapping`. + let node = self.nodes.remove(*idx).unwrap(); + for (_perms_range, perms) in self.rperms.iter_mut_all() { + perms.remove(*idx); + } + self.tag_mapping.remove(&node.tag); + // now delete it + false + } else { + // do nothing, but retain + true + } + }); // Put back the now-filtered vector. self.nodes.get_mut(*tag).unwrap().children = children_of_node; - // Now, all that is left is unregistering the children saved in `to_remove`. - for idx in to_remove { - // Note: In the rest of this comment, "this node" refers to `idx`. - // This node has no more children (if there were any, they have already been removed). - // It is also unreachable as determined by the GC, so we can remove it everywhere. - // Due to the API of UniMap we must make sure to call - // `UniValMap::remove` for the key of this node on *all* maps that used it - // (which are `self.nodes` and every range of `self.rperms`) - // before we can safely apply `UniKeyMap::remove` to truly remove - // this tag from the `tag_mapping`. - let node = self.nodes.remove(idx).unwrap(); - for (_perms_range, perms) in self.rperms.iter_mut_all() { - perms.remove(idx); - } - self.tag_mapping.remove(&node.tag); - } + // We are done, the parent can continue. stack.pop(); continue; From 7f968ba98d853f29042c8e917d2d8047043551f8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 24 Aug 2024 09:58:16 +0200 Subject: [PATCH 05/35] fix calling pipe, pipe2, socketpair with a pointer-to-array --- src/tools/miri/src/shims/unix/unnamed_socket.rs | 2 +- src/tools/miri/tests/pass-dep/libc/libc-pipe.rs | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index 745f27398d0a8..127aef29244c5 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -339,7 +339,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let pipefd = this.deref_pointer(pipefd)?; + let pipefd = this.deref_pointer_as(pipefd, this.machine.layouts.i32)?; let flags = match flags { Some(flags) => this.read_scalar(flags)?.to_i32()?, None => 0, diff --git a/src/tools/miri/tests/pass-dep/libc/libc-pipe.rs b/src/tools/miri/tests/pass-dep/libc/libc-pipe.rs index 5dff612bd8930..90dbd888392bf 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-pipe.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-pipe.rs @@ -6,6 +6,7 @@ fn main() { test_pipe(); test_pipe_threaded(); test_race(); + test_pipe_array(); } fn test_pipe() { @@ -97,3 +98,13 @@ fn test_race() { thread::yield_now(); thread1.join().unwrap(); } + +fn test_pipe_array() { + // Declare `pipe` to take an array rather than a `*mut i32`. + extern "C" { + fn pipe(pipefd: &mut [i32; 2]) -> i32; + } + + let mut fds: [i32; 2] = [0; 2]; + assert_eq!(unsafe { pipe(&mut fds) }, 0); +} From 14f22c6e66b75aab02fc2a50ce18f1af638050ec Mon Sep 17 00:00:00 2001 From: tiif Date: Fri, 23 Aug 2024 21:23:09 +0800 Subject: [PATCH 06/35] epoll: Add EINVAL case --- src/tools/miri/src/shims/unix/linux/epoll.rs | 7 +++++++ src/tools/miri/tests/pass-dep/libc/libc-epoll.rs | 14 ++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index fb1e0afdf9ed7..ea430fec59340 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -251,6 +251,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_unsup_format!("epoll_ctl: encountered unknown unsupported operation {:#x}", op); } + // Throw EINVAL if epfd and fd have the same value. + if epfd_value == fd { + let einval = this.eval_libc("EINVAL"); + this.set_last_error(einval)?; + return Ok(Scalar::from_i32(-1)); + } + // Check if epfd is a valid epoll file descriptor. let Some(epfd) = this.machine.fds.get(epfd_value) else { return Ok(Scalar::from_i32(this.fd_not_found()?)); diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs index 052ce73de237f..7b8acbd120061 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs @@ -21,6 +21,7 @@ fn main() { test_socketpair_epollerr(); test_epoll_lost_events(); test_ready_list_fetching_logic(); + test_epoll_ctl_epfd_equal_fd(); } // Using `as` cast since `EPOLLET` wraps around @@ -630,3 +631,16 @@ fn test_ready_list_fetching_logic() { let expected_value1 = fd1 as u64; check_epoll_wait::<1>(epfd, &[(expected_event1, expected_value1)]); } + +// In epoll_ctl, if the value of epfd equals to fd, EINVAL should be returned. +fn test_epoll_ctl_epfd_equal_fd() { + // Create an epoll instance. + let epfd = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd, -1); + + let array_ptr = std::ptr::without_provenance_mut::(0x100); + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, epfd, array_ptr) }; + let e = std::io::Error::last_os_error(); + assert_eq!(e.raw_os_error(), Some(libc::EINVAL)); + assert_eq!(res, -1); +} From 23f308b5db6ad3f0c9fe8ad451090be17e66d9f6 Mon Sep 17 00:00:00 2001 From: tiif Date: Tue, 20 Aug 2024 16:48:05 +0800 Subject: [PATCH 07/35] Handle edge case for epoll_ctl --- src/tools/miri/src/shims/unix/linux/epoll.rs | 58 +++++++++++-------- .../libc/libc_epoll_unsupported_fd.rs | 18 ++++++ .../libc/libc_epoll_unsupported_fd.stderr | 14 +++++ .../miri/tests/pass-dep/libc/libc-epoll.rs | 39 +++++++++++++ 4 files changed, 105 insertions(+), 24 deletions(-) create mode 100644 src/tools/miri/tests/fail-dep/libc/libc_epoll_unsupported_fd.rs create mode 100644 src/tools/miri/tests/fail-dep/libc/libc_epoll_unsupported_fd.stderr diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index ea430fec59340..53f8b06ca6ad1 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -269,10 +269,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let mut interest_list = epoll_file_description.interest_list.borrow_mut(); let ready_list = &epoll_file_description.ready_list; - let Some(file_descriptor) = this.machine.fds.get(fd) else { + let Some(fd_ref) = this.machine.fds.get(fd) else { return Ok(Scalar::from_i32(this.fd_not_found()?)); }; - let id = file_descriptor.get_id(); + let id = fd_ref.get_id(); if op == epoll_ctl_add || op == epoll_ctl_mod { // Read event bitmask and data from epoll_event passed by caller. @@ -332,7 +332,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } } - let id = file_descriptor.get_id(); // Create an epoll_interest. let interest = Rc::new(RefCell::new(EpollEventInterest { file_descriptor: fd, @@ -344,7 +343,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if op == epoll_ctl_add { // Insert an epoll_interest to global epoll_interest list. this.machine.epoll_interests.insert_epoll_interest(id, Rc::downgrade(&interest)); - interest_list.insert(epoll_key, interest); + interest_list.insert(epoll_key, Rc::clone(&interest)); } else { // Directly modify the epoll_interest so the global epoll_event_interest table // will be updated too. @@ -353,9 +352,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { epoll_interest.data = data; } - // Readiness will be updated immediately when the epoll_event_interest is added or modified. - this.check_and_update_readiness(&file_descriptor)?; - + // Notification will be returned for current epfd if there is event in the file + // descriptor we registered. + check_and_update_one_event_interest(&fd_ref, interest, id, this)?; return Ok(Scalar::from_i32(0)); } else if op == epoll_ctl_del { let epoll_key = (id, fd); @@ -489,25 +488,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let id = fd_ref.get_id(); // Get a list of EpollEventInterest that is associated to a specific file description. if let Some(epoll_interests) = this.machine.epoll_interests.get_epoll_interest(id) { - let epoll_ready_events = fd_ref.get_epoll_ready_events()?; - // Get the bitmask of ready events. - let ready_events = epoll_ready_events.get_event_bitmask(this); - for weak_epoll_interest in epoll_interests { if let Some(epoll_interest) = weak_epoll_interest.upgrade() { - // This checks if any of the events specified in epoll_event_interest.events - // match those in ready_events. - let epoll_event_interest = epoll_interest.borrow(); - let flags = epoll_event_interest.events & ready_events; - // If there is any event that we are interested in being specified as ready, - // insert an epoll_return to the ready list. - if flags != 0 { - let epoll_key = (id, epoll_event_interest.file_descriptor); - let ready_list = &mut epoll_event_interest.ready_list.borrow_mut(); - let event_instance = - EpollEventInstance::new(flags, epoll_event_interest.data); - ready_list.insert(epoll_key, event_instance); - } + check_and_update_one_event_interest(fd_ref, epoll_interest, id, this)?; } } } @@ -532,3 +515,30 @@ fn ready_list_next( } return None; } + +/// This helper function checks whether an epoll notification should be triggered for a specific +/// epoll_interest and, if necessary, triggers the notification. Unlike check_and_update_readiness, +/// this function sends a notification to only one epoll instance. +fn check_and_update_one_event_interest<'tcx>( + fd_ref: &FileDescriptionRef, + interest: Rc>, + id: FdId, + ecx: &MiriInterpCx<'tcx>, +) -> InterpResult<'tcx> { + // 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 epoll_event_interest = interest.borrow(); + // This checks if any of the events specified in epoll_event_interest.events + // match those in ready_events. + let flags = epoll_event_interest.events & ready_events_bitmask; + // If there is any event that we are interested in being specified as ready, + // insert an epoll_return to the ready list. + if flags != 0 { + let epoll_key = (id, epoll_event_interest.file_descriptor); + let ready_list = &mut epoll_event_interest.ready_list.borrow_mut(); + let event_instance = EpollEventInstance::new(flags, epoll_event_interest.data); + // Triggers the notification by inserting it to the ready list. + ready_list.insert(epoll_key, event_instance); + } + Ok(()) +} diff --git a/src/tools/miri/tests/fail-dep/libc/libc_epoll_unsupported_fd.rs b/src/tools/miri/tests/fail-dep/libc/libc_epoll_unsupported_fd.rs new file mode 100644 index 0000000000000..fb2426f7b66d8 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/libc/libc_epoll_unsupported_fd.rs @@ -0,0 +1,18 @@ +//@only-target-linux + +// This is a test for registering unsupported fd with epoll. +// Register epoll fd with epoll is allowed in real system, but we do not support this. +fn main() { + // Create two epoll instance. + let epfd0 = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd0, -1); + let epfd1 = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd1, -1); + + // Register epoll with epoll. + let mut ev = + libc::epoll_event { events: (libc::EPOLLIN | libc::EPOLLET) as _, u64: epfd1 as u64 }; + let res = unsafe { libc::epoll_ctl(epfd0, libc::EPOLL_CTL_ADD, epfd1, &mut ev) }; + //~^ERROR: epoll does not support this file description + assert_eq!(res, 0); +} diff --git a/src/tools/miri/tests/fail-dep/libc/libc_epoll_unsupported_fd.stderr b/src/tools/miri/tests/fail-dep/libc/libc_epoll_unsupported_fd.stderr new file mode 100644 index 0000000000000..6f9b988d4ee2d --- /dev/null +++ b/src/tools/miri/tests/fail-dep/libc/libc_epoll_unsupported_fd.stderr @@ -0,0 +1,14 @@ +error: unsupported operation: epoll: epoll does not support this file description + --> $DIR/libc_epoll_unsupported_fd.rs:LL:CC + | +LL | let res = unsafe { libc::epoll_ctl(epfd0, libc::EPOLL_CTL_ADD, epfd1, &mut ev) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ epoll: epoll does not support this file description + | + = help: this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support + = note: BACKTRACE: + = note: inside `main` at $DIR/libc_epoll_unsupported_fd.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs index 7b8acbd120061..647b5e60649e8 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs @@ -22,6 +22,7 @@ fn main() { test_epoll_lost_events(); test_ready_list_fetching_logic(); test_epoll_ctl_epfd_equal_fd(); + test_epoll_ctl_notification(); } // Using `as` cast since `EPOLLET` wraps around @@ -644,3 +645,41 @@ fn test_epoll_ctl_epfd_equal_fd() { assert_eq!(e.raw_os_error(), Some(libc::EINVAL)); assert_eq!(res, -1); } + +// We previously used check_and_update_readiness the moment a file description is registered in an +// epoll instance. But this has an unfortunate side effect of returning notification to another +// epfd that shouldn't receive notification. +fn test_epoll_ctl_notification() { + // Create an epoll instance. + let epfd0 = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd0, -1); + + // Create a socketpair instance. + let mut fds = [-1, -1]; + let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; + assert_eq!(res, 0); + + // Register one side of the socketpair with epoll. + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fds[0] as u64 }; + let res = unsafe { libc::epoll_ctl(epfd0, libc::EPOLL_CTL_ADD, fds[0], &mut ev) }; + assert_eq!(res, 0); + + // epoll_wait to clear notification for epfd0. + let expected_event = u32::try_from(libc::EPOLLOUT).unwrap(); + let expected_value = fds[0] as u64; + check_epoll_wait::<1>(epfd0, &[(expected_event, expected_value)]); + + // Create another epoll instance. + let epfd1 = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd1, -1); + + // Register the same file description for epfd1. + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fds[0] as u64 }; + let res = unsafe { libc::epoll_ctl(epfd1, libc::EPOLL_CTL_ADD, fds[0], &mut ev) }; + assert_eq!(res, 0); + check_epoll_wait::<1>(epfd1, &[(expected_event, expected_value)]); + + // Previously this epoll_wait will receive a notification, but we shouldn't return notification + // for this epfd, because there is no I/O event between the two epoll_wait. + check_epoll_wait::<1>(epfd0, &[]); +} From f71cdbbc467ee0f13703dabe025fdd1f49f287f2 Mon Sep 17 00:00:00 2001 From: tiif Date: Mon, 19 Aug 2024 15:13:12 +0800 Subject: [PATCH 08/35] Support blocking for epoll --- src/tools/miri/src/concurrency/thread.rs | 2 + src/tools/miri/src/shims/unix/fd.rs | 8 + src/tools/miri/src/shims/unix/linux/epoll.rs | 151 +++++++++++++++--- .../src/shims/unix/linux/foreign_items.rs | 3 +- .../miri/tests/fail-dep/tokio/sleep.stderr | 15 -- .../pass-dep/libc/libc-epoll-blocking.rs | 98 ++++++++++++ ...ibc-epoll.rs => libc-epoll-no-blocking.rs} | 0 .../{fail-dep => pass-dep}/tokio/sleep.rs | 2 - 8 files changed, 239 insertions(+), 40 deletions(-) delete mode 100644 src/tools/miri/tests/fail-dep/tokio/sleep.stderr create mode 100644 src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs rename src/tools/miri/tests/pass-dep/libc/{libc-epoll.rs => libc-epoll-no-blocking.rs} (100%) rename src/tools/miri/tests/{fail-dep => pass-dep}/tokio/sleep.rs (80%) diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index f72591f0c4bdf..1b119ae71924e 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -172,6 +172,8 @@ pub enum BlockReason { Futex { addr: u64 }, /// Blocked on an InitOnce. InitOnce(InitOnceId), + /// Blocked on epoll + Epoll, } /// The state of a thread. diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index e3b9835e360a6..3ca5f6bb2dff5 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -278,6 +278,14 @@ impl WeakFileDescriptionRef { } } +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 diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 53f8b06ca6ad1..a0baa781dea3a 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -2,8 +2,9 @@ use std::cell::RefCell; use std::collections::BTreeMap; use std::io; use std::rc::{Rc, Weak}; +use std::time::Duration; -use crate::shims::unix::fd::{FdId, FileDescriptionRef}; +use crate::shims::unix::fd::{FdId, FileDescriptionRef, WeakFileDescriptionRef}; use crate::shims::unix::*; use crate::*; @@ -19,6 +20,8 @@ struct Epoll { // This is an Rc because EpollInterest need to hold a reference to update // it. ready_list: Rc>>, + /// A list of thread ids blocked on this epoll instance. + thread_id: RefCell>, } /// EpollEventInstance contains information that will be returned by epoll_wait. @@ -58,6 +61,8 @@ pub struct EpollEventInterest { data: u64, /// Ready list of the epoll instance under which this EpollEventInterest is registered. ready_list: Rc>>, + /// The file descriptor value that this EpollEventInterest is registered under. + epfd: i32, } /// EpollReadyEvents reflects the readiness of a file description. @@ -338,6 +343,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { events, data, ready_list: Rc::clone(ready_list), + epfd: epfd_value, })); if op == epoll_ctl_add { @@ -395,7 +401,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { /// The `timeout` argument specifies the number of milliseconds that /// `epoll_wait()` will block. Time is measured against the - /// CLOCK_MONOTONIC clock. + /// CLOCK_MONOTONIC clock. If the timeout is zero, the function will not block, + /// while if the timeout is -1, the function will block + /// until at least one event has been retrieved (or an error + /// occurred). /// A call to `epoll_wait()` will block until either: /// • a file descriptor delivers an event; @@ -421,35 +430,107 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { events_op: &OpTy<'tcx>, maxevents: &OpTy<'tcx>, timeout: &OpTy<'tcx>, - ) -> InterpResult<'tcx, Scalar> { + dest: MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - let epfd = this.read_scalar(epfd)?.to_i32()?; + let epfd_value = this.read_scalar(epfd)?.to_i32()?; let events = this.read_immediate(events_op)?; let maxevents = this.read_scalar(maxevents)?.to_i32()?; let timeout = this.read_scalar(timeout)?.to_i32()?; - if epfd <= 0 || maxevents <= 0 { + if epfd_value <= 0 || maxevents <= 0 { let einval = this.eval_libc("EINVAL"); this.set_last_error(einval)?; - return Ok(Scalar::from_i32(-1)); + this.write_int(-1, &dest)?; + return Ok(()); } // This needs to come after the maxevents value check, or else maxevents.try_into().unwrap() // will fail. - let events = this.deref_pointer_as( + let event = this.deref_pointer_as( &events, this.libc_array_ty_layout("epoll_event", maxevents.try_into().unwrap()), )?; - // FIXME: Implement blocking support - if timeout != 0 { - throw_unsup_format!("epoll_wait: timeout value can only be 0"); + let Some(epfd) = this.machine.fds.get(epfd_value) else { + let result_value: i32 = this.fd_not_found()?; + this.write_int(result_value, &dest)?; + return Ok(()); + }; + // Create a weak ref of epfd and pass it to callback so we will make sure that epfd + // is not close after the thread unblocks. + let weak_epfd = epfd.downgrade(); + + // We just need to know if the ready list is empty and borrow the thread_ids out. + // The whole logic is wrapped inside a block so we don't need to manually drop epfd later. + let ready_list_empty; + let mut thread_ids; + { + let epoll_file_description = epfd + .downcast::() + .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; + let binding = epoll_file_description.get_ready_list(); + ready_list_empty = binding.borrow_mut().is_empty(); + thread_ids = epoll_file_description.thread_id.borrow_mut(); + } + if timeout == 0 || !ready_list_empty { + // If the ready list is not empty, or the timeout is 0, we can return immediately. + this.blocking_epoll_callback(epfd_value, weak_epfd, &dest, &event)?; + } else { + // Blocking + let timeout = match timeout { + 0.. => { + let duration = Duration::from_millis(timeout.try_into().unwrap()); + Some((TimeoutClock::Monotonic, TimeoutAnchor::Relative, duration)) + } + -1 => None, + ..-1 => { + throw_unsup_format!( + "epoll_wait: Only timeout values greater than -1 are supported." + ); + } + }; + thread_ids.push(this.active_thread()); + this.block_thread( + BlockReason::Epoll, + timeout, + callback!( + @capture<'tcx> { + epfd_value: i32, + weak_epfd: WeakFileDescriptionRef, + dest: MPlaceTy<'tcx>, + event: MPlaceTy<'tcx>, + } + @unblock = |this| { + this.blocking_epoll_callback(epfd_value, weak_epfd, &dest, &event)?; + Ok(()) + } + @timeout = |this| { + // No notification after blocking timeout. + this.write_int(0, &dest)?; + Ok(()) + } + ), + ); } + Ok(()) + } - let Some(epfd) = this.machine.fds.get(epfd) else { - return Ok(Scalar::from_i32(this.fd_not_found()?)); + /// Callback function after epoll_wait unblocks + fn blocking_epoll_callback( + &mut self, + epfd_value: i32, + weak_epfd: WeakFileDescriptionRef, + dest: &MPlaceTy<'tcx>, + event: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { + let this = self.eval_context_mut(); + + let Some(epfd) = weak_epfd.upgrade() else { + throw_unsup_format!("epoll FD {epfd_value} is closed while blocking.") }; + let epoll_file_description = epfd .downcast::() .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; @@ -457,7 +538,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let ready_list = epoll_file_description.get_ready_list(); let mut ready_list = ready_list.borrow_mut(); let mut num_of_events: i32 = 0; - let mut array_iter = this.project_array_fields(&events)?; + let mut array_iter = this.project_array_fields(event)?; while let Some(des) = array_iter.next(this)? { if let Some(epoll_event_instance) = ready_list_next(this, &mut ready_list) { @@ -473,7 +554,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { break; } } - Ok(Scalar::from_i32(num_of_events)) + this.write_int(num_of_events, dest)?; + Ok(()) } /// For a specific file description, get its ready events and update the corresponding ready @@ -483,17 +565,42 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { /// /// This *will* report an event if anyone is subscribed to it, without any further filtering, so /// do not call this function when an FD didn't have anything happen to it! - fn check_and_update_readiness(&self, fd_ref: &FileDescriptionRef) -> InterpResult<'tcx, ()> { - let this = self.eval_context_ref(); + fn check_and_update_readiness( + &mut self, + fd_ref: &FileDescriptionRef, + ) -> InterpResult<'tcx, ()> { + let this = self.eval_context_mut(); let id = fd_ref.get_id(); + let mut waiter = Vec::new(); // Get a list of EpollEventInterest that is associated to a specific file description. if let Some(epoll_interests) = this.machine.epoll_interests.get_epoll_interest(id) { for weak_epoll_interest in epoll_interests { if let Some(epoll_interest) = weak_epoll_interest.upgrade() { - check_and_update_one_event_interest(fd_ref, epoll_interest, id, this)?; + let is_updated = check_and_update_one_event_interest(fd_ref, epoll_interest, id, this)?; + if is_updated { + // Edge-triggered notification only notify one thread even if there are + // multiple threads block on the same epfd. + let epfd = this.machine.fds.get(epoll_event_interest.epfd).unwrap(); + // FIXME: We can randomly pick a thread to unblock. + + // This unwrap can never fail because if the current epoll instance were + // closed and its epfd value reused, the upgrade of weak_epoll_interest + // above would fail. This guarantee holds because only the epoll instance + // holds a strong ref to epoll_interest. + if let Some(thread_id) = + epfd.downcast::().unwrap().thread_id.borrow_mut().pop() + { + waiter.push(thread_id); + }; + } } } } + waiter.sort(); + waiter.dedup(); + for thread_id in waiter { + this.unblock_thread(thread_id, BlockReason::Epoll)?; + } Ok(()) } } @@ -517,14 +624,15 @@ fn ready_list_next( } /// This helper function checks whether an epoll notification should be triggered for a specific -/// epoll_interest and, if necessary, triggers the notification. Unlike check_and_update_readiness, -/// this function sends a notification to only one epoll instance. +/// epoll_interest and, if necessary, triggers the notification, and returns whether the +/// event interest was updated. Unlike check_and_update_readiness, this function sends a +/// notification to only one epoll instance. fn check_and_update_one_event_interest<'tcx>( fd_ref: &FileDescriptionRef, interest: Rc>, id: FdId, ecx: &MiriInterpCx<'tcx>, -) -> InterpResult<'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 epoll_event_interest = interest.borrow(); @@ -539,6 +647,7 @@ fn check_and_update_one_event_interest<'tcx>( let event_instance = EpollEventInstance::new(flags, epoll_event_interest.data); // Triggers the notification by inserting it to the ready list. ready_list.insert(epoll_key, event_instance); + return Ok(true); } - Ok(()) + return Ok(false); } 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 581f0db42e1dc..d21f0e8f3e662 100644 --- a/src/tools/miri/src/shims/unix/linux/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/linux/foreign_items.rs @@ -62,8 +62,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "epoll_wait" => { let [epfd, events, maxevents, timeout] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.epoll_wait(epfd, events, maxevents, timeout)?; - this.write_scalar(result, dest)?; + this.epoll_wait(epfd, events, maxevents, timeout, dest.clone())?; } "eventfd" => { let [val, flag] = diff --git a/src/tools/miri/tests/fail-dep/tokio/sleep.stderr b/src/tools/miri/tests/fail-dep/tokio/sleep.stderr deleted file mode 100644 index d5bf00fc17503..0000000000000 --- a/src/tools/miri/tests/fail-dep/tokio/sleep.stderr +++ /dev/null @@ -1,15 +0,0 @@ -error: unsupported operation: epoll_wait: timeout value can only be 0 - --> CARGO_REGISTRY/.../epoll.rs:LL:CC - | -LL | / syscall!(epoll_wait( -LL | | self.ep.as_raw_fd(), -LL | | events.as_mut_ptr(), -LL | | events.capacity() as i32, -LL | | timeout, -LL | | )) - | |__________^ epoll_wait: timeout value can only be 0 - | - = help: this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support - -error: aborting due to 1 previous error - diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs new file mode 100644 index 0000000000000..e1b4d3d85be66 --- /dev/null +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs @@ -0,0 +1,98 @@ +//@only-target-linux + +use std::convert::TryInto; +use std::thread; +use std::thread::spawn; + +// This is a set of testcases for blocking epoll. + +fn main() { + test_epoll_block_without_notification(); + test_epoll_block_then_unblock(); +} + +// Using `as` cast since `EPOLLET` wraps around +const EPOLL_IN_OUT_ET: u32 = (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET) as _; + +#[track_caller] +fn check_epoll_wait( + epfd: i32, + expected_notifications: &[(u32, u64)], + timeout: i32, +) { + let epoll_event = libc::epoll_event { events: 0, u64: 0 }; + let mut array: [libc::epoll_event; N] = [epoll_event; N]; + let maxsize = N; + let array_ptr = array.as_mut_ptr(); + let res = unsafe { libc::epoll_wait(epfd, array_ptr, maxsize.try_into().unwrap(), timeout) }; + if res < 0 { + panic!("epoll_wait failed: {}", std::io::Error::last_os_error()); + } + assert_eq!( + res, + expected_notifications.len().try_into().unwrap(), + "got wrong number of notifications" + ); + let slice = unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; + for (return_event, expected_event) in slice.iter().zip(expected_notifications.iter()) { + let event = return_event.events; + let data = return_event.u64; + assert_eq!(event, expected_event.0, "got wrong events"); + assert_eq!(data, expected_event.1, "got wrong data"); + } +} +fn test_epoll_block_without_notification() { + // Create an epoll instance. + let epfd = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd, -1); + + // Create an eventfd instances. + let flags = libc::EFD_NONBLOCK | libc::EFD_CLOEXEC; + let fd = unsafe { libc::eventfd(0, flags) }; + + // Register eventfd with epoll. + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fd as u64 }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; + assert_eq!(res, 0); + + // epoll_wait to clear notification. + let expected_event = u32::try_from(libc::EPOLLOUT).unwrap(); + let expected_value = fd as u64; + check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], 0); + + // epoll_wait before triggering notification so it will block then unblock. + check_epoll_wait::<1>(epfd, &[], 5); +} + +fn test_epoll_block_then_unblock() { + // Create an epoll instance. + let epfd = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd, -1); + + // Create a socketpair instance. + let mut fds = [-1, -1]; + let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; + assert_eq!(res, 0); + + // Register one side of the socketpair with epoll. + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fds[0] as u64 }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[0], &mut ev) }; + assert_eq!(res, 0); + + // epoll_wait to clear notification. + let expected_event = u32::try_from(libc::EPOLLOUT).unwrap(); + let expected_value = fds[0] as u64; + check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], 0); + + // epoll_wait before triggering notification so it will block then get unblocked before timeout. + let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); + let expected_value = fds[0] as u64; + let thread1 = spawn(move || { + thread::yield_now(); + 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); + }); + check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], 10); + thread1.join().unwrap(); +} diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs similarity index 100% rename from src/tools/miri/tests/pass-dep/libc/libc-epoll.rs rename to src/tools/miri/tests/pass-dep/libc/libc-epoll-no-blocking.rs diff --git a/src/tools/miri/tests/fail-dep/tokio/sleep.rs b/src/tools/miri/tests/pass-dep/tokio/sleep.rs similarity index 80% rename from src/tools/miri/tests/fail-dep/tokio/sleep.rs rename to src/tools/miri/tests/pass-dep/tokio/sleep.rs index 0fa5080d48460..00cc68eba3eac 100644 --- a/src/tools/miri/tests/fail-dep/tokio/sleep.rs +++ b/src/tools/miri/tests/pass-dep/tokio/sleep.rs @@ -1,7 +1,5 @@ //@compile-flags: -Zmiri-permissive-provenance -Zmiri-backtrace=full //@only-target-x86_64-unknown-linux: support for tokio only on linux and x86 -//@error-in-other-file: timeout value can only be 0 -//@normalize-stderr-test: " += note:.*\n" -> "" use tokio::time::{sleep, Duration, Instant}; From e8175a42f78c2036845e8252061623eed9171820 Mon Sep 17 00:00:00 2001 From: byt <55319043+tiif@users.noreply.github.com> Date: Sat, 24 Aug 2024 23:16:05 +0800 Subject: [PATCH 09/35] Improve comment Co-authored-by: Ralf Jung --- src/tools/miri/src/shims/unix/linux/epoll.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index a0baa781dea3a..471ff5af11b75 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -528,7 +528,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); let Some(epfd) = weak_epfd.upgrade() else { - throw_unsup_format!("epoll FD {epfd_value} is closed while blocking.") + throw_unsup_format!("epoll FD {epfd_value} got closed while blocking.") }; let epoll_file_description = epfd From 41ab4ecc0338958ea28463e8116b348c08cf62be Mon Sep 17 00:00:00 2001 From: tiif Date: Sat, 24 Aug 2024 23:28:23 +0800 Subject: [PATCH 10/35] Pass dest place reference to epoll_wait --- src/tools/miri/src/shims/unix/linux/epoll.rs | 9 +++++---- src/tools/miri/src/shims/unix/linux/foreign_items.rs | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 471ff5af11b75..fb2eba75b878b 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -430,7 +430,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { events_op: &OpTy<'tcx>, maxevents: &OpTy<'tcx>, timeout: &OpTy<'tcx>, - dest: MPlaceTy<'tcx>, + dest: &MPlaceTy<'tcx>, ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); @@ -442,7 +442,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if epfd_value <= 0 || maxevents <= 0 { let einval = this.eval_libc("EINVAL"); this.set_last_error(einval)?; - this.write_int(-1, &dest)?; + this.write_int(-1, dest)?; return Ok(()); } @@ -455,7 +455,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let Some(epfd) = this.machine.fds.get(epfd_value) else { let result_value: i32 = this.fd_not_found()?; - this.write_int(result_value, &dest)?; + this.write_int(result_value, dest)?; return Ok(()); }; // Create a weak ref of epfd and pass it to callback so we will make sure that epfd @@ -476,7 +476,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } if timeout == 0 || !ready_list_empty { // If the ready list is not empty, or the timeout is 0, we can return immediately. - this.blocking_epoll_callback(epfd_value, weak_epfd, &dest, &event)?; + this.blocking_epoll_callback(epfd_value, weak_epfd, dest, &event)?; } else { // Blocking let timeout = match timeout { @@ -492,6 +492,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } }; thread_ids.push(this.active_thread()); + let dest = dest.clone(); this.block_thread( BlockReason::Epoll, timeout, 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 d21f0e8f3e662..d64f13f63d91f 100644 --- a/src/tools/miri/src/shims/unix/linux/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/linux/foreign_items.rs @@ -62,7 +62,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { "epoll_wait" => { let [epfd, events, maxevents, timeout] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - this.epoll_wait(epfd, events, maxevents, timeout, dest.clone())?; + this.epoll_wait(epfd, events, maxevents, timeout, dest)?; } "eventfd" => { let [val, flag] = From 36235b9e0d23b911d2ebcf2b87c93cd78e4b0920 Mon Sep 17 00:00:00 2001 From: tiif Date: Sat, 24 Aug 2024 23:30:57 +0800 Subject: [PATCH 11/35] Rename event to events --- src/tools/miri/src/shims/unix/linux/epoll.rs | 4 ++-- 1 file 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 fb2eba75b878b..4efdf9518658a 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -524,7 +524,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { epfd_value: i32, weak_epfd: WeakFileDescriptionRef, dest: &MPlaceTy<'tcx>, - event: &MPlaceTy<'tcx>, + events: &MPlaceTy<'tcx>, ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); @@ -539,7 +539,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let ready_list = epoll_file_description.get_ready_list(); let mut ready_list = ready_list.borrow_mut(); let mut num_of_events: i32 = 0; - let mut array_iter = this.project_array_fields(event)?; + let mut array_iter = this.project_array_fields(events)?; while let Some(des) = array_iter.next(this)? { if let Some(epoll_event_instance) = ready_list_next(this, &mut ready_list) { From cd67e47872d0f23d5388fe1ec9717e3b442ec1ee Mon Sep 17 00:00:00 2001 From: tiif Date: Sat, 24 Aug 2024 23:36:18 +0800 Subject: [PATCH 12/35] Fix error in the timeout value error message --- src/tools/miri/src/shims/unix/linux/epoll.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 4efdf9518658a..ade1ca73378de 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -487,7 +487,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { -1 => None, ..-1 => { throw_unsup_format!( - "epoll_wait: Only timeout values greater than -1 are supported." + "epoll_wait: Only timeout values greater than or equal to -1 are supported." ); } }; From 8f178e48125cbadc66f965f55c6521c2262b6bf0 Mon Sep 17 00:00:00 2001 From: tiif Date: Sat, 24 Aug 2024 23:45:46 +0800 Subject: [PATCH 13/35] Change timeout value --- src/tools/miri/tests/pass-dep/tokio/sleep.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tools/miri/tests/pass-dep/tokio/sleep.rs b/src/tools/miri/tests/pass-dep/tokio/sleep.rs index 00cc68eba3eac..e6b02c02d0308 100644 --- a/src/tools/miri/tests/pass-dep/tokio/sleep.rs +++ b/src/tools/miri/tests/pass-dep/tokio/sleep.rs @@ -6,7 +6,7 @@ use tokio::time::{sleep, Duration, Instant}; #[tokio::main] async fn main() { let start = Instant::now(); - sleep(Duration::from_secs(1)).await; + sleep(Duration::from_millis(100)).await; let time_elapsed = &start.elapsed().as_millis(); - assert!((1000..1100).contains(time_elapsed), "{}", time_elapsed); + assert!((100..1000).contains(time_elapsed), "{}", time_elapsed); } From 7aff7110136e51674f31adff0c39f0afc622f8be Mon Sep 17 00:00:00 2001 From: tiif Date: Sun, 25 Aug 2024 01:05:20 +0800 Subject: [PATCH 14/35] Check if the thread is blocked before waking them up --- src/tools/miri/src/concurrency/thread.rs | 10 ++++++++++ src/tools/miri/src/shims/unix/linux/epoll.rs | 4 +++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index 1b119ae71924e..246e480a6f8da 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -578,6 +578,10 @@ impl<'tcx> ThreadManager<'tcx> { self.threads[thread_id].state.is_terminated() } + fn has_blocked_on_epoll(&self, thread_id: ThreadId) -> bool { + self.threads[thread_id].state.is_blocked_on(BlockReason::Epoll) + } + /// Have all threads terminated? fn have_all_terminated(&self) -> bool { self.threads.iter().all(|thread| thread.state.is_terminated()) @@ -1137,6 +1141,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.machine.threads.enable_thread(thread_id); } + #[inline] + fn has_blocked_on_epoll(&self, thread_id: ThreadId) -> bool { + let this = self.eval_context_ref(); + this.machine.threads.has_blocked_on_epoll(thread_id) + } + #[inline] fn active_thread_stack<'a>(&'a self) -> &'a [Frame<'tcx, Provenance, FrameExtra<'tcx>>] { let this = self.eval_context_ref(); diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index ade1ca73378de..57731d746a9be 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -600,7 +600,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { waiter.sort(); waiter.dedup(); for thread_id in waiter { - this.unblock_thread(thread_id, BlockReason::Epoll)?; + if this.has_blocked_on_epoll(thread_id) { + this.unblock_thread(thread_id, BlockReason::Epoll)?; + } } Ok(()) } From a4d7564219460e952f50120b19afe0b1a72b8d58 Mon Sep 17 00:00:00 2001 From: tiif Date: Sun, 25 Aug 2024 01:20:01 +0800 Subject: [PATCH 15/35] Fix error introduced by rebase --- src/tools/miri/src/shims/unix/linux/epoll.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 57731d746a9be..53d89b3917fb4 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -577,17 +577,22 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if let Some(epoll_interests) = this.machine.epoll_interests.get_epoll_interest(id) { for weak_epoll_interest in epoll_interests { if let Some(epoll_interest) = weak_epoll_interest.upgrade() { - let is_updated = check_and_update_one_event_interest(fd_ref, epoll_interest, id, this)?; + let is_updated = check_and_update_one_event_interest( + fd_ref, + epoll_interest.clone(), + id, + this, + )?; if is_updated { // Edge-triggered notification only notify one thread even if there are // multiple threads block on the same epfd. - let epfd = this.machine.fds.get(epoll_event_interest.epfd).unwrap(); - // FIXME: We can randomly pick a thread to unblock. + let epfd = this.machine.fds.get(epoll_interest.borrow().epfd).unwrap(); // This unwrap can never fail because if the current epoll instance were // closed and its epfd value reused, the upgrade of weak_epoll_interest // above would fail. This guarantee holds because only the epoll instance // holds a strong ref to epoll_interest. + // FIXME: We can randomly pick a thread to unblock. if let Some(thread_id) = epfd.downcast::().unwrap().thread_id.borrow_mut().pop() { @@ -628,7 +633,7 @@ fn ready_list_next( /// This helper function checks whether an epoll notification should be triggered for a specific /// epoll_interest and, if necessary, triggers the notification, and returns whether the -/// event interest was updated. Unlike check_and_update_readiness, this function sends a +/// notification was added/updated. Unlike check_and_update_readiness, this function sends a /// notification to only one epoll instance. fn check_and_update_one_event_interest<'tcx>( fd_ref: &FileDescriptionRef, From 7e8ca571d0b6b1c551b7fe8af499ae7a7d787a6e Mon Sep 17 00:00:00 2001 From: tiif Date: Sun, 25 Aug 2024 02:05:21 +0800 Subject: [PATCH 16/35] Remove thread_id from epoll::thread_ids after timeout --- src/tools/miri/src/concurrency/thread.rs | 10 ---------- src/tools/miri/src/shims/unix/linux/epoll.rs | 12 +++++++++--- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index 246e480a6f8da..1b119ae71924e 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -578,10 +578,6 @@ impl<'tcx> ThreadManager<'tcx> { self.threads[thread_id].state.is_terminated() } - fn has_blocked_on_epoll(&self, thread_id: ThreadId) -> bool { - self.threads[thread_id].state.is_blocked_on(BlockReason::Epoll) - } - /// Have all threads terminated? fn have_all_terminated(&self) -> bool { self.threads.iter().all(|thread| thread.state.is_terminated()) @@ -1141,12 +1137,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.machine.threads.enable_thread(thread_id); } - #[inline] - fn has_blocked_on_epoll(&self, thread_id: ThreadId) -> bool { - let this = self.eval_context_ref(); - this.machine.threads.has_blocked_on_epoll(thread_id) - } - #[inline] fn active_thread_stack<'a>(&'a self) -> &'a [Frame<'tcx, Provenance, FrameExtra<'tcx>>] { let this = self.eval_context_ref(); diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 53d89b3917fb4..7228665e4b3de 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -509,6 +509,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } @timeout = |this| { // No notification after blocking timeout. + let Some(epfd) = weak_epfd.upgrade() else { + throw_unsup_format!("epoll FD {epfd_value} got closed while blocking.") + }; + // Remove the current active thread_id from the blocked thread_id list. + epfd.downcast::() + .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))? + .thread_id.borrow_mut() + .retain(|&id| id != this.active_thread()); this.write_int(0, &dest)?; Ok(()) } @@ -605,9 +613,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { waiter.sort(); waiter.dedup(); for thread_id in waiter { - if this.has_blocked_on_epoll(thread_id) { - this.unblock_thread(thread_id, BlockReason::Epoll)?; - } + this.unblock_thread(thread_id, BlockReason::Epoll)?; } Ok(()) } From 25ca8554311fc307d00de5ceb1dd81a2de79285d Mon Sep 17 00:00:00 2001 From: tiif Date: Sun, 25 Aug 2024 02:19:03 +0800 Subject: [PATCH 17/35] Make blocking_epoll_callback a private function --- src/tools/miri/src/shims/unix/linux/epoll.rs | 84 ++++++++++---------- 1 file changed, 41 insertions(+), 43 deletions(-) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 7228665e4b3de..ee86cf5f26d7f 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -476,7 +476,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } if timeout == 0 || !ready_list_empty { // If the ready list is not empty, or the timeout is 0, we can return immediately. - this.blocking_epoll_callback(epfd_value, weak_epfd, dest, &event)?; + blocking_epoll_callback(epfd_value, weak_epfd, dest, &event, this)?; } else { // Blocking let timeout = match timeout { @@ -504,7 +504,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { event: MPlaceTy<'tcx>, } @unblock = |this| { - this.blocking_epoll_callback(epfd_value, weak_epfd, &dest, &event)?; + blocking_epoll_callback(epfd_value, weak_epfd, &dest, &event, this)?; Ok(()) } @timeout = |this| { @@ -526,47 +526,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(()) } - /// Callback function after epoll_wait unblocks - fn blocking_epoll_callback( - &mut self, - epfd_value: i32, - weak_epfd: WeakFileDescriptionRef, - dest: &MPlaceTy<'tcx>, - events: &MPlaceTy<'tcx>, - ) -> InterpResult<'tcx> { - let this = self.eval_context_mut(); - - let Some(epfd) = weak_epfd.upgrade() else { - throw_unsup_format!("epoll FD {epfd_value} got closed while blocking.") - }; - - let epoll_file_description = epfd - .downcast::() - .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; - - let ready_list = epoll_file_description.get_ready_list(); - let mut ready_list = ready_list.borrow_mut(); - let mut num_of_events: i32 = 0; - let mut array_iter = this.project_array_fields(events)?; - - while let Some(des) = array_iter.next(this)? { - if let Some(epoll_event_instance) = ready_list_next(this, &mut ready_list) { - this.write_int_fields_named( - &[ - ("events", epoll_event_instance.events.into()), - ("u64", epoll_event_instance.data.into()), - ], - &des.1, - )?; - num_of_events = num_of_events.strict_add(1); - } else { - break; - } - } - this.write_int(num_of_events, dest)?; - Ok(()) - } - /// For a specific file description, get its ready events and update the corresponding ready /// list. This function should be called whenever an event causes more bytes or an EOF to become /// newly readable from an FD, and whenever more bytes can be written to an FD or no more future @@ -665,3 +624,42 @@ fn check_and_update_one_event_interest<'tcx>( } return Ok(false); } + +/// Callback function after epoll_wait unblocks +fn blocking_epoll_callback<'tcx>( + epfd_value: i32, + weak_epfd: WeakFileDescriptionRef, + dest: &MPlaceTy<'tcx>, + events: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, +) -> InterpResult<'tcx> { + let Some(epfd) = weak_epfd.upgrade() else { + throw_unsup_format!("epoll FD {epfd_value} got closed while blocking.") + }; + + let epoll_file_description = epfd + .downcast::() + .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; + + let ready_list = epoll_file_description.get_ready_list(); + let mut ready_list = ready_list.borrow_mut(); + let mut num_of_events: i32 = 0; + let mut array_iter = ecx.project_array_fields(events)?; + + while let Some(des) = array_iter.next(ecx)? { + if let Some(epoll_event_instance) = ready_list_next(ecx, &mut ready_list) { + ecx.write_int_fields_named( + &[ + ("events", epoll_event_instance.events.into()), + ("u64", epoll_event_instance.data.into()), + ], + &des.1, + )?; + num_of_events = num_of_events.strict_add(1); + } else { + break; + } + } + ecx.write_int(num_of_events, dest)?; + Ok(()) +} From 7d5be0629b4161dda9dde8dfdeda5e9dd50649fd Mon Sep 17 00:00:00 2001 From: tiif Date: Sun, 25 Aug 2024 03:14:42 +0800 Subject: [PATCH 18/35] Add 0 preemption rate flag for blocking test and fix comment --- src/tools/miri/src/concurrency/thread.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/src/concurrency/thread.rs b/src/tools/miri/src/concurrency/thread.rs index 1b119ae71924e..af457848de668 100644 --- a/src/tools/miri/src/concurrency/thread.rs +++ b/src/tools/miri/src/concurrency/thread.rs @@ -172,7 +172,7 @@ pub enum BlockReason { Futex { addr: u64 }, /// Blocked on an InitOnce. InitOnce(InitOnceId), - /// Blocked on epoll + /// Blocked on epoll. Epoll, } From 09993ce6d5a45384adc8cab709c5517fe60b9161 Mon Sep 17 00:00:00 2001 From: tiif Date: Sun, 25 Aug 2024 03:15:52 +0800 Subject: [PATCH 19/35] Add 0 preemption rate flag --- src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs | 2 ++ 1 file changed, 2 insertions(+) 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 e1b4d3d85be66..1a1d058b98365 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,6 @@ //@only-target-linux +// test_epoll_block_then_unblock depends on a deterministic schedule. +//@compile-flags: -Zmiri-preemption-rate=0 use std::convert::TryInto; use std::thread; From 0ed13eb48a6ea9a2757541bdf3c2cd1f906e1f69 Mon Sep 17 00:00:00 2001 From: tiif Date: Sun, 25 Aug 2024 17:15:19 +0800 Subject: [PATCH 20/35] Add test for triggering notification after timeout --- .../pass-dep/libc/libc-epoll-blocking.rs | 41 ++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) 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 1a1d058b98365..2a5d3dff07f91 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 @@ -11,6 +11,7 @@ use std::thread::spawn; fn main() { test_epoll_block_without_notification(); test_epoll_block_then_unblock(); + test_notification_after_timeout(); } // Using `as` cast since `EPOLLET` wraps around @@ -43,6 +44,8 @@ fn check_epoll_wait( assert_eq!(data, expected_event.1, "got wrong data"); } } + +// This test allows epoll_wait to block, then unblock without notification. fn test_epoll_block_without_notification() { // Create an epoll instance. let epfd = unsafe { libc::epoll_create1(0) }; @@ -62,10 +65,11 @@ fn test_epoll_block_without_notification() { let expected_value = fd as u64; check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], 0); - // epoll_wait before triggering notification so it will block then unblock. + // This epoll wait blocks, and timeout without notification. check_epoll_wait::<1>(epfd, &[], 5); } +// This test triggers notification and unblocks the epoll_wait before timeout. fn test_epoll_block_then_unblock() { // Create an epoll instance. let epfd = unsafe { libc::epoll_create1(0) }; @@ -98,3 +102,38 @@ fn test_epoll_block_then_unblock() { check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], 10); thread1.join().unwrap(); } + +// This test triggers a notification after epoll_wait times out. +fn test_notification_after_timeout() { + // Create an epoll instance. + let epfd = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd, -1); + + // Create a socketpair instance. + let mut fds = [-1, -1]; + let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; + assert_eq!(res, 0); + + // Register one side of the socketpair with epoll. + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fds[0] as u64 }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[0], &mut ev) }; + assert_eq!(res, 0); + + // epoll_wait to clear notification. + let expected_event = u32::try_from(libc::EPOLLOUT).unwrap(); + let expected_value = fds[0] as u64; + check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], 0); + + // epoll_wait timeouts without notification. + check_epoll_wait::<1>(epfd, &[], 10); + + // Trigger epoll notification after timeout. + 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); + + // Check the result of the notification. + let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); + let expected_value = fds[0] as u64; + check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], 10); +} From e01bc0424cc0d6caa3b9108eaf103c733739976c Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Mon, 26 Aug 2024 05:00:48 +0000 Subject: [PATCH 21/35] 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 1eca86baeaa21..6124cc327ad09 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -fdf61d499c8a8421ecf98e7924bb87caf43a9938 +3f121b9461cce02a703a0e7e450568849dfaa074 From 9fb611cc46661ad6bfe589222e627e54a24f4a79 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 26 Aug 2024 10:00:52 +0200 Subject: [PATCH 22/35] silence an overreaching clippy lint --- src/tools/miri/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs index f796e845a2399..ece393caf9a1e 100644 --- a/src/tools/miri/src/lib.rs +++ b/src/tools/miri/src/lib.rs @@ -35,6 +35,7 @@ clippy::box_default, clippy::needless_question_mark, clippy::needless_lifetimes, + clippy::too_long_first_doc_paragraph, rustc::diagnostic_outside_of_impl, // We are not implementing queries here so it's fine rustc::potential_query_instability, From 25e5ac48af02e56f5a25795b15913423638234b9 Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Mon, 26 Aug 2024 22:46:14 +0200 Subject: [PATCH 23/35] Disable tree traversal optimization that is wrong due to lazy nodes. See #3846 for more information. --- src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs index d51e1109ed4e2..5fbb7b927768d 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -150,7 +150,10 @@ impl LocationState { // the propagation can be skipped next time. // It is a performance loss not to call this function when a foreign access occurs. // It is unsound not to call this function when a child access occurs. - fn skip_if_known_noop( + // 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. + fn update_last_foreign_access( &mut self, access_kind: AccessKind, rel_pos: AccessRelatedness, @@ -613,10 +616,7 @@ impl<'tcx> Tree { let old_state = perm.or_insert(LocationState::new_uninit(node.default_initial_perm)); - match old_state.skip_if_known_noop(access_kind, rel_pos) { - ContinueTraversal::SkipChildren => return Ok(ContinueTraversal::SkipChildren), - _ => {} - } + old_state.update_last_foreign_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)?; From 6f8fb3c15d46c36e6d8103d5107a15b967ba5e17 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 27 Aug 2024 13:38:29 +0200 Subject: [PATCH 24/35] tree_borrows test: ensure we can actually read the variable --- src/tools/miri/tests/pass/tree_borrows/sb_fails.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/tests/pass/tree_borrows/sb_fails.rs b/src/tools/miri/tests/pass/tree_borrows/sb_fails.rs index 1bae30bde60c4..7da20e76229db 100644 --- a/src/tools/miri/tests/pass/tree_borrows/sb_fails.rs +++ b/src/tools/miri/tests/pass/tree_borrows/sb_fails.rs @@ -67,10 +67,11 @@ mod static_memory_modification { #[allow(mutable_transmutes)] pub fn main() { - let _x = unsafe { + let x = unsafe { std::mem::transmute::<&usize, &mut usize>(&X) // In SB this mutable reborrow fails. // But in TB we are allowed to transmute as long as we don't write. }; + assert_eq!(*&*x, 5); } } From 664640f57811864c615d5c91c238af967899417e Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Tue, 27 Aug 2024 14:12:05 +0200 Subject: [PATCH 25/35] Add testcase for #3846 --- .../repeated_foreign_read_lazy_conflicted.rs | 23 ++++++++++++++ ...peated_foreign_read_lazy_conflicted.stderr | 31 +++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 src/tools/miri/tests/fail/tree_borrows/repeated_foreign_read_lazy_conflicted.rs create mode 100644 src/tools/miri/tests/fail/tree_borrows/repeated_foreign_read_lazy_conflicted.stderr diff --git a/src/tools/miri/tests/fail/tree_borrows/repeated_foreign_read_lazy_conflicted.rs b/src/tools/miri/tests/fail/tree_borrows/repeated_foreign_read_lazy_conflicted.rs new file mode 100644 index 0000000000000..36b47a33b181e --- /dev/null +++ b/src/tools/miri/tests/fail/tree_borrows/repeated_foreign_read_lazy_conflicted.rs @@ -0,0 +1,23 @@ +//@compile-flags: -Zmiri-tree-borrows + +use std::ptr::addr_of_mut; + +fn do_something(_: u8) {} + +unsafe fn access_after_sub_1(x: &mut u8, orig_ptr: *mut u8) { + // causes a second access, which should make the lazy part of `x` be `Reserved {conflicted: true}` + do_something(*orig_ptr); + // read from the conflicted pointer + *(x as *mut u8).byte_sub(1) = 42; //~ ERROR: /write access through .* is forbidden/ +} + +pub fn main() { + unsafe { + let mut alloc = [0u8, 0u8]; + let orig_ptr = addr_of_mut!(alloc) as *mut u8; + let foo = &mut *orig_ptr; + // cause a foreign read access to foo + do_something(alloc[0]); + access_after_sub_1(&mut *(foo as *mut u8).byte_add(1), orig_ptr); + } +} diff --git a/src/tools/miri/tests/fail/tree_borrows/repeated_foreign_read_lazy_conflicted.stderr b/src/tools/miri/tests/fail/tree_borrows/repeated_foreign_read_lazy_conflicted.stderr new file mode 100644 index 0000000000000..963e8e5eca9f6 --- /dev/null +++ b/src/tools/miri/tests/fail/tree_borrows/repeated_foreign_read_lazy_conflicted.stderr @@ -0,0 +1,31 @@ +error: Undefined Behavior: write access through at ALLOC[0x0] is forbidden + --> $DIR/repeated_foreign_read_lazy_conflicted.rs:LL:CC + | +LL | *(x as *mut u8).byte_sub(1) = 42; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ write access through at ALLOC[0x0] is forbidden + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental + = help: the accessed tag has state Reserved (conflicted) which forbids this child write access +help: the accessed tag was created here, in the initial state Reserved + --> $DIR/repeated_foreign_read_lazy_conflicted.rs:LL:CC + | +LL | unsafe fn access_after_sub_1(x: &mut u8, orig_ptr: *mut u8) { + | ^ +help: the accessed tag later transitioned to Reserved (conflicted) due to a foreign read access at offsets [0x0..0x1] + --> $DIR/repeated_foreign_read_lazy_conflicted.rs:LL:CC + | +LL | do_something(*orig_ptr); + | ^^^^^^^^^ + = help: this transition corresponds to a temporary loss of write permissions until function exit + = note: BACKTRACE (of the first span): + = note: inside `access_after_sub_1` at $DIR/repeated_foreign_read_lazy_conflicted.rs:LL:CC +note: inside `main` + --> $DIR/repeated_foreign_read_lazy_conflicted.rs:LL:CC + | +LL | access_after_sub_1(&mut *(foo as *mut u8).byte_add(1), orig_ptr); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + From 2765444c15617b234a38a736fd7d7c1c0e1024df Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Sun, 25 Aug 2024 17:33:01 +0200 Subject: [PATCH 26/35] Make TB tree traversal bottom-up In preparation for #3837, the tree traversal needs to be made bottom-up, because the current top-down tree traversal, coupled with that PR's changes to the garbage collector, can introduce non-deterministic error messages if the GC removes a parent tag of the accessed tag that would have triggered the error first. This is a breaking change for the diagnostics emitted by TB. The implemented semantics stay the same. --- .../src/borrow_tracker/tree_borrows/tree.rs | 295 +++++++++++------- .../src/borrow_tracker/tree_borrows/unimap.rs | 4 + .../alias_through_mutation.tree.stderr | 14 +- .../box_exclusive_violation1.tree.stderr | 12 +- .../buggy_as_mut_slice.tree.stderr | 12 +- .../buggy_split_at_mut.tree.stderr | 12 +- .../both_borrows/illegal_write5.tree.stderr | 12 +- .../both_borrows/load_invalid_shr.tree.stderr | 12 +- .../mut_exclusive_violation1.tree.stderr | 12 +- .../mut_exclusive_violation2.tree.stderr | 12 +- .../pass_invalid_shr_option.tree.stderr | 12 +- .../pass_invalid_shr_tuple.tree.stderr | 12 +- .../return_invalid_shr_option.tree.stderr | 12 +- .../return_invalid_shr_tuple.tree.stderr | 12 +- .../shr_frozen_violation1.tree.stderr | 10 +- .../tree_borrows/alternate-read-write.stderr | 14 +- .../children-can-alias.uniq.stderr | 12 +- .../fail/tree_borrows/error-range.stderr | 12 +- .../fail/tree_borrows/strongly-protected.rs | 7 +- .../tree_borrows/strongly-protected.stderr | 7 +- 20 files changed, 237 insertions(+), 270 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs index 5fbb7b927768d..6aba61527d467 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -149,12 +149,11 @@ impl LocationState { // 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. - // It is unsound not to call this function when a child 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. - fn update_last_foreign_access( - &mut self, + fn skip_if_known_noop( + &self, access_kind: AccessKind, rel_pos: AccessRelatedness, ) -> ContinueTraversal { @@ -177,22 +176,37 @@ impl LocationState { // No need to update `self.latest_foreign_access`, // the type of the current streak among nonempty read-only // or nonempty with at least one write has not changed. - ContinueTraversal::SkipChildren + ContinueTraversal::SkipSelfAndChildren } else { // Otherwise propagate this time, and also record the // access that just occurred so that we can skip the propagation // next time. - self.latest_foreign_access = Some(access_kind); ContinueTraversal::Recurse } } else { // A child access occurred, this breaks the streak of foreign // accesses in a row and the sequence since the previous child access // is now empty. - self.latest_foreign_access = None; ContinueTraversal::Recurse } } + + /// 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)] + 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; + } + } } impl fmt::Display for LocationState { @@ -285,13 +299,16 @@ struct TreeVisitor<'tree> { /// Whether to continue exploring the children recursively or not. enum ContinueTraversal { Recurse, - SkipChildren, + SkipSelfAndChildren, } /// Stack of nodes left to explore in a tree traversal. -struct TreeVisitorStack { +struct TreeVisitorStack { /// Identifier of the original access. initial: UniIndex, + /// Function describing whether to continue at a tag. + /// This is only invoked for foreign accesses. + f_continue: NodeContinue, /// Function to apply to each tag. f_propagate: NodeApp, /// Handler to add the required context to diagnostics. @@ -299,152 +316,178 @@ struct TreeVisitorStack { /// Mutable state of the visit: the tags left to handle. /// Every tag pushed should eventually be handled, /// and the precise order is relevant for diagnostics. - stack: Vec<(UniIndex, AccessRelatedness)>, + /// Since the traversal is bottom-up, we need to remember + /// whether we're here initially (false) or after visiting all + /// children (true). The bool indicates this. + stack: Vec<(UniIndex, AccessRelatedness, bool)>, } -impl TreeVisitorStack +impl + TreeVisitorStack where - NodeApp: Fn(NodeAppArgs<'_>) -> Result, + NodeContinue: Fn(&NodeAppArgs<'_>) -> ContinueTraversal, + NodeApp: Fn(NodeAppArgs<'_>) -> Result<(), InnErr>, ErrHandler: Fn(ErrHandlerArgs<'_, InnErr>) -> OutErr, { - /// Apply the function to the current `tag`, and push its children - /// to the stack of future tags to visit. - fn exec_and_visit( + fn should_continue_at( + &self, + this: &mut TreeVisitor<'_>, + idx: UniIndex, + rel_pos: AccessRelatedness, + ) -> ContinueTraversal { + let node = this.nodes.get_mut(idx).unwrap(); + let args = NodeAppArgs { node, perm: this.perms.entry(idx), rel_pos }; + (self.f_continue)(&args) + } + + fn propagate_at( &mut self, this: &mut TreeVisitor<'_>, idx: UniIndex, - exclude: Option, rel_pos: AccessRelatedness, ) -> Result<(), OutErr> { - // 1. apply the propagation function let node = this.nodes.get_mut(idx).unwrap(); - let recurse = - (self.f_propagate)(NodeAppArgs { node, perm: this.perms.entry(idx), rel_pos }) - .map_err(|error_kind| { - (self.err_builder)(ErrHandlerArgs { - error_kind, - conflicting_info: &this.nodes.get(idx).unwrap().debug_info, - accessed_info: &this.nodes.get(self.initial).unwrap().debug_info, - }) - })?; - let node = this.nodes.get(idx).unwrap(); - // 2. add the children to the stack for future traversal - if matches!(recurse, ContinueTraversal::Recurse) { - let general_child_rel = rel_pos.for_child(); - for &child in node.children.iter() { - // Some child might be excluded from here and handled separately, - // e.g. the initially accessed tag. - if Some(child) != exclude { - // We should still ensure that if we don't skip the initially accessed - // it will receive the proper `AccessRelatedness`. - let this_child_rel = if child == self.initial { - AccessRelatedness::This - } else { - general_child_rel - }; - self.stack.push((child, this_child_rel)); + (self.f_propagate)(NodeAppArgs { node, perm: this.perms.entry(idx), rel_pos }).map_err( + |error_kind| { + (self.err_builder)(ErrHandlerArgs { + error_kind, + conflicting_info: &this.nodes.get(idx).unwrap().debug_info, + accessed_info: &this.nodes.get(self.initial).unwrap().debug_info, + }) + }, + ) + } + + fn go_upwards_from_accessed( + &mut self, + this: &mut TreeVisitor<'_>, + accessed_node: UniIndex, + push_children_of_accessed: bool, + ) -> Result<(), OutErr> { + assert!(self.stack.is_empty()); + // First, handle accessed node. A bunch of things need to + // be handled differently here compared to the further parents + // of `accesssed_node`. + { + self.propagate_at(this, accessed_node, AccessRelatedness::This)?; + if push_children_of_accessed { + let accessed_node = this.nodes.get(accessed_node).unwrap(); + for &child in accessed_node.children.iter().rev() { + self.stack.push((child, AccessRelatedness::AncestorAccess, false)); } } } + // Then, handle the accessed node's parent. Here, we need to + // make sure we only mark the "cousin" subtrees for later visitation, + // not the subtree that contains the accessed node. + let mut last_node = accessed_node; + while let Some(current) = this.nodes.get(last_node).unwrap().parent { + self.propagate_at(this, current, AccessRelatedness::StrictChildAccess)?; + let node = this.nodes.get(current).unwrap(); + for &child in node.children.iter().rev() { + if last_node == child { + continue; + } + self.stack.push((child, AccessRelatedness::DistantAccess, false)); + } + last_node = current; + } + self.stack.reverse(); Ok(()) } - fn new(initial: UniIndex, f_propagate: NodeApp, err_builder: ErrHandler) -> Self { - Self { initial, f_propagate, err_builder, stack: Vec::new() } - } - - /// Finish the exploration by applying `exec_and_visit` until - /// the stack is empty. - fn finish(&mut self, visitor: &mut TreeVisitor<'_>) -> Result<(), OutErr> { - while let Some((idx, rel_pos)) = self.stack.pop() { - self.exec_and_visit(visitor, idx, /* no children to exclude */ None, rel_pos)?; + fn finish_foreign_accesses(&mut self, this: &mut TreeVisitor<'_>) -> Result<(), OutErr> { + while let Some((idx, rel_pos, is_final)) = self.stack.last_mut() { + let idx = *idx; + let rel_pos = *rel_pos; + if *is_final { + self.stack.pop(); + self.propagate_at(this, idx, rel_pos)?; + } else { + *is_final = true; + let handle_children = self.should_continue_at(this, idx, rel_pos); + match handle_children { + ContinueTraversal::Recurse => { + // add all children, and also leave the node itself + // on the stack so that it can be visited later. + let node = this.nodes.get(idx).unwrap(); + for &child in node.children.iter() { + self.stack.push((child, rel_pos, false)); + } + } + ContinueTraversal::SkipSelfAndChildren => { + // skip self + self.stack.pop(); + continue; + } + } + } } Ok(()) } - /// Push all ancestors to the exploration stack in order of nearest ancestor - /// towards the top. - fn push_and_visit_strict_ancestors( - &mut self, - visitor: &mut TreeVisitor<'_>, - ) -> Result<(), OutErr> { - let mut path_ascend = Vec::new(); - // First climb to the root while recording the path - let mut curr = self.initial; - while let Some(ancestor) = visitor.nodes.get(curr).unwrap().parent { - path_ascend.push((ancestor, curr)); - curr = ancestor; - } - // Then descend: - // - execute f_propagate on each node - // - record children in visit - while let Some((ancestor, next_in_path)) = path_ascend.pop() { - // Explore ancestors in descending order. - // `next_in_path` is excluded from the recursion because it - // will be the `ancestor` of the next iteration. - // It also needs a different `AccessRelatedness` than the other - // children of `ancestor`. - self.exec_and_visit( - visitor, - ancestor, - Some(next_in_path), - AccessRelatedness::StrictChildAccess, - )?; - } - Ok(()) + fn new( + initial: UniIndex, + f_continue: NodeContinue, + f_propagate: NodeApp, + err_builder: ErrHandler, + ) -> Self { + Self { initial, f_continue, f_propagate, err_builder, stack: Vec::new() } } } impl<'tree> TreeVisitor<'tree> { - // Applies `f_propagate` to every vertex of the tree top-down in the following order: first - // all ancestors of `start`, then `start` itself, then children of `start`, then the rest. + // Applies `f_propagate` to every vertex of the tree bottom-up in the following order: first + // all ancestors of `start` (starting with `start` itself), then children of `start`, then the rest, + // always going bottom-up. // This ensures that errors are triggered in the following order - // - first invalid accesses with insufficient permissions, closest to the root first, - // - then protector violations, closest to `start` first. + // - first invalid accesses with insufficient permissions, closest to the accessed node first, + // - then protector violations, bottom-up, starting with the children of the accessed node, and then + // going upwards and outwards. // // `f_propagate` should follow the following format: for a given `Node` it updates its // `Permission` depending on the position relative to `start` (given by an // `AccessRelatedness`). - // It outputs whether the tree traversal for this subree should continue or not. - fn traverse_parents_this_children_others( + // `f_continue` is called before on foreign nodes, and describes whether to continue + // with the subtree at that node. + fn traverse_this_parents_children_other( mut self, start: BorTag, - f_propagate: impl Fn(NodeAppArgs<'_>) -> Result, + f_continue: impl Fn(&NodeAppArgs<'_>) -> ContinueTraversal, + f_propagate: impl Fn(NodeAppArgs<'_>) -> Result<(), InnErr>, err_builder: impl Fn(ErrHandlerArgs<'_, InnErr>) -> OutErr, ) -> Result<(), OutErr> { let start_idx = self.tag_mapping.get(&start).unwrap(); - let mut stack = TreeVisitorStack::new(start_idx, f_propagate, err_builder); - stack.push_and_visit_strict_ancestors(&mut self)?; - // All (potentially zero) ancestors have been explored, - // it's time to explore the `start` tag. - stack.exec_and_visit( - &mut self, - start_idx, - /* no children to exclude */ None, - AccessRelatedness::This, - )?; - // Then finish with a normal DFS. - stack.finish(&mut self) + let mut stack = TreeVisitorStack::new(start_idx, f_continue, f_propagate, err_builder); + // Visits the accessed node itself, and all its parents, i.e. all nodes + // undergoing a child access. Also pushes the children and the other + // cousin nodes (i.e. all nodes undergoing a foreign access) to the stack + // to be processed later. + stack.go_upwards_from_accessed(&mut self, start_idx, true)?; + // Now visit all the foreign nodes we remembered earlier. + // For this we go bottom-up, but also allow f_continue to skip entire + // subtrees from being visited if it would be a NOP. + stack.finish_foreign_accesses(&mut self) } - // Applies `f_propagate` to every non-child vertex of the tree (ancestors first). - // - // `f_propagate` should follow the following format: for a given `Node` it updates its - // `Permission` depending on the position relative to `start` (given by an - // `AccessRelatedness`). - // It outputs whether the tree traversal for this subree should continue or not. + // Like `traverse_this_parents_children_other`, but skips the children of `start`. fn traverse_nonchildren( mut self, start: BorTag, - f_propagate: impl Fn(NodeAppArgs<'_>) -> Result, + f_continue: impl Fn(&NodeAppArgs<'_>) -> ContinueTraversal, + f_propagate: impl Fn(NodeAppArgs<'_>) -> Result<(), InnErr>, err_builder: impl Fn(ErrHandlerArgs<'_, InnErr>) -> OutErr, ) -> Result<(), OutErr> { let start_idx = self.tag_mapping.get(&start).unwrap(); - let mut stack = TreeVisitorStack::new(start_idx, f_propagate, err_builder); - stack.push_and_visit_strict_ancestors(&mut self)?; - // We *don't* visit the `start` tag, and we don't push its children. - // Only finish the DFS with the cousins. - stack.finish(&mut self) + let mut stack = TreeVisitorStack::new(start_idx, f_continue, f_propagate, err_builder); + // Visits the accessed node itself, and all its parents, i.e. all nodes + // undergoing a child access. Also pushes the other cousin nodes to the + // stack, but not the children of the accessed node. + stack.go_upwards_from_accessed(&mut self, start_idx, false)?; + // Now visit all the foreign nodes we remembered earlier. + // For this we go bottom-up, but also allow f_continue to skip entire + // subtrees from being visited if it would be a NOP. + stack.finish_foreign_accesses(&mut self) } } @@ -541,16 +584,18 @@ impl<'tcx> Tree { )?; for (perms_range, perms) in self.rperms.iter_mut(access_range.start, access_range.size) { TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms } - .traverse_parents_this_children_others( + .traverse_this_parents_children_other( tag, - |args: NodeAppArgs<'_>| -> Result { + // visit all children, skipping none + |_| ContinueTraversal::Recurse, + |args: NodeAppArgs<'_>| -> Result<(), TransitionError> { let NodeAppArgs { node, .. } = args; if global.borrow().protected_tags.get(&node.tag) == Some(&ProtectorKind::StrongProtector) { Err(TransitionError::ProtectedDealloc) } else { - Ok(ContinueTraversal::Recurse) + Ok(()) } }, |args: ErrHandlerArgs<'_, TransitionError>| -> InterpError<'tcx> { @@ -607,16 +652,28 @@ impl<'tcx> Tree { // // `perms_range` is only for diagnostics (it is the range of // the `RangeMap` on which we are currently working). + 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 node_app = |perms_range: Range, access_kind: AccessKind, access_cause: diagnostics::AccessCause, args: NodeAppArgs<'_>| - -> Result { + -> Result<(), TransitionError> { let NodeAppArgs { node, mut perm, rel_pos } = args; let old_state = perm.or_insert(LocationState::new_uninit(node.default_initial_perm)); - old_state.update_last_foreign_access(access_kind, rel_pos); + // FIXME: See #3684 + // 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)?; @@ -631,7 +688,7 @@ impl<'tcx> Tree { span, }); } - Ok(ContinueTraversal::Recurse) + Ok(()) }; // Error handler in case `node_app` goes wrong. @@ -658,8 +715,9 @@ impl<'tcx> Tree { for (perms_range, perms) in self.rperms.iter_mut(access_range.start, access_range.size) { TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms } - .traverse_parents_this_children_others( + .traverse_this_parents_children_other( tag, + |args| node_skipper(access_kind, args), |args| node_app(perms_range.clone(), access_kind, access_cause, args), |args| err_handler(perms_range.clone(), access_cause, args), )?; @@ -686,6 +744,7 @@ impl<'tcx> Tree { TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms } .traverse_nonchildren( tag, + |args| node_skipper(access_kind, args), |args| node_app(perms_range.clone(), access_kind, access_cause, args), |args| err_handler(perms_range.clone(), access_cause, args), )?; diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/unimap.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/unimap.rs index 92bae6203b3c0..cbc25724cb685 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/unimap.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/unimap.rs @@ -221,6 +221,10 @@ impl<'a, V> UniEntry<'a, V> { } self.inner.as_mut().unwrap() } + + pub fn get(&self) -> Option<&V> { + self.inner.as_ref() + } } mod tests { diff --git a/src/tools/miri/tests/fail/both_borrows/alias_through_mutation.tree.stderr b/src/tools/miri/tests/fail/both_borrows/alias_through_mutation.tree.stderr index f26e8444a9aaf..0bd196eab1b83 100644 --- a/src/tools/miri/tests/fail/both_borrows/alias_through_mutation.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/alias_through_mutation.tree.stderr @@ -5,24 +5,18 @@ LL | let _val = *target_alias; | ^^^^^^^^^^^^^ read access through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this child read access -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this child read access +help: the accessed tag was created here, in the initial state Frozen --> $DIR/alias_through_mutation.rs:LL:CC | LL | *x = &mut *(target as *mut _); | ^^^^^^^^^^^^^^^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Reserved - --> $DIR/alias_through_mutation.rs:LL:CC - | -LL | retarget(&mut target_alias, target); - | ^^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] --> $DIR/alias_through_mutation.rs:LL:CC | LL | *target = 13; | ^^^^^^^^^^^^ - = help: this transition corresponds to a loss of read and write permissions + = help: this transition corresponds to a loss of read permissions = note: BACKTRACE (of the first span): = note: inside `main` at $DIR/alias_through_mutation.rs:LL:CC diff --git a/src/tools/miri/tests/fail/both_borrows/box_exclusive_violation1.tree.stderr b/src/tools/miri/tests/fail/both_borrows/box_exclusive_violation1.tree.stderr index ee5ef0c5ea8b2..27d987feb574d 100644 --- a/src/tools/miri/tests/fail/both_borrows/box_exclusive_violation1.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/box_exclusive_violation1.tree.stderr @@ -5,19 +5,13 @@ LL | *LEAK = 7; | ^^^^^^^^^ write access through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this child write access -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this child write access +help: the accessed tag was created here, in the initial state Frozen --> $DIR/box_exclusive_violation1.rs:LL:CC | LL | fn unknown_code_1(x: &i32) { | ^ -help: the conflicting tag was created here, in the initial state Frozen - --> $DIR/box_exclusive_violation1.rs:LL:CC - | -LL | unknown_code_1(&*our); - | ^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] --> $DIR/box_exclusive_violation1.rs:LL:CC | LL | *our = 5; diff --git a/src/tools/miri/tests/fail/both_borrows/buggy_as_mut_slice.tree.stderr b/src/tools/miri/tests/fail/both_borrows/buggy_as_mut_slice.tree.stderr index 5593db899710d..16e87c4d4ebc2 100644 --- a/src/tools/miri/tests/fail/both_borrows/buggy_as_mut_slice.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/buggy_as_mut_slice.tree.stderr @@ -5,19 +5,13 @@ LL | v2[1] = 7; | ^^^^^^^^^ write access through at ALLOC[0x4] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this child write access -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this child write access +help: the accessed tag was created here, in the initial state Reserved --> $DIR/buggy_as_mut_slice.rs:LL:CC | LL | let v2 = safe::as_mut_slice(&v); | ^^^^^^^^^^^^^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Reserved - --> $DIR/buggy_as_mut_slice.rs:LL:CC - | -LL | unsafe { from_raw_parts_mut(self_.as_ptr() as *mut T, self_.len()) } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x4..0x8] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x4..0x8] --> $DIR/buggy_as_mut_slice.rs:LL:CC | LL | v1[1] = 5; diff --git a/src/tools/miri/tests/fail/both_borrows/buggy_split_at_mut.tree.stderr b/src/tools/miri/tests/fail/both_borrows/buggy_split_at_mut.tree.stderr index 7d3725d611a2c..aaf7c2256a78a 100644 --- a/src/tools/miri/tests/fail/both_borrows/buggy_split_at_mut.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/buggy_split_at_mut.tree.stderr @@ -5,19 +5,13 @@ LL | b[1] = 6; | ^^^^^^^^ write access through at ALLOC[0x4] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this child write access -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this child write access +help: the accessed tag was created here, in the initial state Reserved --> $DIR/buggy_split_at_mut.rs:LL:CC | LL | let (a, b) = safe::split_at_mut(&mut array, 0); | ^ -help: the conflicting tag was created here, in the initial state Reserved - --> $DIR/buggy_split_at_mut.rs:LL:CC - | -LL | from_raw_parts_mut(ptr.offset(mid as isize), len - mid), - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x4..0x8] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x4..0x8] --> $DIR/buggy_split_at_mut.rs:LL:CC | LL | a[1] = 5; diff --git a/src/tools/miri/tests/fail/both_borrows/illegal_write5.tree.stderr b/src/tools/miri/tests/fail/both_borrows/illegal_write5.tree.stderr index 1f3d6e387d437..dec2bb6880f87 100644 --- a/src/tools/miri/tests/fail/both_borrows/illegal_write5.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/illegal_write5.tree.stderr @@ -5,19 +5,13 @@ LL | let _val = *xref; | ^^^^^ read access through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this child read access -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this child read access +help: the accessed tag was created here, in the initial state Reserved --> $DIR/illegal_write5.rs:LL:CC | LL | let xref = unsafe { &mut *xraw }; | ^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Reserved - --> $DIR/illegal_write5.rs:LL:CC - | -LL | let xref = unsafe { &mut *xraw }; - | ^^^^^^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] --> $DIR/illegal_write5.rs:LL:CC | LL | unsafe { *xraw = 15 }; diff --git a/src/tools/miri/tests/fail/both_borrows/load_invalid_shr.tree.stderr b/src/tools/miri/tests/fail/both_borrows/load_invalid_shr.tree.stderr index 292dc8150f17f..0fcfc20e43696 100644 --- a/src/tools/miri/tests/fail/both_borrows/load_invalid_shr.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/load_invalid_shr.tree.stderr @@ -5,19 +5,13 @@ LL | let _val = *xref_in_mem; | ^^^^^^^^^^^^ reborrow through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this reborrow (acting as a child read access) -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this reborrow (acting as a child read access) +help: the accessed tag was created here, in the initial state Frozen --> $DIR/load_invalid_shr.rs:LL:CC | LL | let xref_in_mem = Box::new(xref); | ^^^^^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Frozen - --> $DIR/load_invalid_shr.rs:LL:CC - | -LL | let xref = unsafe { &*xraw }; - | ^^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] --> $DIR/load_invalid_shr.rs:LL:CC | LL | unsafe { *xraw = 42 }; // unfreeze diff --git a/src/tools/miri/tests/fail/both_borrows/mut_exclusive_violation1.tree.stderr b/src/tools/miri/tests/fail/both_borrows/mut_exclusive_violation1.tree.stderr index a5e62e9ea3381..5b4ee4a891302 100644 --- a/src/tools/miri/tests/fail/both_borrows/mut_exclusive_violation1.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/mut_exclusive_violation1.tree.stderr @@ -5,19 +5,13 @@ LL | *LEAK = 7; | ^^^^^^^^^ write access through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this child write access -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this child write access +help: the accessed tag was created here, in the initial state Frozen --> $DIR/mut_exclusive_violation1.rs:LL:CC | LL | fn unknown_code_1(x: &i32) { | ^ -help: the conflicting tag was created here, in the initial state Frozen - --> $DIR/mut_exclusive_violation1.rs:LL:CC - | -LL | unknown_code_1(&*our); - | ^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] --> $DIR/mut_exclusive_violation1.rs:LL:CC | LL | *our = 5; diff --git a/src/tools/miri/tests/fail/both_borrows/mut_exclusive_violation2.tree.stderr b/src/tools/miri/tests/fail/both_borrows/mut_exclusive_violation2.tree.stderr index 6b6eecbe41362..d3bc54b7ef3c9 100644 --- a/src/tools/miri/tests/fail/both_borrows/mut_exclusive_violation2.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/mut_exclusive_violation2.tree.stderr @@ -5,19 +5,13 @@ LL | *raw1 = 3; | ^^^^^^^^^ write access through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this child write access -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this child write access +help: the accessed tag was created here, in the initial state Reserved --> $DIR/mut_exclusive_violation2.rs:LL:CC | LL | let raw1 = ptr1.as_mut(); | ^^^^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Reserved - --> $DIR/mut_exclusive_violation2.rs:LL:CC - | -LL | let raw1 = ptr1.as_mut(); - | ^^^^^^^^^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] --> $DIR/mut_exclusive_violation2.rs:LL:CC | LL | *raw2 = 2; diff --git a/src/tools/miri/tests/fail/both_borrows/pass_invalid_shr_option.tree.stderr b/src/tools/miri/tests/fail/both_borrows/pass_invalid_shr_option.tree.stderr index f2eaf767a47af..f687b3f867fc3 100644 --- a/src/tools/miri/tests/fail/both_borrows/pass_invalid_shr_option.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/pass_invalid_shr_option.tree.stderr @@ -5,19 +5,13 @@ LL | foo(some_xref); | ^^^^^^^^^ reborrow through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this reborrow (acting as a child read access) -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this reborrow (acting as a child read access) +help: the accessed tag was created here, in the initial state Frozen --> $DIR/pass_invalid_shr_option.rs:LL:CC | LL | let some_xref = unsafe { Some(&*xraw) }; | ^^^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Frozen - --> $DIR/pass_invalid_shr_option.rs:LL:CC - | -LL | let some_xref = unsafe { Some(&*xraw) }; - | ^^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] --> $DIR/pass_invalid_shr_option.rs:LL:CC | LL | unsafe { *xraw = 42 }; // unfreeze diff --git a/src/tools/miri/tests/fail/both_borrows/pass_invalid_shr_tuple.tree.stderr b/src/tools/miri/tests/fail/both_borrows/pass_invalid_shr_tuple.tree.stderr index 9cfd4239c85f8..845838f93d57d 100644 --- a/src/tools/miri/tests/fail/both_borrows/pass_invalid_shr_tuple.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/pass_invalid_shr_tuple.tree.stderr @@ -5,19 +5,13 @@ LL | foo(pair_xref); | ^^^^^^^^^ reborrow through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this reborrow (acting as a child read access) -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this reborrow (acting as a child read access) +help: the accessed tag was created here, in the initial state Frozen --> $DIR/pass_invalid_shr_tuple.rs:LL:CC | LL | let pair_xref = unsafe { (&*xraw0, &*xraw1) }; | ^^^^^^^^^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Frozen - --> $DIR/pass_invalid_shr_tuple.rs:LL:CC - | -LL | let pair_xref = unsafe { (&*xraw0, &*xraw1) }; - | ^^^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x4] --> $DIR/pass_invalid_shr_tuple.rs:LL:CC | LL | unsafe { *xraw0 = 42 }; // unfreeze diff --git a/src/tools/miri/tests/fail/both_borrows/return_invalid_shr_option.tree.stderr b/src/tools/miri/tests/fail/both_borrows/return_invalid_shr_option.tree.stderr index 1169200b57c1f..e770372925130 100644 --- a/src/tools/miri/tests/fail/both_borrows/return_invalid_shr_option.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/return_invalid_shr_option.tree.stderr @@ -5,19 +5,13 @@ LL | ret | ^^^ reborrow through at ALLOC[0x4] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this reborrow (acting as a child read access) -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this reborrow (acting as a child read access) +help: the accessed tag was created here, in the initial state Frozen --> $DIR/return_invalid_shr_option.rs:LL:CC | LL | let ret = Some(unsafe { &(*xraw).1 }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Frozen - --> $DIR/return_invalid_shr_option.rs:LL:CC - | -LL | let ret = Some(unsafe { &(*xraw).1 }); - | ^^^^^^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x8] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x8] --> $DIR/return_invalid_shr_option.rs:LL:CC | LL | unsafe { *xraw = (42, 23) }; // unfreeze diff --git a/src/tools/miri/tests/fail/both_borrows/return_invalid_shr_tuple.tree.stderr b/src/tools/miri/tests/fail/both_borrows/return_invalid_shr_tuple.tree.stderr index af410534140fe..a5c6be3f13aa8 100644 --- a/src/tools/miri/tests/fail/both_borrows/return_invalid_shr_tuple.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/return_invalid_shr_tuple.tree.stderr @@ -5,19 +5,13 @@ LL | ret | ^^^ reborrow through at ALLOC[0x4] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this reborrow (acting as a child read access) -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this reborrow (acting as a child read access) +help: the accessed tag was created here, in the initial state Frozen --> $DIR/return_invalid_shr_tuple.rs:LL:CC | LL | let ret = (unsafe { &(*xraw).1 },); | ^^^^^^^^^^^^^^^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Frozen - --> $DIR/return_invalid_shr_tuple.rs:LL:CC - | -LL | let ret = (unsafe { &(*xraw).1 },); - | ^^^^^^^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x8] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x8] --> $DIR/return_invalid_shr_tuple.rs:LL:CC | LL | unsafe { *xraw = (42, 23) }; // unfreeze diff --git a/src/tools/miri/tests/fail/both_borrows/shr_frozen_violation1.tree.stderr b/src/tools/miri/tests/fail/both_borrows/shr_frozen_violation1.tree.stderr index 97cd64c03be8c..45b3bceb7287e 100644 --- a/src/tools/miri/tests/fail/both_borrows/shr_frozen_violation1.tree.stderr +++ b/src/tools/miri/tests/fail/both_borrows/shr_frozen_violation1.tree.stderr @@ -5,18 +5,12 @@ LL | *(x as *const i32 as *mut i32) = 7; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ write access through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Frozen which forbids this child write access -help: the accessed tag was created here + = help: the accessed tag has state Frozen which forbids this child write access +help: the accessed tag was created here, in the initial state Frozen --> $DIR/shr_frozen_violation1.rs:LL:CC | LL | fn unknown_code(x: &i32) { | ^ -help: the conflicting tag was created here, in the initial state Frozen - --> $DIR/shr_frozen_violation1.rs:LL:CC - | -LL | unknown_code(&*x); - | ^^^ = note: BACKTRACE (of the first span): = note: inside `unknown_code` at $DIR/shr_frozen_violation1.rs:LL:CC note: inside `foo` diff --git a/src/tools/miri/tests/fail/tree_borrows/alternate-read-write.stderr b/src/tools/miri/tests/fail/tree_borrows/alternate-read-write.stderr index 98371cbe7a262..bd969d089c37a 100644 --- a/src/tools/miri/tests/fail/tree_borrows/alternate-read-write.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/alternate-read-write.stderr @@ -5,25 +5,19 @@ LL | *y += 1; // Failure | ^^^^^^^ write access through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Frozen which forbids this child write access -help: the accessed tag was created here + = help: the accessed tag has state Frozen which forbids this child write access +help: the accessed tag was created here, in the initial state Reserved --> $DIR/alternate-read-write.rs:LL:CC | LL | let y = unsafe { &mut *(x as *mut u8) }; | ^^^^^^^^^^^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Reserved - --> $DIR/alternate-read-write.rs:LL:CC - | -LL | let y = unsafe { &mut *(x as *mut u8) }; - | ^^^^^^^^^^^^^^^^^^^^ -help: the conflicting tag later transitioned to Active due to a child write access at offsets [0x0..0x1] +help: the accessed tag later transitioned to Active due to a child write access at offsets [0x0..0x1] --> $DIR/alternate-read-write.rs:LL:CC | LL | *y += 1; // Success | ^^^^^^^ = help: this transition corresponds to the first write to a 2-phase borrowed mutable reference -help: the conflicting tag later transitioned to Frozen due to a foreign read access at offsets [0x0..0x1] +help: the accessed tag later transitioned to Frozen due to a foreign read access at offsets [0x0..0x1] --> $DIR/alternate-read-write.rs:LL:CC | LL | let _val = *x; diff --git a/src/tools/miri/tests/fail/tree_borrows/children-can-alias.uniq.stderr b/src/tools/miri/tests/fail/tree_borrows/children-can-alias.uniq.stderr index cc1fea30f53e4..cdfa8a74238a4 100644 --- a/src/tools/miri/tests/fail/tree_borrows/children-can-alias.uniq.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/children-can-alias.uniq.stderr @@ -5,19 +5,13 @@ LL | child2.write(2); | ^^^^^^^^^^^^^^^ write access through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this child write access -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this child write access +help: the accessed tag was created here, in the initial state Reserved --> $DIR/children-can-alias.rs:LL:CC | LL | let child2 = x.as_ptr(); | ^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Reserved - --> $DIR/children-can-alias.rs:LL:CC - | -LL | let child2 = x.as_ptr(); - | ^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x1] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x1] --> $DIR/children-can-alias.rs:LL:CC | LL | child1.write(1); diff --git a/src/tools/miri/tests/fail/tree_borrows/error-range.stderr b/src/tools/miri/tests/fail/tree_borrows/error-range.stderr index 37f0b9b62b037..090ae4507bd75 100644 --- a/src/tools/miri/tests/fail/tree_borrows/error-range.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/error-range.stderr @@ -5,19 +5,13 @@ LL | rmut[5] += 1; | ^^^^^^^^^^^^ read access through at ALLOC[0x5] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental - = help: the accessed tag is a child of the conflicting tag - = help: the conflicting tag has state Disabled which forbids this child read access -help: the accessed tag was created here + = help: the accessed tag has state Disabled which forbids this child read access +help: the accessed tag was created here, in the initial state Reserved --> $DIR/error-range.rs:LL:CC | LL | let rmut = &mut *addr_of_mut!(data[0..6]); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: the conflicting tag was created here, in the initial state Reserved - --> $DIR/error-range.rs:LL:CC - | -LL | let rmut = &mut *addr_of_mut!(data[0..6]); - | ^^^^ -help: the conflicting tag later transitioned to Disabled due to a foreign write access at offsets [0x5..0x6] +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x5..0x6] --> $DIR/error-range.rs:LL:CC | LL | data[5] = 1; diff --git a/src/tools/miri/tests/fail/tree_borrows/strongly-protected.rs b/src/tools/miri/tests/fail/tree_borrows/strongly-protected.rs index 484c7c3bbff78..037031d0345c4 100644 --- a/src/tools/miri/tests/fail/tree_borrows/strongly-protected.rs +++ b/src/tools/miri/tests/fail/tree_borrows/strongly-protected.rs @@ -1,14 +1,15 @@ //@compile-flags: -Zmiri-tree-borrows //@error-in-other-file: /deallocation through .* is forbidden/ -fn inner(x: &mut i32, f: fn(&mut i32)) { +fn inner(x: &mut i32, f: fn(*mut i32)) { // `f` may mutate, but it may not deallocate! + // `f` takes a raw pointer so that the only protector + // is that on `x` f(x) } fn main() { - inner(Box::leak(Box::new(0)), |x| { - let raw = x as *mut _; + inner(Box::leak(Box::new(0)), |raw| { drop(unsafe { Box::from_raw(raw) }); }); } diff --git a/src/tools/miri/tests/fail/tree_borrows/strongly-protected.stderr b/src/tools/miri/tests/fail/tree_borrows/strongly-protected.stderr index f0afcc7b3ff1e..9da43055af240 100644 --- a/src/tools/miri/tests/fail/tree_borrows/strongly-protected.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/strongly-protected.stderr @@ -15,7 +15,7 @@ LL | drop(unsafe { Box::from_raw(raw) }); help: the strongly protected tag was created here, in the initial state Reserved --> $DIR/strongly-protected.rs:LL:CC | -LL | fn inner(x: &mut i32, f: fn(&mut i32)) { +LL | fn inner(x: &mut i32, f: fn(*mut i32)) { | ^ = note: BACKTRACE (of the first span): = note: inside `std::alloc::dealloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC @@ -28,7 +28,7 @@ note: inside closure | LL | drop(unsafe { Box::from_raw(raw) }); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = note: inside `<{closure@$DIR/strongly-protected.rs:LL:CC} as std::ops::FnOnce<(&mut i32,)>>::call_once - shim` at RUSTLIB/core/src/ops/function.rs:LL:CC + = note: inside `<{closure@$DIR/strongly-protected.rs:LL:CC} as std::ops::FnOnce<(*mut i32,)>>::call_once - shim` at RUSTLIB/core/src/ops/function.rs:LL:CC note: inside `inner` --> $DIR/strongly-protected.rs:LL:CC | @@ -37,8 +37,7 @@ LL | f(x) note: inside `main` --> $DIR/strongly-protected.rs:LL:CC | -LL | / inner(Box::leak(Box::new(0)), |x| { -LL | | let raw = x as *mut _; +LL | / inner(Box::leak(Box::new(0)), |raw| { LL | | drop(unsafe { Box::from_raw(raw) }); LL | | }); | |______^ From 5be5cec23cc2e0ac284caad9d65e4fbcb81157d3 Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Tue, 27 Aug 2024 15:58:38 +0200 Subject: [PATCH 27/35] Add explanation to TB's "piecewise bottom-up" traversal --- .../src/borrow_tracker/tree_borrows/tree.rs | 155 +++++++++++++----- 1 file changed, 111 insertions(+), 44 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs index 6aba61527d467..728a664680da4 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -302,7 +302,20 @@ enum ContinueTraversal { SkipSelfAndChildren, } +#[derive(Clone, Copy)] +pub enum ChildrenVisitMode { + VisitChildrenOfAccessed, + SkipChildrenOfAccessed, +} + +enum RecursionState { + BeforeChildren, + AfterChildren, +} + /// Stack of nodes left to explore in a tree traversal. +/// See the docs of `traverse_this_parents_children_other` for details on the +/// traversal order. struct TreeVisitorStack { /// Identifier of the original access. initial: UniIndex, @@ -316,10 +329,12 @@ struct TreeVisitorStack { /// Mutable state of the visit: the tags left to handle. /// Every tag pushed should eventually be handled, /// and the precise order is relevant for diagnostics. - /// Since the traversal is bottom-up, we need to remember - /// whether we're here initially (false) or after visiting all - /// children (true). The bool indicates this. - stack: Vec<(UniIndex, AccessRelatedness, bool)>, + /// Since the traversal is piecewise bottom-up, we need to + /// remember whether we're here initially, or after visiting all children. + /// The last element indicates this. + /// This is just an artifact of how you hand-roll recursion, + /// it does not have a deeper meaning otherwise. + stack: Vec<(UniIndex, AccessRelatedness, RecursionState)>, } impl @@ -362,64 +377,85 @@ where &mut self, this: &mut TreeVisitor<'_>, accessed_node: UniIndex, - push_children_of_accessed: bool, + visit_children: ChildrenVisitMode, ) -> Result<(), OutErr> { + // We want to visit the accessed node's children first. + // However, we will below walk up our parents and push their children (our cousins) + // onto the stack. To ensure correct iteration order, this method thus finishes + // by reversing the stack. This only works if the stack is empty initially. assert!(self.stack.is_empty()); // First, handle accessed node. A bunch of things need to // be handled differently here compared to the further parents // of `accesssed_node`. { self.propagate_at(this, accessed_node, AccessRelatedness::This)?; - if push_children_of_accessed { + if matches!(visit_children, ChildrenVisitMode::VisitChildrenOfAccessed) { let accessed_node = this.nodes.get(accessed_node).unwrap(); + // We `rev()` here because we reverse the entire stack later. for &child in accessed_node.children.iter().rev() { - self.stack.push((child, AccessRelatedness::AncestorAccess, false)); + self.stack.push(( + child, + AccessRelatedness::AncestorAccess, + RecursionState::BeforeChildren, + )); } } } - // Then, handle the accessed node's parent. Here, we need to + // Then, handle the accessed node's parents. Here, we need to // make sure we only mark the "cousin" subtrees for later visitation, // not the subtree that contains the accessed node. let mut last_node = accessed_node; while let Some(current) = this.nodes.get(last_node).unwrap().parent { self.propagate_at(this, current, AccessRelatedness::StrictChildAccess)?; let node = this.nodes.get(current).unwrap(); + // We `rev()` here because we reverse the entire stack later. for &child in node.children.iter().rev() { if last_node == child { continue; } - self.stack.push((child, AccessRelatedness::DistantAccess, false)); + self.stack.push(( + child, + AccessRelatedness::DistantAccess, + RecursionState::BeforeChildren, + )); } last_node = current; } + // Reverse the stack, as discussed above. self.stack.reverse(); Ok(()) } fn finish_foreign_accesses(&mut self, this: &mut TreeVisitor<'_>) -> Result<(), OutErr> { - while let Some((idx, rel_pos, is_final)) = self.stack.last_mut() { + while let Some((idx, rel_pos, step)) = self.stack.last_mut() { let idx = *idx; let rel_pos = *rel_pos; - if *is_final { - self.stack.pop(); - self.propagate_at(this, idx, rel_pos)?; - } else { - *is_final = true; - let handle_children = self.should_continue_at(this, idx, rel_pos); - match handle_children { - ContinueTraversal::Recurse => { - // add all children, and also leave the node itself - // on the stack so that it can be visited later. - let node = this.nodes.get(idx).unwrap(); - for &child in node.children.iter() { - self.stack.push((child, rel_pos, false)); + match *step { + // How to do bottom-up traversal, 101: Before you handle a node, you handle all children. + // For this, you must first find the children, which is what this code here does. + RecursionState::BeforeChildren => { + // Next time we come back will be when all the children are handled. + *step = RecursionState::AfterChildren; + // Now push the children, except if we are told to skip this subtree. + let handle_children = self.should_continue_at(this, idx, rel_pos); + match handle_children { + ContinueTraversal::Recurse => { + let node = this.nodes.get(idx).unwrap(); + for &child in node.children.iter() { + self.stack.push((child, rel_pos, RecursionState::BeforeChildren)); + } + } + ContinueTraversal::SkipSelfAndChildren => { + // skip self + self.stack.pop(); + continue; } } - ContinueTraversal::SkipSelfAndChildren => { - // skip self - self.stack.pop(); - continue; - } + } + // All the children are handled, let's actually visit this node + RecursionState::AfterChildren => { + self.stack.pop(); + self.propagate_at(this, idx, rel_pos)?; } } } @@ -437,19 +473,42 @@ where } impl<'tree> TreeVisitor<'tree> { - // Applies `f_propagate` to every vertex of the tree bottom-up in the following order: first - // all ancestors of `start` (starting with `start` itself), then children of `start`, then the rest, - // always going bottom-up. - // This ensures that errors are triggered in the following order - // - first invalid accesses with insufficient permissions, closest to the accessed node first, - // - then protector violations, bottom-up, starting with the children of the accessed node, and then - // going upwards and outwards. - // - // `f_propagate` should follow the following format: for a given `Node` it updates its - // `Permission` depending on the position relative to `start` (given by an - // `AccessRelatedness`). - // `f_continue` is called before on foreign nodes, and describes whether to continue - // with the subtree at that node. + /// Applies `f_propagate` to every vertex of the tree in a piecewise bottom-up way: First, visit + /// all ancestors of `start` (starting with `start` itself), then children of `start`, then the rest, + /// going bottom-up in each of these two "pieces" / sections. + /// This ensures that errors are triggered in the following order + /// - first invalid accesses with insufficient permissions, closest to the accessed node first, + /// - then protector violations, bottom-up, starting with the children of the accessed node, and then + /// going upwards and outwards. + /// + /// The following graphic visualizes it, with numbers indicating visitation order and `start` being + /// the node that is visited first ("1"): + /// + /// ```text + /// 3 + /// /| + /// / | + /// 9 2 + /// | |\ + /// | | \ + /// 8 1 7 + /// / \ + /// 4 6 + /// | + /// 5 + /// ``` + /// + /// `f_propagate` should follow the following format: for a given `Node` it updates its + /// `Permission` depending on the position relative to `start` (given by an + /// `AccessRelatedness`). + /// `f_continue` is called earlier on foreign nodes, and describes whether to even start + /// visiting the subtree at that node. If it e.g. returns `SkipSelfAndChildren` on node 6 + /// above, then nodes 5 _and_ 6 would not be visited by `f_propagate`. It is not used for + /// notes having a child access (nodes 1, 2, 3). + /// + /// Finally, remember that the iteration order is not relevant for UB, it only affects + /// diagnostics. It also affects tree traversal optimizations built on top of this, so + /// those need to be reviewed carefully as well whenever this changes. fn traverse_this_parents_children_other( mut self, start: BorTag, @@ -463,14 +522,18 @@ impl<'tree> TreeVisitor<'tree> { // undergoing a child access. Also pushes the children and the other // cousin nodes (i.e. all nodes undergoing a foreign access) to the stack // to be processed later. - stack.go_upwards_from_accessed(&mut self, start_idx, true)?; + stack.go_upwards_from_accessed( + &mut self, + start_idx, + ChildrenVisitMode::VisitChildrenOfAccessed, + )?; // Now visit all the foreign nodes we remembered earlier. // For this we go bottom-up, but also allow f_continue to skip entire // subtrees from being visited if it would be a NOP. stack.finish_foreign_accesses(&mut self) } - // Like `traverse_this_parents_children_other`, but skips the children of `start`. + /// Like `traverse_this_parents_children_other`, but skips the children of `start`. fn traverse_nonchildren( mut self, start: BorTag, @@ -483,7 +546,11 @@ impl<'tree> TreeVisitor<'tree> { // Visits the accessed node itself, and all its parents, i.e. all nodes // undergoing a child access. Also pushes the other cousin nodes to the // stack, but not the children of the accessed node. - stack.go_upwards_from_accessed(&mut self, start_idx, false)?; + stack.go_upwards_from_accessed( + &mut self, + start_idx, + ChildrenVisitMode::SkipChildrenOfAccessed, + )?; // Now visit all the foreign nodes we remembered earlier. // For this we go bottom-up, but also allow f_continue to skip entire // subtrees from being visited if it would be a NOP. From e26779e784cf36e0dc48add24ca961255f4d8a76 Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Fri, 23 Aug 2024 22:22:20 +0200 Subject: [PATCH 28/35] Make Tree Borrows Provenance GC compact the tree Follow-up on #3833 and #3835. In these PRs, the TB GC was fixed to no longer cause a stack overflow. One test that motivated it was the test `fill::horizontal_line` in `tiny_skia`. But not causing stack overflows was not a large improvents, since it did not fix the fundamental issue: The tree was too large. The test now ran, but it required gigabytes of memory and hours of time, whereas it finishes within seconds in Stacked Borrows. The problem in that test was that it used [`slice::chunked`](https://doc.rust-lang.org/std/primitive.slice.html#method.chunks) to iterate a slice in chunks. That iterator is written to reborrow at each call to `next`, which creates a linear tree with a bunch of intermediary nodes, which also fragments the `RangeMap` for that allocation. The solution is to now compact the tree, so that these interior nodes are removed. Care is taken to not remove nodes that are protected, or that otherwise restrict their children. --- .../src/borrow_tracker/tree_borrows/perms.rs | 39 +++++++- .../src/borrow_tracker/tree_borrows/tree.rs | 94 ++++++++++++++++--- .../borrow_tracker/tree_borrows/tree/tests.rs | 65 +++++++++++++ .../tests/pass-dep/concurrency/linux-futex.rs | 4 +- 4 files changed, 185 insertions(+), 17 deletions(-) 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 5461edb51d3af..85c5ba627dfd8 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs @@ -130,7 +130,7 @@ mod transition { Active => if protected { // We wrote, someone else reads -- that's bad. - // (If this is initialized, this move-to-protected will mean insta-UB.) + // (Since Active is always initialized, this move-to-protected will mean insta-UB.) Disabled } else { // We don't want to disable here to allow read-read reordering: it is crucial @@ -267,6 +267,43 @@ impl Permission { transition::perform_access(kind, rel_pos, old_state, protected) .map(|new_state| PermTransition { from: old_state, to: new_state }) } + + /// During a provenance GC, we want to compact the tree. + /// For this, we want to merge nodes upwards if they have a singleton parent. + /// But we need to be careful: If the parent is Frozen, and the child is Reserved, + /// we can not do such a merge. In general, such a merge is possible if the parent + /// allows similar accesses, and in particular if the parent never causes UB on its + /// own. This is enforced by a test, namely `tree_compacting_is_sound`. See that + /// test for more information. + /// This method is only sound if the parent is not protected. We never attempt to + /// remove protected parents. + pub fn can_be_replaced_by_child(self, child: Self) -> bool { + match (self.inner, child.inner) { + // ReservedIM can be replaced by anything, as it allows all + // transitions. + (ReservedIM, _) => true, + // Reserved (as parent, where conflictedness does not matter) + // can be replaced by all but ReservedIM, + // since ReservedIM alone would survive foreign writes + (ReservedFrz { .. }, ReservedIM) => false, + (ReservedFrz { .. }, _) => true, + // Active can not be replaced by something surviving + // foreign reads and then remaining writable + (Active, ReservedIM) => false, + (Active, ReservedFrz { .. }) => false, + (Active, Active) => true, + // Active can be replaced by Frozen, since it is not protected + (Active, Frozen) => true, + (Active, Disabled) => true, + // Frozen can only be replaced by Disabled + (Frozen, Frozen) => true, + (Frozen, Disabled) => true, + (Frozen, _) => false, + // Disabled can not be replaced by anything else + (Disabled, Disabled) => true, + (Disabled, _) => false, + } + } } impl PermTransition { 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 728a664680da4..b9521deeb0f98 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -128,6 +128,22 @@ impl LocationState { Ok(transition) } + /// Like `perform_access`, but ignores the diagnostics, and also is pure. + /// As such, it returns `Some(x)` if the transition succeeded, or `None` + /// if there was an error. + #[allow(unused)] + fn perform_access_no_fluff( + mut self, + access_kind: AccessKind, + rel_pos: AccessRelatedness, + protected: bool, + ) -> Option { + match self.perform_access(access_kind, rel_pos, protected) { + Ok(_) => Some(self), + Err(_) => None, + } + } + // 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`, @@ -840,6 +856,57 @@ impl Tree { node.children.is_empty() && !live.contains(&node.tag) } + /// Checks whether a node can be replaced by its only child. + /// If so, returns the index of said only child. + /// If not, returns none. + fn can_be_replaced_by_single_child( + &self, + idx: UniIndex, + live: &FxHashSet, + ) -> Option { + let node = self.nodes.get(idx).unwrap(); + if node.children.len() != 1 || live.contains(&node.tag) || node.parent.is_none() { + return None; + } + // Since protected nodes are never GC'd (see `borrow_tracker::GlobalStateInner::visit_provenance`), + // we know that `node` is not protected because otherwise `live` would + // have contained `node.tag`. + let child_idx = node.children[0]; + let child = self.nodes.get(child_idx).unwrap(); + for (_, data) in self.rperms.iter_all() { + let parent_perm = + data.get(idx).map(|x| x.permission).unwrap_or_else(|| node.default_initial_perm); + let child_perm = data + .get(child_idx) + .map(|x| x.permission) + .unwrap_or_else(|| child.default_initial_perm); + if !parent_perm.can_be_replaced_by_child(child_perm) { + return None; + } + } + + Some(child_idx) + } + + /// Properly removes a node. + /// The node to be removed should not otherwise be usable. It also + /// should have no children, but this is not checked, so that nodes + /// whose children were rotated somewhere else can be deleted without + /// having to first modify them to clear that array. + /// otherwise (i.e. the GC should have marked it as removable). + fn remove_useless_node(&mut self, this: UniIndex) { + // Due to the API of UniMap we must make sure to call + // `UniValMap::remove` for the key of this node on *all* maps that used it + // (which are `self.nodes` and every range of `self.rperms`) + // before we can safely apply `UniKeyMap::remove` to truly remove + // this tag from the `tag_mapping`. + let node = self.nodes.remove(this).unwrap(); + for (_perms_range, perms) in self.rperms.iter_mut_all() { + perms.remove(this); + } + self.tag_mapping.remove(&node.tag); + } + /// Traverses the entire tree looking for useless tags. /// Removes from the tree all useless child nodes of root. /// It will not delete the root itself. @@ -883,23 +950,20 @@ impl Tree { // Remove all useless children. children_of_node.retain_mut(|idx| { if self.is_useless(*idx, live) { - // Note: In the rest of this comment, "this node" refers to `idx`. - // This node has no more children (if there were any, they have already been removed). - // It is also unreachable as determined by the GC, so we can remove it everywhere. - // Due to the API of UniMap we must make sure to call - // `UniValMap::remove` for the key of this node on *all* maps that used it - // (which are `self.nodes` and every range of `self.rperms`) - // before we can safely apply `UniKeyMap::remove` to truly remove - // this tag from the `tag_mapping`. - let node = self.nodes.remove(*idx).unwrap(); - for (_perms_range, perms) in self.rperms.iter_mut_all() { - perms.remove(*idx); - } - self.tag_mapping.remove(&node.tag); - // now delete it + // delete it everywhere else + self.remove_useless_node(*idx); + // and delete it from children_of_node false } else { - // do nothing, but retain + if let Some(nextchild) = self.can_be_replaced_by_single_child(*idx, live) { + // delete the in-between child + self.remove_useless_node(*idx); + // set the new child's parent + self.nodes.get_mut(nextchild).unwrap().parent = Some(*tag); + // save the new child in children_of_node + *idx = nextchild; + } + // retain it true } }); 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 654419c099d3c..f64f7bf8e8c1e 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 @@ -64,6 +64,71 @@ fn all_read_accesses_commute() { } } +fn as_foreign_or_child(related: AccessRelatedness) -> &'static str { + if related.is_foreign() { "foreign" } else { "child" } +} + +fn as_protected(b: bool) -> &'static str { + if b { " (protected)" } else { "" } +} + +fn as_lazy_or_init(b: bool) -> &'static str { + if b { "initialized" } else { "lazy" } +} + +/// Test that tree compacting (as performed by the GC) is sound. +/// Specifically, the GC will replace a parent by a child if the parent is not +/// protected, and if `can_be_replaced_by_child(parent, child)` is true. +/// To check that this is sound, the function must be a simulation, i.e. +/// if both are accessed, the results must still be in simulation, and also +/// if an access is UB, it must also be UB if done only at the child. +#[test] +fn tree_compacting_is_sound() { + // The parent is unprotected + let parent_protected = false; + for ([parent, child], child_protected) in <([LocationState; 2], bool)>::exhaustive() { + if child_protected { + precondition!(child.compatible_with_protector()) + } + precondition!(parent.permission().can_be_replaced_by_child(child.permission())); + for (kind, rel) in <(AccessKind, AccessRelatedness)>::exhaustive() { + let new_parent = parent.perform_access_no_fluff(kind, rel, parent_protected); + let new_child = child.perform_access_no_fluff(kind, rel, child_protected); + match (new_parent, new_child) { + (Some(np), Some(nc)) => { + assert!( + np.permission().can_be_replaced_by_child(nc.permission()), + "`can_be_replaced_by_child` is not a simulation: on a {} {} to a {} parent and a {} {}{} child, the parent becomes {}, the child becomes {}, and these are not in simulation!", + as_foreign_or_child(rel), + kind, + parent.permission(), + as_lazy_or_init(child.is_initialized()), + child.permission(), + as_protected(child_protected), + np.permission(), + nc.permission() + ) + } + (_, None) => { + // the child produced UB, this is fine no matter what the parent does + } + (None, Some(nc)) => { + panic!( + "`can_be_replaced_by_child` does not have the UB property: on a {} {} to a(n) {} parent and a(n) {} {}{} child, only the parent causes UB, while the child becomes {}, and it is not allowed for only the parent to cause UB!", + as_foreign_or_child(rel), + kind, + parent.permission(), + as_lazy_or_init(child.is_initialized()), + child.permission(), + as_protected(child_protected), + nc.permission() + ) + } + } + } + } +} + #[test] #[rustfmt::skip] // Ensure that of 2 accesses happen, one foreign and one a child, and we are protected, that we diff --git a/src/tools/miri/tests/pass-dep/concurrency/linux-futex.rs b/src/tools/miri/tests/pass-dep/concurrency/linux-futex.rs index d21f953672dee..399d6df73ff12 100644 --- a/src/tools/miri/tests/pass-dep/concurrency/linux-futex.rs +++ b/src/tools/miri/tests/pass-dep/concurrency/linux-futex.rs @@ -158,7 +158,9 @@ fn wait_wake() { ); } - assert!((200..1000).contains(&start.elapsed().as_millis())); + // When running this in stress-gc mode, things can take quite long. + // So the timeout is 3000 ms. + assert!((200..3000).contains(&start.elapsed().as_millis())); t.join().unwrap(); } From ae3c270480940edfae6f008f5f3fd2f9a68efe7c Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Wed, 28 Aug 2024 05:05:02 +0000 Subject: [PATCH 29/35] 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 6124cc327ad09..2c9a861e66e48 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -3f121b9461cce02a703a0e7e450568849dfaa074 +d9a2cc4daee38c63b2f69710ed61d40acc32b709 From abcfc17dc761d1d64859aaad247271500bfafdda Mon Sep 17 00:00:00 2001 From: tiif Date: Tue, 27 Aug 2024 17:15:41 +0800 Subject: [PATCH 30/35] Add test for tokio file io and mpsc --- src/tools/miri/test_dependencies/Cargo.lock | 7 ++++ src/tools/miri/test_dependencies/Cargo.toml | 2 +- .../miri/tests/pass-dep/tokio/file-io.rs | 41 +++++++++++++++++++ .../miri/tests/pass-dep/tokio/mpsc-await.rs | 20 +++++++++ .../tests/pass-dep/tokio/mpsc-await.stdout | 2 + src/tools/miri/tests/pass-dep/tokio/sleep.rs | 3 +- .../miri/tests/pass-dep/tokio/tokio_mvp.rs | 6 --- 7 files changed, 72 insertions(+), 9 deletions(-) create mode 100644 src/tools/miri/tests/pass-dep/tokio/file-io.rs create mode 100644 src/tools/miri/tests/pass-dep/tokio/mpsc-await.rs create mode 100644 src/tools/miri/tests/pass-dep/tokio/mpsc-await.stdout delete mode 100644 src/tools/miri/tests/pass-dep/tokio/tokio_mvp.rs diff --git a/src/tools/miri/test_dependencies/Cargo.lock b/src/tools/miri/test_dependencies/Cargo.lock index bbead8782233c..39d412817289d 100644 --- a/src/tools/miri/test_dependencies/Cargo.lock +++ b/src/tools/miri/test_dependencies/Cargo.lock @@ -44,6 +44,12 @@ version = "3.16.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "79296716171880943b8470b5f8d03aa55eb2e645a4874bdbb28adb49162e012c" +[[package]] +name = "bytes" +version = "1.7.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8318a53db07bb3f8dca91a600466bdb3f2eaadeedfdbcf02e1accbad9271ba50" + [[package]] name = "cc" version = "1.1.7" @@ -304,6 +310,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "daa4fb1bc778bd6f04cbfc4bb2d06a7396a8f299dc33ea1900cedaa316f467b1" dependencies = [ "backtrace", + "bytes", "libc", "mio", "pin-project-lite", diff --git a/src/tools/miri/test_dependencies/Cargo.toml b/src/tools/miri/test_dependencies/Cargo.toml index ce11a8abb0ea9..c24422df26cf2 100644 --- a/src/tools/miri/test_dependencies/Cargo.toml +++ b/src/tools/miri/test_dependencies/Cargo.toml @@ -20,7 +20,7 @@ tempfile = "3" page_size = "0.6" # Avoid pulling in all of tokio's dependencies. # However, without `net` and `signal`, tokio uses fewer relevant system APIs. -tokio = { version = "1.24", features = ["macros", "rt-multi-thread", "time", "net", "fs", "sync", "signal"] } +tokio = { version = "1.24", features = ["macros", "rt-multi-thread", "time", "net", "fs", "sync", "signal", "io-util"] } [target.'cfg(windows)'.dependencies] windows-sys = { version = "0.52", features = [ "Win32_Foundation", "Win32_System_Threading" ] } diff --git a/src/tools/miri/tests/pass-dep/tokio/file-io.rs b/src/tools/miri/tests/pass-dep/tokio/file-io.rs new file mode 100644 index 0000000000000..d14af299cd467 --- /dev/null +++ b/src/tools/miri/tests/pass-dep/tokio/file-io.rs @@ -0,0 +1,41 @@ +//@compile-flags: -Zmiri-disable-isolation +//@only-target-linux: We only support tokio on Linux + +use std::fs::remove_file; +use tokio::fs::{File, OpenOptions}; +use tokio::io::{self, AsyncReadExt, AsyncWriteExt}; + +#[path = "../../utils/mod.rs"] +mod utils; + +#[tokio::main] +async fn main() { + test_create_and_write().await.unwrap(); + test_create_and_read().await.unwrap(); +} + +async fn test_create_and_write() -> io::Result<()> { + let path = utils::prepare("foo.txt"); + let mut file = File::create(&path).await?; + + // Write 10 bytes to the file. + file.write(b"some bytes").await?; + assert_eq!(file.metadata().await.unwrap().len(), 10); + + remove_file(&path).unwrap(); + Ok(()) +} + +async fn test_create_and_read() -> io::Result<()> { + let bytes = b"more bytes"; + let path = utils::prepare_with_content("foo.txt", bytes); + let mut file = OpenOptions::new().read(true).open(&path).await.unwrap(); + let mut buffer = [0u8; 10]; + + // Read the whole file. + file.read(&mut buffer[..]).await?; + assert_eq!(&buffer, b"more bytes"); + + remove_file(&path).unwrap(); + Ok(()) +} diff --git a/src/tools/miri/tests/pass-dep/tokio/mpsc-await.rs b/src/tools/miri/tests/pass-dep/tokio/mpsc-await.rs new file mode 100644 index 0000000000000..7dea07c6e7da9 --- /dev/null +++ b/src/tools/miri/tests/pass-dep/tokio/mpsc-await.rs @@ -0,0 +1,20 @@ +//@only-target-linux: We only support tokio on Linux +use tokio::sync::mpsc; + +#[tokio::main] +async fn main() { + let (tx, mut rx) = mpsc::channel(32); + let tx2 = tx.clone(); + + tokio::spawn(async move { + tx.send("sending from handle").await.unwrap(); + }); + + tokio::spawn(async move { + tx2.send("sending from handle").await.unwrap(); + }); + + while let Some(message) = rx.recv().await { + println!("GOT = {}", message); + } +} diff --git a/src/tools/miri/tests/pass-dep/tokio/mpsc-await.stdout b/src/tools/miri/tests/pass-dep/tokio/mpsc-await.stdout new file mode 100644 index 0000000000000..52dc6483186f3 --- /dev/null +++ b/src/tools/miri/tests/pass-dep/tokio/mpsc-await.stdout @@ -0,0 +1,2 @@ +GOT = sending from handle +GOT = sending from handle diff --git a/src/tools/miri/tests/pass-dep/tokio/sleep.rs b/src/tools/miri/tests/pass-dep/tokio/sleep.rs index e6b02c02d0308..5e63037c8a652 100644 --- a/src/tools/miri/tests/pass-dep/tokio/sleep.rs +++ b/src/tools/miri/tests/pass-dep/tokio/sleep.rs @@ -1,5 +1,4 @@ -//@compile-flags: -Zmiri-permissive-provenance -Zmiri-backtrace=full -//@only-target-x86_64-unknown-linux: support for tokio only on linux and x86 +//@only-target-linux: We only support tokio on Linux use tokio::time::{sleep, Duration, Instant}; diff --git a/src/tools/miri/tests/pass-dep/tokio/tokio_mvp.rs b/src/tools/miri/tests/pass-dep/tokio/tokio_mvp.rs deleted file mode 100644 index 769a7a7d3849d..0000000000000 --- a/src/tools/miri/tests/pass-dep/tokio/tokio_mvp.rs +++ /dev/null @@ -1,6 +0,0 @@ -// Need to disable preemption to stay on the supported MVP codepath in mio. -//@compile-flags: -Zmiri-permissive-provenance -Zmiri-preemption-rate=0 -//@only-target-x86_64-unknown-linux: support for tokio exists only on linux and x86 - -#[tokio::main] -async fn main() {} From e34f35edd8bad1e6139ea1558689f4ef3ef1ab3b Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Tue, 27 Aug 2024 21:09:33 +0200 Subject: [PATCH 31/35] Add benchmark for TB slowdown --- .../bench-cargo-miri/slice-chunked/Cargo.lock | 7 +++++ .../bench-cargo-miri/slice-chunked/Cargo.toml | 8 ++++++ .../slice-chunked/src/main.rs | 26 +++++++++++++++++++ 3 files changed, 41 insertions(+) create mode 100644 src/tools/miri/bench-cargo-miri/slice-chunked/Cargo.lock create mode 100644 src/tools/miri/bench-cargo-miri/slice-chunked/Cargo.toml create mode 100644 src/tools/miri/bench-cargo-miri/slice-chunked/src/main.rs diff --git a/src/tools/miri/bench-cargo-miri/slice-chunked/Cargo.lock b/src/tools/miri/bench-cargo-miri/slice-chunked/Cargo.lock new file mode 100644 index 0000000000000..0f9e5db52d634 --- /dev/null +++ b/src/tools/miri/bench-cargo-miri/slice-chunked/Cargo.lock @@ -0,0 +1,7 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 3 + +[[package]] +name = "slice-chunked" +version = "0.1.0" diff --git a/src/tools/miri/bench-cargo-miri/slice-chunked/Cargo.toml b/src/tools/miri/bench-cargo-miri/slice-chunked/Cargo.toml new file mode 100644 index 0000000000000..8fed6ad2f3159 --- /dev/null +++ b/src/tools/miri/bench-cargo-miri/slice-chunked/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "slice-chunked" +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/slice-chunked/src/main.rs b/src/tools/miri/bench-cargo-miri/slice-chunked/src/main.rs new file mode 100644 index 0000000000000..b498c8bec7582 --- /dev/null +++ b/src/tools/miri/bench-cargo-miri/slice-chunked/src/main.rs @@ -0,0 +1,26 @@ +//! This is a small example using slice::chunks, which creates a very large Tree Borrows tree. +//! Thanks to ##3837, the GC now compacts the tree, so this test can be run in a reasonable time again. +//! The actual code is adapted from tiny_skia, see https://github.com/RazrFalcon/tiny-skia/blob/master/src/pixmap.rs#L121 +//! To make this benchmark demonstrate the effectiveness, run with MIRIFLAGS="-Zmiri-tree-borrows -Zmiri-provenance-gc=100" + +const N: usize = 1000; + +fn input_vec() -> Vec { + vec![0; N] +} + +fn main() { + let data_len = 2 * N; + let mut rgba_data = Vec::with_capacity(data_len); + let img_data = input_vec(); + for slice in img_data.chunks(2) { + let gray = slice[0]; + let alpha = slice[1]; + rgba_data.push(gray); + rgba_data.push(gray); + rgba_data.push(gray); + rgba_data.push(alpha); + } + + assert_eq!(rgba_data.len(), data_len); +} From 84134c61bc4e616bf144bb8498810df2bf7a48e4 Mon Sep 17 00:00:00 2001 From: Johannes Hostert Date: Wed, 28 Aug 2024 13:55:30 +0200 Subject: [PATCH 32/35] address nits --- src/tools/miri/src/borrow_tracker/mod.rs | 6 +++++ .../src/borrow_tracker/tree_borrows/perms.rs | 9 ++++--- .../src/borrow_tracker/tree_borrows/tree.rs | 26 +++++++++++-------- 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/mod.rs b/src/tools/miri/src/borrow_tracker/mod.rs index 7a3d76a9beb35..9e205cd0064aa 100644 --- a/src/tools/miri/src/borrow_tracker/mod.rs +++ b/src/tools/miri/src/borrow_tracker/mod.rs @@ -71,6 +71,12 @@ pub struct FrameState { impl VisitProvenance for FrameState { fn visit_provenance(&self, visit: &mut VisitWith<'_>) { + // Visit all protected tags. At least in Tree Borrows, + // protected tags can not be GC'd because they still have + // an access coming when the protector ends. Additionally, + // the tree compacting mechanism of TB's GC relies on the fact + // that all protected tags are marked as live for correctness, + // so we _have_ to visit them here. for (id, tag) in &self.protected_tags { visit(Some(*id), Some(*tag)); } 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 85c5ba627dfd8..c29bd719b15df 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs @@ -288,18 +288,19 @@ impl Permission { (ReservedFrz { .. }, ReservedIM) => false, (ReservedFrz { .. }, _) => true, // Active can not be replaced by something surviving - // foreign reads and then remaining writable + // foreign reads and then remaining writable. (Active, ReservedIM) => false, (Active, ReservedFrz { .. }) => false, + // Replacing a state by itself is always okay, even if the child state is protected. (Active, Active) => true, - // Active can be replaced by Frozen, since it is not protected + // Active can be replaced by Frozen, since it is not protected. (Active, Frozen) => true, (Active, Disabled) => true, - // Frozen can only be replaced by Disabled + // Frozen can only be replaced by Disabled (and itself). (Frozen, Frozen) => true, (Frozen, Disabled) => true, (Frozen, _) => false, - // Disabled can not be replaced by anything else + // Disabled can not be replaced by anything else. (Disabled, Disabled) => true, (Disabled, _) => false, } 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 b9521deeb0f98..53e722259fdfb 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs @@ -128,10 +128,10 @@ impl LocationState { Ok(transition) } - /// Like `perform_access`, but ignores the diagnostics, and also is pure. - /// As such, it returns `Some(x)` if the transition succeeded, or `None` - /// if there was an error. - #[allow(unused)] + /// Like `perform_access`, but ignores the concrete error cause and also uses state-passing + /// rather than a mutable reference. As such, it returns `Some(x)` if the transition succeeded, + /// or `None` if there was an error. + #[cfg(test)] fn perform_access_no_fluff( mut self, access_kind: AccessKind, @@ -865,14 +865,18 @@ impl Tree { live: &FxHashSet, ) -> Option { let node = self.nodes.get(idx).unwrap(); + + // We never want to replace the root node, as it is also kept in `root_ptr_tags`. if node.children.len() != 1 || live.contains(&node.tag) || node.parent.is_none() { return None; } - // Since protected nodes are never GC'd (see `borrow_tracker::GlobalStateInner::visit_provenance`), + // Since protected nodes are never GC'd (see `borrow_tracker::FrameExtra::visit_provenance`), // we know that `node` is not protected because otherwise `live` would // have contained `node.tag`. let child_idx = node.children[0]; let child = self.nodes.get(child_idx).unwrap(); + // Check that for that one child, `can_be_replaced_by_child` holds for the permission + // on all locations. for (_, data) in self.rperms.iter_all() { let parent_perm = data.get(idx).map(|x| x.permission).unwrap_or_else(|| node.default_initial_perm); @@ -893,7 +897,6 @@ impl Tree { /// should have no children, but this is not checked, so that nodes /// whose children were rotated somewhere else can be deleted without /// having to first modify them to clear that array. - /// otherwise (i.e. the GC should have marked it as removable). fn remove_useless_node(&mut self, this: UniIndex) { // Due to the API of UniMap we must make sure to call // `UniValMap::remove` for the key of this node on *all* maps that used it @@ -950,17 +953,18 @@ impl Tree { // Remove all useless children. children_of_node.retain_mut(|idx| { if self.is_useless(*idx, live) { - // delete it everywhere else + // Delete `idx` node everywhere else. self.remove_useless_node(*idx); - // and delete it from children_of_node + // And delete it from children_of_node. false } else { if let Some(nextchild) = self.can_be_replaced_by_single_child(*idx, live) { - // delete the in-between child + // `nextchild` is our grandchild, and will become our direct child. + // Delete the in-between node, `idx`. self.remove_useless_node(*idx); - // set the new child's parent + // Set the new child's parent. self.nodes.get_mut(nextchild).unwrap().parent = Some(*tag); - // save the new child in children_of_node + // Save the new child in children_of_node. *idx = nextchild; } // retain it From b0c3324838a7f5666147536f59ec52e9d9ce33b9 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Thu, 29 Aug 2024 04:59:24 +0000 Subject: [PATCH 33/35] 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 2c9a861e66e48..b0910b8116e54 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -d9a2cc4daee38c63b2f69710ed61d40acc32b709 +acb4e8b6251f1d8da36f08e7a70fa23fc581839e From b5be3ab38b50ca0dc32e022feca9993c64e583e6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 29 Aug 2024 07:50:18 +0200 Subject: [PATCH 34/35] fix wasm test --- .../function_calls}/target_feature_wasm.rs | 4 ++-- .../fail/function_calls/target_feature_wasm.stderr | 13 +++++++++++++ .../fail/intrinsics/intrinsic_target_feature.rs | 2 +- .../miri/tests/pass/shims/x86/intrinsics-sha.rs | 2 +- .../miri/tests/pass/shims/x86/intrinsics-x86-adx.rs | 2 +- .../tests/pass/shims/x86/intrinsics-x86-aes-vaes.rs | 2 +- .../miri/tests/pass/shims/x86/intrinsics-x86-avx.rs | 2 +- .../tests/pass/shims/x86/intrinsics-x86-avx2.rs | 2 +- .../tests/pass/shims/x86/intrinsics-x86-avx512.rs | 2 +- .../miri/tests/pass/shims/x86/intrinsics-x86-bmi.rs | 2 +- .../shims/x86/intrinsics-x86-pause-without-sse2.rs | 2 +- .../pass/shims/x86/intrinsics-x86-pclmulqdq.rs | 2 +- .../pass/shims/x86/intrinsics-x86-sse3-ssse3.rs | 2 +- .../tests/pass/shims/x86/intrinsics-x86-sse41.rs | 2 +- .../tests/pass/shims/x86/intrinsics-x86-sse42.rs | 2 +- 15 files changed, 28 insertions(+), 15 deletions(-) rename src/tools/miri/tests/{panic => fail/function_calls}/target_feature_wasm.rs (79%) create mode 100644 src/tools/miri/tests/fail/function_calls/target_feature_wasm.stderr diff --git a/src/tools/miri/tests/panic/target_feature_wasm.rs b/src/tools/miri/tests/fail/function_calls/target_feature_wasm.rs similarity index 79% rename from src/tools/miri/tests/panic/target_feature_wasm.rs rename to src/tools/miri/tests/fail/function_calls/target_feature_wasm.rs index c67d2983f78fc..bd400e8824ac3 100644 --- a/src/tools/miri/tests/panic/target_feature_wasm.rs +++ b/src/tools/miri/tests/fail/function_calls/target_feature_wasm.rs @@ -1,4 +1,4 @@ -//@only-target-wasm32: tests WASM-specific behavior +//@only-target-wasm: tests WASM-specific behavior //@compile-flags: -C target-feature=-simd128 fn main() { @@ -6,7 +6,7 @@ fn main() { // But if the compiler actually uses the target feature, it will lead to an error when the module is loaded. // We emulate this with an "unsupported" error. assert!(!cfg!(target_feature = "simd128")); - simd128_fn(); + simd128_fn(); //~ERROR: unavailable target features } #[target_feature(enable = "simd128")] diff --git a/src/tools/miri/tests/fail/function_calls/target_feature_wasm.stderr b/src/tools/miri/tests/fail/function_calls/target_feature_wasm.stderr new file mode 100644 index 0000000000000..dc0aca77f9eb5 --- /dev/null +++ b/src/tools/miri/tests/fail/function_calls/target_feature_wasm.stderr @@ -0,0 +1,13 @@ +error: abnormal termination: calling a function that requires unavailable target features: simd128 + --> $DIR/target_feature_wasm.rs:LL:CC + | +LL | simd128_fn(); + | ^^^^^^^^^^^^ calling a function that requires unavailable target features: simd128 + | + = note: BACKTRACE: + = note: inside `main` at $DIR/target_feature_wasm.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/fail/intrinsics/intrinsic_target_feature.rs b/src/tools/miri/tests/fail/intrinsics/intrinsic_target_feature.rs index 860798f2ab156..eb5a16360fff8 100644 --- a/src/tools/miri/tests/fail/intrinsics/intrinsic_target_feature.rs +++ b/src/tools/miri/tests/fail/intrinsics/intrinsic_target_feature.rs @@ -7,7 +7,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm // Explicitly disable SSE4.1 because it is enabled by default on macOS //@compile-flags: -C target-feature=-sse4.1 diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-sha.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-sha.rs index e65fdc3fbed68..79ac4432dffac 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-sha.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-sha.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm //@compile-flags: -C target-feature=+sha,+sse2,+ssse3,+sse4.1 #[cfg(target_arch = "x86")] diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-adx.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-adx.rs index 431e7f2c5eb60..0fd4b7c091097 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-adx.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-adx.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm //@compile-flags: -C target-feature=+adx #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-aes-vaes.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-aes-vaes.rs index 7363c75361779..d4d1b6180a7b3 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-aes-vaes.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-aes-vaes.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm //@compile-flags: -C target-feature=+aes,+vaes,+avx512f #![feature(avx512_target_feature, stdarch_x86_avx512)] diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx.rs index 728f57d48f17e..3847a80be9053 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm //@compile-flags: -C target-feature=+avx #[cfg(target_arch = "x86")] diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx2.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx2.rs index 80d125bb85650..8b8d8880e3bed 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx2.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx2.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm //@compile-flags: -C target-feature=+avx2 #[cfg(target_arch = "x86")] diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx512.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx512.rs index 66bfcb20f1c99..a40eddde97c15 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx512.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-avx512.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm //@compile-flags: -C target-feature=+avx512f,+avx512vl,+avx512bitalg,+avx512vpopcntdq #![feature(avx512_target_feature)] diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-bmi.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-bmi.rs index 33424117c4542..02f57f4b451e1 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-bmi.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-bmi.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm //@compile-flags: -C target-feature=+bmi1,+bmi2 #[cfg(target_arch = "x86")] diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-pause-without-sse2.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-pause-without-sse2.rs index c8b92fd545850..60da88df046ef 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-pause-without-sse2.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-pause-without-sse2.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm //@compile-flags: -C target-feature=-sse2 #[cfg(target_arch = "x86")] diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-pclmulqdq.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-pclmulqdq.rs index 2f242dd5379d2..86ac5835a168b 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-pclmulqdq.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-pclmulqdq.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm //@compile-flags: -C target-feature=+pclmulqdq #[cfg(target_arch = "x86")] diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse3-ssse3.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse3-ssse3.rs index 7566be4431b7f..0b3be7f3cbd00 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse3-ssse3.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse3-ssse3.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm // SSSE3 implicitly enables SSE3 //@compile-flags: -C target-feature=+ssse3 diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse41.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse41.rs index 06607f3fd59e1..8cd4e6308e29f 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse41.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse41.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm //@compile-flags: -C target-feature=+sse4.1 #[cfg(target_arch = "x86")] diff --git a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse42.rs b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse42.rs index 3ac53ea8b933d..c87eb51877431 100644 --- a/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse42.rs +++ b/src/tools/miri/tests/pass/shims/x86/intrinsics-x86-sse42.rs @@ -6,7 +6,7 @@ //@ignore-target-avr //@ignore-target-s390x //@ignore-target-thumbv7em -//@ignore-target-wasm32 +//@ignore-target-wasm //@compile-flags: -C target-feature=+sse4.2 #[cfg(target_arch = "x86")] From 0453d9bee8f7a6035d76baefce52b35db188387f Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Fri, 30 Aug 2024 05:02:07 +0000 Subject: [PATCH 35/35] 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 b0910b8116e54..c33d9ad86149b 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -acb4e8b6251f1d8da36f08e7a70fa23fc581839e +0d634185dfddefe09047881175f35c65d68dcff1