Add a lot more information to SB fatal errors

src/diagnostics.rs

@@ -17,6 +17,7 @@ pub enum TerminationInfo {
     UnsupportedInIsolation(String),
     ExperimentalUb {
         msg: String,
+        help: Option<String>,
         url: String,
     },
     Deadlock,
@@ -133,6 +134,8 @@ pub fn report_error<'tcx, 'mir>(
 ) -> Option<i64> {
     use InterpError::*;
 
+    let mut msg = vec![];
+
     let (title, helps) = match &e.kind() {
         MachineStop(info) => {
             let info = info.downcast_ref::<TerminationInfo>().expect("invalid MachineStop payload");
@@ -152,11 +155,13 @@ pub fn report_error<'tcx, 'mir>(
                         (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, .. } => {
+                    msg.extend(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)),
@@ -211,11 +216,11 @@ pub fn report_error<'tcx, 'mir>(
     let stacktrace = ecx.generate_stacktrace();
     let (stacktrace, was_pruned) = prune_stacktrace(ecx, stacktrace);
     e.print_backtrace();
-    let msg = e.to_string();
+    msg.insert(0, e.to_string());
     report_msg(
         *ecx.tcx,
         DiagLevel::Error,
-        &if let Some(title) = title { format!("{}: {}", title, msg) } else { msg.clone() },
+        &if let Some(title) = title { format!("{}: {}", title, msg[0]) } else { msg[0].clone() },
         msg,
         helps,
         &stacktrace,
@@ -256,11 +261,14 @@ pub fn report_error<'tcx, 'mir>(
 
 /// Report an error or note (depending on the `error` argument) with the given stacktrace.
 /// Also emits a full stacktrace of the interpreter stack.
+/// We want to present a multi-line span message for some errors. Diagnostics do not support this
+/// directly, so we pass the lines as a `Vec<String>` and display each line after the first with an
+/// additional `span_label` or `note` call.
 fn report_msg<'tcx>(
     tcx: TyCtxt<'tcx>,
     diag_level: DiagLevel,
     title: &str,
-    span_msg: String,
+    span_msg: Vec<String>,
     mut helps: Vec<(Option<SpanData>, String)>,
     stacktrace: &[FrameInfo<'tcx>],
 ) {
@@ -273,12 +281,17 @@ fn report_msg<'tcx>(
 
     // Show main message.
     if span != DUMMY_SP {
-        err.span_label(span, span_msg);
+        for line in span_msg {
+            err.span_label(span, line);
+        }
     } else {
         // Make sure we show the message even when it is a dummy span.
-        err.note(&span_msg);
+        for line in span_msg {
+            err.note(&line);
+        }
         err.note("(no span available)");
     }
+
     // Show help messages.
     if !helps.is_empty() {
         // Add visual separator before backtrace.
@@ -413,7 +426,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, vec![msg], 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 offset into the
+    /// allocation 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 {:?} for {:?} permission at {}[{:#x}]",
+            derived_from,
+            new.perm,
+            alloc_id,
+            error_offset.bytes(),
+        );
+        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 {} using {:?} at {}[{:#x}]",
+            access,
+            tag,
+            alloc_id,
+            error_offset.bytes(),
+        );
+        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 location"
+        } else {
+            ", but that tag does not exist in the borrow stack for this location"
+        }
+    }
 }
 // # 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 {