Print more in SB error diagnostics

This tries to clarify exactly why an access is not valid by printing
what memory range the access was over, which in combination with
tag-tracking may help a user figure out the source of the problem.

Author: saethlin

src/diagnostics.rs

@@ -17,6 +17,7 @@ pub enum TerminationInfo {
     UnsupportedInIsolation(String),
     ExperimentalUb {
         msg: String,
+        help: Option<String>,
         url: String,
     },
     Deadlock,
@@ -133,7 +134,7 @@ pub fn report_error<'tcx, 'mir>(
 ) -> Option<i64> {
     use InterpError::*;
 
-    let (title, helps) = match &e.kind() {
+    let (title, labels, helps) = match &e.kind() {
         MachineStop(info) => {
             let info = info.downcast_ref::<TerminationInfo>().expect("invalid MachineStop payload");
             use TerminationInfo::*;
@@ -145,18 +146,23 @@ pub fn report_error<'tcx, 'mir>(
                 Deadlock => Some("deadlock"),
                 MultipleSymbolDefinitions { .. } | SymbolShimClashing { .. } => None,
             };
+            let mut labels = vec![];
             #[rustfmt::skip]
             let helps = match info {
                 UnsupportedInIsolation(_) =>
                     vec![
                         (None, format!("pass the flag `-Zmiri-disable-isolation` to disable isolation;")),
                         (None, format!("or pass `-Zmiri-isolation-error=warn to configure Miri to return an error code from isolated operations (if supported for that operation) and continue with a warning")),
                     ],
-                ExperimentalUb { url, .. } =>
+                ExperimentalUb { url, help, .. } => {
+                    if let Some(help) = help {
+                        labels.push(help.clone());
+                    }
                     vec![
                         (None, format!("this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental")),
-                        (None, format!("see {} for further information", url)),
-                    ],
+                        (None, format!("see {} for further information", url))
+                    ]
+                }
                 MultipleSymbolDefinitions { first, first_crate, second, second_crate, .. } =>
                     vec![
                         (Some(*first), format!("it's first defined here, in crate `{}`", first_crate)),
@@ -166,7 +172,7 @@ pub fn report_error<'tcx, 'mir>(
                     vec![(Some(*span), format!("the `{}` symbol is defined here", link_name))],
                 _ => vec![],
             };
-            (title, helps)
+            (title, labels, helps)
         }
         _ => {
             #[rustfmt::skip]
@@ -204,7 +210,7 @@ pub fn report_error<'tcx, 'mir>(
                     ],
                 _ => vec![],
             };
-            (Some(title), helps)
+            (Some(title), vec![], helps)
         }
     };
 
@@ -217,6 +223,7 @@ pub fn report_error<'tcx, 'mir>(
         DiagLevel::Error,
         &if let Some(title) = title { format!("{}: {}", title, msg) } else { msg.clone() },
         msg,
+        labels,
         helps,
         &stacktrace,
     );
@@ -261,6 +268,7 @@ fn report_msg<'tcx>(
     diag_level: DiagLevel,
     title: &str,
     span_msg: String,
+    labels: Vec<String>,
     mut helps: Vec<(Option<SpanData>, String)>,
     stacktrace: &[FrameInfo<'tcx>],
 ) {
@@ -274,11 +282,18 @@ fn report_msg<'tcx>(
     // Show main message.
     if span != DUMMY_SP {
         err.span_label(span, span_msg);
+        for label in labels {
+            err.span_label(span, label);
+        }
     } else {
         // Make sure we show the message even when it is a dummy span.
         err.note(&span_msg);
         err.note("(no span available)");
+        for label in labels {
+            err.note(&label);
+        }
     }
+
     // Show help messages.
     if !helps.is_empty() {
         // Add visual separator before backtrace.
@@ -413,7 +428,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                     _ => ("tracking was triggered", DiagLevel::Note),
                 };
 
-                report_msg(*this.tcx, diag_level, title, msg, vec![], &stacktrace);
+                report_msg(*this.tcx, diag_level, title, msg, vec![], vec![], &stacktrace);
             }
         });
     }

src/stacked_borrows.rs

@@ -220,9 +220,10 @@ impl GlobalState {
 }
 
 /// Error reporting
-fn err_sb_ub(msg: String) -> InterpError<'static> {
+fn err_sb_ub(msg: String, help: Option<String>) -> InterpError<'static> {
     err_machine_stop!(TerminationInfo::ExperimentalUb {
         msg,
+        help,
         url: format!(
             "https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md"
         ),
@@ -320,12 +321,18 @@ impl<'tcx> Stack {
         if let Some(call) = item.protector {
             if global.is_active(call) {
                 if let Some((tag, _)) = provoking_access {
-                    Err(err_sb_ub(format!(
-                        "not granting access to tag {:?} because incompatible item is protected: {:?}",
-                        tag, item
-                    )))?
+                    Err(err_sb_ub(
+                        format!(
+                            "not granting access to tag {:?} because incompatible item is protected: {:?}",
+                            tag, item
+                        ),
+                        None,
+                    ))?
                 } else {
-                    Err(err_sb_ub(format!("deallocating while item is protected: {:?}", item)))?
+                    Err(err_sb_ub(
+                        format!("deallocating while item is protected: {:?}", item),
+                        None,
+                    ))?
                 }
             }
         }
@@ -334,22 +341,21 @@ impl<'tcx> Stack {
 
     /// Test if a memory `access` using pointer tagged `tag` is granted.
     /// If yes, return the index of the item that granted it.
+    /// `range` refers the entire operation, and `offset` refers to the specific location in
+    /// `range` that we are currently checking.
     fn access(
         &mut self,
         access: AccessKind,
         tag: SbTag,
-        dbg_ptr: Pointer<AllocId>, // just for debug printing amd error messages
+        (alloc_id, range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages
         global: &GlobalState,
     ) -> InterpResult<'tcx> {
         // Two main steps: Find granting item, remove incompatible items above.
 
         // Step 1: Find granting item.
-        let granting_idx = self.find_granting(access, tag).ok_or_else(|| {
-            err_sb_ub(format!(
-                "no item granting {} to tag {:?} at {:?} found in borrow stack.",
-                access, tag, dbg_ptr,
-            ))
-        })?;
+        let granting_idx = self
+            .find_granting(access, tag)
+            .ok_or_else(|| self.access_error(access, tag, alloc_id, range, offset))?;
 
         // Step 2: Remove incompatible items above them.  Make sure we do not remove protected
         // items.  Behavior differs for reads and writes.
@@ -389,15 +395,15 @@ impl<'tcx> Stack {
     fn dealloc(
         &mut self,
         tag: SbTag,
-        dbg_ptr: Pointer<AllocId>, // just for debug printing amd error messages
+        dbg_ptr: Pointer<AllocId>, // just for debug printing and error messages
         global: &GlobalState,
     ) -> InterpResult<'tcx> {
         // Step 1: Find granting item.
         self.find_granting(AccessKind::Write, tag).ok_or_else(|| {
             err_sb_ub(format!(
                 "no item granting write access for deallocation to tag {:?} at {:?} found in borrow stack",
                 tag, dbg_ptr,
-            ))
+            ), None)
         })?;
 
         // Step 2: Remove all items.  Also checks for protectors.
@@ -412,23 +418,23 @@ impl<'tcx> Stack {
     /// `weak` controls whether this operation is weak or strong: weak granting does not act as
     /// an access, and they add the new item directly on top of the one it is derived
     /// from instead of all the way at the top of the stack.
+    /// `range` refers the entire operation, and `offset` refers to the specific location in
+    /// `range` that we are currently checking.
     fn grant(
         &mut self,
         derived_from: SbTag,
         new: Item,
-        dbg_ptr: Pointer<AllocId>,
+        (alloc_id, alloc_range, offset): (AllocId, AllocRange, Size), // just for debug printing and error messages
         global: &GlobalState,
     ) -> InterpResult<'tcx> {
         // Figure out which access `perm` corresponds to.
         let access =
             if new.perm.grants(AccessKind::Write) { AccessKind::Write } else { AccessKind::Read };
         // Now we figure out which item grants our parent (`derived_from`) this kind of access.
         // We use that to determine where to put the new item.
-        let granting_idx = self.find_granting(access, derived_from)
-            .ok_or_else(|| err_sb_ub(format!(
-                "trying to reborrow for {:?} at {:?}, but parent tag {:?} does not have an appropriate item in the borrow stack",
-                new.perm, dbg_ptr, derived_from,
-            )))?;
+        let granting_idx = self
+            .find_granting(access, derived_from)
+            .ok_or_else(|| self.grant_error(derived_from, new, alloc_id, alloc_range, offset))?;
 
         // Compute where to put the new item.
         // Either way, we ensure that we insert the new item in a way such that between
@@ -447,7 +453,7 @@ impl<'tcx> Stack {
             // A "safe" reborrow for a pointer that actually expects some aliasing guarantees.
             // Here, creating a reference actually counts as an access.
             // This ensures F2b for `Unique`, by removing offending `SharedReadOnly`.
-            self.access(access, derived_from, dbg_ptr, global)?;
+            self.access(access, derived_from, (alloc_id, alloc_range, offset), global)?;
 
             // We insert "as far up as possible": We know only compatible items are remaining
             // on top of `derived_from`, and we want the new item at the top so that we
@@ -467,6 +473,72 @@ impl<'tcx> Stack {
 
         Ok(())
     }
+
+    /// Report a descriptive error when `new` could not be granted from `derived_from`.
+    fn grant_error(
+        &self,
+        derived_from: SbTag,
+        new: Item,
+        alloc_id: AllocId,
+        alloc_range: AllocRange,
+        error_offset: Size,
+    ) -> InterpError<'static> {
+        let action = format!(
+            "trying to reborrow {:?}[{:#x}] with {:?} permission via tag {:?}",
+            alloc_id,
+            error_offset.bytes(),
+            new.perm,
+            derived_from,
+        );
+        err_sb_ub(
+            format!("{}{}", action, self.error_cause(derived_from)),
+            Some(Self::operation_summary("a reborrow", alloc_id, alloc_range)),
+        )
+    }
+
+    /// Report a descriptive error when `access` is not permitted based on `tag`.
+    fn access_error(
+        &self,
+        access: AccessKind,
+        tag: SbTag,
+        alloc_id: AllocId,
+        alloc_range: AllocRange,
+        error_offset: Size,
+    ) -> InterpError<'static> {
+        let action = format!(
+            "attempting a {} at {}[{:#x}] via tag {:?}",
+            access,
+            alloc_id,
+            error_offset.bytes(),
+            tag,
+        );
+        err_sb_ub(
+            format!("{}{}", action, self.error_cause(tag)),
+            Some(Self::operation_summary("an access", alloc_id, alloc_range)),
+        )
+    }
+
+    fn operation_summary(
+        operation: &'static str,
+        alloc_id: AllocId,
+        alloc_range: AllocRange,
+    ) -> String {
+        format!(
+            "this error occurs as part of {} at {:?}[{:#x}..{:#x}]",
+            operation,
+            alloc_id,
+            alloc_range.start.bytes(),
+            alloc_range.end().bytes()
+        )
+    }
+
+    fn error_cause(&self, tag: SbTag) -> &'static str {
+        if self.borrows.iter().any(|item| item.tag == tag && item.perm != Permission::Disabled) {
+            ", but that tag only grants SharedReadOnly permission for this memory"
+        } else {
+            ", but that tag does not exist in the borrow stack for this memory"
+        }
+    }
 }
 // # Stacked Borrows Core End
 
@@ -566,7 +638,7 @@ impl Stacks {
         );
         let global = &*extra.borrow();
         self.for_each(range, move |offset, stack| {
-            stack.access(AccessKind::Read, tag, Pointer::new(alloc_id, offset), global)
+            stack.access(AccessKind::Read, tag, (alloc_id, range, offset), global)
         })
     }
 
@@ -586,7 +658,7 @@ impl Stacks {
         );
         let global = extra.get_mut();
         self.for_each_mut(range, move |offset, stack| {
-            stack.access(AccessKind::Write, tag, Pointer::new(alloc_id, offset), global)
+            stack.access(AccessKind::Write, tag, (alloc_id, range, offset), global)
         })
     }
 
@@ -693,7 +765,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                     };
                     let item = Item { perm, tag: new_tag, protector };
                     stacked_borrows.for_each(range, |offset, stack| {
-                        stack.grant(orig_tag, item, Pointer::new(alloc_id, offset), &*global)
+                        stack.grant(orig_tag, item, (alloc_id, range, offset), &*global)
                     })
                 })?;
                 return Ok(());
@@ -707,8 +779,9 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             alloc_extra.stacked_borrows.as_mut().expect("we should have Stacked Borrows data");
         let global = memory_extra.stacked_borrows.as_mut().unwrap().get_mut();
         let item = Item { perm, tag: new_tag, protector };
+        let range = alloc_range(base_offset, size);
         stacked_borrows.for_each_mut(alloc_range(base_offset, size), |offset, stack| {
-            stack.grant(orig_tag, item, Pointer::new(alloc_id, offset), global)
+            stack.grant(orig_tag, item, (alloc_id, range, offset), global)
         })?;
         Ok(())
     }

tests/compile-fail/box-cell-alias.rs

@@ -6,7 +6,7 @@ use std::cell::Cell;
 
 fn helper(val: Box<Cell<u8>>, ptr: *const Cell<u8>) -> u8 {
     val.set(10);
-    unsafe { (*ptr).set(20); } //~ ERROR does not have an appropriate item in the borrow stack
+    unsafe { (*ptr).set(20); } //~ ERROR does not exist in the borrow stack
     val.get()
 }
 

tests/compile-fail/stacked_borrows/illegal_write3.rs

@@ -3,6 +3,6 @@ fn main() {
     // Make sure raw ptr with raw tag cannot mutate frozen location without breaking the shared ref.
     let r#ref = ⌖ // freeze
     let ptr = r#ref as *const _ as *mut _; // raw ptr, with raw tag
-    unsafe { *ptr = 42; } //~ ERROR borrow stack
+    unsafe { *ptr = 42; } //~ ERROR only grants SharedReadOnly permission
     let _val = *r#ref;
 }

tests/compile-fail/stacked_borrows/raw_tracking.rs

@@ -7,6 +7,6 @@ fn main() {
     let raw2 = &mut l as *mut _; // invalidates raw1
     // Without raw pointer tracking, Stacked Borrows cannot distinguish raw1 and raw2, and thus
     // fails to realize that raw1 should not be used any more.
-    unsafe { *raw1 = 13; } //~ ERROR no item granting write access to tag
+    unsafe { *raw1 = 13; } //~ ERROR does not exist in the borrow stack
     unsafe { *raw2 = 13; }
 }

tests/compile-fail/stacked_borrows/shr_frozen_violation1.rs

@@ -9,5 +9,5 @@ fn main() {
 }
 
 fn unknown_code(x: &i32) {
-    unsafe { *(x as *const i32 as *mut i32) = 7; } //~ ERROR borrow stack
+    unsafe { *(x as *const i32 as *mut i32) = 7; } //~ ERROR only grants SharedReadOnly permission
 }

tests/compile-fail/stacked_borrows/zst_slice.rs

@@ -1,5 +1,5 @@
 // compile-flags: -Zmiri-tag-raw-pointers
-// error-pattern: does not have an appropriate item in the borrow stack
+// error-pattern: does not exist in the borrow stack
 
 fn main() {
     unsafe {