tree borrows: consider some retags as writes for the purpose of data races

Author: RalfJung

src/tools/miri/src/borrow_tracker/tree_borrows/diagnostics.rs

@@ -218,7 +218,7 @@ impl<'tcx> Tree {
     }
 }
 
-#[derive(Debug, Clone, Copy, PartialEq)]
+#[derive(Debug, Clone, Copy)]
 pub(super) enum TransitionError {
     /// This access is not allowed because some parent tag has insufficient permissions.
     /// For example, if a tag is `Frozen` and encounters a child write this will

src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs

@@ -117,7 +117,7 @@ impl<'tcx> NewPermission {
         let ty_is_freeze = pointee.is_freeze(*cx.tcx, cx.param_env());
         let ty_is_unpin = pointee.is_unpin(*cx.tcx, cx.param_env());
         let initial_state = match mutability {
-            Mutability::Mut if ty_is_unpin => Permission::new_unique_2phase(ty_is_freeze),
+            Mutability::Mut if ty_is_unpin => Permission::new_reserved(ty_is_freeze),
             Mutability::Not if ty_is_freeze => Permission::new_frozen(),
             // Raw pointers never enter this function so they are not handled.
             // However raw pointers are not the only pointers that take the parent
@@ -146,7 +146,7 @@ impl<'tcx> NewPermission {
             let ty_is_freeze = ty.is_freeze(*cx.tcx, cx.param_env());
             Self {
                 zero_size,
-                initial_state: Permission::new_unique_2phase(ty_is_freeze),
+                initial_state: Permission::new_reserved(ty_is_freeze),
                 protector: (kind == RetagKind::FnEntry).then_some(ProtectorKind::WeakProtector),
             }
         })
@@ -250,6 +250,7 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
 
         let alloc_kind = this.get_alloc_info(alloc_id).2;
         if !matches!(alloc_kind, AllocKind::LiveData) {
+            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 Ok(Some(Provenance::Concrete { alloc_id, tag: new_tag }));
@@ -261,24 +262,39 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
         let mut tree_borrows = alloc_extra.borrow_tracker_tb().borrow_mut();
 
         // All reborrows incur a (possibly zero-sized) read access to the parent
-        {
-            let global = &this.machine.borrow_tracker.as_ref().unwrap();
-            let span = this.machine.current_span();
-            tree_borrows.perform_access(
-                AccessKind::Read,
-                orig_tag,
-                range,
-                global,
-                span,
-                diagnostics::AccessCause::Reborrow,
-            )?;
+        tree_borrows.perform_access(
+            AccessKind::Read,
+            orig_tag,
+            range,
+            this.machine.borrow_tracker.as_ref().unwrap(),
+            this.machine.current_span(),
+            diagnostics::AccessCause::Reborrow,
+        )?;
+        // Record the parent-child pair in the tree.
+        tree_borrows.new_child(orig_tag, new_tag, new_perm.initial_state, range, span)?;
+        drop(tree_borrows);
+
+        // Also inform the data race model (but only if any bytes are actually affected).
+        if range.size.bytes() > 0 {
             if let Some(data_race) = alloc_extra.data_race.as_ref() {
-                data_race.read(alloc_id, range, &this.machine)?;
+                // We sometimes need to make it a write, since not all retags commute with reads!
+                // FIXME: Is that truly the semantics we want? Some optimizations are likely to be
+                // very unhappy without this. We'd tsill ge some UB just by picking a suitable
+                // interleaving, but wether UB happens can depend on whether a write occurs in the
+                // future...
+                let is_write = new_perm.initial_state.is_active()
+                    || (new_perm.initial_state.is_resrved() && new_perm.protector.is_some());
+                if is_write {
+                    // Need to get mutable access to alloc_extra.
+                    // (Cannot always do this as we can do read-only reborrowing on read-only allocations.)
+                    let (alloc_extra, machine) = this.get_alloc_extra_mut(alloc_id)?;
+                    alloc_extra.data_race.as_mut().unwrap().write(alloc_id, range, machine)?;
+                } else {
+                    data_race.read(alloc_id, range, &this.machine)?;
+                }
             }
         }
 
-        // Record the parent-child pair in the tree.
-        tree_borrows.new_child(orig_tag, new_tag, new_perm.initial_state, range, span)?;
         Ok(Some(Provenance::Concrete { alloc_id, tag: new_tag }))
     }
 

src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs

@@ -134,25 +134,32 @@ pub struct PermTransition {
 
 impl Permission {
     /// Default initial permission of the root of a new tree.
-    pub fn new_root() -> Self {
+    pub fn new_active() -> Self {
         Self { inner: Active }
     }
 
     /// Default initial permission of a reborrowed mutable reference.
-    pub fn new_unique_2phase(ty_is_freeze: bool) -> Self {
+    pub fn new_reserved(ty_is_freeze: bool) -> Self {
         Self { inner: Reserved { ty_is_freeze } }
     }
 
-    /// Default initial permission for return place.
-    pub fn new_active() -> Self {
-        Self { inner: Active }
-    }
-
     /// Default initial permission of a reborrowed shared reference
     pub fn new_frozen() -> Self {
         Self { inner: Frozen }
     }
 
+    pub fn is_active(self) -> bool {
+        matches!(self.inner, Active)
+    }
+
+    pub fn is_resrved(self) -> bool {
+        matches!(self.inner, Reserved { .. })
+    }
+
+    pub fn is_frozen(self) -> bool {
+        matches!(self.inner, Frozen)
+    }
+
     /// Apply the transition to the inner PermissionPriv.
     pub fn perform_access(
         kind: AccessKind,

src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs

@@ -376,7 +376,7 @@ where {
 impl Tree {
     /// Create a new tree, with only a root pointer.
     pub fn new(root_tag: BorTag, size: Size, span: Span) -> Self {
-        let root_perm = Permission::new_root();
+        let root_perm = Permission::new_active();
         let mut tag_mapping = UniKeyMap::default();
         let root_idx = tag_mapping.insert(root_tag);
         let nodes = {

src/tools/miri/tests/fail/both_borrows/retag_data_race_protected_read.rs

@@ -0,0 +1,29 @@
+//@revisions: stack tree
+//@compile-flags: -Zmiri-preemption-rate=0
+//@[tree]compile-flags: -Zmiri-tree-borrows
+use std::thread;
+
+#[derive(Copy, Clone)]
+struct SendPtr(*mut i32);
+unsafe impl Send for SendPtr {}
+
+fn main() {
+    let mut mem = 0;
+    let ptr = SendPtr(&mut mem as *mut _);
+
+    let t = thread::spawn(move || {
+        let ptr = ptr;
+        // We do a protected 2phase retag (but no write!) in this thread.
+        fn retag(_x: &mut i32) {} //~[tree]ERROR: Data race detected between (1) Read on thread `main` and (2) Write on thread `<unnamed>`
+        retag(unsafe { &mut *ptr.0 }); //~[stack]ERROR: Data race detected between (1) Read on thread `main` and (2) Write on thread `<unnamed>`
+    });
+
+    // We do a read in the main thread.
+    unsafe { ptr.0.read() };
+
+    // These two operations do not commute -- if the read happens after the retag, the retagged pointer
+    // gets frozen! So we want this to be considered UB so that we can still freely move the read around
+    // in this thread without worrying about reordering with retags in other threads.
+
+    t.join().unwrap();
+}

src/tools/miri/tests/fail/both_borrows/retag_data_race_protected_read.stack.stderr

@@ -0,0 +1,20 @@
+error: Undefined Behavior: Data race detected between (1) Read on thread `main` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
+  --> $DIR/retag_data_race_protected_read.rs:LL:CC
+   |
+LL |         retag(unsafe { &mut *ptr.0 });
+   |                        ^^^^^^^^^^^ Data race detected between (1) Read on thread `main` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
+   |
+help: and (1) occurred earlier here
+  --> $DIR/retag_data_race_protected_read.rs:LL:CC
+   |
+LL |     unsafe { ptr.0.read() };
+   |              ^^^^^^^^^^^^
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE (of the first span):
+   = note: inside closure at $DIR/retag_data_race_protected_read.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to previous error
+

src/tools/miri/tests/fail/both_borrows/retag_data_race_protected_read.tree.stderr

@@ -0,0 +1,25 @@
+error: Undefined Behavior: Data race detected between (1) Read on thread `main` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
+  --> $DIR/retag_data_race_protected_read.rs:LL:CC
+   |
+LL |         fn retag(_x: &mut i32) {}
+   |                  ^^ Data race detected between (1) Read on thread `main` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
+   |
+help: and (1) occurred earlier here
+  --> $DIR/retag_data_race_protected_read.rs:LL:CC
+   |
+LL |     unsafe { ptr.0.read() };
+   |              ^^^^^^^^^^^^
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE (of the first span):
+   = note: inside `main::{closure#0}::retag` at $DIR/retag_data_race_protected_read.rs:LL:CC
+note: inside closure
+  --> $DIR/retag_data_race_protected_read.rs:LL:CC
+   |
+LL | ...   retag(unsafe { &mut *ptr.0 });
+   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to previous error
+

src/tools/miri/tests/fail/stacked_borrows/retag_data_race_write.rs renamed to src/tools/miri/tests/fail/both_borrows/retag_data_race_write.rs

@@ -1,5 +1,7 @@
 //! Make sure that a retag acts like a write for the data race model.
+//@revisions: stack tree
 //@compile-flags: -Zmiri-preemption-rate=0
+//@[tree]compile-flags: -Zmiri-tree-borrows
 #[derive(Copy, Clone)]
 struct SendPtr(*mut u8);
 
@@ -15,7 +17,7 @@ fn thread_1(p: SendPtr) {
 fn thread_2(p: SendPtr) {
     let p = p.0;
     unsafe {
-        *p = 5; //~ ERROR: Data race detected between (1) Write on thread `<unnamed>` and (2) Write on thread `<unnamed>`
+        *p = 5; //~ ERROR: /Data race detected between \(1\) (Read|Write) on thread `<unnamed>` and \(2\) Write on thread `<unnamed>`/
     }
 }
 

src/tools/miri/tests/fail/stacked_borrows/retag_data_race_write.stderr renamed to src/tools/miri/tests/fail/both_borrows/retag_data_race_write.stack.stderr

@@ -0,0 +1,25 @@
+error: Undefined Behavior: Data race detected between (1) Read on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
+  --> $DIR/retag_data_race_write.rs:LL:CC
+   |
+LL |         *p = 5;
+   |         ^^^^^^ Data race detected between (1) Read on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
+   |
+help: and (1) occurred earlier here
+  --> $DIR/retag_data_race_write.rs:LL:CC
+   |
+LL |         let _r = &mut *p;
+   |                  ^^^^^^^
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE (of the first span):
+   = note: inside `thread_2` at $DIR/retag_data_race_write.rs:LL:CC
+note: inside closure
+  --> $DIR/retag_data_race_write.rs:LL:CC
+   |
+LL |     let t2 = std::thread::spawn(move || thread_2(p));
+   |                                         ^^^^^^^^^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to previous error
+