Skip to content

Commit

Permalink
Improve precision of error messages
Browse files Browse the repository at this point in the history
  • Loading branch information
@saethlin
saethlin committed Mar 11, 2022
1 parent 2ad75ff commit 8b66dc1
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 45 deletions.
19 changes: 14 additions & 5 deletions src/diagnostics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ pub enum TerminationInfo {
UnsupportedInIsolation(String),
ExperimentalUb {
msg: String,
help: Option<String>,
url: String,
},
Deadlock,
Expand Down Expand Up @@ -152,11 +153,19 @@ 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, .. } =>
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)),
],
ExperimentalUb { url, help, .. } => {
let mut helps = Vec::new();
if let Some(help) = help {
helps.push((Some(ecx.cur_span().data()), help.clone()));
}
helps.push(
(None, format!("this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental"))
);
helps.push(
(None, format!("see {} for further information", url))
);
helps
}
MultipleSymbolDefinitions { first, first_crate, second, second_crate, .. } =>
vec![
(Some(*first), format!("it's first defined here, in crate `{}`", first_crate)),
Expand Down
114 changes: 74 additions & 40 deletions src/stacked_borrows.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
),
Expand Down Expand Up @@ -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,
))?
}
}
}
Expand All @@ -338,15 +345,15 @@ impl<'tcx> Stack {
&mut self,
access: AccessKind,
tag: SbTag,
(alloc_id, range): (AllocId, AllocRange), // just for debug printing amd error messages
(alloc_id, range, offset): (AllocId, AllocRange, Size), // just for debug printing amd 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(|| self.access_error(access, tag, alloc_id, range))?;
.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.
Expand Down Expand Up @@ -394,7 +401,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.
Expand All @@ -413,7 +420,7 @@ impl<'tcx> Stack {
&mut self,
derived_from: SbTag,
new: Item,
(alloc_id, alloc_range): (AllocId, AllocRange),
(alloc_id, alloc_range, offset): (AllocId, AllocRange, Size),
global: &GlobalState,
) -> InterpResult<'tcx> {
// Figure out which access `perm` corresponds to.
Expand All @@ -423,7 +430,7 @@ impl<'tcx> Stack {
// We use that to determine where to put the new item.
let granting_idx = self
.find_granting(access, derived_from)
.ok_or_else(|| self.grant_error(derived_from, new, alloc_id, alloc_range))?;
.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
Expand All @@ -442,7 +449,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, (alloc_id, alloc_range), 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
Expand Down Expand Up @@ -470,54 +477,81 @@ impl<'tcx> Stack {
new: Item,
alloc_id: AllocId,
alloc_range: AllocRange,
error_offset: Size,
) -> InterpError<'static> {
let action = format!(
"trying to reborrow {:?}[{:#x}..{:#x}] with {:?} permission",
"trying to reborrow {:?}+{:#x} with {:?} permission",
alloc_id,
alloc_range.start.bytes(),
alloc_range.end().bytes(),
error_offset.bytes(),
new.perm,
);
let help = Some(format!(
"this is a reborrow of {:?}[{:#x}..{:#x}]",
alloc_id,
alloc_range.start.bytes(),
alloc_range.end().bytes()
));
if self
.borrows
.iter()
.any(|item| item.tag == derived_from && item.perm != Permission::Disabled)
{
err_sb_ub(format!(
"{}, but parent tag {:?} only grants SharedReadOnly permission",
action, derived_from
))
err_sb_ub(
format!(
"{}, but parent tag {:?} only grants SharedReadOnly permission for this memory",
action, derived_from
),
help,
)
} else {
err_sb_ub(format!(
"{}, but parent tag {:?} does not exist in the borrow stack for this memory range",
action, derived_from
))
err_sb_ub(
format!(
"{}, but parent tag {:?} does not exist in the borrow stack for this memory",
action, derived_from,
),
help,
)
}
}

/// Report a descriptive error when `new` could not be granted from `derived_from`.
/// 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 {} of {}[{:#x}..{:#x}] via tag {:?}",
"attempting a {} of {}+{:#x} via tag {:?}",
access,
alloc_id,
alloc_range.start.bytes(),
alloc_range.end().bytes(),
error_offset.bytes(),
tag,
);
let help = Some(format!(
"this is an access of {:?}[{:#x}..{:#x}]",
alloc_id,
alloc_range.start.bytes(),
alloc_range.end().bytes()
));
if self.borrows.iter().any(|item| item.tag == tag && item.perm != Permission::Disabled) {
err_sb_ub(format!("{}, but {:?} only grants SharedReadOnly permission", action, tag))
err_sb_ub(
format!(
"{}, but {:?} only grants SharedReadOnly permission for this memory",
action, tag
),
help,
)
} else {
err_sb_ub(format!(
"{}, but {:?} does not exist in the borrow stack for this memory range",
action, tag
))
err_sb_ub(
format!(
"{}, but {:?} does not exist in the borrow stack for this memory",
action, tag
),
help,
)
}
}
}
Expand Down Expand Up @@ -618,8 +652,8 @@ impl Stacks {
range.size.bytes()
);
let global = &*extra.borrow();
self.for_each(range, move |_offset, stack| {
stack.access(AccessKind::Read, tag, (alloc_id, range), global)
self.for_each(range, move |offset, stack| {
stack.access(AccessKind::Read, tag, (alloc_id, range, offset), global)
})
}

Expand All @@ -638,8 +672,8 @@ impl Stacks {
range.size.bytes()
);
let global = extra.get_mut();
self.for_each_mut(range, move |_offset, stack| {
stack.access(AccessKind::Write, tag, (alloc_id, range), global)
self.for_each_mut(range, move |offset, stack| {
stack.access(AccessKind::Write, tag, (alloc_id, range, offset), global)
})
}

Expand Down Expand Up @@ -745,8 +779,8 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
Permission::SharedReadWrite
};
let item = Item { perm, tag: new_tag, protector };
stacked_borrows.for_each(range, |_offset, stack| {
stack.grant(orig_tag, item, (alloc_id, range), &*global)
stacked_borrows.for_each(range, |offset, stack| {
stack.grant(orig_tag, item, (alloc_id, range, offset), &*global)
})
})?;
return Ok(());
Expand All @@ -761,8 +795,8 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
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, (alloc_id, range), global)
stacked_borrows.for_each_mut(alloc_range(base_offset, size), |offset, stack| {
stack.grant(orig_tag, item, (alloc_id, range, offset), global)
})?;
Ok(())
}
Expand Down

0 comments on commit 8b66dc1

Please sign in to comment.