Report retags as distinct from real memory accesses for data races

Author: Zoxc

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

@@ -18,6 +18,7 @@ use crate::borrow_tracker::{
     stacked_borrows::diagnostics::{AllocHistory, DiagnosticCx, DiagnosticCxBuilder},
     GlobalStateInner, ProtectorKind,
 };
+use crate::concurrency::data_race::{NaReadType, NaWriteType};
 use crate::*;
 
 use diagnostics::{RetagCause, RetagInfo};
@@ -751,7 +752,13 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
                     assert_eq!(access, AccessKind::Write);
                     // Make sure the data race model also knows about this.
                     if let Some(data_race) = alloc_extra.data_race.as_mut() {
-                        data_race.write(alloc_id, range, machine)?;
+                        data_race.write(
+                            alloc_id,
+                            range,
+                            NaWriteType::Retag,
+                            Some(place.layout.ty),
+                            machine,
+                        )?;
                     }
                 }
             }
@@ -794,7 +801,13 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
                         assert_eq!(access, AccessKind::Read);
                         // Make sure the data race model also knows about this.
                         if let Some(data_race) = alloc_extra.data_race.as_ref() {
-                            data_race.read(alloc_id, range, &this.machine)?;
+                            data_race.read(
+                                alloc_id,
+                                range,
+                                NaReadType::Retag,
+                                Some(place.layout.ty),
+                                &this.machine,
+                            )?;
                         }
                     }
                     Ok(())

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

@@ -9,8 +9,11 @@ use rustc_middle::{
 use rustc_span::def_id::DefId;
 use rustc_target::abi::{Abi, Size};
 
-use crate::borrow_tracker::{GlobalState, GlobalStateInner, ProtectorKind};
 use crate::*;
+use crate::{
+    borrow_tracker::{GlobalState, GlobalStateInner, ProtectorKind},
+    concurrency::data_race::NaReadType,
+};
 
 pub mod diagnostics;
 mod perms;
@@ -312,7 +315,13 @@ trait EvalContextPrivExt<'mir: 'ecx, 'tcx: 'mir, 'ecx>: crate::MiriInterpCxExt<'
         // 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)?;
+                data_race.read(
+                    alloc_id,
+                    range,
+                    NaReadType::Retag,
+                    Some(place.layout.ty),
+                    &this.machine,
+                )?;
             }
         }
 

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

@@ -49,7 +49,7 @@ use std::{
 use rustc_ast::Mutability;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_index::{Idx, IndexVec};
-use rustc_middle::mir;
+use rustc_middle::{mir, ty::Ty};
 use rustc_span::Span;
 use rustc_target::abi::{Align, HasDataLayout, Size};
 
@@ -200,18 +200,38 @@ enum AtomicAccessType {
     Rmw,
 }
 
-/// Type of write operation: allocating memory
-/// non-atomic writes and deallocating memory
-/// are all treated as writes for the purpose
-/// of the data-race detector.
+/// Type of a non-atomic read operation.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
-enum NaWriteType {
+pub enum NaReadType {
+    /// Standard unsynchronized write.
+    Read,
+
+    // An implicit read generated by a retag.
+    Retag,
+}
+
+impl NaReadType {
+    fn description(self) -> &'static str {
+        match self {
+            NaReadType::Read => "non-atomic read",
+            NaReadType::Retag => "retag read",
+        }
+    }
+}
+
+/// Type of a non-atomic write operation: allocating memory, non-atomic writes, and
+/// deallocating memory are all treated as writes for the purpose of the data-race detector.
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+pub enum NaWriteType {
     /// Allocate memory.
     Allocate,
 
     /// Standard unsynchronized write.
     Write,
 
+    // An implicit write generated by a retag.
+    Retag,
+
     /// Deallocate memory.
     /// Note that when memory is deallocated first, later non-atomic accesses
     /// will be reported as use-after-free, not as data races.
@@ -224,44 +244,64 @@ impl NaWriteType {
         match self {
             NaWriteType::Allocate => "creating a new allocation",
             NaWriteType::Write => "non-atomic write",
+            NaWriteType::Retag => "retag write",
             NaWriteType::Deallocate => "deallocation",
         }
     }
 }
 
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 enum AccessType {
-    NaRead,
+    NaRead(NaReadType),
     NaWrite(NaWriteType),
     AtomicLoad,
     AtomicStore,
     AtomicRmw,
 }
 
 impl AccessType {
-    fn description(self) -> &'static str {
-        match self {
-            AccessType::NaRead => "non-atomic read",
+    fn description(self, ty: Option<Ty<'_>>, size: Option<Size>) -> String {
+        let mut msg = String::new();
+
+        if let Some(size) = size {
+            msg.push_str(&format!("{}-byte {}", size.bytes(), msg))
+        }
+
+        msg.push_str(match self {
+            AccessType::NaRead(w) => w.description(),
             AccessType::NaWrite(w) => w.description(),
             AccessType::AtomicLoad => "atomic load",
             AccessType::AtomicStore => "atomic store",
             AccessType::AtomicRmw => "atomic read-modify-write",
+        });
+
+        if let Some(ty) = ty {
+            msg.push_str(&format!(" of type `{}`", ty));
         }
+
+        msg
     }
 
     fn is_atomic(self) -> bool {
         match self {
             AccessType::AtomicLoad | AccessType::AtomicStore | AccessType::AtomicRmw => true,
-            AccessType::NaRead | AccessType::NaWrite(_) => false,
+            AccessType::NaRead(_) | AccessType::NaWrite(_) => false,
         }
     }
 
     fn is_read(self) -> bool {
         match self {
-            AccessType::AtomicLoad | AccessType::NaRead => true,
+            AccessType::AtomicLoad | AccessType::NaRead(_) => true,
             AccessType::NaWrite(_) | AccessType::AtomicStore | AccessType::AtomicRmw => false,
         }
     }
+
+    fn is_retag(self) -> bool {
+        matches!(
+            self,
+            AccessType::NaRead(NaReadType::Retag) | AccessType::NaWrite(NaWriteType::Retag)
+        )
+    }
 }
 
 /// Memory Cell vector clock metadata
@@ -502,12 +542,14 @@ impl MemoryCellClocks {
         &mut self,
         thread_clocks: &mut ThreadClockSet,
         index: VectorIdx,
+        read_type: NaReadType,
         current_span: Span,
     ) -> Result<(), DataRace> {
         trace!("Unsynchronized read with vectors: {:#?} :: {:#?}", self, thread_clocks);
         if !current_span.is_dummy() {
             thread_clocks.clock[index].span = current_span;
         }
+        thread_clocks.clock[index].set_read_type(read_type);
         if self.write_was_before(&thread_clocks.clock) {
             let race_free = if let Some(atomic) = self.atomic() {
                 // We must be ordered-after all atomic accesses, reads and writes.
@@ -875,7 +917,8 @@ impl VClockAlloc {
     /// This finds the two racing threads and the type
     /// of data-race that occurred. This will also
     /// return info about the memory location the data-race
-    /// occurred in.
+    /// occurred in. The `ty` parameter is used for diagnostics, letting
+    /// the user know which type was involved in the access.
     #[cold]
     #[inline(never)]
     fn report_data_race<'tcx>(
@@ -885,6 +928,7 @@ impl VClockAlloc {
         access: AccessType,
         access_size: Size,
         ptr_dbg: Pointer<AllocId>,
+        ty: Option<Ty<'_>>,
     ) -> InterpResult<'tcx> {
         let (current_index, current_clocks) = global.current_thread_state(thread_mgr);
         let mut other_size = None; // if `Some`, this was a size-mismatch race
@@ -908,7 +952,7 @@ impl VClockAlloc {
                 write_clock = mem_clocks.write();
                 (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) {
-                (AccessType::NaRead, idx, &mem_clocks.read)
+                (AccessType::NaRead(mem_clocks.read[idx].read_type()), idx, &mem_clocks.read)
             // Finally, mixed-size races.
             } 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
@@ -950,37 +994,33 @@ impl VClockAlloc {
         Err(err_machine_stop!(TerminationInfo::DataRace {
             involves_non_atomic,
             extra,
+            retag_explain: access.is_retag() || other_access.is_retag(),
             ptr: ptr_dbg,
             op1: RacingOp {
-                action: if let Some(other_size) = other_size {
-                    format!("{}-byte {}", other_size.bytes(), other_access.description())
-                } else {
-                    other_access.description().to_owned()
-                },
+                action: other_access.description(None, other_size),
                 thread_info: other_thread_info,
                 span: other_clock.as_slice()[other_thread.index()].span_data(),
             },
             op2: RacingOp {
-                action: if other_size.is_some() {
-                    format!("{}-byte {}", access_size.bytes(), access.description())
-                } else {
-                    access.description().to_owned()
-                },
+                action: access.description(ty, other_size.map(|_| access_size)),
                 thread_info: current_thread_info,
                 span: current_clocks.clock.as_slice()[current_index.index()].span_data(),
             },
         }))?
     }
 
-    /// Detect data-races for an unsynchronized read operation, will not perform
+    /// Detect data-races for an unsynchronized read operation. It will not perform
     /// data-race detection if `race_detecting()` is false, either due to no threads
     /// being created or if it is temporarily disabled during a racy read or write
     /// operation for which data-race detection is handled separately, for example
-    /// atomic read operations.
+    /// atomic read operations. The `ty` parameter is used for diagnostics, letting
+    /// the user know which type was read.
     pub fn read<'tcx>(
         &self,
         alloc_id: AllocId,
         access_range: AllocRange,
+        read_type: NaReadType,
+        ty: Option<Ty<'_>>,
         machine: &MiriMachine<'_, '_>,
     ) -> InterpResult<'tcx> {
         let current_span = machine.current_span();
@@ -992,17 +1032,18 @@ impl VClockAlloc {
                 alloc_ranges.iter_mut(access_range.start, access_range.size)
             {
                 if let Err(DataRace) =
-                    mem_clocks.read_race_detect(&mut thread_clocks, index, current_span)
+                    mem_clocks.read_race_detect(&mut thread_clocks, index, read_type, current_span)
                 {
                     drop(thread_clocks);
                     // Report data-race.
                     return Self::report_data_race(
                         global,
                         &machine.threads,
                         mem_clocks,
-                        AccessType::NaRead,
+                        AccessType::NaRead(read_type),
                         access_range.size,
                         Pointer::new(alloc_id, Size::from_bytes(mem_clocks_range.start)),
+                        ty,
                     );
                 }
             }
@@ -1012,12 +1053,17 @@ impl VClockAlloc {
         }
     }
 
-    // Shared code for detecting data-races on unique access to a section of memory
-    fn unique_access<'tcx>(
+    /// Detect data-races for an unsynchronized write operation. It will not perform
+    /// data-race detection if `race_detecting()` is false, either due to no threads
+    /// being created or if it is temporarily disabled during a racy read or write
+    /// operation. The `ty` parameter is used for diagnostics, letting
+    /// the user know which type was written.
+    pub fn write<'tcx>(
         &mut self,
         alloc_id: AllocId,
         access_range: AllocRange,
         write_type: NaWriteType,
+        ty: Option<Ty<'_>>,
         machine: &mut MiriMachine<'_, '_>,
     ) -> InterpResult<'tcx> {
         let current_span = machine.current_span();
@@ -1042,6 +1088,7 @@ impl VClockAlloc {
                         AccessType::NaWrite(write_type),
                         access_range.size,
                         Pointer::new(alloc_id, Size::from_bytes(mem_clocks_range.start)),
+                        ty,
                     );
                 }
             }
@@ -1050,37 +1097,6 @@ impl VClockAlloc {
             Ok(())
         }
     }
-
-    /// Detect data-races for an unsynchronized write operation, will not perform
-    /// data-race threads if `race_detecting()` is false, either due to no threads
-    /// being created or if it is temporarily disabled during a racy read or write
-    /// operation
-    pub fn write<'tcx>(
-        &mut self,
-        alloc_id: AllocId,
-        range: AllocRange,
-        machine: &mut MiriMachine<'_, '_>,
-    ) -> InterpResult<'tcx> {
-        self.unique_access(alloc_id, range, NaWriteType::Write, machine)
-    }
-
-    /// Detect data-races for an unsynchronized deallocate operation, will not perform
-    /// data-race threads if `race_detecting()` is false, either due to no threads
-    /// being created or if it is temporarily disabled during a racy read or write
-    /// operation
-    pub fn deallocate<'tcx>(
-        &mut self,
-        alloc_id: AllocId,
-        size: Size,
-        machine: &mut MiriMachine<'_, '_>,
-    ) -> InterpResult<'tcx> {
-        self.unique_access(
-            alloc_id,
-            alloc_range(Size::ZERO, size),
-            NaWriteType::Deallocate,
-            machine,
-        )
-    }
 }
 
 impl<'mir, 'tcx: 'mir> EvalContextPrivExt<'mir, 'tcx> for MiriInterpCx<'mir, 'tcx> {}
@@ -1279,7 +1295,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
                 let alloc_meta = this.get_alloc_extra(alloc_id)?.data_race.as_ref().unwrap();
                 trace!(
                     "Atomic op({}) with ordering {:?} on {:?} (size={})",
-                    access.description(),
+                    access.description(None, None),
                     &atomic,
                     place.ptr(),
                     size.bytes()
@@ -1307,6 +1323,7 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriInterpCxExt<'mir, 'tcx> {
                                         alloc_id,
                                         Size::from_bytes(mem_clocks_range.start),
                                     ),
+                                    None,
                                 )
                                 .map(|_| true);
                             }