diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 1ccf3c5dba..fbcb2e0d0f 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -17,6 +17,7 @@ pub enum TerminationInfo { UnsupportedInIsolation(String), ExperimentalUb { msg: String, + help: Option, url: String, }, Deadlock, @@ -133,6 +134,8 @@ pub fn report_error<'tcx, 'mir>( ) -> Option { use InterpError::*; + let mut msg = vec![]; + let (title, helps) = match &e.kind() { MachineStop(info) => { let info = info.downcast_ref::().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` 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, mut helps: Vec<(Option, 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); } }); } diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 0e47a9e1c3..777b8e9331 100644 --- a/src/stacked_borrows.rs +++ b/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) -> 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, // 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,7 +395,7 @@ impl<'tcx> Stack { fn dealloc( &mut self, tag: SbTag, - dbg_ptr: Pointer, // just for debug printing amd error messages + dbg_ptr: Pointer, // just for debug printing and error messages global: &GlobalState, ) -> InterpResult<'tcx> { // Step 1: Find granting item. @@ -397,7 +403,7 @@ impl<'tcx> Stack { 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,11 +418,13 @@ 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, + (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. @@ -424,11 +432,9 @@ impl<'tcx> Stack { 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(()) } diff --git a/tests/compile-fail/box-cell-alias.rs b/tests/compile-fail/box-cell-alias.rs index 58fc3530d7..5b9614f79f 100644 --- a/tests/compile-fail/box-cell-alias.rs +++ b/tests/compile-fail/box-cell-alias.rs @@ -6,7 +6,7 @@ use std::cell::Cell; fn helper(val: Box>, ptr: *const Cell) -> 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() } diff --git a/tests/compile-fail/stacked_borrows/illegal_write3.rs b/tests/compile-fail/stacked_borrows/illegal_write3.rs index d2d8528d90..7851eeb026 100644 --- a/tests/compile-fail/stacked_borrows/illegal_write3.rs +++ b/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; } diff --git a/tests/compile-fail/stacked_borrows/raw_tracking.rs b/tests/compile-fail/stacked_borrows/raw_tracking.rs index 0d6d0198fb..a8e1d806cb 100644 --- a/tests/compile-fail/stacked_borrows/raw_tracking.rs +++ b/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; } } diff --git a/tests/compile-fail/stacked_borrows/shr_frozen_violation1.rs b/tests/compile-fail/stacked_borrows/shr_frozen_violation1.rs index 5031210c54..1ea96086d3 100644 --- a/tests/compile-fail/stacked_borrows/shr_frozen_violation1.rs +++ b/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 } diff --git a/tests/compile-fail/stacked_borrows/zst_slice.rs b/tests/compile-fail/stacked_borrows/zst_slice.rs index 14d5d77a2b..065bf77d04 100644 --- a/tests/compile-fail/stacked_borrows/zst_slice.rs +++ b/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 {