data-race: preserve structured access information longer, and don't u…

…pper-case access types

src/tools/miri/src/concurrency/data_race.rs

@@ -41,7 +41,6 @@
 //! on the data-race detection code.
 
 use std::{
-    borrow::Cow,
     cell::{Cell, Ref, RefCell, RefMut},
     fmt::Debug,
     mem,
@@ -199,7 +198,7 @@ struct AtomicMemoryCellClocks {
 /// are all treated as writes for the purpose
 /// of the data-race detector.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
-enum WriteType {
+enum NaWriteType {
     /// Allocate memory.
     Allocate,
 
@@ -212,12 +211,41 @@ enum WriteType {
     /// (Same for `Allocate` above.)
     Deallocate,
 }
-impl WriteType {
-    fn get_descriptor(self) -> &'static str {
+
+impl NaWriteType {
+    fn description(self) -> &'static str {
+        match self {
+            NaWriteType::Allocate => "creating a new allocation",
+            NaWriteType::Write => "non-atomic write",
+            NaWriteType::Deallocate => "deallocation",
+        }
+    }
+}
+
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+enum AccessType {
+    NaRead,
+    NaWrite(NaWriteType),
+    AtomicLoad,
+    AtomicStore,
+    AtomicRmw,
+}
+
+impl AccessType {
+    fn description(self) -> &'static str {
+        match self {
+            AccessType::NaRead => "non-atomic read",
+            AccessType::NaWrite(w) => w.description(),
+            AccessType::AtomicLoad => "atomic load",
+            AccessType::AtomicStore => "atomic store",
+            AccessType::AtomicRmw => "atomic read-modify-write",
+        }
+    }
+
+    fn is_atomic(self) -> bool {
         match self {
-            WriteType::Allocate => "Allocate",
-            WriteType::Write => "Write",
-            WriteType::Deallocate => "Deallocate",
+            AccessType::AtomicLoad | AccessType::AtomicStore | AccessType::AtomicRmw => true,
+            AccessType::NaRead | AccessType::NaWrite(_) => false,
         }
     }
 }
@@ -234,7 +262,7 @@ struct MemoryCellClocks {
     /// The type of operation that the write index represents,
     /// either newly allocated memory, a non-atomic write or
     /// a deallocation of memory.
-    write_type: WriteType,
+    write_type: NaWriteType,
 
     /// The vector-clock of all non-atomic reads that happened since the last non-atomic write
     /// (i.e., we join together the "singleton" clocks corresponding to each read). It is reset to
@@ -265,7 +293,7 @@ impl MemoryCellClocks {
         MemoryCellClocks {
             read: VClock::default(),
             write: (alloc_index, alloc),
-            write_type: WriteType::Allocate,
+            write_type: NaWriteType::Allocate,
             atomic_ops: None,
         }
     }
@@ -488,7 +516,7 @@ impl MemoryCellClocks {
         &mut self,
         thread_clocks: &mut ThreadClockSet,
         index: VectorIdx,
-        write_type: WriteType,
+        write_type: NaWriteType,
         current_span: Span,
     ) -> Result<(), DataRace> {
         log::trace!("Unsynchronized write with vectors: {:#?} :: {:#?}", self, thread_clocks);
@@ -838,48 +866,47 @@ impl VClockAlloc {
         global: &GlobalState,
         thread_mgr: &ThreadManager<'_, '_>,
         mem_clocks: &MemoryCellClocks,
-        action: &str,
-        is_atomic: bool,
+        access: AccessType,
         access_size: Size,
         ptr_dbg: Pointer<AllocId>,
     ) -> InterpResult<'tcx> {
         let (current_index, current_clocks) = global.current_thread_state(thread_mgr);
-        let mut action = Cow::Borrowed(action);
+        let mut other_size = None; // if `Some`, this was a size-mismatch race
         let mut involves_non_atomic = true;
         let write_clock;
         let (other_action, other_thread, other_clock) =
             // First check the atomic-nonatomic cases. If it looks like multiple
             // cases apply, this one should take precedence, else it might look like
             // we are reporting races between two non-atomic reads.
-            if !is_atomic &&
+            if !access.is_atomic() &&
                 let Some(atomic) = mem_clocks.atomic() &&
                 let Some(idx) = Self::find_gt_index(&atomic.write_vector, &current_clocks.clock)
             {
-                (format!("Atomic Store"), idx, &atomic.write_vector)
-            } else if !is_atomic &&
+                (AccessType::AtomicStore, idx, &atomic.write_vector)
+            } else if !access.is_atomic() &&
                 let Some(atomic) = mem_clocks.atomic() &&
                 let Some(idx) = Self::find_gt_index(&atomic.read_vector, &current_clocks.clock)
             {
-                (format!("Atomic Load"), idx, &atomic.read_vector)
+                (AccessType::AtomicLoad, idx, &atomic.read_vector)
             // Then check races with non-atomic writes/reads.
             } else if mem_clocks.write.1 > current_clocks.clock[mem_clocks.write.0] {
                 write_clock = mem_clocks.write();
-                (mem_clocks.write_type.get_descriptor().to_owned(), mem_clocks.write.0, &write_clock)
+                (AccessType::NaWrite(mem_clocks.write_type), mem_clocks.write.0, &write_clock)
             } else if let Some(idx) = Self::find_gt_index(&mem_clocks.read, &current_clocks.clock) {
-                (format!("Read"), idx, &mem_clocks.read)
+                (AccessType::NaRead, idx, &mem_clocks.read)
             // Finally, mixed-size races.
-            } else if is_atomic && let Some(atomic) = mem_clocks.atomic() && atomic.size != access_size {
+            } else if access.is_atomic() && let Some(atomic) = mem_clocks.atomic() && atomic.size != access_size {
                 // This is only a race if we are not synchronized with all atomic accesses, so find
                 // the one we are not synchronized with.
                 involves_non_atomic = false;
-                action = format!("{}-byte (different-size) {action}", access_size.bytes()).into();
+                other_size = Some(atomic.size);
                 if let Some(idx) = Self::find_gt_index(&atomic.write_vector, &current_clocks.clock)
                     {
-                        (format!("{}-byte Atomic Store", atomic.size.bytes()), idx, &atomic.write_vector)
+                        (AccessType::AtomicStore, idx, &atomic.write_vector)
                     } else if let Some(idx) =
                         Self::find_gt_index(&atomic.read_vector, &current_clocks.clock)
                     {
-                        (format!("{}-byte Atomic Load", atomic.size.bytes()), idx, &atomic.read_vector)
+                        (AccessType::AtomicLoad, idx, &atomic.read_vector)
                     } else {
                         unreachable!(
                             "Failed to report data-race for mixed-size access: no race found"
@@ -898,12 +925,24 @@ impl VClockAlloc {
             involves_non_atomic,
             ptr: ptr_dbg,
             op1: RacingOp {
-                action: other_action.to_string(),
+                action: if let Some(other_size) = other_size {
+                    format!("{}-byte {}", other_size.bytes(), other_action.description())
+                } else {
+                    other_action.description().to_owned()
+                },
                 thread_info: other_thread_info,
                 span: other_clock.as_slice()[other_thread.index()].span_data(),
             },
             op2: RacingOp {
-                action: action.to_string(),
+                action: if other_size.is_some() {
+                    format!(
+                        "{}-byte (different-size) {}",
+                        access_size.bytes(),
+                        access.description()
+                    )
+                } else {
+                    access.description().to_owned()
+                },
                 thread_info: current_thread_info,
                 span: current_clocks.clock.as_slice()[current_index.index()].span_data(),
             },
@@ -938,8 +977,7 @@ impl VClockAlloc {
                         global,
                         &machine.threads,
                         mem_clocks,
-                        "Read",
-                        /* is_atomic */ false,
+                        AccessType::NaRead,
                         access_range.size,
                         Pointer::new(alloc_id, Size::from_bytes(mem_clocks_range.start)),
                     );
@@ -956,7 +994,7 @@ impl VClockAlloc {
         &mut self,
         alloc_id: AllocId,
         access_range: AllocRange,
-        write_type: WriteType,
+        write_type: NaWriteType,
         machine: &mut MiriMachine<'_, '_>,
     ) -> InterpResult<'tcx> {
         let current_span = machine.current_span();
@@ -978,8 +1016,7 @@ impl VClockAlloc {
                         global,
                         &machine.threads,
                         mem_clocks,
-                        write_type.get_descriptor(),
-                        /* is_atomic */ false,
+                        AccessType::NaWrite(write_type),
                         access_range.size,
                         Pointer::new(alloc_id, Size::from_bytes(mem_clocks_range.start)),
                     );
@@ -1001,7 +1038,7 @@ impl VClockAlloc {
         range: AllocRange,
         machine: &mut MiriMachine<'_, '_>,
     ) -> InterpResult<'tcx> {
-        self.unique_access(alloc_id, range, WriteType::Write, machine)
+        self.unique_access(alloc_id, range, NaWriteType::Write, machine)
     }
 
     /// Detect data-races for an unsynchronized deallocate operation, will not perform
@@ -1014,7 +1051,7 @@ impl VClockAlloc {
         range: AllocRange,
         machine: &mut MiriMachine<'_, '_>,
     ) -> InterpResult<'tcx> {
-        self.unique_access(alloc_id, range, WriteType::Deallocate, machine)
+        self.unique_access(alloc_id, range, NaWriteType::Deallocate, machine)
     }
 }
 
@@ -1104,7 +1141,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
         this.validate_atomic_op(
             place,
             atomic,
-            "Atomic Load",
+            AccessType::AtomicLoad,
             move |memory, clocks, index, atomic| {
                 if atomic == AtomicReadOrd::Relaxed {
                     memory.load_relaxed(&mut *clocks, index, place.layout.size)
@@ -1126,7 +1163,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
         this.validate_atomic_op(
             place,
             atomic,
-            "Atomic Store",
+            AccessType::AtomicStore,
             move |memory, clocks, index, atomic| {
                 if atomic == AtomicWriteOrd::Relaxed {
                     memory.store_relaxed(clocks, index, place.layout.size)
@@ -1148,26 +1185,31 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
         let acquire = matches!(atomic, Acquire | AcqRel | SeqCst);
         let release = matches!(atomic, Release | AcqRel | SeqCst);
         let this = self.eval_context_mut();
-        this.validate_atomic_op(place, atomic, "Atomic RMW", move |memory, clocks, index, _| {
-            if acquire {
-                memory.load_acquire(clocks, index, place.layout.size)?;
-            } else {
-                memory.load_relaxed(clocks, index, place.layout.size)?;
-            }
-            if release {
-                memory.rmw_release(clocks, index, place.layout.size)
-            } else {
-                memory.rmw_relaxed(clocks, index, place.layout.size)
-            }
-        })
+        this.validate_atomic_op(
+            place,
+            atomic,
+            AccessType::AtomicRmw,
+            move |memory, clocks, index, _| {
+                if acquire {
+                    memory.load_acquire(clocks, index, place.layout.size)?;
+                } else {
+                    memory.load_relaxed(clocks, index, place.layout.size)?;
+                }
+                if release {
+                    memory.rmw_release(clocks, index, place.layout.size)
+                } else {
+                    memory.rmw_relaxed(clocks, index, place.layout.size)
+                }
+            },
+        )
     }
 
     /// Generic atomic operation implementation
     fn validate_atomic_op<A: Debug + Copy>(
         &self,
         place: &MPlaceTy<'tcx, Provenance>,
         atomic: A,
-        description: &str,
+        access: AccessType,
         mut op: impl FnMut(
             &mut MemoryCellClocks,
             &mut ThreadClockSet,
@@ -1176,6 +1218,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
         ) -> Result<(), DataRace>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_ref();
+        assert!(access.is_atomic());
         if let Some(data_race) = &this.machine.data_race {
             if data_race.race_detecting() {
                 let size = place.layout.size;
@@ -1185,7 +1228,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
                 let alloc_meta = this.get_alloc_extra(alloc_id)?.data_race.as_ref().unwrap();
                 log::trace!(
                     "Atomic op({}) with ordering {:?} on {:?} (size={})",
-                    description,
+                    access.description(),
                     &atomic,
                     place.ptr(),
                     size.bytes()
@@ -1207,8 +1250,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
                                     data_race,
                                     &this.machine.threads,
                                     mem_clocks,
-                                    description,
-                                    /* is_atomic */ true,
+                                    access,
                                     place.layout.size,
                                     Pointer::new(
                                         alloc_id,

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

@@ -17,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\) (Read|Write) on thread `<unnamed>` and \(2\) Write on thread `<unnamed>`/
+        *p = 5; //~ ERROR: /Data race detected between \(1\) non-atomic (read|write) on thread `<unnamed>` and \(2\) non-atomic write on thread `<unnamed>`/
     }
 }
 

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

@@ -1,8 +1,8 @@
-error: Undefined Behavior: Data race detected between (1) Write on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
+error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `<unnamed>` and (2) non-atomic 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) Write on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
+   |         ^^^^^^ Data race detected between (1) non-atomic write on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at ALLOC. (2) just happened here
    |
 help: and (1) occurred earlier here
   --> $DIR/retag_data_race_write.rs:LL:CC

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

@@ -1,8 +1,8 @@
-error: Undefined Behavior: Data race detected between (1) Read on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
+error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `<unnamed>` and (2) non-atomic 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
+   |         ^^^^^^ Data race detected between (1) non-atomic read on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at ALLOC. (2) just happened here
    |
 help: and (1) occurred earlier here
   --> $DIR/retag_data_race_write.rs:LL:CC

src/tools/miri/tests/fail/data_race/alloc_read_race.rs

@@ -39,7 +39,7 @@ pub fn main() {
             let pointer = &*ptr.0;
 
             // Note: could also error due to reading uninitialized memory, but the data-race detector triggers first.
-            *pointer.load(Ordering::Relaxed) //~ ERROR: Data race detected between (1) Allocate on thread `<unnamed>` and (2) Read on thread `<unnamed>`
+            *pointer.load(Ordering::Relaxed) //~ ERROR: Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic read on thread `<unnamed>`
         });
 
         j1.join().unwrap();

src/tools/miri/tests/fail/data_race/alloc_read_race.stderr

@@ -1,8 +1,8 @@
-error: Undefined Behavior: Data race detected between (1) Allocate on thread `<unnamed>` and (2) Read on thread `<unnamed>` at ALLOC. (2) just happened here
+error: Undefined Behavior: Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic read on thread `<unnamed>` at ALLOC. (2) just happened here
   --> $DIR/alloc_read_race.rs:LL:CC
    |
 LL |             *pointer.load(Ordering::Relaxed)
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) Allocate on thread `<unnamed>` and (2) Read on thread `<unnamed>` at ALLOC. (2) just happened here
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic read on thread `<unnamed>` at ALLOC. (2) just happened here
    |
 help: and (1) occurred earlier here
   --> $DIR/alloc_read_race.rs:LL:CC

src/tools/miri/tests/fail/data_race/alloc_write_race.rs

@@ -37,7 +37,7 @@ pub fn main() {
         let j2 = spawn(move || {
             let ptr = ptr; // avoid field capturing
             let pointer = &*ptr.0;
-            *pointer.load(Ordering::Relaxed) = 2; //~ ERROR: Data race detected between (1) Allocate on thread `<unnamed>` and (2) Write on thread `<unnamed>`
+            *pointer.load(Ordering::Relaxed) = 2; //~ ERROR: Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>`
         });
 
         j1.join().unwrap();

src/tools/miri/tests/fail/data_race/alloc_write_race.stderr

@@ -1,8 +1,8 @@
-error: Undefined Behavior: Data race detected between (1) Allocate on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
+error: Undefined Behavior: Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at ALLOC. (2) just happened here
   --> $DIR/alloc_write_race.rs:LL:CC
    |
 LL |             *pointer.load(Ordering::Relaxed) = 2;
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) Allocate on thread `<unnamed>` and (2) Write on thread `<unnamed>` at ALLOC. (2) just happened here
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Data race detected between (1) creating a new allocation on thread `<unnamed>` and (2) non-atomic write on thread `<unnamed>` at ALLOC. (2) just happened here
    |
 help: and (1) occurred earlier here
   --> $DIR/alloc_write_race.rs:LL:CC

src/tools/miri/tests/fail/data_race/atomic_read_na_write_race1.rs

@@ -22,7 +22,7 @@ pub fn main() {
 
         let j2 = spawn(move || {
             let c = c; // avoid field capturing
-            (&*c.0).load(Ordering::SeqCst) //~ ERROR: Data race detected between (1) Write on thread `<unnamed>` and (2) Atomic Load on thread `<unnamed>`
+            (&*c.0).load(Ordering::SeqCst) //~ ERROR: Data race detected between (1) non-atomic write on thread `<unnamed>` and (2) atomic load on thread `<unnamed>`
         });
 
         j1.join().unwrap();