diff --git a/ci/san.sh b/ci/san.sh index d1d634209..f5a17d286 100755 --- a/ci/san.sh +++ b/ci/san.sh @@ -34,6 +34,5 @@ RUSTFLAGS="${RUSTFLAGS:-} -Z sanitizer=memory" \ # TODO: Use x86_64-unknown-linux-gnutsan once https://github.com/rust-lang/rust/pull/152757 merged # Run thread sanitizer cargo clean -TSAN_OPTIONS="${TSAN_OPTIONS:-} suppressions=$(pwd)/ci/tsan" \ RUSTFLAGS="${RUSTFLAGS:-} -Z sanitizer=thread" \ cargo test -Z build-std --all --all-features --release --target x86_64-unknown-linux-gnu --tests --exclude benchmarks -- --test-threads=1 diff --git a/ci/tsan b/ci/tsan deleted file mode 100644 index 67fad001a..000000000 --- a/ci/tsan +++ /dev/null @@ -1,10 +0,0 @@ -# TSAN suppressions file for crossbeam - -# Push and steal operations in crossbeam-deque may cause data races, but such -# data races are safe. If a data race happens, the value read by `steal` is -# forgotten and the steal operation is then retried. -race:crossbeam_deque*push -race:crossbeam_deque*steal - -# Non-lock-free AtomicCell uses SeqLock which uses fences. -race:crossbeam_utils::atomic::atomic_cell::atomic_compare_exchange_weak diff --git a/crossbeam-deque/Cargo.toml b/crossbeam-deque/Cargo.toml index 9c1f4ac43..f8aad1416 100644 --- a/crossbeam-deque/Cargo.toml +++ b/crossbeam-deque/Cargo.toml @@ -35,6 +35,7 @@ std = ["crossbeam-epoch/std", "crossbeam-utils/std"] [dependencies] crossbeam-epoch = { version = "0.9.17", path = "../crossbeam-epoch", default-features = false } crossbeam-utils = { version = "0.8.18", path = "../crossbeam-utils", default-features = false } +atomic-maybe-uninit = { version = "0.3.15", git = "https://github.com/taiki-e/atomic-maybe-uninit", branch = "memcpy" } [dev-dependencies] fastrand = "2" diff --git a/crossbeam-deque/build.rs b/crossbeam-deque/build.rs index 58e568f26..482588127 100644 --- a/crossbeam-deque/build.rs +++ b/crossbeam-deque/build.rs @@ -4,11 +4,14 @@ use std::env; fn main() { println!("cargo:rerun-if-changed=build.rs"); - println!("cargo:rustc-check-cfg=cfg(crossbeam_sanitize_thread)"); + println!("cargo:rustc-check-cfg=cfg(crossbeam_sanitize_thread,crossbeam_sanitize_any)"); // `cfg(sanitize = "..")` is not stabilized. let sanitize = env::var("CARGO_CFG_SANITIZE").unwrap_or_default(); - if sanitize.contains("thread") { - println!("cargo:rustc-cfg=crossbeam_sanitize_thread"); + if !sanitize.is_empty() { + println!("cargo:rustc-cfg=crossbeam_sanitize_any"); + if sanitize.contains("thread") { + println!("cargo:rustc-cfg=crossbeam_sanitize_thread"); + } } } diff --git a/crossbeam-deque/src/atomic_memcpy.rs b/crossbeam-deque/src/atomic_memcpy.rs new file mode 120000 index 000000000..0dcc56fb2 --- /dev/null +++ b/crossbeam-deque/src/atomic_memcpy.rs @@ -0,0 +1 @@ +../../crossbeam-utils/src/atomic/atomic_memcpy.rs \ No newline at end of file diff --git a/crossbeam-deque/src/deque.rs b/crossbeam-deque/src/deque.rs index ba9dbc3a8..f05118cae 100644 --- a/crossbeam-deque/src/deque.rs +++ b/crossbeam-deque/src/deque.rs @@ -9,10 +9,11 @@ use core::{ sync::atomic::{self, AtomicIsize, AtomicPtr, AtomicUsize, Ordering}, }; +use atomic_maybe_uninit::PerByteAtomicMaybeUninit; use crossbeam_epoch::{self as epoch, Atomic, Owned}; use crossbeam_utils::{Backoff, CachePadded}; -use crate::alloc_helper::Global; +use crate::{alloc_helper::Global, atomic_memcpy}; // Minimum buffer capacity. const MIN_CAP: usize = 64; @@ -43,7 +44,7 @@ impl Buffer { let ptr = Box::into_raw( (0..cap) - .map(|_| MaybeUninit::::uninit()) + .map(|_| PerByteAtomicMaybeUninit::new(MaybeUninit::::uninit())) .collect::>(), ) .cast::(); @@ -70,23 +71,13 @@ impl Buffer { } /// Writes `task` into the specified `index`. - /// - /// This method might be concurrently called with another `read` at the same index, which is - /// technically speaking a data race and therefore UB. We should use an atomic store here, but - /// that would be more expensive and difficult to implement generically for all types `T`. - /// Hence, as a hack, we use a volatile write instead. unsafe fn write(&self, index: isize, task: MaybeUninit) { - unsafe { ptr::write_volatile(self.at(index).cast::>(), task) } + unsafe { atomic_memcpy::store(self.at(index), task) } } /// Reads a task from the specified `index`. - /// - /// This method might be concurrently called with another `write` at the same index, which is - /// technically speaking a data race and therefore UB. We should use an atomic load here, but - /// that would be more expensive and difficult to implement generically for all types `T`. - /// Hence, as a hack, we use a volatile load instead. unsafe fn read(&self, index: isize) -> MaybeUninit { - unsafe { ptr::read_volatile(self.at(index).cast::>()) } + unsafe { atomic_memcpy::load(self.at(index)) } } } diff --git a/crossbeam-deque/src/lib.rs b/crossbeam-deque/src/lib.rs index 0585ca35a..ecf6e17bd 100644 --- a/crossbeam-deque/src/lib.rs +++ b/crossbeam-deque/src/lib.rs @@ -102,6 +102,8 @@ extern crate std; #[cfg(feature = "std")] mod alloc_helper; +#[cfg(feature = "std")] +mod atomic_memcpy; #[cfg(feature = "std")] mod deque; diff --git a/crossbeam-utils/Cargo.toml b/crossbeam-utils/Cargo.toml index 607841c6e..2acf81a00 100644 --- a/crossbeam-utils/Cargo.toml +++ b/crossbeam-utils/Cargo.toml @@ -36,7 +36,7 @@ std = [] atomic = ["atomic-maybe-uninit"] [dependencies] -atomic-maybe-uninit = { version = "0.3.4", optional = true } +atomic-maybe-uninit = { version = "0.3.15", optional = true, git = "https://github.com/taiki-e/atomic-maybe-uninit", branch = "memcpy" } # Enable the use of loom for concurrency testing. # diff --git a/crossbeam-utils/build.rs b/crossbeam-utils/build.rs index 1ce6735dd..737d7bdc4 100644 --- a/crossbeam-utils/build.rs +++ b/crossbeam-utils/build.rs @@ -18,7 +18,7 @@ include!("build-common.rs"); fn main() { println!("cargo:rerun-if-changed=no_atomic.rs"); println!( - "cargo:rustc-check-cfg=cfg(crossbeam_no_atomic,crossbeam_sanitize_thread,crossbeam_atomic_cell_force_fallback)" + "cargo:rustc-check-cfg=cfg(crossbeam_no_atomic,crossbeam_sanitize_thread,crossbeam_sanitize_any,crossbeam_atomic_cell_force_fallback)" ); let target = match env::var("TARGET") { @@ -41,10 +41,12 @@ fn main() { } // `cfg(sanitize = "..")` is not stabilized. - if let Ok(sanitize) = env::var("CARGO_CFG_SANITIZE") { + let sanitize = env::var("CARGO_CFG_SANITIZE").unwrap_or_default(); + if !sanitize.is_empty() { + println!("cargo:rustc-cfg=crossbeam_sanitize_any"); + println!("cargo:rustc-cfg=crossbeam_atomic_cell_force_fallback"); if sanitize.contains("thread") { println!("cargo:rustc-cfg=crossbeam_sanitize_thread"); } - println!("cargo:rustc-cfg=crossbeam_atomic_cell_force_fallback"); } } diff --git a/crossbeam-utils/src/atomic/atomic_cell.rs b/crossbeam-utils/src/atomic/atomic_cell.rs index 02d37a309..4baa7cbee 100644 --- a/crossbeam-utils/src/atomic/atomic_cell.rs +++ b/crossbeam-utils/src/atomic/atomic_cell.rs @@ -9,7 +9,10 @@ use core::{ ptr, }; -use super::seq_lock::SeqLock; +use super::{ + atomic_memcpy::{self, store as write}, + seq_lock::SeqLock, +}; use crate::{ CachePadded, primitive::sync::atomic::{self, Ordering}, @@ -407,10 +410,10 @@ macro_rules! impl_arithmetic { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value = value.wrapping_add(val); + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(old.wrapping_add(val))) } old } } @@ -446,10 +449,10 @@ macro_rules! impl_arithmetic { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value = value.wrapping_sub(val); + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(old.wrapping_sub(val))) } old } } @@ -483,10 +486,10 @@ macro_rules! impl_arithmetic { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value &= val; + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(old & val)) } old } } @@ -520,10 +523,10 @@ macro_rules! impl_arithmetic { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value = !(old & val); + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(!(old & val))) } old } } @@ -557,10 +560,10 @@ macro_rules! impl_arithmetic { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value |= val; + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(old | val)) } old } } @@ -594,10 +597,10 @@ macro_rules! impl_arithmetic { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value ^= val; + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(old ^ val)) } old } } @@ -632,10 +635,10 @@ macro_rules! impl_arithmetic { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value = cmp::max(old, val); + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(cmp::max(old, val))) } old } } @@ -670,10 +673,10 @@ macro_rules! impl_arithmetic { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value = cmp::min(old, val); + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(cmp::min(old, val))) } old } } @@ -792,10 +795,10 @@ impl AtomicCell { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value &= val; + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(old & val)) } old } } @@ -835,10 +838,10 @@ impl AtomicCell { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value = !(old & val); + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(!(old & val))) } old } } @@ -875,10 +878,10 @@ impl AtomicCell { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value |= val; + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(old | val)) } old } } @@ -915,10 +918,10 @@ impl AtomicCell { } }, { - let _guard = lock(self.as_ptr() as usize).write(); - let value = unsafe { &mut *(self.as_ptr()) }; - let old = *value; - *value ^= val; + let dst = self.as_ptr(); + let _guard = lock(dst as usize).write(); + let old = unsafe { ptr::read(dst) }; + unsafe { write(dst, MaybeUninit::new(old ^ val)) } old } } @@ -1045,13 +1048,7 @@ where // Try doing an optimistic read first. if let Some(stamp) = lock.optimistic_read() { - // We need a volatile read here because other threads might concurrently modify the - // value. In theory, data races are *always* UB, even if we use volatile reads and - // discard the data when a data race is detected. The proper solution would be to - // do atomic reads and atomic writes, but we can't atomically read and write all - // kinds of data since `AtomicU8` is not available on stable Rust yet. - // Load as `MaybeUninit` because we may load a value that is not valid as `T`. - let val = unsafe { ptr::read_volatile(src.cast::>()) }; + let val = unsafe { atomic_memcpy::load(src) }; if lock.validate_read(stamp) { return unsafe { val.assume_init() }; @@ -1082,7 +1079,7 @@ unsafe fn atomic_store(dst: *mut T, val: T) { }, { let _guard = lock(dst as usize).write(); - unsafe { ptr::write(dst, val) } + unsafe { write(dst, MaybeUninit::new(val)) } } } } @@ -1102,7 +1099,9 @@ unsafe fn atomic_swap(dst: *mut T, val: T) -> T { }, { let _guard = lock(dst as usize).write(); - unsafe { ptr::replace(dst, val) } + let old = unsafe { ptr::read(dst.cast::>()) }; + unsafe { write(dst, MaybeUninit::new(val)) } + unsafe { old.assume_init() } } } } @@ -1156,7 +1155,7 @@ where let old = unsafe { ptr::read(dst) }; if T::eq(&old, ¤t) { - unsafe { ptr::write(dst, new) } + unsafe { write(dst, MaybeUninit::new(new)) } Ok(old) } else { // The value hasn't been changed. Drop the guard without incrementing the stamp. diff --git a/crossbeam-utils/src/atomic/atomic_memcpy.rs b/crossbeam-utils/src/atomic/atomic_memcpy.rs new file mode 100644 index 000000000..2fb3e6af4 --- /dev/null +++ b/crossbeam-utils/src/atomic/atomic_memcpy.rs @@ -0,0 +1,93 @@ +use core::mem::MaybeUninit; +#[cfg(crossbeam_sanitize_thread)] +use core::{ + mem, slice, + sync::atomic::{AtomicU8, Ordering}, +}; + +pub(crate) unsafe fn load(src: *const T) -> MaybeUninit { + atomic_maybe_uninit::cfg_has_atomic_memcpy! { + use core::sync::atomic::Ordering; + use atomic_maybe_uninit::PerByteAtomicMaybeUninit; + if cfg!(not(any(miri, crossbeam_sanitize_any))) { + unsafe { + (*src.cast::>()).load(Ordering::Relaxed) + } + } else { + unsafe { load_fallback(src) } + } + } + atomic_maybe_uninit::cfg_no_atomic_memcpy! { + unsafe { load_fallback(src) } + } +} +unsafe fn load_fallback(src: *const T) -> MaybeUninit { + #[cfg(not(crossbeam_sanitize_thread))] + // We need a volatile read here because other threads might concurrently modify the + // value. In theory, data races are *always* UB, even if we use volatile reads and + // discard the data when a data race is detected. The proper solution would be to + // do atomic reads and atomic writes like the code above, but inline assembly is + // not stable on some tier 2/3 targets and unsupported in Miri/Sanitizer. Hence, + // as a hack, we use a volatile write instead. + // See also https://github.com/rust-lang/rfcs/pull/3301. + // Load as `MaybeUninit` because we may load a value that is not valid as `T`. + unsafe { + src.cast::>().read_volatile() + } + #[cfg(crossbeam_sanitize_thread)] + // TODO: comment + unsafe { + let mut out: MaybeUninit = MaybeUninit::uninit(); + for (src, out) in slice::from_raw_parts(src.cast::(), mem::size_of::()) + .iter() + .zip(slice::from_raw_parts_mut( + out.as_mut_ptr().cast::>(), + mem::size_of::(), + )) + { + *out = mem::transmute::>(src.load(Ordering::Relaxed)); + } + out + } +} + +pub(crate) unsafe fn store(dst: *mut T, val: MaybeUninit) { + atomic_maybe_uninit::cfg_has_atomic_memcpy! { + use core::sync::atomic::Ordering; + use atomic_maybe_uninit::PerByteAtomicMaybeUninit; + if cfg!(not(any(miri, crossbeam_sanitize_any))) { + unsafe { + (*dst.cast::>()).store(val, Ordering::Relaxed); + } + return; + } + } + unsafe { store_fallback(dst, val) } +} +unsafe fn store_fallback(dst: *mut T, val: MaybeUninit) { + #[cfg(not(crossbeam_sanitize_thread))] + // This method might be concurrently called with another `read` at the same index, which is + // technically speaking a data race and therefore UB. We should use an atomic store here + // like the code above, but inline assembly is not stable on some tier 2/3 targets and + // unsupported in Miri/Sanitizer. Hence, as a hack, we use a volatile write instead. + // See also https://github.com/rust-lang/rfcs/pull/3301. + unsafe { + dst.cast::>().write_volatile(val); + } + #[cfg(crossbeam_sanitize_thread)] + // TODO: comment + unsafe { + for (dst, val) in slice::from_raw_parts(dst.cast::(), mem::size_of::()) + .iter() + .zip(slice::from_raw_parts( + val.as_ptr().cast::>(), + mem::size_of::(), + )) + { + dst.store( + mem::transmute::, u8>(*val), + Ordering::Relaxed, + ); + } + } +} diff --git a/crossbeam-utils/src/atomic/mod.rs b/crossbeam-utils/src/atomic/mod.rs index ee72b6537..353e231da 100644 --- a/crossbeam-utils/src/atomic/mod.rs +++ b/crossbeam-utils/src/atomic/mod.rs @@ -26,6 +26,9 @@ mod seq_lock; mod atomic_cell; #[cfg(target_has_atomic = "ptr")] #[cfg(not(crossbeam_loom))] +mod atomic_memcpy; +#[cfg(target_has_atomic = "ptr")] +#[cfg(not(crossbeam_loom))] pub use self::atomic_cell::AtomicCell; mod consume;