Fix review changes

Author: JCTyblaidd

src/data_race.rs

@@ -9,6 +9,9 @@
 //! Relaxed stores now unconditionally block all currently active release sequences and so per-thread tracking of release
 //! sequences is not needed.
 //!
+//! The implementation also models races with memory allocation and deallocation via treating allocation and
+//! deallocation as a type of write internally for detecting data-races.
+//!
 //! This does not explore weak memory orders and so can still miss data-races
 //! but should not report false-positives
 //!
@@ -192,13 +195,22 @@ struct AtomicMemoryCellClocks {
     sync_vector: VClock,
 }
 
+/// 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.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 enum WriteType {
     /// Allocate memory.
     Allocate,
+
     /// Standard unsynchronized write.
     Write,
-    /// Deallocate memory
+
+    /// Deallocate memory.
+    /// Some races with deallocation will be missed and instead
+    /// reported as invalid accesses of freed memory due to
+    /// the order of checks.
     Deallocate,
 }
 impl WriteType {
@@ -681,7 +693,7 @@ impl VClockAlloc {
             let (alloc_index, clocks) = global.current_thread_state();
             let alloc_timestamp = clocks.clock[alloc_index];
             (alloc_timestamp, alloc_index)
-        }else{
+        } else {
             (0, VectorIdx::MAX_INDEX)
         };
         VClockAlloc {
@@ -695,7 +707,7 @@ impl VClockAlloc {
     // Find an index, if one exists where the value
     // in `l` is greater than the value in `r`.
     fn find_gt_index(l: &VClock, r: &VClock) -> Option<VectorIdx> {
-        log::info!("Find index where not {:?} <= {:?}", l, r);
+        log::trace!("Find index where not {:?} <= {:?}", l, r);
         let l_slice = l.as_slice();
         let r_slice = r.as_slice();
         l_slice
@@ -1168,7 +1180,7 @@ impl GlobalState {
             vector_info.push(thread)
         };
 
-        log::info!("Creating thread = {:?} with vector index = {:?}", thread, created_index);
+        log::trace!("Creating thread = {:?} with vector index = {:?}", thread, created_index);
 
         // Mark the chosen vector index as in use by the thread.
         thread_info[thread].vector_index = Some(created_index);

src/machine.rs

@@ -480,7 +480,10 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
         let race_alloc = if let Some(data_race) = &memory_extra.data_race {
             match kind {
                 // V-Table generation is lazy and so racy, so do not track races.
-                // Also V-Tables are read only so no data races can be detected.
+                // Also V-Tables are read only so no data races can be occur.
+                // Must be disabled since V-Tables are initialized via interpreter
+                // writes on demand and can incorrectly cause the data-race detector
+                // to trigger.
                 MemoryKind::Vtable => None,
                 // User allocated and stack memory should track allocation.
                 MemoryKind::Machine(

tests/compile-fail/data_race/alloc_read_race.rs

@@ -3,6 +3,7 @@
 use std::thread::spawn;
 use std::ptr::null_mut;
 use std::sync::atomic::{Ordering, AtomicPtr};
+use std::mem::MaybeUninit;
 
 #[derive(Copy, Clone)]
 struct EvilSend<T>(pub T);
@@ -12,8 +13,8 @@ unsafe impl<T> Sync for EvilSend<T> {}
 
 pub fn main() {
     // Shared atomic pointer
-    let pointer = AtomicPtr::new(null_mut::<usize>());
-    let ptr = EvilSend(&pointer as *const AtomicPtr<usize>);
+    let pointer = AtomicPtr::new(null_mut::<MaybeUninit<usize>>());
+    let ptr = EvilSend(&pointer as *const AtomicPtr<MaybeUninit<usize>>);
 
     // Note: this is scheduler-dependent
     // the operations need to occur in
@@ -28,14 +29,14 @@ pub fn main() {
             // Uses relaxed semantics to not generate
             // a release sequence.
             let pointer = &*ptr.0;
-            pointer.store(Box::into_raw(Box::new(0usize)), Ordering::Relaxed);
+            pointer.store(Box::into_raw(Box::new(MaybeUninit::uninit())), Ordering::Relaxed);
         });
 
         let j2 = spawn(move || {
             let pointer = &*ptr.0;
 
-            //Note detects with write due to the initialization of memory
-            *pointer.load(Ordering::Relaxed) //~ ERROR Data race detected between Read on Thread(id = 2) and Write on Thread(id = 1)
+            // 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 Read on Thread(id = 2) and Allocate on Thread(id = 1)
         });
 
         j1.join().unwrap();

tests/compile-fail/data_race/dangling_thread_async_race.rs

@@ -26,7 +26,7 @@ fn main() {
 
     // Detach the thread and sleep until it terminates
     mem::drop(join);
-    sleep(Duration::from_millis(1000));
+    sleep(Duration::from_millis(200));
 
     // Spawn and immediately join a thread
     // to execute the join code-path

tests/compile-fail/data_race/dangling_thread_race.rs

@@ -24,9 +24,9 @@ fn main() {
         })
     };
 
-    // Detatch the thread and sleep until it terminates
+    // Detach the thread and sleep until it terminates
     mem::drop(join);
-    sleep(Duration::from_millis(1000));
+    sleep(Duration::from_millis(200));
 
     // Spawn and immediately join a thread
     // to execute the join code-path

tests/compile-fail/data_race/dealloc_read_race.rs renamed to tests/compile-fail/data_race/dealloc_read_race1.rs

@@ -0,0 +1,34 @@
+// ignore-windows: Concurrency on Windows is not supported yet.
+
+use std::thread::spawn;
+
+#[derive(Copy, Clone)]
+struct EvilSend<T>(pub T);
+
+unsafe impl<T> Send for EvilSend<T> {}
+unsafe impl<T> Sync for EvilSend<T> {}
+
+extern "Rust" {
+    fn __rust_dealloc(ptr: *mut u8, size: usize, align: usize);
+}
+
+pub fn main() {
+    // Shared atomic pointer
+    let pointer: *mut usize = Box::into_raw(Box::new(0usize));
+    let ptr = EvilSend(pointer);
+
+    unsafe {
+        let j1 = spawn(move || {
+            __rust_dealloc(ptr.0 as *mut _, std::mem::size_of::<usize>(), std::mem::align_of::<usize>())
+        });
+
+        let j2 = spawn(move || {
+            // Also an error of the form: Data race detected between Read on Thread(id = 2) and Deallocate on Thread(id = 1) 
+            // but the invalid allocation is detected first.
+            *ptr.0 //~ ERROR dereferenced after this allocation got freed
+        });
+
+        j1.join().unwrap();
+        j2.join().unwrap();
+    }
+}

tests/compile-fail/data_race/dealloc_read_race2.rs

@@ -36,7 +36,7 @@ pub fn main() {
 
                 pointer.store(&mut stack_var as *mut _, Ordering::Release);
 
-                sleep(Duration::from_millis(1000));
+                sleep(Duration::from_millis(200));
 
             } //~ ERROR Data race detected between Deallocate on Thread(id = 1) and Read on Thread(id = 2)
         });

tests/compile-fail/data_race/dealloc_read_race_stack.rs

@@ -36,7 +36,7 @@ pub fn main() {
 
             pointer.store(&mut stack_var as *mut _, Ordering::Release);
 
-            sleep(Duration::from_millis(1000));
+            sleep(Duration::from_millis(200));
 
             drop(stack_var);
         });  //~ ERROR Data race detected between Deallocate on Thread(id = 1) and Read on Thread(id = 2)