From 2250e161bc71731df5687f67dbfed29787ada28b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Tue, 19 Mar 2024 14:30:19 +0100 Subject: [PATCH] Address comments --- src/concurrency/data_race.rs | 24 +++++++------- src/concurrency/vector_clock.rs | 31 +++++++++++++------ src/lib.rs | 1 + .../retag_data_race_protected_read.rs | 2 +- .../retag_data_race_protected_read.stderr | 4 +-- 5 files changed, 39 insertions(+), 23 deletions(-) diff --git a/src/concurrency/data_race.rs b/src/concurrency/data_race.rs index 56c2d18c32..faf6284c04 100644 --- a/src/concurrency/data_race.rs +++ b/src/concurrency/data_race.rs @@ -200,15 +200,13 @@ 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 read operation #[derive(Copy, Clone, PartialEq, Eq, Debug)] pub enum NaReadType { /// Standard unsynchronized write. Read, + // An implicit read generated by a retag. Retag, } @@ -233,6 +231,7 @@ pub enum NaWriteType { /// Standard unsynchronized write. Write, + // An implicit write generated by a retag. Retag, /// Deallocate memory. @@ -279,7 +278,7 @@ impl AccessType { }); if let Some(ty) = ty { - msg.push_str(&format!(" with type `{}`", ty)); + msg.push_str(&format!(" of type `{}`", ty)); } msg @@ -920,7 +919,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>( @@ -1011,11 +1011,12 @@ impl VClockAlloc { }))? } - /// 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, @@ -1054,10 +1055,11 @@ impl VClockAlloc { } } - /// Detect data-races for an unsynchronized write operation, will not perform - /// data-race threads if `race_detecting()` is false, either due to no threads + /// 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 + /// 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, diff --git a/src/concurrency/vector_clock.rs b/src/concurrency/vector_clock.rs index 302ca5eb5e..cd7f6cf8ce 100644 --- a/src/concurrency/vector_clock.rs +++ b/src/concurrency/vector_clock.rs @@ -52,13 +52,25 @@ const SMALL_VECTOR: usize = 4; /// so that diagnostics can report what code was responsible for an operation. #[derive(Clone, Copy, Debug)] pub struct VTimestamp { - // The lowest bit indicates read type, the rest is the time. + /// The lowest bit indicates read type, the rest is the time. + /// `1` indicates a retag read, `0` a regular read. time_and_read_type: u32, pub span: Span, } impl VTimestamp { - pub const ZERO: VTimestamp = VTimestamp { time_and_read_type: 0, span: DUMMY_SP }; + pub const ZERO: VTimestamp = VTimestamp::new(0, NaReadType::Read, DUMMY_SP); + + const fn new(time: u32, read_type: NaReadType, span: Span) -> Self { + let read_type = match read_type { + NaReadType::Read => 0, + NaReadType::Retag => 1, + }; + Self { + time_and_read_type: read_type | time.checked_mul(2).expect("Vector clock overflow"), + span, + } + } fn time(&self) -> u32 { self.time_and_read_type.shr(1) @@ -66,11 +78,7 @@ impl VTimestamp { fn set_time(&mut self, time: u32) { self.time_and_read_type = - (self.time_and_read_type & 1) | time.checked_shl(1).expect("Vector clock overflow") - } - - pub fn span_data(&self) -> SpanData { - self.span.data() + (self.time_and_read_type & 1) | time.checked_mul(2).expect("Vector clock overflow") } pub fn read_type(&self) -> NaReadType { @@ -80,6 +88,10 @@ impl VTimestamp { pub fn set_read_type(&mut self, read_type: NaReadType) { self.time_and_read_type = (self.time_and_read_type & !1) | read_type as u32 } + + pub fn span_data(&self) -> SpanData { + self.span.data() + } } impl PartialEq for VTimestamp { @@ -400,8 +412,9 @@ impl IndexMut for VClock { #[cfg(test)] mod tests { use super::{VClock, VTimestamp, VectorIdx}; + use crate::concurrency::data_race::NaReadType; use rustc_span::DUMMY_SP; - use std::{cmp::Ordering, ops::Shl}; + use std::cmp::Ordering; #[test] fn test_equal() { @@ -471,7 +484,7 @@ mod tests { slice .iter() .copied() - .map(|time| VTimestamp { time_and_read_type: time.shl(1), span: DUMMY_SP }) + .map(|time| VTimestamp::new(time, NaReadType::Read, DUMMY_SP)) .collect(), ) } diff --git a/src/lib.rs b/src/lib.rs index 416d0cda8f..7821aa9efd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,5 +1,6 @@ #![feature(rustc_private)] #![feature(cell_update)] +#![feature(const_option)] #![feature(float_gamma)] #![feature(generic_nonzero)] #![feature(map_try_insert)] diff --git a/tests/fail/stacked_borrows/retag_data_race_protected_read.rs b/tests/fail/stacked_borrows/retag_data_race_protected_read.rs index 36adf28d1f..3de517055e 100644 --- a/tests/fail/stacked_borrows/retag_data_race_protected_read.rs +++ b/tests/fail/stacked_borrows/retag_data_race_protected_read.rs @@ -13,7 +13,7 @@ fn main() { let ptr = ptr; // We do a protected mutable retag (but no write!) in this thread. fn retag(_x: &mut i32) {} - retag(unsafe { &mut *ptr.0 }); //~ERROR: Data race detected between (1) non-atomic read on thread `main` and (2) retag write with type `i32` on thread `unnamed-1` + retag(unsafe { &mut *ptr.0 }); //~ERROR: Data race detected between (1) non-atomic read on thread `main` and (2) retag write of type `i32` on thread `unnamed-1` }); // We do a read in the main thread. diff --git a/tests/fail/stacked_borrows/retag_data_race_protected_read.stderr b/tests/fail/stacked_borrows/retag_data_race_protected_read.stderr index ae2a36ccc2..50c1112ce4 100644 --- a/tests/fail/stacked_borrows/retag_data_race_protected_read.stderr +++ b/tests/fail/stacked_borrows/retag_data_race_protected_read.stderr @@ -1,8 +1,8 @@ -error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `main` and (2) retag write with type `i32` on thread `unnamed-ID` at ALLOC. (2) just happened here +error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `main` and (2) retag write of type `i32` on thread `unnamed-ID` 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) non-atomic read on thread `main` and (2) retag write with type `i32` on thread `unnamed-ID` at ALLOC. (2) just happened here + | ^^^^^^^^^^^ Data race detected between (1) non-atomic read on thread `main` and (2) retag write of type `i32` on thread `unnamed-ID` at ALLOC. (2) just happened here | help: and (1) occurred earlier here --> $DIR/retag_data_race_protected_read.rs:LL:CC