From 0f5ac8372bff11744ae0965c9833f558d774a6d0 Mon Sep 17 00:00:00 2001 From: Oli Scherer Date: Tue, 23 May 2023 07:51:09 +0000 Subject: [PATCH 01/13] Hide full miri command line in `./miri run-dep` --- src/tools/miri/tests/compiletest.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/tools/miri/tests/compiletest.rs b/src/tools/miri/tests/compiletest.rs index fa06c4b6a1288..71959fa115892 100644 --- a/src/tools/miri/tests/compiletest.rs +++ b/src/tools/miri/tests/compiletest.rs @@ -277,7 +277,6 @@ fn run_dep_mode(target: String, mut args: impl Iterator) -> Res // the arguments to the interpreted prog cmd.arg("--"); cmd.args(args); - println!("{cmd:?}"); if cmd.spawn()?.wait()?.success() { Ok(()) } else { std::process::exit(1) } } From 9ffd3f941081b37f5c34afbaf62eb87af9b19b9d Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Fri, 26 May 2023 07:47:25 -0400 Subject: [PATCH 02/13] 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 0bff100dc140e..2b3aa19805c68 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -8b4b20836b832e91aa605a2faf5e2a55190202c8 +be72f2587c91579406117f99fa332383d66b7dcd From 5a54d34ff71f9dcaa10bc701627e6982135b9254 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Fri, 26 May 2023 08:06:06 -0400 Subject: [PATCH 03/13] rustup --- src/tools/miri/tests/pass/shims/fs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/tests/pass/shims/fs.rs b/src/tools/miri/tests/pass/shims/fs.rs index 3258a2be460b0..af245aa89aa36 100644 --- a/src/tools/miri/tests/pass/shims/fs.rs +++ b/src/tools/miri/tests/pass/shims/fs.rs @@ -31,7 +31,7 @@ fn main() { } fn host_to_target_path(path: String) -> PathBuf { - use std::ffi::{c_char, CStr, CString}; + use std::ffi::{CStr, CString}; let path = CString::new(path).unwrap(); let mut out = Vec::with_capacity(1024); From cb75f21b05cafd65f0c060735c635ac8b97e7413 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Wed, 10 May 2023 17:11:25 +0200 Subject: [PATCH 04/13] more precise filtering of events impl of is_relevant on transitions makes for much less noise in error messages Co-authored-by: Ralf Jung --- .../tree_borrows/diagnostics.rs | 7 +- .../src/borrow_tracker/tree_borrows/perms.rs | 183 ++++++++++++++---- .../tree-borrows/alternate-read-write.stderr | 6 +- .../fail/tree-borrows/error-range.stderr | 14 +- .../tree-borrows/fragile-data-race.stderr | 2 +- .../fail/tree-borrows/read-to-local.stderr | 6 +- .../tree-borrows/strongly-protected.stderr | 6 - .../tree-borrows/write-during-2phase.stderr | 2 +- 8 files changed, 160 insertions(+), 66 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs index 10873c46a6c5d..cea2cffa913ba 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs @@ -92,7 +92,7 @@ impl HistoryData { { // NOTE: `transition_range` is explicitly absent from the error message, it has no significance // to the user. The meaningful one is `access_range`. - self.events.push((Some(span.data()), format!("{this} then transitioned {transition} due to a {rel} {access_kind} at offsets {access_range:?}", rel = if is_foreign { "foreign" } else { "child" }))); + self.events.push((Some(span.data()), format!("{this} later transitioned to {endpoint} due to a {rel} {access_kind} at offsets {access_range:?}", endpoint = transition.endpoint(), rel = if is_foreign { "foreign" } else { "child" }))); self.events.push((None, format!("this corresponds to {}", transition.summary()))); } } @@ -212,12 +212,13 @@ impl History { /// Reconstruct the history relevant to `error_offset` by filtering /// only events whose range contains the offset we are interested in. - fn extract_relevant(&self, error_offset: u64) -> Self { + fn extract_relevant(&self, error_offset: u64, error_kind: TransitionError) -> Self { History { events: self .events .iter() .filter(|e| e.transition_range.contains(&error_offset)) + .filter(|e| e.transition.is_relevant(error_kind)) .cloned() .collect::>(), created: self.created, @@ -303,7 +304,7 @@ impl TbError<'_> { history.extend(self.accessed_info.history.forget(), "accessed", false); } history.extend( - self.conflicting_info.history.extract_relevant(self.error_offset), + self.conflicting_info.history.extract_relevant(self.error_offset, self.error_kind), conflicting_tag_name, true, ); 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 7e3e587db7248..6b1e722b65e67 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs @@ -1,6 +1,7 @@ use std::cmp::{Ordering, PartialOrd}; use std::fmt; +use crate::borrow_tracker::tree_borrows::diagnostics::TransitionError; use crate::borrow_tracker::tree_borrows::tree::AccessRelatedness; use crate::borrow_tracker::AccessKind; @@ -115,26 +116,31 @@ mod transition { /// Public interface to the state machine that controls read-write permissions. /// This is the "private `enum`" pattern. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct Permission(PermissionPriv); +pub struct Permission { + inner: PermissionPriv, +} /// Transition from one permission to the next. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct PermTransition(PermissionPriv, PermissionPriv); +pub struct PermTransition { + from: PermissionPriv, + to: PermissionPriv, +} impl Permission { /// Default initial permission of the root of a new tree. pub fn new_root() -> Self { - Self(Active) + Self { inner: Active } } /// Default initial permission of a reborrowed mutable reference. pub fn new_unique_2phase(ty_is_freeze: bool) -> Self { - Self(Reserved { ty_is_freeze }) + Self { inner: Reserved { ty_is_freeze } } } /// Default initial permission of a reborrowed shared reference pub fn new_frozen() -> Self { - Self(Frozen) + Self { inner: Frozen } } /// Apply the transition to the inner PermissionPriv. @@ -144,9 +150,9 @@ impl Permission { old_perm: Self, protected: bool, ) -> Option { - let old_state = old_perm.0; + let old_state = old_perm.inner; transition::perform_access(kind, rel_pos, old_state, protected) - .map(|new_state| PermTransition(old_state, new_state)) + .map(|new_state| PermTransition { from: old_state, to: new_state }) } } @@ -155,26 +161,27 @@ impl PermTransition { /// should be possible, but the same is not guaranteed by construction of /// transitions inferred by diagnostics. This checks that a transition /// reconstructed by diagnostics is indeed one that could happen. - fn is_possible(old: PermissionPriv, new: PermissionPriv) -> bool { - old <= new + fn is_possible(self) -> bool { + self.from <= self.to } - pub fn from(old: Permission, new: Permission) -> Option { - Self::is_possible(old.0, new.0).then_some(Self(old.0, new.0)) + pub fn from(from: Permission, to: Permission) -> Option { + let t = Self { from: from.inner, to: to.inner }; + t.is_possible().then_some(t) } pub fn is_noop(self) -> bool { - self.0 == self.1 + self.from == self.to } /// Extract result of a transition (checks that the starting point matches). pub fn applied(self, starting_point: Permission) -> Option { - (starting_point.0 == self.0).then_some(Permission(self.1)) + (starting_point.inner == self.from).then_some(Permission { inner: self.to }) } /// Extract starting point of a transition pub fn started(self) -> Permission { - Permission(self.0) + Permission { inner: self.from } } /// Determines whether a transition that occured is compatible with the presence @@ -190,10 +197,9 @@ impl PermTransition { /// }; /// ``` pub fn is_allowed_by_protector(&self) -> bool { - let &Self(old, new) = self; - assert!(Self::is_possible(old, new)); - match (old, new) { - _ if old == new => true, + assert!(self.is_possible()); + match (self.from, self.to) { + _ if self.from == self.to => true, // It is always a protector violation to not be readable anymore (_, Disabled) => false, // In the case of a `Reserved` under a protector, both transitions @@ -204,16 +210,9 @@ impl PermTransition { (Reserved { .. }, Active) | (Reserved { .. }, Frozen) => true, // This pointer should have stayed writeable for the whole function (Active, Frozen) => false, - _ => unreachable!("Transition from {old:?} to {new:?} should never be possible"), + _ => unreachable!("Transition {} should never be possible", self), } } - - /// Composition function: get the transition that can be added after `app` to - /// produce `self`. - pub fn apply_start(self, app: Self) -> Option { - let new_start = app.applied(Permission(self.0))?; - Self::from(new_start, Permission(self.1)) - } } pub mod diagnostics { @@ -224,10 +223,10 @@ pub mod diagnostics { f, "{}", match self { - PermissionPriv::Reserved { .. } => "Reserved", - PermissionPriv::Active => "Active", - PermissionPriv::Frozen => "Frozen", - PermissionPriv::Disabled => "Disabled", + Reserved { .. } => "Reserved", + Active => "Active", + Frozen => "Frozen", + Disabled => "Disabled", } ) } @@ -235,13 +234,13 @@ pub mod diagnostics { impl fmt::Display for PermTransition { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "from {} to {}", self.0, self.1) + write!(f, "from {} to {}", self.from, self.to) } } impl fmt::Display for Permission { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0) + write!(f, "{}", self.inner) } } @@ -251,7 +250,7 @@ pub mod diagnostics { // Make sure there are all of the same length as each other // and also as `diagnostics::DisplayFmtPermission.uninit` otherwise // alignment will be incorrect. - match self.0 { + match self.inner { Reserved { ty_is_freeze: true } => "Res", Reserved { ty_is_freeze: false } => "Re*", Active => "Act", @@ -269,9 +268,9 @@ pub mod diagnostics { /// to have write permissions, because that's what the diagnostics care about /// (otherwise `Reserved -> Frozen` would be considered a noop). pub fn summary(&self) -> &'static str { - assert!(Self::is_possible(self.0, self.1)); - match (self.0, self.1) { - (_, Active) => "an activation", + assert!(self.is_possible()); + match (self.from, self.to) { + (_, Active) => "the first write to a 2-phase borrowed mutable reference", (_, Frozen) => "a loss of write permissions", (Frozen, Disabled) => "a loss of read permissions", (_, Disabled) => "a loss of read and write permissions", @@ -279,6 +278,118 @@ pub mod diagnostics { unreachable!("Transition from {old:?} to {new:?} should never be possible"), } } + + /// Determines whether `self` is a relevant transition for the error `err`. + /// `self` will be a transition that happened to a tag some time before + /// that tag caused the error. + /// + /// Irrelevant events: + /// - modifications of write permissions when the error is related to read permissions + /// (on failed reads and protected `Frozen -> Disabled`, ignore `Reserved -> Active`, + /// `Reserved -> Frozen`, and `Active -> Frozen`) + /// - all transitions for attempts to deallocate strongly protected tags + /// + /// # Panics + /// + /// This function assumes that its arguments apply to the same location + /// and that they were obtained during a normal execution. It will panic otherwise. + /// - `err` cannot be a `ProtectedTransition(_)` of a noop transition, as those + /// never trigger protectors; + /// - all transitions involved in `self` and `err` should be increasing + /// (Reserved < Active < Frozen < Disabled); + /// - between `self` and `err` the permission should also be increasing, + /// so all permissions inside `err` should be greater than `self.1`; + /// - `Active` and `Reserved` cannot cause an error due to insufficient permissions, + /// so `err` cannot be a `ChildAccessForbidden(_)` of either of them; + pub(in super::super) fn is_relevant(&self, err: TransitionError) -> bool { + // NOTE: `super::super` is the visibility of `TransitionError` + assert!(self.is_possible()); + if self.is_noop() { + return false; + } + match err { + TransitionError::ChildAccessForbidden(insufficient) => { + // Show where the permission was gained then lost, + // but ignore unrelated permissions. + // This eliminates transitions like `Active -> Frozen` + // when the error is a failed `Read`. + match (self.to, insufficient.inner) { + (Frozen, Frozen) => true, + (Active, Frozen) => true, + (Disabled, Disabled) => true, + // A pointer being `Disabled` is a strictly stronger source of + // errors than it being `Frozen`. If we try to access a `Disabled`, + // then where it became `Frozen` (or `Active`) is the least of our concerns for now. + (Active | Frozen, Disabled) => false, + + // `Active` and `Reserved` have all permissions, so a + // `ChildAccessForbidden(Reserved | Active)` can never exist. + (_, Active) | (_, Reserved { .. }) => + unreachable!("this permission cannot cause an error"), + // No transition has `Reserved` as its `.to` unless it's a noop. + (Reserved { .. }, _) => unreachable!("self is a noop transition"), + // All transitions produced in normal executions (using `apply_access`) + // change permissions in the order `Reserved -> Active -> Frozen -> Disabled`. + // We assume that the error was triggered on the same location that + // the transition `self` applies to, so permissions found must be increasing + // in the order `self.from < self.to <= insufficient.inner` + (Disabled, Frozen) => + unreachable!("permissions between self and err must be increasing"), + } + } + TransitionError::ProtectedTransition(forbidden) => { + assert!(!forbidden.is_noop()); + // Show how we got to the starting point of the forbidden transition, + // but ignore what came before. + // This eliminates transitions like `Reserved -> Active` + // when the error is a `Frozen -> Disabled`. + match (self.to, forbidden.from, forbidden.to) { + // We absolutely want to know where it was activated. + (Active, Active, Frozen | Disabled) => true, + // And knowing where it became Frozen is also important. + (Frozen, Frozen, Disabled) => true, + // If the error is a transition `Frozen -> Disabled`, then we don't really + // care whether before that was `Reserved -> Active -> Frozen` or + // `Reserved -> Frozen` or even `Frozen` directly. + // The error will only show either + // - created as Frozen, then Frozen -> Disabled is forbidden + // - created as Reserved, later became Frozen, then Frozen -> Disabled is forbidden + // In both cases the `Reserved -> Active` part is inexistant or irrelevant. + (Active, Frozen, Disabled) => false, + + // `Reserved -> Frozen` does not trigger protectors. + (_, Reserved { .. }, Frozen) => + unreachable!("this transition cannot cause an error"), + // No transition has `Reserved` as its `.to` unless it's a noop. + (Reserved { .. }, _, _) => unreachable!("self is a noop transition"), + (_, Disabled, Disabled) | (_, Frozen, Frozen) | (_, Active, Active) => + unreachable!("err contains a noop transition"), + + // Permissions only evolve in the order `Reserved -> Active -> Frozen -> Disabled`, + // so permissions found must be increasing in the order + // `self.from < self.to <= forbidden.from < forbidden.to`. + (Disabled, Reserved { .. } | Active | Frozen, _) + | (Frozen, Reserved { .. } | Active, _) + | (Active, Reserved { .. }, _) => + unreachable!("permissions between self and err must be increasing"), + (_, Disabled, Reserved { .. } | Active | Frozen) + | (_, Frozen, Reserved { .. } | Active) + | (_, Active, Reserved { .. }) => + unreachable!("permissions within err must be increasing"), + } + } + // We don't care because protectors evolve independently from + // permissions. + TransitionError::ProtectedDealloc => false, + } + } + + /// Endpoint of a transition. + /// Meant only for diagnostics, use `applied` in non-diagnostics + /// code, which also checks that the starting point matches the current state. + pub fn endpoint(&self) -> Permission { + Permission { inner: self.to } + } } } 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 bb601fc88352d..d82cb8288a38f 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 @@ -17,13 +17,13 @@ help: the conflicting tag was created here, in the initial state Reserved | LL | let y = unsafe { &mut *(x as *mut u8) }; | ^^^^^^^^^^^^^^^^^^^^ -help: the conflicting tag then transitioned from Reserved to Active due to a child write access at offsets [0x0..0x1] +help: the conflicting 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 corresponds to an activation -help: the conflicting tag then transitioned from Active to Frozen due to a foreign read access at offsets [0x0..0x1] + = help: this 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] --> $DIR/alternate-read-write.rs:LL:CC | LL | let _val = *x; 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 bc829fd86d350..10c2e95ca2f34 100644 --- a/src/tools/miri/tests/fail/tree-borrows/error-range.stderr +++ b/src/tools/miri/tests/fail/tree-borrows/error-range.stderr @@ -17,19 +17,7 @@ help: the conflicting tag was created here, in the initial state Reserved | LL | let rmut = &mut *addr_of_mut!(data[0..6]); | ^^^^ -help: the conflicting tag then transitioned from Reserved to Active due to a child write access at offsets [0x5..0x6] - --> $DIR/error-range.rs:LL:CC - | -LL | rmut[5] += 1; - | ^^^^^^^^^^^^ - = help: this corresponds to an activation -help: the conflicting tag then transitioned from Active to Frozen due to a foreign read access at offsets [0x5..0x6] - --> $DIR/error-range.rs:LL:CC - | -LL | let _v = data[5]; - | ^^^^^^^ - = help: this corresponds to a loss of write permissions -help: the conflicting tag then transitioned from Frozen to Disabled due to a foreign write access at offsets [0x5..0x6] +help: the conflicting 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/fragile-data-race.stderr b/src/tools/miri/tests/fail/tree-borrows/fragile-data-race.stderr index 79744964a8bcc..86fdf97ac41a3 100644 --- a/src/tools/miri/tests/fail/tree-borrows/fragile-data-race.stderr +++ b/src/tools/miri/tests/fail/tree-borrows/fragile-data-race.stderr @@ -17,7 +17,7 @@ help: the conflicting tag was created here, in the initial state Reserved | LL | pub fn catch_unwind R + UnwindSafe, R>(f: F) -> Result { | ^ -help: the conflicting tag then transitioned from Reserved to Frozen due to a foreign read access at offsets [0x0..0x1] +help: the conflicting tag later transitioned to Frozen due to a foreign read access at offsets [0x0..0x1] --> RUSTLIB/core/src/ptr/mod.rs:LL:CC | LL | crate::intrinsics::read_via_copy(src) diff --git a/src/tools/miri/tests/fail/tree-borrows/read-to-local.stderr b/src/tools/miri/tests/fail/tree-borrows/read-to-local.stderr index 8c9c2f8f9656b..6deb4b43f6a03 100644 --- a/src/tools/miri/tests/fail/tree-borrows/read-to-local.stderr +++ b/src/tools/miri/tests/fail/tree-borrows/read-to-local.stderr @@ -11,13 +11,13 @@ help: the accessed tag was created here, in the initial state Reserved | LL | let mref = &mut root; | ^^^^^^^^^ -help: the accessed tag then transitioned from Reserved 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/read-to-local.rs:LL:CC | LL | *ptr = 0; // Write | ^^^^^^^^ - = help: this corresponds to an activation -help: the accessed tag then transitioned from Active to Frozen due to a foreign read access at offsets [0x0..0x1] + = help: this corresponds to the first write to a 2-phase borrowed mutable reference +help: the accessed tag later transitioned to Frozen due to a foreign read access at offsets [0x0..0x1] --> $DIR/read-to-local.rs:LL:CC | LL | assert_eq!(root, 0); // Parent Read 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 071b216ff982b..97088d5854cc9 100644 --- a/src/tools/miri/tests/fail/tree-borrows/strongly-protected.stderr +++ b/src/tools/miri/tests/fail/tree-borrows/strongly-protected.stderr @@ -17,12 +17,6 @@ help: the strongly protected tag was created here, in the initial state Re | LL | fn inner(x: &mut i32, f: fn(&mut i32)) { | ^ -help: the strongly protected tag then transitioned from Reserved to Active due to a child write access at offsets [0x0..0x4] - --> $DIR/strongly-protected.rs:LL:CC - | -LL | drop(unsafe { Box::from_raw(raw) }); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - = help: this corresponds to an activation = note: BACKTRACE (of the first span): = note: inside `std::alloc::dealloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC = note: inside `::deallocate` at RUSTLIB/alloc/src/alloc.rs:LL:CC diff --git a/src/tools/miri/tests/fail/tree-borrows/write-during-2phase.stderr b/src/tools/miri/tests/fail/tree-borrows/write-during-2phase.stderr index f6285bdcf16d4..898f6108ccbdc 100644 --- a/src/tools/miri/tests/fail/tree-borrows/write-during-2phase.stderr +++ b/src/tools/miri/tests/fail/tree-borrows/write-during-2phase.stderr @@ -18,7 +18,7 @@ LL | | *inner = 42; LL | | n LL | | }); | |______^ -help: the accessed tag then transitioned from Reserved 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/write-during-2phase.rs:LL:CC | LL | *inner = 42; From c5013ce996ff018d8c1de89c6505a76d2a83cfe8 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sun, 28 May 2023 22:03:00 -0400 Subject: [PATCH 05/13] 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 2b3aa19805c68..e16c19b6bdfe1 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -be72f2587c91579406117f99fa332383d66b7dcd +089677eb32af83318467325edbef9b64053df532 From 4cff01e1c6976ca348a79b8f80aec3a91276ff9f Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Mon, 29 May 2023 20:26:59 -0400 Subject: [PATCH 06/13] 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 e16c19b6bdfe1..2851208e78061 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -089677eb32af83318467325edbef9b64053df532 +498553fc04f6a3fdc53412320f4e913bc53bc267 From 70e6b2680cbe87262d73d1c79616df54b8ca5440 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 31 May 2023 10:44:29 +0200 Subject: [PATCH 07/13] 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 2851208e78061..f24d448e98085 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -498553fc04f6a3fdc53412320f4e913bc53bc267 +617d3d6d722c432cdcbf210e6db55c3bdeafe381 From bfc897ec07eece6a180c2279bc470d08d6f2e23e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 31 May 2023 11:00:34 +0200 Subject: [PATCH 08/13] CI: test ./miri bench --- src/tools/miri/ci.sh | 21 ++++++++++----------- src/tools/miri/miri | 9 +++++---- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/tools/miri/ci.sh b/src/tools/miri/ci.sh index b5b3b211b05f0..a8aae524e7101 100755 --- a/src/tools/miri/ci.sh +++ b/src/tools/miri/ci.sh @@ -17,11 +17,11 @@ begingroup "Building Miri" echo "Installing release version of Miri" export RUSTFLAGS="-D warnings" export CARGO_INCREMENTAL=0 -./miri install # implicitly locked +export CARGO_EXTRA_FLAGS="--locked" +./miri install # Prepare debug build for direct `./miri` invocations echo "Building debug version of Miri" -export CARGO_EXTRA_FLAGS="--locked" ./miri check --no-default-features # make sure this can be built ./miri check --all-features # and this, too ./miri build --all-targets # the build that all the `./miri test` below will use @@ -39,8 +39,11 @@ function run_tests { ## ui test suite ./miri test if [ -z "${MIRI_TEST_TARGET+exists}" ]; then - # Only for host architecture: tests with optimizations (`-O` is what cargo passes, but crank MIR - # optimizations up all the way, too). + # Host-only tests: running these on all targets is unlikely to catch more problems and would + # cost a lot of CI time. + + # Tests with optimizations (`-O` is what cargo passes, but crank MIR optimizations up all the + # way, too). # Optimizations change diagnostics (mostly backtraces), so we don't check # them. Also error locations change so we don't run the failing tests. # We explicitly enable debug-assertions here, they are disabled by -O but we have tests @@ -51,6 +54,9 @@ function run_tests { for FILE in tests/many-seeds/*.rs; do MIRI_SEEDS=64 CARGO_EXTRA_FLAGS="$CARGO_EXTRA_FLAGS -q" ./miri many-seeds ./miri run "$FILE" done + + # Check that the benchmarks build and run, but without actually benchmarking. + HYPERFINE="bash -c" ./miri bench fi ## test-cargo-miri @@ -75,13 +81,6 @@ function run_tests { unset RUSTC MIRI rm -rf .cargo - # Ensure that our benchmarks all work, but only on Linux hosts. - if [ -z "${MIRI_TEST_TARGET+exists}" ] && [ "$HOST_TARGET" = x86_64-unknown-linux-gnu ] ; then - for BENCH in $(ls "bench-cargo-miri"); do - cargo miri run --manifest-path bench-cargo-miri/$BENCH/Cargo.toml - done - fi - endgroup } diff --git a/src/tools/miri/miri b/src/tools/miri/miri index 48a46a76a1297..ce3fcbba118c9 100755 --- a/src/tools/miri/miri +++ b/src/tools/miri/miri @@ -184,6 +184,8 @@ many-seeds) exit 0 ;; bench) + # The hyperfine to use + HYPERFINE=${HYPERFINE:-hyperfine -w 1 -m 5 --shell=none} # Make sure we have an up-to-date Miri installed "$0" install # Run the requested benchmarks @@ -193,7 +195,7 @@ bench) BENCHES=("$@") fi for BENCH in "${BENCHES[@]}"; do - hyperfine -w 1 -m 5 --shell=none "cargo +$TOOLCHAIN miri run --manifest-path $MIRIDIR/bench-cargo-miri/$BENCH/Cargo.toml" + $HYPERFINE "cargo +$TOOLCHAIN miri run --manifest-path $MIRIDIR/bench-cargo-miri/$BENCH/Cargo.toml" done exit 0 ;; @@ -280,10 +282,9 @@ find_sysroot() { # Run command. case "$COMMAND" in install) - # "--locked" to respect the Cargo.lock file if it exists. # Install binaries to the miri toolchain's sysroot so they do not interact with other toolchains. - $CARGO install $CARGO_EXTRA_FLAGS --path "$MIRIDIR" --force --locked --root "$SYSROOT" "$@" - $CARGO install $CARGO_EXTRA_FLAGS --path "$MIRIDIR"/cargo-miri --force --locked --root "$SYSROOT" "$@" + $CARGO install $CARGO_EXTRA_FLAGS --path "$MIRIDIR" --force --root "$SYSROOT" "$@" + $CARGO install $CARGO_EXTRA_FLAGS --path "$MIRIDIR"/cargo-miri --force --root "$SYSROOT" "$@" ;; check) # Check, and let caller control flags. From 91cb951ee908a35ab1050876e62411698cb2cc84 Mon Sep 17 00:00:00 2001 From: Piotr Osiewicz <24362066+osiewicz@users.noreply.github.com> Date: Fri, 5 May 2023 18:37:19 +0200 Subject: [PATCH 09/13] miri-script: Transform Windows paths to unix. python3 snippet used to fill $MIRIDIR variable returns native paths. Down the line in `bench` subcommand this leads to benchmark setup failure, preventing contributors from running benchmarks on Windows hosts. This commit replaces usage of native os.path module with pathlib, which explicitly converts paths to Unix flavour. --- src/tools/miri/miri | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/miri b/src/tools/miri/miri index ce3fcbba118c9..7cda995879cfb 100755 --- a/src/tools/miri/miri +++ b/src/tools/miri/miri @@ -77,7 +77,7 @@ if [ -z "$COMMAND" ]; then fi shift # macOS does not have a useful readlink/realpath so we have to use Python instead... -MIRIDIR=$(python3 -c 'import os, sys; print(os.path.dirname(os.path.realpath(sys.argv[1])))' "$0") +MIRIDIR=$(python3 -c 'import pathlib, sys; print(pathlib.Path(sys.argv[1]).resolve().parent.as_posix())' "$0") # Used for rustc syncs. JOSH_FILTER=":rev(75dd959a3a40eb5b4574f8d2e23aa6efbeb33573:prefix=src/tools/miri):/src/tools/miri" # Needed for `./miri bench`. From 777db72b08f316345cffde2fc83478668b9da7cf Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 2 Jun 2023 10:03:43 +0200 Subject: [PATCH 10/13] 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 f24d448e98085..ee815ae6f8fc0 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -617d3d6d722c432cdcbf210e6db55c3bdeafe381 +33c3d101280c8eb3cd8af421bfb56a8afcc3881d From be4e05a1906d975ac0e09ca1a5ea2daf6a0018a0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 2 Jun 2023 10:09:32 +0200 Subject: [PATCH 11/13] fmt --- src/tools/miri/src/diagnostics.rs | 17 ++++++++++------- src/tools/miri/src/helpers.rs | 6 +++--- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/tools/miri/src/diagnostics.rs b/src/tools/miri/src/diagnostics.rs index 8c788c915febe..2a06bd871ef92 100644 --- a/src/tools/miri/src/diagnostics.rs +++ b/src/tools/miri/src/diagnostics.rs @@ -97,8 +97,12 @@ impl MachineStopType for TerminationInfo { } fn add_args( self: Box, - _: &mut dyn FnMut(std::borrow::Cow<'static, str>, rustc_errors::DiagnosticArgValue<'static>), - ) {} + _: &mut dyn FnMut( + std::borrow::Cow<'static, str>, + rustc_errors::DiagnosticArgValue<'static>, + ), + ) { + } } /// Miri specific diagnostics @@ -324,10 +328,9 @@ pub fn report_error<'tcx, 'mir>( // We want to dump the allocation if this is `InvalidUninitBytes`. Since `add_args` consumes // the `InterpError`, we extract the variables it before that. let extra = match e { - UndefinedBehavior(UndefinedBehaviorInfo::InvalidUninitBytes(Some((alloc_id, access)))) => { - Some((alloc_id, access)) - } - _ => None + UndefinedBehavior(UndefinedBehaviorInfo::InvalidUninitBytes(Some((alloc_id, access)))) => + Some((alloc_id, access)), + _ => None, }; // FIXME(fee1-dead), HACK: we want to use the error as title therefore we can just extract the @@ -477,7 +480,7 @@ pub fn report_msg<'tcx>( } let (mut err, handler) = err.into_diagnostic().unwrap(); - + // Add backtrace for (idx, frame_info) in stacktrace.iter().enumerate() { let is_local = machine.is_local(frame_info); diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index f11902652393a..befdddfa8c901 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/src/helpers.rs @@ -162,9 +162,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> { let instance = this.resolve_path(path, Namespace::ValueNS); let cid = GlobalId { instance, promoted: None }; // We don't give a span -- this isn't actually used directly by the program anyway. - let const_val = this - .eval_global(cid, None) - .unwrap_or_else(|err| panic!("failed to evaluate required Rust item: {path:?}\n{err:?}")); + let const_val = this.eval_global(cid, None).unwrap_or_else(|err| { + panic!("failed to evaluate required Rust item: {path:?}\n{err:?}") + }); this.read_scalar(&const_val.into()) .unwrap_or_else(|err| panic!("failed to read required Rust item: {path:?}\n{err:?}")) } From e13d2380aeabe9ea4cef805448543449a06ad2d9 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 3 Jun 2023 17:03:24 +0200 Subject: [PATCH 12/13] add unchecked_shl test --- .../miri/tests/fail/intrinsics/unchecked_shl.rs | 8 ++++++++ .../tests/fail/intrinsics/unchecked_shl.stderr | 15 +++++++++++++++ ...rflowing-unchecked-rsh.rs => unchecked_shr.rs} | 0 ...-unchecked-rsh.stderr => unchecked_shr.stderr} | 4 ++-- 4 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 src/tools/miri/tests/fail/intrinsics/unchecked_shl.rs create mode 100644 src/tools/miri/tests/fail/intrinsics/unchecked_shl.stderr rename src/tools/miri/tests/fail/intrinsics/{overflowing-unchecked-rsh.rs => unchecked_shr.rs} (100%) rename src/tools/miri/tests/fail/intrinsics/{overflowing-unchecked-rsh.stderr => unchecked_shr.stderr} (84%) diff --git a/src/tools/miri/tests/fail/intrinsics/unchecked_shl.rs b/src/tools/miri/tests/fail/intrinsics/unchecked_shl.rs new file mode 100644 index 0000000000000..4554d0cb82ba0 --- /dev/null +++ b/src/tools/miri/tests/fail/intrinsics/unchecked_shl.rs @@ -0,0 +1,8 @@ +#![feature(unchecked_math)] + +fn main() { + unsafe { + let _n = 1i8.unchecked_shl(8); + //~^ ERROR: overflowing shift by 8 in `unchecked_shl` + } +} diff --git a/src/tools/miri/tests/fail/intrinsics/unchecked_shl.stderr b/src/tools/miri/tests/fail/intrinsics/unchecked_shl.stderr new file mode 100644 index 0000000000000..264e9baf053d6 --- /dev/null +++ b/src/tools/miri/tests/fail/intrinsics/unchecked_shl.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: overflowing shift by 8 in `unchecked_shl` + --> $DIR/unchecked_shl.rs:LL:CC + | +LL | let _n = 1i8.unchecked_shl(8); + | ^^^^^^^^^^^^^^^^^^^^ overflowing shift by 8 in `unchecked_shl` + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/unchecked_shl.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + diff --git a/src/tools/miri/tests/fail/intrinsics/overflowing-unchecked-rsh.rs b/src/tools/miri/tests/fail/intrinsics/unchecked_shr.rs similarity index 100% rename from src/tools/miri/tests/fail/intrinsics/overflowing-unchecked-rsh.rs rename to src/tools/miri/tests/fail/intrinsics/unchecked_shr.rs diff --git a/src/tools/miri/tests/fail/intrinsics/overflowing-unchecked-rsh.stderr b/src/tools/miri/tests/fail/intrinsics/unchecked_shr.stderr similarity index 84% rename from src/tools/miri/tests/fail/intrinsics/overflowing-unchecked-rsh.stderr rename to src/tools/miri/tests/fail/intrinsics/unchecked_shr.stderr index 9c5d0d13108ce..378ff9734c16e 100644 --- a/src/tools/miri/tests/fail/intrinsics/overflowing-unchecked-rsh.stderr +++ b/src/tools/miri/tests/fail/intrinsics/unchecked_shr.stderr @@ -1,5 +1,5 @@ error: Undefined Behavior: overflowing shift by 64 in `unchecked_shr` - --> $DIR/overflowing-unchecked-rsh.rs:LL:CC + --> $DIR/unchecked_shr.rs:LL:CC | LL | let _n = 1i64.unchecked_shr(64); | ^^^^^^^^^^^^^^^^^^^^^^ overflowing shift by 64 in `unchecked_shr` @@ -7,7 +7,7 @@ LL | let _n = 1i64.unchecked_shr(64); = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information = note: BACKTRACE: - = note: inside `main` at $DIR/overflowing-unchecked-rsh.rs:LL:CC + = note: inside `main` at $DIR/unchecked_shr.rs:LL:CC note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace From ca720fdfee83c7b63ee5acb29b6b5b07a493e236 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 3 Jun 2023 16:52:33 +0200 Subject: [PATCH 13/13] miri compiletest: no longer allow some warnings in rustc test suite --- src/tools/miri/tests/compiletest.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/tools/miri/tests/compiletest.rs b/src/tools/miri/tests/compiletest.rs index 71959fa115892..9e6e37031535e 100644 --- a/src/tools/miri/tests/compiletest.rs +++ b/src/tools/miri/tests/compiletest.rs @@ -51,18 +51,9 @@ fn test_config(target: &str, path: &str, mode: Mode, with_dependencies: bool) -> let mut program = CommandBuilder::rustc(); program.program = miri_path(); - let in_rustc_test_suite = option_env!("RUSTC_STAGE").is_some(); - // Add some flags we always want. - if in_rustc_test_suite { - // Less aggressive warnings to make the rustc toolstate management less painful. - // (We often get warnings when e.g. a feature gets stabilized or some lint gets added/improved.) - program.args.push("-Astable-features".into()); - program.args.push("-Aunused".into()); - } else { - program.args.push("-Dwarnings".into()); - program.args.push("-Dunused".into()); - } + program.args.push("-Dwarnings".into()); + program.args.push("-Dunused".into()); if let Ok(extra_flags) = env::var("MIRIFLAGS") { for flag in extra_flags.split_whitespace() { program.args.push(flag.into());