Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify event selection in TB diagnostics #2867

Merged
merged 1 commit into from
May 6, 2023
Merged

Simplify event selection in TB diagnostics #2867

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/borrow_tracker/stacked_borrows/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ impl<'tcx> Stack {
impl Stacks {
pub fn remove_unreachable_tags(&mut self, live_tags: &FxHashSet<BorTag>) {
if self.modified_since_last_gc {
for stack in self.stacks.iter_mut_all() {
for (_stack_range, stack) in self.stacks.iter_mut_all() {
if stack.len() > 64 {
stack.retain(live_tags);
}
Expand Down Expand Up @@ -511,8 +511,8 @@ impl<'tcx> Stacks {
) -> InterpResult<'tcx>,
) -> InterpResult<'tcx> {
self.modified_since_last_gc = true;
for (offset, stack) in self.stacks.iter_mut(range.start, range.size) {
let mut dcx = dcx_builder.build(&mut self.history, offset);
for (stack_range, stack) in self.stacks.iter_mut(range.start, range.size) {
let mut dcx = dcx_builder.build(&mut self.history, Size::from_bytes(stack_range.start));
f(stack, &mut dcx, &mut self.exposed_tags)?;
dcx_builder = dcx.unbuild();
}
Expand Down
110 changes: 48 additions & 62 deletions src/borrow_tracker/tree_borrows/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use std::ops::Range;

use rustc_data_structures::fx::FxHashMap;
use rustc_span::{Span, SpanData};
use rustc_target::abi::Size;

use crate::borrow_tracker::tree_borrows::{
perms::{PermTransition, Permission},
Expand All @@ -14,18 +13,30 @@ use crate::borrow_tracker::{AccessKind, ProtectorKind};
use crate::*;

/// Complete data for an event:
/// - `kind` is what happened to the permissions
/// - `access_kind` and `access_range` describe the access that caused the event
/// - `offset` allows filtering only the relevant events for a given memory location
/// (see how we perform the filtering in `History::extract_relevant`.
/// - `span` is the line of code in question
#[derive(Clone, Debug)]
pub struct Event {
/// Transformation of permissions that occured because of this event
pub transition: PermTransition,
/// Kind of the access that triggered this event
pub access_kind: AccessKind,
/// 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,
pub offset: Size,
/// 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
/// should not be recorded as events, so the union of all `transition_range` is not
/// necessarily the entire `access_range`.
///
/// No data from any `transition_range` should ever be user-visible, because
/// both the start and end of `transition_range` are entirely dependent on the
/// internal representation of `RangeMap` which is supposed to be opaque.
/// What will be shown in the error message is the first byte `error_offset` of
/// the `TbError`, which should satisfy
/// `event.transition_range.contains(error.error_offset)`.
pub transition_range: Range<u64>,
/// Line of code that triggered this event
pub span: Span,
}

Expand All @@ -35,9 +46,9 @@ pub struct Event {
/// Available filtering methods include `History::forget` and `History::extract_relevant`.
#[derive(Clone, Debug)]
pub struct History {
pub tag: BorTag,
pub created: (Span, Permission),
pub events: Vec<Event>,
tag: BorTag,
created: (Span, Permission),
events: Vec<Event>,
}

/// History formatted for use by `src/diagnostics.rs`.
Expand All @@ -60,12 +71,7 @@ impl HistoryData {
// Format events from `new_history` into those recorded by `self`.
//
// NOTE: also converts `Span` to `SpanData`.
pub fn extend(
&mut self,
new_history: History,
tag_name: &'static str,
show_initial_state: bool,
) {
fn extend(&mut self, new_history: History, tag_name: &'static str, show_initial_state: bool) {
let History { tag, created, events } = new_history;
let this = format!("the {tag_name} tag {tag:?}");
let msg_initial_state = format!(", in the initial state {}", created.1);
Expand All @@ -75,9 +81,16 @@ impl HistoryData {
);

self.events.push((Some(created.0.data()), msg_creation));
for &Event { transition, access_kind, is_foreign, access_range, span, offset: _ } in &events
for &Event {
transition,
access_kind,
is_foreign,
access_range,
span,
transition_range: _,
} in &events
{
// NOTE: `offset` is explicitly absent from the error message, it has no significance
// NOTE: `transition_range` is explicitly absent from the error message, it has no significance
// to the user. The meaningful one is `access_range`.
self.events.push((Some(span.data()), format!("{this} then transitioned {transition} due to a {rel} {access_kind} at offsets {access_range:?}", rel = if is_foreign { "foreign" } else { "child" })));
self.events.push((None, format!("this corresponds to {}", transition.summary())));
Expand Down Expand Up @@ -197,53 +210,28 @@ impl History {
History { events: Vec::new(), created: self.created, tag: self.tag }
}

/// Reconstruct the history relevant to `error_offset` knowing that
/// its permission followed `complete_transition`.
///
/// Here's how we do this:
/// - we know `full := complete_transition` the transition of the permission from
/// its initialization to the state just before the error was caused,
/// we want to find a chain of events that produces `full`
/// - we decompose `full` into `pre o post` where
/// `pre` is the best applicable transition from recorded events
/// - we select the event that caused `pre` and iterate
/// to find the chain of events that produces `full := post`
///
/// To find the "best applicable transition" for full:
/// - eliminate events that cannot be applied because their offset is too big
/// - eliminate events that cannot be applied because their starting point is wrong
/// - select the one that happened closest to the range of interest
fn extract_relevant(&self, complete_transition: PermTransition, error_offset: Size) -> Self {
let mut selected_events: Vec<Event> = Vec::new();
let mut full = complete_transition;
while !full.is_noop() {
let (pre, post) = self
/// Reconstruct the history relevant to `error_offset` by filtering
/// only events whose range contains the offset we are interested in.
fn extract_relevant(&self, error_offset: u64) -> Self {
History {
events: self
.events
.iter()
.filter(|e| e.offset <= error_offset)
.filter_map(|pre_canditate| {
full.apply_start(pre_canditate.transition)
.map(|post_canditate| (pre_canditate, post_canditate))
})
.max_by_key(|(pre_canditate, _post_candidate)| pre_canditate.offset)
.unwrap();
// If this occurs we will loop infinitely !
// Make sure to only put non-noop transitions in `History`.
assert!(!pre.transition.is_noop());
full = post;
selected_events.push(pre.clone());
.filter(|e| e.transition_range.contains(&error_offset))
.cloned()
.collect::<Vec<_>>(),
created: self.created,
tag: self.tag,
}

History { events: selected_events, created: self.created, tag: self.tag }
}
}

/// Failures that can occur during the execution of Tree Borrows procedures.
pub(super) struct TbError<'node> {
/// What failure occurred.
pub error_kind: TransitionError,
/// The byte at which the conflict occured.
pub error_offset: Size,
/// The offset (into the allocation) at which the conflict occurred.
pub error_offset: u64,
/// The tag on which the error was triggered.
/// On protector violations, this is the tag that was protected.
/// On accesses rejected due to insufficient permissions, this is the
Expand All @@ -261,12 +249,11 @@ impl TbError<'_> {
/// Produce a UB error.
pub fn build<'tcx>(self) -> InterpError<'tcx> {
use TransitionError::*;
let started_as = self.conflicting_info.history.created.1;
let kind = self.access_kind;
let accessed = self.accessed_info;
let conflicting = self.conflicting_info;
let accessed_is_conflicting = accessed.tag == conflicting.tag;
let (pre_error, title, details, conflicting_tag_name) = match self.error_kind {
let (title, details, conflicting_tag_name) = match self.error_kind {
ChildAccessForbidden(perm) => {
let conflicting_tag_name =
if accessed_is_conflicting { "accessed" } else { "conflicting" };
Expand All @@ -280,7 +267,7 @@ impl TbError<'_> {
details.push(format!(
"the {conflicting_tag_name} tag {conflicting} has state {perm} which forbids child {kind}es"
));
(perm, title, details, conflicting_tag_name)
(title, details, conflicting_tag_name)
}
ProtectedTransition(transition) => {
let conflicting_tag_name = "protected";
Expand All @@ -297,7 +284,7 @@ impl TbError<'_> {
loss = transition.summary(),
),
];
(transition.started(), title, details, conflicting_tag_name)
(title, details, conflicting_tag_name)
}
ProtectedDealloc => {
let conflicting_tag_name = "strongly protected";
Expand All @@ -308,16 +295,15 @@ impl TbError<'_> {
),
format!("the {conflicting_tag_name} tag {conflicting} disallows deallocations"),
];
(started_as, title, details, conflicting_tag_name)
(title, details, conflicting_tag_name)
}
};
let pre_transition = PermTransition::from(started_as, pre_error).unwrap();
let mut history = HistoryData::default();
if !accessed_is_conflicting {
history.extend(self.accessed_info.history.forget(), "accessed", false);
}
history.extend(
self.conflicting_info.history.extract_relevant(pre_transition, self.error_offset),
self.conflicting_info.history.extract_relevant(self.error_offset),
conflicting_tag_name,
true,
);
Expand Down
25 changes: 13 additions & 12 deletions src/borrow_tracker/tree_borrows/tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ impl<'tcx> Tree {
parent_tag: BorTag,
new_tag: BorTag,
default_initial_perm: Permission,
range: AllocRange,
reborrow_range: AllocRange,
span: Span,
) -> InterpResult<'tcx> {
assert!(!self.tag_mapping.contains_key(&new_tag));
Expand All @@ -332,7 +332,8 @@ impl<'tcx> Tree {
self.nodes.get_mut(parent_idx).unwrap().children.push(idx);
// Initialize perms
let perm = LocationState::new(default_initial_perm).with_access();
for (_range, perms) in self.rperms.iter_mut(range.start, range.size) {
for (_perms_range, perms) in self.rperms.iter_mut(reborrow_range.start, reborrow_range.size)
{
perms.insert(idx, perm);
}
Ok(())
Expand All @@ -344,12 +345,12 @@ impl<'tcx> Tree {
pub fn dealloc(
&mut self,
tag: BorTag,
range: AllocRange,
access_range: AllocRange,
global: &GlobalState,
span: Span, // diagnostics
) -> InterpResult<'tcx> {
self.perform_access(AccessKind::Write, tag, range, global, span)?;
for (offset, perms) in self.rperms.iter_mut(range.start, range.size) {
self.perform_access(AccessKind::Write, tag, access_range, global, span)?;
for (perms_range, perms) in self.rperms.iter_mut(access_range.start, access_range.size) {
TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms }
.traverse_parents_this_children_others(
tag,
Expand All @@ -368,7 +369,7 @@ impl<'tcx> Tree {
TbError {
conflicting_info,
access_kind: AccessKind::Write,
error_offset: offset,
error_offset: perms_range.start,
error_kind,
accessed_info,
}
Expand All @@ -388,11 +389,11 @@ impl<'tcx> Tree {
&mut self,
access_kind: AccessKind,
tag: BorTag,
range: AllocRange,
access_range: AllocRange,
global: &GlobalState,
span: Span, // diagnostics
) -> InterpResult<'tcx> {
for (offset, perms) in self.rperms.iter_mut(range.start, range.size) {
for (perms_range, perms) in self.rperms.iter_mut(access_range.start, access_range.size) {
TreeVisitor { nodes: &mut self.nodes, tag_mapping: &self.tag_mapping, perms }
.traverse_parents_this_children_others(
tag,
Expand Down Expand Up @@ -456,9 +457,9 @@ impl<'tcx> Tree {
node.debug_info.history.push(diagnostics::Event {
transition,
access_kind,
access_range: range,
is_foreign: rel_pos.is_foreign(),
offset,
access_range,
transition_range: perms_range.clone(),
span,
});
old_state.permission =
Expand All @@ -472,7 +473,7 @@ impl<'tcx> Tree {
TbError {
conflicting_info,
access_kind,
error_offset: offset,
error_offset: perms_range.start,
error_kind,
accessed_info,
}
Expand Down Expand Up @@ -530,7 +531,7 @@ impl Tree {
// the tag from the mapping.
let tag = node.tag;
self.nodes.remove(idx);
for perms in self.rperms.iter_mut_all() {
for (_perms_range, perms) in self.rperms.iter_mut_all() {
perms.remove(idx);
}
self.tag_mapping.remove(&tag);
Expand Down
23 changes: 14 additions & 9 deletions src/concurrency/data_race.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,15 +868,17 @@ impl VClockAlloc {
pub fn read<'tcx>(
&self,
alloc_id: AllocId,
range: AllocRange,
access_range: AllocRange,
machine: &MiriMachine<'_, '_>,
) -> InterpResult<'tcx> {
let current_span = machine.current_span();
let global = machine.data_race.as_ref().unwrap();
if global.race_detecting() {
let (index, mut thread_clocks) = global.current_thread_state_mut(&machine.threads);
let mut alloc_ranges = self.alloc_ranges.borrow_mut();
for (offset, mem_clocks) in alloc_ranges.iter_mut(range.start, range.size) {
for (mem_clocks_range, mem_clocks) in
alloc_ranges.iter_mut(access_range.start, access_range.size)
{
if let Err(DataRace) =
mem_clocks.read_race_detect(&mut thread_clocks, index, current_span)
{
Expand All @@ -888,7 +890,7 @@ impl VClockAlloc {
mem_clocks,
"Read",
false,
Pointer::new(alloc_id, offset),
Pointer::new(alloc_id, Size::from_bytes(mem_clocks_range.start)),
);
}
}
Expand All @@ -902,16 +904,16 @@ impl VClockAlloc {
fn unique_access<'tcx>(
&mut self,
alloc_id: AllocId,
range: AllocRange,
access_range: AllocRange,
write_type: WriteType,
machine: &mut MiriMachine<'_, '_>,
) -> InterpResult<'tcx> {
let current_span = machine.current_span();
let global = machine.data_race.as_mut().unwrap();
if global.race_detecting() {
let (index, mut thread_clocks) = global.current_thread_state_mut(&machine.threads);
for (offset, mem_clocks) in
self.alloc_ranges.get_mut().iter_mut(range.start, range.size)
for (mem_clocks_range, mem_clocks) in
self.alloc_ranges.get_mut().iter_mut(access_range.start, access_range.size)
{
if let Err(DataRace) = mem_clocks.write_race_detect(
&mut thread_clocks,
Expand All @@ -927,7 +929,7 @@ impl VClockAlloc {
mem_clocks,
write_type.get_descriptor(),
false,
Pointer::new(alloc_id, offset),
Pointer::new(alloc_id, Size::from_bytes(mem_clocks_range.start)),
);
}
}
Expand Down Expand Up @@ -1150,7 +1152,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
&this.machine.threads,
current_span,
|index, mut thread_clocks| {
for (offset, mem_clocks) in
for (mem_clocks_range, mem_clocks) in
alloc_meta.alloc_ranges.borrow_mut().iter_mut(base_offset, size)
{
if let Err(DataRace) = op(mem_clocks, &mut thread_clocks, index, atomic)
Expand All @@ -1162,7 +1164,10 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
mem_clocks,
description,
true,
Pointer::new(alloc_id, offset),
Pointer::new(
alloc_id,
Size::from_bytes(mem_clocks_range.start),
),
)
.map(|_| true);
}
Expand Down
Loading