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.

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

Ideas on getting information about borrow stacks during execution #2322

Merged
merged 4 commits into from
Oct 19, 2022
Merged
Changes from all 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
11 changes: 11 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -531,6 +531,17 @@ extern "Rust" {
/// This is internal and unstable and should not be used; we give it here
/// just to be complete.
fn miri_start_panic(payload: *mut u8) -> !;

/// Miri-provided extern function to get the internal unique identifier for the allocation that a pointer
/// points to. This is only useful as an input to `miri_print_stacks`, and it is a separate call because
/// getting a pointer to an allocation at runtime can change the borrow stacks in the allocation.
Copy link
Member

Choose a reason for hiding this comment

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

The documentation should describe what happens when the ptr does not have an AllocId. provenance could be None or Wildcard, what happens then?

fn miri_get_alloc_id(ptr: *const ()) -> u64;

/// Miri-provided extern function to print (from the interpreter, not the program) the contents of all
/// borrow stacks in an allocation. The format of what this emits is unstable and may change at any time.
/// In particular, users should be aware that Miri will periodically attempt to garbage collect the
/// contents of all stacks. Callers of this function may wish to pass `-Zmiri-tag-gc=0` to disable the GC.
fn miri_print_stacks(alloc_id: u64);
Copy link
Member

@RalfJung RalfJung Oct 22, 2022

Choose a reason for hiding this comment

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

I think we should rename this function. 'stacks' sounds like it is referring to the call stacks.

We should also make it clear that this is extremely unstable -- not only the format can change, the entire function can be removed from Miri any time or have its signature changed.

}
```

4 changes: 4 additions & 0 deletions src/range_map.rs
Original file line number Diff line number Diff line change
@@ -91,6 +91,10 @@ impl<T> RangeMap<T> {
self.v.iter_mut().map(|elem| &mut elem.data)
}

pub fn iter_all(&self) -> impl Iterator<Item = (ops::Range<u64>, &T)> {
self.v.iter().map(|elem| (elem.range.clone(), &elem.data))
}

// Splits the element situated at the given `index`, such that the 2nd one starts at offset
// `split_offset`. Do nothing if the element already starts there.
// Returns whether a split was necessary.
13 changes: 13 additions & 0 deletions src/shims/foreign_items.rs
Original file line number Diff line number Diff line change
@@ -417,6 +417,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
// shim, add it to the corresponding submodule.
match link_name.as_str() {
// Miri-specific extern functions
"miri_get_alloc_id" => {
let [ptr] = this.check_shim(abi, Abi::Rust, link_name, args)?;
let ptr = this.read_pointer(ptr)?;
let (alloc_id, _, _) = this.ptr_get_alloc_id(ptr)?;
this.write_scalar(Scalar::from_u64(alloc_id.0.get()), dest)?;
}
"miri_print_stacks" => {
let [id] = this.check_shim(abi, Abi::Rust, link_name, args)?;
let id = this.read_scalar(id)?.to_u64()?;
if let Some(id) = std::num::NonZeroU64::new(id) {
this.print_stacks(AllocId(id))?;
}
}
Comment on lines +426 to +432
Copy link
Contributor

Choose a reason for hiding this comment

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

is this separate (and on an alloc id) so we can avoid adding more stacked borrows to things when printing the stack?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. As it is, you usually end up perturbing the stacked borrows runtime a bit to pass the pointer to miri_get_alloc_id, but at least you don't perturb it on every call.

Some people have suggested that I invent some magic Miri macro that can avoid this instead of extern functions, but I have no idea how to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

no no, I like it. Should document that though, so people don't just miri_print_stacks(miri_get_alloc_id(&x.y)) everywhere

Copy link
Member Author

@saethlin saethlin Jul 6, 2022

Choose a reason for hiding this comment

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

True, but where?

This whole workflow where end users declare extern "Rust" functions is strange to say the least. Would it make sense to have something on crates.io that at least provides safe wrappers around these? Having a crate for these would definitely ease documenting them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have something on crates.io that at least provides safe wrappers around these? Having a crate for these would definitely ease documenting them.

yea, I was wondering about that, too. Such a crate is complicated to tie to a miri version, so I'm not sure publishing it will make things easier for people if we ever change anything in that crate. But it may be better than just having documentation in markdown for these magic extern functions. idk, maybe start out with docs in this repo? It's not like such a crate would be on crates.io or godbolt, so people may have to resort to locally using extern anyway

Copy link
Member

Choose a reason for hiding this comment

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

True, but where?

The README has a section on these magic extern functions; they need to at least be documented there.

And then we should have a chapter in the Miri Book about how to debug Stacked Borrows things. Which book, you ask? Yes that is indeed the point.^^

yea, I was wondering about that, too. Such a crate is complicated to tie to a miri version, so I'm not sure publishing it will make things easier for people if we ever change anything in that crate.

Yes we should have such a create! This interface does not change very often, and when it does we anyway need to do some clever updating thing like we did when changing the backtrace API. So we can add a new API to Miri, ship that, then update the crate, ship that, then maybe remove the old thing from Miri.

Which reminds me, all these magic functions that Miri exposes should have some kind of flag/version number parameter, to make them extensible in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

This documentation is still missing

Copy link
Member Author

Choose a reason for hiding this comment

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

Somehow I missed this...

Do we want to add a version mangle to these new functions?

The functions in the README aren't in alphabetical order 😅 so I guess I'll just stick the new ones at the bottom?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can add the version to everything in one go when we figure out how we want to distribute these functions to users

"miri_static_root" => {
let [ptr] = this.check_shim(abi, Abi::Rust, link_name, args)?;
let ptr = this.read_pointer(ptr)?;
15 changes: 15 additions & 0 deletions src/stacked_borrows/mod.rs
Original file line number Diff line number Diff line change
@@ -1123,4 +1123,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
}
Ok(())
}

fn print_stacks(&mut self, alloc_id: AllocId) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
let alloc_extra = this.get_alloc_extra(alloc_id)?;
let stacks = alloc_extra.stacked_borrows.as_ref().unwrap().borrow();
for (range, stack) in stacks.stacks.iter_all() {
print!("{:?}: [", range);
for i in 0..stack.len() {
let item = stack.get(i).unwrap();
print!(" {:?}{:?}", item.perm(), item.tag());
}
println!(" ]");
}
Ok(())
}
}
2 changes: 2 additions & 0 deletions tests/compiletest.rs
Original file line number Diff line number Diff line change
@@ -138,6 +138,8 @@ regexes! {
STDOUT:
// Windows file paths
r"\\" => "/",
// erase Stacked Borrows tags
"<[0-9]+>" => "<TAG>",
}

regexes! {
29 changes: 29 additions & 0 deletions tests/pass/stacked-borrows/stack-printing.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
use std::{
alloc::{self, Layout},
mem::ManuallyDrop,
};

extern "Rust" {
fn miri_get_alloc_id(ptr: *const u8) -> u64;
fn miri_print_stacks(alloc_id: u64);
}

fn main() {
let ptr = unsafe { alloc::alloc(Layout::new::<u8>()) };
let alloc_id = unsafe { miri_get_alloc_id(ptr) };
unsafe { miri_print_stacks(alloc_id) };

assert!(!ptr.is_null());
unsafe { miri_print_stacks(alloc_id) };

unsafe { *ptr = 42 };
unsafe { miri_print_stacks(alloc_id) };

let _b = unsafe { ManuallyDrop::new(Box::from_raw(ptr)) };
unsafe { miri_print_stacks(alloc_id) };

let _ptr = unsafe { &*ptr };
unsafe { miri_print_stacks(alloc_id) };

unsafe { alloc::dealloc(ptr, Layout::new::<u8>()) };
}
5 changes: 5 additions & 0 deletions tests/pass/stacked-borrows/stack-printing.stdout
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
0..1: [ SharedReadWrite<TAG> ]
Copy link
Member

@RalfJung RalfJung Oct 22, 2022

Choose a reason for hiding this comment

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

Might be worth adding a comment that left is the bottom of the stack? Hm, but getting this each time is annoying... but the miri_print_stacks docs should say that, at least.

Also please add a test that involves an unknown bottom, to see how that prints.

0..1: [ SharedReadWrite<TAG> ]
0..1: [ SharedReadWrite<TAG> ]
0..1: [ SharedReadWrite<TAG> Unique<TAG> Unique<TAG> Unique<TAG> Unique<TAG> Unique<TAG> ]
0..1: [ SharedReadWrite<TAG> Disabled<TAG> Disabled<TAG> Disabled<TAG> Disabled<TAG> Disabled<TAG> SharedReadOnly<TAG> ]