Skip to content

Commit

Permalink
Implement implicit read on function exit
Browse files Browse the repository at this point in the history
Protected tags receive a read access on initialized locations.
This makes tests/fail/tree_borrows/spurious_read.rs fail as expected,
but it also introduces an issue in tests/pass/vec.rs
Because the new error in tests/pass/vec.rs is cryptic and to ensure
that we have a proper test for this issue, a new function is added
to tests/pass/tree_borrows/tree-borrows.rs
  • Loading branch information
Vanille-N committed Sep 20, 2023
1 parent 64d5fe5 commit 3492e7e
Show file tree
Hide file tree
Showing 9 changed files with 306 additions and 111 deletions.
20 changes: 18 additions & 2 deletions src/borrow_tracker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,13 @@ pub struct FrameState {
/// `stacked_borrows::GlobalState` upon function return, and if we attempt to pop a protected
/// tag, to identify which call is responsible for protecting the tag.
/// See `Stack::item_popped` for more explanation.
/// Stacked Borrows also needs to know which allocation these tags
/// belong to so that it can perform a read through them immediately before
/// the frame gets popped.
///
/// This will contain one tag per reference passed to the function, so
/// a size of 2 is enough for the vast majority of functions.
pub protected_tags: SmallVec<[BorTag; 2]>,
pub protected_tags: SmallVec<[(AllocId, BorTag); 2]>,
}

impl VisitTags for FrameState {
Expand Down Expand Up @@ -208,7 +211,7 @@ impl GlobalStateInner {
}

pub fn end_call(&mut self, frame: &machine::FrameExtra<'_>) {
for tag in &frame
for (_, tag) in &frame
.borrow_tracker
.as_ref()
.expect("we should have borrow tracking data")
Expand Down Expand Up @@ -450,6 +453,19 @@ impl AllocState {
AllocState::TreeBorrows(tb) => tb.borrow_mut().remove_unreachable_tags(tags),
}
}

/// Tree Borrows needs to be told when a tag stops being protected.
pub fn release_protector<'tcx>(
&self,
machine: &MiriMachine<'_, 'tcx>,
global: &GlobalState,
tag: BorTag,
) -> InterpResult<'tcx> {
match self {
AllocState::StackedBorrows(_sb) => Ok(()),
AllocState::TreeBorrows(tb) => tb.borrow_mut().release_protector(machine, global, tag),
}
}
}

impl VisitTags for AllocState {
Expand Down
3 changes: 2 additions & 1 deletion src/borrow_tracker/stacked_borrows/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
ProtectorKind::WeakProtector => "weakly protected",
ProtectorKind::StrongProtector => "strongly protected",
};
let item_tag = item.tag();
let call_id = self
.machine
.threads
Expand All @@ -437,7 +438,7 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
.map(|frame| {
frame.extra.borrow_tracker.as_ref().expect("we should have borrow tracking data")
})
.find(|frame| frame.protected_tags.contains(&item.tag()))
.find(|frame| frame.protected_tags.iter().any(|(_, tag)| tag == &item_tag))
.map(|frame| frame.call_id)
.unwrap(); // FIXME: Surely we should find something, but a panic seems wrong here?
match self.operation {
Expand Down
8 changes: 7 additions & 1 deletion src/borrow_tracker/stacked_borrows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,13 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'

if let Some(protect) = new_perm.protector() {
// See comment in `Stack::item_invalidated` for why we store the tag twice.
this.frame_mut().extra.borrow_tracker.as_mut().unwrap().protected_tags.push(new_tag);
this.frame_mut()
.extra
.borrow_tracker
.as_mut()
.unwrap()
.protected_tags
.push((alloc_id, new_tag));
this.machine
.borrow_tracker
.as_mut()
Expand Down
18 changes: 16 additions & 2 deletions src/borrow_tracker/tree_borrows/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub enum AccessCause {
Explicit(AccessKind),
Reborrow,
Dealloc,
FnExit,
}

impl fmt::Display for AccessCause {
Expand All @@ -27,6 +28,7 @@ impl fmt::Display for AccessCause {
Self::Explicit(kind) => write!(f, "{kind}"),
Self::Reborrow => write!(f, "reborrow"),
Self::Dealloc => write!(f, "deallocation"),
Self::FnExit => write!(f, "protector release"),
}
}
}
Expand All @@ -38,6 +40,7 @@ impl AccessCause {
Self::Explicit(kind) => format!("{rel} {kind}"),
Self::Reborrow => format!("reborrow (acting as a {rel} read access)"),
Self::Dealloc => format!("deallocation (acting as a {rel} write access)"),
Self::FnExit => format!("protector release (acting as a {rel} read access)"),
}
}
}
Expand All @@ -52,7 +55,8 @@ pub struct Event {
/// Relative position of the tag to the one used for the access.
pub is_foreign: bool,
/// User-visible range of the access.
pub access_range: AllocRange,
/// `None` means that this is an implicit access to the entire allocation.
pub access_range: Option<AllocRange>,
/// The transition recorded by this event only occured on a subrange of
/// `access_range`: a single access on `access_range` triggers several events,
/// each with their own mutually disjoint `transition_range`. No-op transitions
Expand Down Expand Up @@ -123,7 +127,17 @@ impl HistoryData {
// NOTE: `transition_range` is explicitly absent from the error message, it has no significance
// to the user. The meaningful one is `access_range`.
let access = access_cause.print_as_access(is_foreign);
self.events.push((Some(span.data()), format!("{this} later transitioned to {endpoint} due to a {access} at offsets {access_range:?}", endpoint = transition.endpoint())));
let access_range_text = match access_range {
Some(r) => format!("at offsets {r:?}"),
None => format!("over the entire allocation"),
};
self.events.push((
Some(span.data()),
format!(
"{this} later transitioned to {endpoint} due to a {access} {access_range_text}",
endpoint = transition.endpoint()
),
));
self.events
.push((None, format!("this transition corresponds to {}", transition.summary())));
}
Expand Down
39 changes: 35 additions & 4 deletions src/borrow_tracker/tree_borrows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ use log::trace;

use rustc_target::abi::{Abi, Align, Size};

use crate::borrow_tracker::{AccessKind, GlobalStateInner, ProtectorKind, RetagFields};
use crate::borrow_tracker::{
AccessKind, GlobalState, GlobalStateInner, ProtectorKind, RetagFields,
};
use rustc_middle::{
mir::{Mutability, RetagKind},
ty::{self, layout::HasParamEnv, Ty},
Expand Down Expand Up @@ -66,7 +68,7 @@ impl<'tcx> Tree {
self.perform_access(
access_kind,
tag,
range,
Some(range),
global,
span,
diagnostics::AccessCause::Explicit(access_kind),
Expand Down Expand Up @@ -95,6 +97,29 @@ impl<'tcx> Tree {
pub fn expose_tag(&mut self, _tag: BorTag) {
// TODO
}

/// A tag just lost its protector.
///
/// This emits a special kind of access that is only applied
/// to initialized locations, as a protection against other
/// tags not having been made aware of the existence of this
/// protector.
pub fn release_protector(
&mut self,
machine: &MiriMachine<'_, 'tcx>,
global: &GlobalState,
tag: BorTag,
) -> InterpResult<'tcx> {
let span = machine.current_span();
self.perform_access(
AccessKind::Read,
tag,
None, // no specified range because it occurs on the entire allocation
global,
span,
diagnostics::AccessCause::FnExit,
)
}
}

/// Policy for a new borrow.
Expand Down Expand Up @@ -242,7 +267,13 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
// We register the protection in two different places.
// This makes creating a protector slower, but checking whether a tag
// is protected faster.
this.frame_mut().extra.borrow_tracker.as_mut().unwrap().protected_tags.push(new_tag);
this.frame_mut()
.extra
.borrow_tracker
.as_mut()
.unwrap()
.protected_tags
.push((alloc_id, new_tag));
this.machine
.borrow_tracker
.as_mut()
Expand All @@ -269,7 +300,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
tree_borrows.perform_access(
AccessKind::Read,
orig_tag,
range,
Some(range),
this.machine.borrow_tracker.as_ref().unwrap(),
this.machine.current_span(),
diagnostics::AccessCause::Reborrow,
Expand Down
Loading

0 comments on commit 3492e7e

Please sign in to comment.