Auto merge of #1971 - saethlin:sb-details, r=RalfJung

Add a lot more information to SB fatal errors

In fatal errors, this clarifies the difference between a tag not being present in the borrow stack at all, and the tag being present but granting SRO. It also introduces a little notation for memory ranges so we can mention to the user that the span may point to code that operates on multiple memory locations, but we are reporting an error at a particular offset.

This also gets rid of the unqualified phrase "the borrow stack" in errors, and clarifies that it is the borrow stack _for some location_.

The crate `pdqselect` v0.1.1:
Before:
```
2103 |     unsafe { copy_nonoverlapping(src, dst, count) }
     |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no item granting read access to tag <2357> at alloc1029 found in borrow stack.
```
After:
```
2103 |     unsafe { copy_nonoverlapping(src, dst, count) }
     |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |              |
     |              attempting a read access using <2357> at alloc1029[0x0], but that tag does not exist in the borrow stack for this location
     |              this error occurs as part of an access at alloc1029[0x0..0x4]
```

And the crate `half` v1.8.2
Before:
```
131 |     unsafe { &mut *ptr::slice_from_raw_parts_mut(data, len) }
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to reborrow for Unique at alloc1051, but parent tag <2091> does not have an appropriate item in the borrow stack
```
After:
```
131 |     unsafe { &mut *ptr::slice_from_raw_parts_mut(data, len) }
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |              |
    |              trying to reborrow <2091> for Unique permission at alloc1051[0x0], but that tag only grants SharedReadOnly permission for this location
    |              this error occurs as part of a reborrow at alloc1051[0x0..0x6]
```

Author: bors

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 {