Tree Borrows: improved diagnostics

Author: Vanille-N

src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs

@@ -1,14 +1,90 @@
-use rustc_data_structures::fx::FxHashMap;
-
 use std::fmt;
 use std::ops::Range;
 
+use rustc_data_structures::fx::FxHashMap;
+use rustc_span::{Span, SpanData};
+use rustc_target::abi::Size;
+
 use crate::borrow_tracker::tree_borrows::{
-    err_tb_ub, perms::Permission, tree::LocationState, unimap::UniIndex,
+    perms::{PermTransition, Permission},
+    tree::LocationState,
+    unimap::UniIndex,
 };
 use crate::borrow_tracker::{AccessKind, ProtectorKind};
 use crate::*;
 
+/// Complete data for an event:
+/// - `kind` is what happened to the permissions
+/// - `access_kind` and `access_range` describe the access that caused the event
+/// - `offset` allows filtering only the relevant events for a given memory location
+/// (see how we perform the filtering in `History::extract_relevant`.
+/// - `span` is the line of code in question
+#[derive(Clone, Debug)]
+pub struct Event {
+    pub transition: PermTransition,
+    pub access_kind: AccessKind,
+    pub is_foreign: bool,
+    pub access_range: AllocRange,
+    pub offset: Size,
+    pub span: Span,
+}
+
+/// List of all events that affected a tag.
+/// NOTE: not all of these events are relevant for a particular location,
+/// the events should be filtered before the generation of diagnostics.
+/// Available filtering methods include `History::forget` and `History::extract_relevant`.
+#[derive(Clone, Debug)]
+pub struct History {
+    pub tag: BorTag,
+    pub created: (Span, Permission),
+    pub events: Vec<Event>,
+}
+
+/// History formatted for use by `src/diagnostics.rs`.
+///
+/// NOTE: needs to be `Send` because of a bound on `MachineStopType`, hence
+/// the use of `SpanData` rather than `Span`.
+#[derive(Debug, Clone, Default)]
+pub struct HistoryData {
+    pub events: Vec<(Option<SpanData>, String)>, // includes creation
+}
+
+impl History {
+    /// Record an additional event to the history.
+    pub fn push(&mut self, event: Event) {
+        self.events.push(event);
+    }
+}
+
+impl HistoryData {
+    // Format events from `new_history` into those recorded by `self`.
+    //
+    // NOTE: also converts `Span` to `SpanData`.
+    pub fn extend(
+        &mut self,
+        new_history: History,
+        tag_name: &'static str,
+        show_initial_state: bool,
+    ) {
+        let History { tag, created, events } = new_history;
+        let this = format!("the {tag_name} tag {tag:?}");
+        let msg_initial_state = format!(", in the initial state {}", created.1);
+        let msg_creation = format!(
+            "{this} was created here{maybe_msg_initial_state}.",
+            maybe_msg_initial_state = if show_initial_state { &msg_initial_state } else { "" },
+        );
+
+        self.events.push((Some(created.0.data()), msg_creation));
+        for &Event { transition, access_kind, is_foreign, access_range, span, offset: _ } in &events
+        {
+            // NOTE: `offset` 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((None, format!("this corresponds to {}.", transition.summary())));
+        }
+    }
+}
+
 /// Some information that is irrelevant for the algorithm but very
 /// convenient to know about a tag for debugging and testing.
 #[derive(Clone, Debug)]
@@ -20,18 +96,29 @@ pub struct NodeDebugInfo {
     /// pointer in the source code.
     /// Helps match tag numbers to human-readable names.
     pub name: Option<String>,
+    /// Notable events in the history of this tag, used for
+    /// diagnostics.
+    ///
+    /// NOTE: by virtue of being part of `NodeDebugInfo`,
+    /// the history is automatically cleaned up by the GC.
+    /// NOTE: this is `!Send`, it needs to be converted before displaying
+    /// the actual diagnostics because `src/diagnostics.rs` requires `Send`.
+    pub history: History,
 }
+
 impl NodeDebugInfo {
-    /// New node info with a name.
-    pub fn new(tag: BorTag) -> Self {
-        Self { tag, name: None }
+    /// Information for a new node. By default it has no
+    /// name and an empty history.
+    pub fn new(tag: BorTag, initial: Permission, span: Span) -> Self {
+        let history = History { tag, created: (span, initial), events: Vec::new() };
+        Self { tag, name: None, history }
     }
 
     /// Add a name to the tag. If a same tag is associated to several pointers,
     /// it can have several names which will be separated by commas.
-    fn add_name(&mut self, name: &str) {
+    pub fn add_name(&mut self, name: &str) {
         if let Some(ref mut prev_name) = &mut self.name {
-            prev_name.push(',');
+            prev_name.push_str(", ");
             prev_name.push_str(name);
         } else {
             self.name = Some(String::from(name));
@@ -42,7 +129,7 @@ impl NodeDebugInfo {
 impl fmt::Display for NodeDebugInfo {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         if let Some(ref name) = self.name {
-            write!(f, "{tag:?} (also named '{name}')", tag = self.tag)
+            write!(f, "{tag:?} ({name})", tag = self.tag)
         } else {
             write!(f, "{tag:?}", tag = self.tag)
         }
@@ -86,7 +173,7 @@ impl<'tcx> Tree {
     }
 }
 
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone, Copy, PartialEq)]
 pub(super) enum TransitionError {
     /// This access is not allowed because some parent tag has insufficient permissions.
     /// For example, if a tag is `Frozen` and encounters a child write this will
@@ -96,63 +183,146 @@ pub(super) enum TransitionError {
     /// A protector was triggered due to an invalid transition that loses
     /// too much permissions.
     /// For example, if a protected tag goes from `Active` to `Frozen` due
-    /// to a foreign write this will produce a `ProtectedTransition(Active, Frozen)`.
+    /// to a foreign write this will produce a `ProtectedTransition(PermTransition(Active, Frozen))`.
     /// This kind of error can only occur on foreign accesses.
-    ProtectedTransition(Permission, Permission),
+    ProtectedTransition(PermTransition),
     /// Cannot deallocate because some tag in the allocation is strongly protected.
     /// This kind of error can only occur on deallocations.
     ProtectedDealloc,
 }
 
+impl History {
+    /// Keep only the tag and creation
+    fn forget(&self) -> Self {
+        History { events: Vec::new(), created: self.created, tag: self.tag }
+    }
+
+    /// Reconstruct the history relevant to `error_offset` knowing that
+    /// its permission followed `complete_transition`.
+    ///
+    /// Here's how we do this:
+    /// - we know `full := complete_transition` the transition of the permission from
+    /// its initialization to the state just before the error was caused,
+    /// we want to find a chain of events that produces `full`
+    /// - we decompose `full` into `pre o post` where
+    /// `pre` is the best applicable transition from recorded events
+    /// - we select the event that caused `pre` and iterate
+    /// to find the chain of events that produces `full := post`
+    ///
+    /// To find the "best applicable transition" for full:
+    /// - eliminate events that cannot be applied because their offset is too big
+    /// - eliminate events that cannot be applied because their starting point is wrong
+    /// - select the one that happened closest to the range of interest
+    fn extract_relevant(&self, complete_transition: PermTransition, error_offset: Size) -> Self {
+        let mut selected_events: Vec<Event> = Vec::new();
+        let mut full = complete_transition;
+        while !full.is_noop() {
+            let (pre, post) = self
+                .events
+                .iter()
+                .filter(|e| e.offset <= error_offset)
+                .filter_map(|pre_canditate| {
+                    full.apply_start(pre_canditate.transition)
+                        .map(|post_canditate| (pre_canditate, post_canditate))
+                })
+                .max_by_key(|(pre_canditate, _post_candidate)| pre_canditate.offset)
+                .unwrap();
+            // If this occurs we will loop infinitely !
+            // Make sure to only put non-noop transitions in `History`.
+            assert!(!pre.transition.is_noop());
+            full = post;
+            selected_events.push(pre.clone());
+        }
+
+        History { events: selected_events, created: self.created, tag: self.tag }
+    }
+}
+
 /// Failures that can occur during the execution of Tree Borrows procedures.
 pub(super) struct TbError<'node> {
     /// What failure occurred.
     pub error_kind: TransitionError,
+    /// The byte at which the conflict occured.
+    pub error_offset: Size,
     /// The tag on which the error was triggered.
     /// On protector violations, this is the tag that was protected.
     /// On accesses rejected due to insufficient permissions, this is the
     /// tag that lacked those permissions.
-    pub faulty_tag: &'node NodeDebugInfo,
+    pub conflicting_info: &'node NodeDebugInfo,
     /// Whether this was a Read or Write access. This field is ignored
     /// when the error was triggered by a deallocation.
     pub access_kind: AccessKind,
     /// Which tag the access that caused this error was made through, i.e.
     /// which tag was used to read/write/deallocate.
-    pub tag_of_access: &'node NodeDebugInfo,
+    pub accessed_info: &'node NodeDebugInfo,
 }
 
 impl TbError<'_> {
     /// Produce a UB error.
-    pub fn build<'tcx>(self) -> InterpErrorInfo<'tcx> {
+    pub fn build<'tcx>(self) -> InterpError<'tcx> {
         use TransitionError::*;
-        err_tb_ub(match self.error_kind {
+        let started_as = self.conflicting_info.history.created.1;
+        let kind = self.access_kind;
+        let accessed = self.accessed_info;
+        let conflicting = self.conflicting_info;
+        let accessed_is_conflicting = accessed.tag == conflicting.tag;
+        let (pre_error, title, relation, problem, conflicting_tag_name) = match self.error_kind {
             ChildAccessForbidden(perm) => {
-                format!(
-                    "{kind} through {initial} is forbidden because it is a child of {current} which is {perm}.",
-                    kind=self.access_kind,
-                    initial=self.tag_of_access,
-                    current=self.faulty_tag,
-                    perm=perm,
+                let conflicting_tag_name =
+                    if accessed_is_conflicting { "accessed" } else { "conflicting" };
+                (
+                    perm,
+                    format!("{kind} through {accessed} is forbidden."),
+                    (!accessed_is_conflicting).then_some(format!(
+                        "the accessed tag {accessed} is a child of the conflicting tag {conflicting}."
+                    )),
+                    format!(
+                        "the {conflicting_tag_name} tag {conflicting} has state {perm} which forbids child {kind}es."
+                    ),
+                    conflicting_tag_name,
                 )
             }
-            ProtectedTransition(start, end) => {
-                format!(
-                    "{kind} through {initial} is forbidden because it is a foreign tag for {current}, which would hence change from {start} to {end}, but {current} is protected",
-                    current=self.faulty_tag,
-                    start=start,
-                    end=end,
-                    kind=self.access_kind,
-                    initial=self.tag_of_access,
+            ProtectedTransition(transition) => {
+                let conflicting_tag_name = "protected";
+                (
+                    transition.started(),
+                    format!("{kind} through {accessed} is forbidden."),
+                    Some(format!(
+                        "the accessed tag {accessed} is a foreign tag for the {conflicting_tag_name} tag {conflicting}."
+                    )),
+                    format!(
+                        "the access would cause the {conflicting_tag_name} tag {conflicting} to transition {transition}. This is {loss}, which is not allowed for protected tags.",
+                        loss = transition.summary(),
+                    ),
+                    conflicting_tag_name,
                 )
             }
             ProtectedDealloc => {
-                format!(
-                    "the allocation of {initial} also contains {current} which is strongly protected, cannot deallocate",
-                    initial=self.tag_of_access,
-                    current=self.faulty_tag,
+                let conflicting_tag_name = "strongly protected";
+                (
+                    started_as,
+                    format!("deallocation through {accessed} is forbidden."),
+                    Some(format!(
+                        "the allocation of the accessed tag {accessed} also contains the {conflicting_tag_name} tag {conflicting}."
+                    )),
+                    format!(
+                        "the {conflicting_tag_name} tag {conflicting} disallows deallocations."
+                    ),
+                    conflicting_tag_name,
                 )
             }
-        }).into()
+        };
+        let pre_transition = PermTransition::from(started_as, pre_error).unwrap();
+        let mut history = HistoryData::default();
+        if !accessed_is_conflicting {
+            history.extend(self.accessed_info.history.forget(), "accessed", false);
+        }
+        history.extend(
+            self.conflicting_info.history.extract_relevant(pre_transition, self.error_offset),
+            conflicting_tag_name,
+            true,
+        );
+        err_machine_stop!(TerminationInfo::TreeBorrowsUb { title, relation, problem, history })
     }
 }
 

src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs

@@ -14,7 +14,7 @@ use rustc_middle::{
 
 use crate::*;
 
-mod diagnostics;
+pub mod diagnostics;
 mod perms;
 mod tree;
 mod unimap;
@@ -23,10 +23,6 @@ pub use tree::Tree;
 
 pub type AllocState = Tree;
 
-pub fn err_tb_ub<'tcx>(msg: String) -> InterpError<'tcx> {
-    err_machine_stop!(TerminationInfo::TreeBorrowsUb { msg })
-}
-
 impl<'tcx> Tree {
     /// Create a new allocation, i.e. a new tree
     pub fn new_allocation(
@@ -37,7 +33,8 @@ impl<'tcx> Tree {
         machine: &MiriMachine<'_, 'tcx>,
     ) -> Self {
         let tag = state.base_ptr_tag(id, machine); // Fresh tag for the root
-        Tree::new(tag, size)
+        let span = machine.current_span();
+        Tree::new(tag, size, span)
     }
 
     /// Check that an access on the entire range is permitted, and update
@@ -64,7 +61,8 @@ impl<'tcx> Tree {
             ProvenanceExtra::Wildcard => return Ok(()),
         };
         let global = machine.borrow_tracker.as_ref().unwrap();
-        self.perform_access(access_kind, tag, range, global)
+        let span = machine.current_span();
+        self.perform_access(access_kind, tag, range, global, span)
     }
 
     /// Check that this pointer has permission to deallocate this range.
@@ -82,7 +80,8 @@ impl<'tcx> Tree {
             ProvenanceExtra::Wildcard => return Ok(()),
         };
         let global = machine.borrow_tracker.as_ref().unwrap();
-        self.dealloc(tag, range, global)
+        let span = machine.current_span();
+        self.dealloc(tag, range, global, span)
     }
 
     pub fn expose_tag(&mut self, _tag: BorTag) {
@@ -265,21 +264,23 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
                 .insert(new_tag, protect);
         }
 
+        let span = this.machine.current_span();
         let alloc_extra = this.get_alloc_extra(alloc_id)?;
         let range = alloc_range(base_offset, ptr_size);
         let mut tree_borrows = alloc_extra.borrow_tracker_tb().borrow_mut();
 
         if new_perm.perform_read_access {
             // Count this reborrow as a read access
             let global = &this.machine.borrow_tracker.as_ref().unwrap();
-            tree_borrows.perform_access(AccessKind::Read, orig_tag, range, global)?;
+            let span = this.machine.current_span();
+            tree_borrows.perform_access(AccessKind::Read, orig_tag, range, global, span)?;
             if let Some(data_race) = alloc_extra.data_race.as_ref() {
                 data_race.read(alloc_id, range, &this.machine)?;
             }
         }
 
         // Record the parent-child pair in the tree.
-        tree_borrows.new_child(orig_tag, new_tag, new_perm.initial_state, range)?;
+        tree_borrows.new_child(orig_tag, new_tag, new_perm.initial_state, range, span)?;
         Ok(Some((alloc_id, new_tag)))
     }