Auto merge of rust-lang#2888 - Vanille-N:tb-diags, r=RalfJung

TB diagnostics: avoid printing irrelevant events

History contains some events that are relevant to the location but not useful to understand the error.
We can make the selection of events more precise, from only "does it affect the location" to also "is it relevant for this kind of error"

This is also the occasion to fix rust-lang/miri#2867 (comment)

[Solved] Draft: find a way for blanks in the history to not be confusing, as with the current version the history can show the creation as `Reserved` then show where it transitioned from `Frozen` to `Disabled`, but it will say nothing of the `Reserved -> Frozen` leading up to it.

Author: bors

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::<Vec<_>>(),
             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,
         );

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<PermTransition> {
-        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> {
-        Self::is_possible(old.0, new.0).then_some(Self(old.0, new.0))
+    pub fn from(from: Permission, to: Permission) -> Option<Self> {
+        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<Permission> {
-        (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<Self> {
-        let new_start = app.applied(Permission(self.0))?;
-        Self::from(new_start, Permission(self.1))
-    }
 }
 
 pub mod diagnostics {
@@ -224,24 +223,24 @@ 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",
                 }
             )
         }
     }
 
     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,16 +268,128 @@ 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",
                 (old, new) =>
                     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 }
+        }
     }
 }
 

src/tools/miri/tests/fail/tree-borrows/alternate-read-write.stderr

@@ -17,13 +17,13 @@ help: the conflicting tag <TAG> was created here, in the initial state Reserved
    |
 LL |     let y = unsafe { &mut *(x as *mut u8) };
    |                      ^^^^^^^^^^^^^^^^^^^^
-help: the conflicting tag <TAG> then transitioned from Reserved to Active due to a child write access at offsets [0x0..0x1]
+help: the conflicting tag <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 <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 <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;

src/tools/miri/tests/fail/tree-borrows/error-range.stderr

@@ -17,19 +17,7 @@ help: the conflicting tag <TAG> was created here, in the initial state Reserved
    |
 LL |         let rmut = &mut *addr_of_mut!(data[0..6]);
    |                                       ^^^^
-help: the conflicting tag <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 <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 <TAG> then transitioned from Frozen to Disabled due to a foreign write access at offsets [0x5..0x6]
+help: the conflicting tag <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;

src/tools/miri/tests/fail/tree-borrows/fragile-data-race.stderr

@@ -17,7 +17,7 @@ help: the conflicting tag <TAG> was created here, in the initial state Reserved
    |
 LL | pub fn catch_unwind<F: FnOnce() -> R + UnwindSafe, R>(f: F) -> Result<R> {
    |                                                       ^
-help: the conflicting tag <TAG> then transitioned from Reserved to Frozen due to a foreign read access at offsets [0x0..0x1]
+help: the conflicting tag <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)

src/tools/miri/tests/fail/tree-borrows/read-to-local.stderr

@@ -11,13 +11,13 @@ help: the accessed tag <TAG> was created here, in the initial state Reserved
    |
 LL |         let mref = &mut root;
    |                    ^^^^^^^^^
-help: the accessed tag <TAG> then transitioned from Reserved to Active due to a child write access at offsets [0x0..0x1]
+help: the accessed tag <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 <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 <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