From 449882c33621c1a4fc45584c4b3f76f61d0d757a Mon Sep 17 00:00:00 2001 From: Taiki Endo Date: Sat, 26 Feb 2022 16:03:56 +0900 Subject: [PATCH] Fix docs about uninitialized bytes --- README.md | 4 +-- src/lib.rs | 2 ++ tests/padding.rs | 66 -------------------------------------------- tests/uninit.rs | 71 ------------------------------------------------ 4 files changed, 4 insertions(+), 139 deletions(-) delete mode 100644 tests/padding.rs delete mode 100644 tests/uninit.rs diff --git a/README.md b/README.md index e1f08ca..4057adf 100644 --- a/README.md +++ b/README.md @@ -18,7 +18,7 @@ See [P1478R1][p1478r1] for more. - If the alignment of the type being copied is the same as the pointer width, `atomic_load` is possible to produce an assembly roughly equivalent to the case of using volatile read + atomic fence on many platforms. (e.g., [aarch64](https://github.com/taiki-e/atomic-memcpy/blob/HEAD/tests/asm-test/asm/aarch64-unknown-linux-gnu/atomic_memcpy_load_align8), [riscv64](https://github.com/taiki-e/atomic-memcpy/blob/main/tests/asm-test/asm/riscv64gc-unknown-linux-gnu/atomic_memcpy_load_align8). See [`tests/asm-test/asm`][asm-test] directory for more). - If the alignment of the type being copied is smaller than the pointer width, there will be some performance degradation. However, it is implemented in such a way that it does not cause extreme performance degradation at least on x86_64. (See [the implementation comments of `atomic_load`][implementation] for more.) It is possible that there is still room for improvement, especially on non-x86_64 platforms. - Optimization for the case where the alignment of the type being copied is larger than the pointer width has not yet been fully investigated. It is possible that there is still room for improvement, especially on 32-bit platforms where `AtomicU64` is available. -- If the type being copied contains uninitialized bytes (e.g., padding), it is incompatible with `-Zmiri-check-number-validity`. This will probably not be resolved until something like `AtomicMaybeUninit` is supported. **Note:** Due to [Miri does not track uninitialized bytes on a per byte basis for partially initialized scalars][rust-lang/rust#69488], Miri may report this case as an access to an uninitialized byte, regardless of whether the uninitialized byte is actually accessed or not. +- If the type being copied contains uninitialized bytes (e.g., padding) [it is undefined behavior because the copy goes through integers][undefined-behavior]. This problem will probably not be resolved until something like `AtomicMaybeUninit` is supported. ## Related Projects @@ -28,7 +28,7 @@ See [P1478R1][p1478r1] for more. [implementation]: https://github.com/taiki-e/atomic-memcpy/blob/570de7be73b3cb086741cc6cff80dea4c706349c/src/lib.rs#L339-L383 [p1478r1]: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1478r1.html [portable-atomic]: https://github.com/taiki-e/portable-atomic -[rust-lang/rust#69488]: https://github.com/rust-lang/rust/issues/69488 +[undefined-behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html ## License diff --git a/src/lib.rs b/src/lib.rs index f830548..130f8f2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -59,6 +59,7 @@ use core::sync::atomic::{self, Ordering}; /// - `src` must be valid for reads. /// - `src` must be properly aligned. /// - `src` must go through [`UnsafeCell::get`](core::cell::UnsafeCell::get). +/// - `T` must not contain uninitialized bytes. /// - There are no concurrent non-atomic write operations. /// - There are no concurrent atomic write operations of different /// granularity. The granularity of atomic operations is an implementation @@ -126,6 +127,7 @@ pub unsafe fn atomic_load(src: *const T, order: Ordering) -> core::mem::Maybe /// - `dst` must be [valid] for writes. /// - `dst` must be properly aligned. /// - `dst` must go through [`UnsafeCell::get`](core::cell::UnsafeCell::get). +/// - `T` must not contain uninitialized bytes. /// - There are no concurrent non-atomic operations. /// - There are no concurrent atomic operations of different /// granularity. The granularity of atomic operations is an implementation diff --git a/tests/padding.rs b/tests/padding.rs deleted file mode 100644 index 1df6127..0000000 --- a/tests/padding.rs +++ /dev/null @@ -1,66 +0,0 @@ -use std::{cell::UnsafeCell, mem, sync::atomic::Ordering}; - -use atomic_memcpy::{atomic_load, atomic_store}; - -#[test] -fn enum_padding() { - // Miri cannot track uninitialized bytes on a per byte basis for partially - // initialized scalars: https://github.com/rust-lang/rust/issues/69488 - // See also https://github.com/crossbeam-rs/crossbeam/issues/748#issuecomment-1022432401 - if cfg!(miri) { - return; - } - - #[allow(dead_code)] - #[repr(align(8))] - #[derive(Debug, Clone, Copy, PartialEq, Eq)] - enum Test { - Field(u32), - FieldLess, - FieldLess2, - } - - assert_eq!(mem::align_of::(), 8); - assert_eq!(mem::size_of::(), 8); - unsafe { - let x = UnsafeCell::new(Test::Field(0)); - assert_eq!(atomic_load(x.get(), Ordering::Relaxed).assume_init(), Test::Field(0)); - atomic_store(x.get(), Test::FieldLess, Ordering::Relaxed); - assert_eq!(atomic_load(x.get(), Ordering::Relaxed).assume_init(), Test::FieldLess); - assert_ne!(atomic_load(x.get(), Ordering::Relaxed).assume_init(), Test::FieldLess2); - atomic_store(x.get(), Test::FieldLess2, Ordering::Relaxed); - assert_ne!(atomic_load(x.get(), Ordering::Relaxed).assume_init(), Test::FieldLess); - assert_eq!(atomic_load(x.get(), Ordering::Relaxed).assume_init(), Test::FieldLess2); - atomic_store(x.get(), Test::Field(1), Ordering::Relaxed); - assert_eq!(atomic_load(x.get(), Ordering::Relaxed).assume_init(), Test::Field(1)); - } -} - -#[test] -fn union_padding() { - // Miri cannot track uninitialized bytes on a per byte basis for partially - // initialized scalars: https://github.com/rust-lang/rust/issues/69488 - // See also https://github.com/crossbeam-rs/crossbeam/issues/748#issuecomment-1022432401 - if cfg!(miri) { - return; - } - - #[allow(dead_code)] - #[repr(C, align(8))] - #[derive(Clone, Copy)] - union Test { - u32: u32, - u8: u8, - } - - assert_eq!(mem::align_of::(), 8); - assert_eq!(mem::size_of::(), 8); - unsafe { - let x = UnsafeCell::new(Test { u32: 0 }); - assert_eq!(atomic_load(x.get(), Ordering::Relaxed).assume_init().u32, Test { u32: 0 }.u32); - atomic_store(x.get(), Test { u8: 0 }, Ordering::Relaxed); - assert_eq!(atomic_load(x.get(), Ordering::Relaxed).assume_init().u8, Test { u8: 0 }.u8); - atomic_store(x.get(), Test { u32: 0 }, Ordering::Relaxed); - assert_eq!(atomic_load(x.get(), Ordering::Relaxed).assume_init().u8, Test { u8: 0 }.u8); - } -} diff --git a/tests/uninit.rs b/tests/uninit.rs deleted file mode 100644 index d80015c..0000000 --- a/tests/uninit.rs +++ /dev/null @@ -1,71 +0,0 @@ -// This is a test to check if valgrind/sanitizer/miri can properly detect -// the use of uninitialized bytes. -// -// All tests are ignored by default. -// Do not run this test without valgrind/sanitizer/miri. -// -// With miri: -// cargo miri test --test uninit -- --test-threads=1 --ignored - -use std::{cell::UnsafeCell, env, mem, sync::atomic::Ordering}; - -use atomic_memcpy::{atomic_load, atomic_store}; - -#[test] -#[ignore] -fn uninit() { - check_context(); - - #[allow(dead_code)] - #[repr(C, align(8))] - #[derive(Clone, Copy)] - union Test { - u32: u32, - u8: u8, - } - - assert_eq!(mem::align_of::(), 8); - assert_eq!(mem::size_of::(), 8); - unsafe { - let x = UnsafeCell::new(Test { u32: 0 }); - assert_eq!(atomic_load(x.get(), Ordering::Relaxed).assume_init().u32, Test { u32: 0 }.u32); - atomic_store(x.get(), Test { u8: 0 }, Ordering::Relaxed); - // UNINIT! - assert_eq!(atomic_load(x.get(), Ordering::Relaxed).assume_init().u32, Test { u32: 0 }.u32); - } -} - -/// Panics if the test runs without valgrind/sanitizer/miri. -/// -/// Note that this is heuristic not a complete check, -/// as users can break this check by setting --cfg themselves. -#[track_caller] -fn check_context() { - // miri - if cfg!(miri) { - return; - } - - // sanitizer - if let Ok(rustflags) = env::var("RUSTFLAGS") { - if rustflags.contains("-Z sanitizer=memory") || rustflags.contains("-Zsanitizer=memory") { - return; - } - }; - - // valgrind - if let Ok(runner) = env::var(&format!( - "CARGO_TARGET_{}_RUNNER", - target_spec::Platform::current() - .unwrap() - .triple_str() - .replace(|c| matches!(c, '-' | '.'), "_") // there are targets containing `.` like `thumbv8m.base-none-eabi`. - .to_ascii_uppercase() - )) { - if runner.split(' ').next().unwrap() == "valgrind" { - return; - } - } - - panic!("Do not run this test without valgrind/sanitizer/miri") -}