Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ harness = false
default = ["stack-cache", "native-lib"]
genmc = ["dep:genmc-sys"]
stack-cache = []
stack-cache-consistency-check = ["stack-cache"]
expensive-consistency-checks = ["stack-cache"]
tracing = ["serde_json"]
native-lib = ["dep:libffi", "dep:libloading", "dep:capstone", "dep:ipc-channel", "dep:nix", "dep:serde"]

Expand Down
14 changes: 4 additions & 10 deletions src/bin/miri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,6 @@ fn main() {
Some(BorrowTrackerMethod::TreeBorrows(TreeBorrowsParams {
precise_interior_mut: true,
}));
miri_config.provenance_mode = ProvenanceMode::Strict;
} else if arg == "-Zmiri-tree-borrows-no-precise-interior-mut" {
match &mut miri_config.borrow_tracker {
Some(BorrowTrackerMethod::TreeBorrows(params)) => {
Expand Down Expand Up @@ -721,15 +720,10 @@ fn main() {
}
}
// Tree Borrows implies strict provenance, and is not compatible with native calls.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is now outdated.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

native functions are only incompatible with strict provenance? So this check can be removed completely?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

"Tree Borrows implies strict provenance" is not true any more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with the nativelib interface. So I'm unsure if there is any other reason (besides strict provenance) why tree borrows would be incompatible with nativelib.
I assume there isn't, and I will remove the comment along with the check directly after it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, we can now allow TB + native_lib. (I didn't even realize the check can be removed entirely, I just noted that the comment states an incorrect implication.)

if matches!(miri_config.borrow_tracker, Some(BorrowTrackerMethod::TreeBorrows { .. })) {
if miri_config.provenance_mode != ProvenanceMode::Strict {
fatal_error!(
"Tree Borrows does not support integer-to-pointer casts, and hence requires strict provenance"
);
}
if !miri_config.native_lib.is_empty() {
fatal_error!("Tree Borrows is not compatible with calling native functions");
}
if matches!(miri_config.borrow_tracker, Some(BorrowTrackerMethod::TreeBorrows { .. }))
&& !miri_config.native_lib.is_empty()
{
fatal_error!("Tree Borrows is not compatible with calling native functions");
}

// Native calls and strict provenance are not compatible.
Expand Down
2 changes: 2 additions & 0 deletions src/borrow_tracker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let borrow_tracker = this.machine.borrow_tracker.as_ref().unwrap();
// The body of this loop needs `borrow_tracker` immutably
// so we can't move this code inside the following `end_call`.

// TODO support protected wildcard pointers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear what "support" means.

for (alloc_id, tag) in &frame
.extra
.borrow_tracker
Expand Down
10 changes: 5 additions & 5 deletions src/borrow_tracker/stacked_borrows/stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ impl<'tcx> Stack {
/// Panics if any of the caching mechanisms have broken,
/// - The StackCache indices don't refer to the parallel items,
/// - There are no Unique items outside of first_unique..last_unique
#[cfg(feature = "stack-cache-consistency-check")]
#[cfg(feature = "expensive-consistency-checks")]
fn verify_cache_consistency(&self) {
// Only a full cache needs to be valid. Also see the comments in find_granting_cache
// and set_unknown_bottom.
Expand Down Expand Up @@ -197,7 +197,7 @@ impl<'tcx> Stack {
tag: ProvenanceExtra,
exposed_tags: &FxHashSet<BorTag>,
) -> Result<Option<usize>, ()> {
#[cfg(feature = "stack-cache-consistency-check")]
#[cfg(feature = "expensive-consistency-checks")]
self.verify_cache_consistency();

let ProvenanceExtra::Concrete(tag) = tag else {
Expand Down Expand Up @@ -334,7 +334,7 @@ impl<'tcx> Stack {
// This primes the cache for the next access, which is almost always the just-added tag.
self.cache.add(new_idx, new);

#[cfg(feature = "stack-cache-consistency-check")]
#[cfg(feature = "expensive-consistency-checks")]
self.verify_cache_consistency();
}

Expand Down Expand Up @@ -417,7 +417,7 @@ impl<'tcx> Stack {
self.unique_range.end = self.unique_range.end.min(disable_start);
}

#[cfg(feature = "stack-cache-consistency-check")]
#[cfg(feature = "expensive-consistency-checks")]
self.verify_cache_consistency();

interp_ok(())
Expand Down Expand Up @@ -472,7 +472,7 @@ impl<'tcx> Stack {
self.unique_range = 0..0;
}

#[cfg(feature = "stack-cache-consistency-check")]
#[cfg(feature = "expensive-consistency-checks")]
self.verify_cache_consistency();
interp_ok(())
}
Expand Down
39 changes: 29 additions & 10 deletions src/borrow_tracker/tree_borrows/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ pub(super) struct TbError<'node> {
pub access_cause: AccessCause,
/// Which tag the access that caused this error was made through, i.e.
/// which tag was used to read/write/deallocate.
pub accessed_info: &'node NodeDebugInfo,
pub accessed_info: Option<&'node NodeDebugInfo>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an explanation of what None means.

}

impl TbError<'_> {
Expand All @@ -302,10 +302,12 @@ impl TbError<'_> {
use TransitionError::*;
let cause = self.access_cause;
let accessed = self.accessed_info;
let accessed_str =
self.accessed_info.map(|v| format!("{v}")).unwrap_or_else(|| "wildcard".into());
let conflicting = self.conflicting_info;
let accessed_is_conflicting = accessed.tag == conflicting.tag;
let accessed_is_conflicting = accessed.map(|a| a.tag == conflicting.tag).unwrap_or(false);
let title = format!(
"{cause} through {accessed} at {alloc_id:?}[{offset:#x}] is forbidden",
"{cause} through {accessed_str} at {alloc_id:?}[{offset:#x}] is forbidden",
alloc_id = self.alloc_id,
offset = self.error_offset
);
Expand All @@ -316,7 +318,7 @@ impl TbError<'_> {
let mut details = Vec::new();
if !accessed_is_conflicting {
details.push(format!(
"the accessed tag {accessed} is a child of the conflicting tag {conflicting}"
"the accessed tag {accessed_str} is a child of the conflicting tag {conflicting}"
));
}
let access = cause.print_as_access(/* is_foreign */ false);
Expand All @@ -330,7 +332,7 @@ impl TbError<'_> {
let access = cause.print_as_access(/* is_foreign */ true);
let details = vec![
format!(
"the accessed tag {accessed} is foreign to the {conflicting_tag_name} tag {conflicting} (i.e., it is not a child)"
"the accessed tag {accessed_str} is foreign to the {conflicting_tag_name} tag {conflicting} (i.e., it is not a child)"
),
format!(
"this {access} would cause the {conflicting_tag_name} tag {conflicting} (currently {before_disabled}) to become Disabled"
Expand All @@ -343,16 +345,18 @@ impl TbError<'_> {
let conflicting_tag_name = "strongly protected";
let details = vec![
format!(
"the allocation of the accessed tag {accessed} also contains the {conflicting_tag_name} tag {conflicting}"
"the allocation of the accessed tag {accessed_str} also contains the {conflicting_tag_name} tag {conflicting}"
),
format!("the {conflicting_tag_name} tag {conflicting} disallows deallocations"),
];
(title, details, conflicting_tag_name)
}
};
let mut history = HistoryData::default();
if !accessed_is_conflicting {
history.extend(self.accessed_info.history.forget(), "accessed", false);
if let Some(accessed_info) = self.accessed_info
&& !accessed_is_conflicting
{
history.extend(accessed_info.history.forget(), "accessed", false);
}
history.extend(
self.conflicting_info.history.extract_relevant(self.error_offset, self.error_kind),
Expand All @@ -362,6 +366,21 @@ impl TbError<'_> {
err_machine_stop!(TerminationInfo::TreeBorrowsUb { title, details, history })
}
}
/// Cannot access this allocation with wildcard provenance, as there are no
/// Valid exposed references for this access kind.
pub fn no_valid_exposed_references_error<'tcx>(
alloc_id: AllocId,
offset: u64,
access_cause: AccessCause,
) -> InterpErrorKind<'tcx> {
let title =
format!("{access_cause} through wildcard at {alloc_id:?}[{offset:#x}] is forbidden");
let details = vec![format!(
"there are no exposed references who have {access_cause} permissions to this location"
)];
let history = HistoryData::default();
err_machine_stop!(TerminationInfo::TreeBorrowsUb { title, details, history })
}

type S = &'static str;
/// Pretty-printing details
Expand Down Expand Up @@ -625,8 +644,8 @@ impl DisplayRepr {
let rperm = tree
.rperms
.iter_all()
.map(move |(_offset, perms)| {
let perm = perms.get(idx);
.map(move |(_offset, loc)| {
let perm = loc.perms.get(idx);
perm.cloned()
})
.collect::<Vec<_>>();
Expand Down
110 changes: 65 additions & 45 deletions src/borrow_tracker/tree_borrows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ mod foreign_access_skipping;
mod perms;
mod tree;
mod unimap;
mod wildcard;

#[cfg(test)]
mod exhaustive;
Expand Down Expand Up @@ -54,21 +55,25 @@ impl<'tcx> Tree {
interpret::Pointer::new(alloc_id, range.start),
range.size.bytes(),
);
// TODO: for now we bail out on wildcard pointers. Eventually we should
// handle them as much as we can.
let tag = match prov {
ProvenanceExtra::Concrete(tag) => tag,
ProvenanceExtra::Wildcard => return interp_ok(()),
};
let global = machine.borrow_tracker.as_ref().unwrap();
let span = machine.current_user_relevant_span();
self.perform_access(
tag,
Some((range, access_kind, diagnostics::AccessCause::Explicit(access_kind))),
global,
alloc_id,
span,
)
match prov {
ProvenanceExtra::Concrete(tag) =>
self.perform_access(
tag,
Some((range, access_kind, diagnostics::AccessCause::Explicit(access_kind))),
global,
alloc_id,
span,
),
ProvenanceExtra::Wildcard =>
self.perform_wildcard_access(
Some((range, access_kind, diagnostics::AccessCause::Explicit(access_kind))),
global,
alloc_id,
span,
),
}
}

/// Check that this pointer has permission to deallocate this range.
Expand All @@ -82,18 +87,14 @@ impl<'tcx> Tree {
// TODO: for now we bail out on wildcard pointers. Eventually we should
// handle them as much as we can.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems outdated now?

let tag = match prov {
ProvenanceExtra::Concrete(tag) => tag,
ProvenanceExtra::Wildcard => return interp_ok(()),
ProvenanceExtra::Concrete(tag) => Some(tag),
ProvenanceExtra::Wildcard => None,
};
let global = machine.borrow_tracker.as_ref().unwrap();
let span = machine.current_user_relevant_span();
self.dealloc(tag, alloc_range(Size::ZERO, size), global, alloc_id, span)
}

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
Expand Down Expand Up @@ -252,10 +253,12 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a ptr_try_get_alloc_id above (which github doesn't let me add a comment to -- thanks github) which is subtle and I think not entirely correct. For a wildcard pointer, this will resolve the pointer to an AllocId if it can. If ptr_size is 0, however, that might not be the only legal AllocId!

If the size is 0 on a wildcard pointer I think we have to bail early, there's not much we can do.

log_creation(this, Some((alloc_id, base_offset, parent_prov)))?;

let orig_tag = match parent_prov {
ProvenanceExtra::Wildcard => return interp_ok(place.ptr().provenance), // TODO: handle wildcard pointers
ProvenanceExtra::Concrete(tag) => tag,
let (orig_tag, provenance) = match parent_prov {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So orig_tag is exactly the same as parent_prov except you converted it from ProvenanceExtra to Option...? What's the point of that?

Also, provenance is way to imprecise as a name. There's so many provenances floating around here, this isn't useful. I'd suggest new_prov.

ProvenanceExtra::Wildcard => (None, Provenance::Wildcard), // TODO: handle wildcard pointers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in this branch we just entirely ignore new_tag? That's kind of odd.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the call site of tb_reborrow it says that new_tag may be ignored. The current implementation handles reborrows of wildcard references by also assigning them wildcard provenance. So we don't need a new tag for it.

ProvenanceExtra::Concrete(tag) =>
(Some(tag), Provenance::Concrete { alloc_id, tag: new_tag }),
};
let is_wildcard = orig_tag.is_none();

trace!(
"reborrow: reference {:?} derived from {:?} (pointee {}): {:?}, size {}",
Expand All @@ -266,7 +269,8 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
ptr_size.bytes()
);

if let Some(protect) = new_perm.protector {
// TODO support protecting wildcard pointers.
if !is_wildcard && let Some(protect) = new_perm.protector {
// We register the protection in two different places.
// This makes creating a protector slower, but checking whether a tag
// is protected faster.
Expand All @@ -291,7 +295,7 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
assert_eq!(ptr_size, Size::ZERO); // we did the deref check above, size has to be 0 here
// There's not actually any bytes here where accesses could even be tracked.
// Just produce the new provenance, nothing else to do.
return interp_ok(Some(Provenance::Concrete { alloc_id, tag: new_tag }));
return interp_ok(Some(provenance));
}

let protected = new_perm.protector.is_some();
Expand Down Expand Up @@ -354,14 +358,30 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
start: Size::from_bytes(perm_range.start) + base_offset,
size: Size::from_bytes(perm_range.end - perm_range.start),
};

tree_borrows.perform_access(
orig_tag,
Some((range_in_alloc, AccessKind::Read, diagnostics::AccessCause::Reborrow)),
this.machine.borrow_tracker.as_ref().unwrap(),
alloc_id,
this.machine.current_user_relevant_span(),
)?;
if let Some(orig_tag) = orig_tag {
tree_borrows.perform_access(
orig_tag,
Some((
range_in_alloc,
AccessKind::Read,
diagnostics::AccessCause::Reborrow,
)),
this.machine.borrow_tracker.as_ref().unwrap(),
alloc_id,
this.machine.current_user_relevant_span(),
)?;
} else {
tree_borrows.perform_wildcard_access(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if-then-else is duplicated in at least 2 places (probably more, haven't seen all the code yet). Please handle this inside perform_access instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should perform_access/dealloc take Option<BorTag> or ProvenanceExtra as an argument?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProvenanceExtra is more informative so let's go with that.

Some((
range_in_alloc,
AccessKind::Read,
diagnostics::AccessCause::Reborrow,
)),
this.machine.borrow_tracker.as_ref().unwrap(),
alloc_id,
this.machine.current_user_relevant_span(),
)?;
}

// Also inform the data race model (but only if any bytes are actually affected).
if range_in_alloc.size.bytes() > 0 {
Expand All @@ -377,20 +397,20 @@ trait EvalContextPrivExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
}
}
}

// Record the parent-child pair in the tree.
tree_borrows.new_child(
base_offset,
orig_tag,
new_tag,
inside_perms,
new_perm.outside_perm,
protected,
this.machine.current_user_relevant_span(),
)?;
if let Some(orig_tag) = orig_tag {
// Record the parent-child pair in the tree.
tree_borrows.new_child(
base_offset,
orig_tag,
new_tag,
inside_perms,
new_perm.outside_perm,
protected,
this.machine.current_user_relevant_span(),
)?;
}
drop(tree_borrows);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That empty line was deliberate. Empty lines serve to separate logically distinct sections of code, similar to paragraphs in a text. Please only remove empty lines if they no longer serve that purpose.

interp_ok(Some(Provenance::Concrete { alloc_id, tag: new_tag }))
interp_ok(Some(provenance))
}

fn tb_retag_place(
Expand Down
Loading