Skip to content

Commit 793f198

Browse files
committed
Auto merge of #2489 - RalfJung:srw-merging, r=saethlin
add test that we do not merge neighboring SRW Turns out that interior_mut2 also already tests this, but that also involves `UnsafeCell` so the new test still seems more clear. Basically the new test is the same as the old except that it uses raw pointers rather than `&UnsafeCell`. (When the old test was written, raw pointers were still untagged, so no such test would have been possible.) I verified that both of these fail when we remove mutable references rather than disabling them. Here is the patch I used for that: <details> ```diff diff --git a/Cargo.toml b/Cargo.toml index 208b3a7..f9d1b0ac 100644 --- a/Cargo.toml +++ b/Cargo.toml `@@` -53,7 +53,7 `@@` name = "compiletest" harness = false [features] -default = ["stack-cache"] +default = [] stack-cache = [] # Be aware that this file is inside a workspace when used via the diff --git a/src/lib.rs b/src/lib.rs index ba337f2..2a3066f4 100644 --- a/src/lib.rs +++ b/src/lib.rs `@@` -9,6 +9,7 `@@` #![feature(is_some_with)] #![feature(nonzero_ops)] #![feature(local_key_cell_methods)] +#![feature(drain_filter)] // Configure clippy and other lints #![allow( clippy::collapsible_else_if, diff --git a/src/stacked_borrows/stack.rs b/src/stacked_borrows/stack.rs index 4a9a13d..37246df7 100644 --- a/src/stacked_borrows/stack.rs +++ b/src/stacked_borrows/stack.rs `@@` -351,6 +351,9 `@@` impl<'tcx> Stack { #[cfg(all(feature = "stack-cache", debug_assertions))] self.verify_cache_consistency(); + // HACK -- now just delete all disabled things. + self.borrows.drain_filter(|b| matches!(b.perm(), Permission::Disabled)); + Ok(()) } ``` </details> r? `@saethlin`
2 parents a000764 + db43ee5 commit 793f198

File tree

4 files changed

+58
-6
lines changed

4 files changed

+58
-6
lines changed

ci.sh

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,12 @@ set -x
55
# Determine configuration
66
export RUSTFLAGS="-D warnings"
77
export CARGO_INCREMENTAL=0
8-
export CARGO_EXTRA_FLAGS="--all-features"
98

109
# Prepare
1110
echo "Build and install miri"
1211
./miri install # implicitly locked
12+
./miri check --no-default-features # make sure this can be built
13+
./miri check --all-features # and this, too
1314
./miri build --all-targets --locked # the build that all the `./miri test` below will use
1415
echo
1516

src/stacked_borrows/stack.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ impl<'tcx> Stack {
8181
/// Panics if any of the caching mechanisms have broken,
8282
/// - The StackCache indices don't refer to the parallel items,
8383
/// - There are no Unique items outside of first_unique..last_unique
84-
#[cfg(debug_assertions)]
84+
#[cfg(all(feature = "stack-cache", debug_assertions))]
8585
fn verify_cache_consistency(&self) {
8686
// Only a full cache needs to be valid. Also see the comments in find_granting_cache
8787
// and set_unknown_bottom.
@@ -128,7 +128,7 @@ impl<'tcx> Stack {
128128
tag: ProvenanceExtra,
129129
exposed_tags: &FxHashSet<SbTag>,
130130
) -> Result<Option<usize>, ()> {
131-
#[cfg(debug_assertions)]
131+
#[cfg(all(feature = "stack-cache", debug_assertions))]
132132
self.verify_cache_consistency();
133133

134134
let ProvenanceExtra::Concrete(tag) = tag else {
@@ -320,13 +320,14 @@ impl<'tcx> Stack {
320320

321321
if disable_start <= unique_range.end {
322322
let lower = unique_range.start.max(disable_start);
323-
let upper = self.unique_range.end;
323+
let upper = unique_range.end;
324324
for item in &mut self.borrows[lower..upper] {
325325
if item.perm() == Permission::Unique {
326326
log::trace!("access: disabling item {:?}", item);
327327
visitor(*item)?;
328328
item.set_permission(Permission::Disabled);
329329
// Also update all copies of this item in the cache.
330+
#[cfg(feature = "stack-cache")]
330331
for it in &mut self.cache.items {
331332
if it.tag() == item.tag() {
332333
it.set_permission(Permission::Disabled);
@@ -347,7 +348,7 @@ impl<'tcx> Stack {
347348
self.unique_range.end = self.unique_range.end.min(disable_start);
348349
}
349350

350-
#[cfg(debug_assertions)]
351+
#[cfg(all(feature = "stack-cache", debug_assertions))]
351352
self.verify_cache_consistency();
352353

353354
Ok(())
@@ -402,7 +403,7 @@ impl<'tcx> Stack {
402403
self.unique_range = 0..0;
403404
}
404405

405-
#[cfg(debug_assertions)]
406+
#[cfg(all(feature = "stack-cache", debug_assertions))]
406407
self.verify_cache_consistency();
407408
Ok(())
408409
}
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// This tests demonstrates the effect of 'Disabling' mutable references on reads, rather than
2+
// removing them from the stack -- the latter would 'merge' neighboring SRW groups which we would
3+
// like to avoid.
4+
fn main() {
5+
unsafe {
6+
let mut mem = 0;
7+
let base = &mut mem as *mut i32; // the base pointer we build the rest of the stack on
8+
let raw = {
9+
let mutref = &mut *base;
10+
mutref as *mut i32
11+
};
12+
// In the stack, we now have [base, mutref, raw].
13+
// We do this in a weird way where `mutref` is out of scope here, just in case
14+
// Miri decides to get smart and argue that items for tags that are no longer
15+
// used by any pointer stored anywhere in the machine can be removed.
16+
let _val = *base;
17+
// now mutref is disabled
18+
*base = 1;
19+
// this should pop raw from the stack, since it is in a different SRW group
20+
let _val = *raw; //~ERROR: that tag does not exist in the borrow stack
21+
}
22+
}
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
error: Undefined Behavior: attempting a read access using <TAG> at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
2+
--> $DIR/disable_mut_does_not_merge_srw.rs:LL:CC
3+
|
4+
LL | let _val = *raw;
5+
| ^^^^
6+
| |
7+
| attempting a read access using <TAG> at ALLOC[0x0], but that tag does not exist in the borrow stack for this location
8+
| this error occurs as part of an access at ALLOC[0x0..0x4]
9+
|
10+
= help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
11+
= help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
12+
help: <TAG> was created by a retag at offsets [0x0..0x4]
13+
--> $DIR/disable_mut_does_not_merge_srw.rs:LL:CC
14+
|
15+
LL | mutref as *mut i32
16+
| ^^^^^^
17+
help: <TAG> was later invalidated at offsets [0x0..0x4]
18+
--> $DIR/disable_mut_does_not_merge_srw.rs:LL:CC
19+
|
20+
LL | *base = 1;
21+
| ^^^^^^^^^
22+
= note: backtrace:
23+
= note: inside `main` at $DIR/disable_mut_does_not_merge_srw.rs:LL:CC
24+
25+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
26+
27+
error: aborting due to previous error
28+

0 commit comments

Comments
 (0)