From d0b60f035fd7362850d1d4de9850d4e98b4a2aac Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Sat, 5 Feb 2022 19:44:56 -0500 Subject: [PATCH] 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. --- src/diagnostics.rs | 29 +++- src/stacked_borrows.rs | 125 ++++++++++++++---- tests/compile-fail/box-cell-alias.rs | 2 +- .../stacked_borrows/illegal_write3.rs | 2 +- .../stacked_borrows/raw_tracking.rs | 2 +- .../stacked_borrows/shr_frozen_violation1.rs | 2 +- .../compile-fail/stacked_borrows/zst_slice.rs | 2 +- 7 files changed, 126 insertions(+), 38 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 3bf8c7bc17..6163d23d9f 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,7 +134,7 @@ pub fn report_error<'tcx, 'mir>( ) -> Option { use InterpError::*; - let (title, helps) = match &e.kind() { + let (title, labels, helps) = match &e.kind() { MachineStop(info) => { let info = info.downcast_ref::().expect("invalid MachineStop payload"); use TerminationInfo::*; @@ -145,6 +146,7 @@ pub fn report_error<'tcx, 'mir>( Deadlock => Some("deadlock"), MultipleSymbolDefinitions { .. } | SymbolShimClashing { .. } => None, }; + let mut labels = vec![]; #[rustfmt::skip] let helps = match info { UnsupportedInIsolation(_) => @@ -152,11 +154,15 @@ 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, .. } => { + 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, mut helps: Vec<(Option, 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); } }); } diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 0e47a9e1c3..299e19c685 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 location in + /// `range` 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 {:?}[{:#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(()) } 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 {