Merge #1

1: Remove miri hack r=taiki-e a=taiki-e

Use currently use a hack to avoid rust-lang/rust#69488 and to make sure that Miri errors for  atomic load/store of integers containing uninitialized bytes (which is probably not a problem and uncharted territory at best [1] [2] [3], and can be detected by `-Zmiri-check-number-validity` [4]), do not mask Miri errors for the use of uninitialized bytes (which is definitely a problem).

https://github.com/taiki-e/atomic-memcpy/blob/3507fef17534e4825b2b303d04702b4678e29dd0/src/lib.rs#L426-L450

[1]: crossbeam-rs/crossbeam#315 
[2]: rust-lang/unsafe-code-guidelines#158 
[3]: rust-lang/unsafe-code-guidelines#71 
[4]: rust-lang/miri#1904 

However, this actually causes another "unsupported operation" Miri error.

```
error: unsupported operation: unable to turn pointer into raw bytes
   --> /Users/taiki/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:701:9
    |
701 |         copy_nonoverlapping(src, tmp.as_mut_ptr(), 1);
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unable to turn pointer into raw bytes
    |
    = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support
```


Co-authored-by: Taiki Endo <[email protected]>

Author: bors[bot]

.github/workflows/ci.yml

@@ -146,11 +146,12 @@ jobs:
           component: miri
       - run: cargo miri test --workspace -- --test-threads=1
         env:
-          MIRIFLAGS: -Zmiri-symbolic-alignment-check -Zmiri-tag-raw-pointers -Zmiri-disable-isolation
-      # tests/padding.rs intentional contains tests for cases that are incompatible with -Zmiri-check-number-validity.
-      - run: cargo miri test --test test -- --test-threads=1
+          MIRIFLAGS: -Zmiri-check-number-validity -Zmiri-tag-raw-pointers -Zmiri-disable-isolation
+      - run: cargo miri test --workspace -- --test-threads=1
         env:
           MIRIFLAGS: -Zmiri-check-number-validity -Zmiri-symbolic-alignment-check -Zmiri-tag-raw-pointers -Zmiri-disable-isolation
+          # -Zmiri-symbolic-alignment-check is incompatible with the code that does manual integer arithmetic to ensure alignment.
+          RUSTFLAGS: ${{ env.RUSTFLAGS }} --cfg atomic_memcpy_symbolic_alignment_check_compat
 
   san:
     strategy:

CHANGELOG.md

@@ -10,6 +10,8 @@ Note: In this file, do not use the hard wrap in the middle of a sentence for com
 
 ## [Unreleased]
 
+- Fix "unsupported operation: unable to turn pointer into raw bytes" Miri error. ([#1](https://github.com/taiki-e/atomic-memcpy/pull/1))
+
 ## [0.1.0] - 2022-02-12
 
 Initial release

README.md

@@ -18,11 +18,12 @@ 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. (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.
+- 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 cannot 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.
 
 [p1478r1]: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1478r1.html
 [implementation]: https://github.com/taiki-e/atomic-memcpy/blob/279d7041e48fae0943a50102ebab97e7ed3977ae/src/lib.rs#L359-L403
 [asm-test]: tests/asm-test/asm
+[rust-lang/rust#69488]: https://github.com/rust-lang/rust/issues/69488
 
 ## License
 

src/lib.rs

@@ -213,13 +213,17 @@ mod imp {
         sync::atomic::{AtomicU16, AtomicUsize, Ordering},
     };
 
+    #[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
     #[cfg(target_pointer_width = "32")]
     type Half = u16;
+    #[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
     #[cfg(target_pointer_width = "32")]
     type AtomicHalf = AtomicU16;
 
+    #[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
     #[cfg(target_pointer_width = "64")]
     type Half = u32;
+    #[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
     #[cfg(target_pointer_width = "64")]
     type AtomicHalf = AtomicU32;
 
@@ -327,9 +331,10 @@ mod imp {
                 }
             }
 
+            #[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
+            #[cfg(not(target_pointer_width = "16"))]
             #[cfg_attr(feature = "inline-always", inline(always))]
             #[cfg_attr(not(feature = "inline-always"), inline)]
-            #[cfg(not(target_pointer_width = "16"))]
             pub(super) unsafe fn atomic_load_half(&mut self) {
                 use super::{AtomicHalf, Half};
                 debug_assert!(self.remaining() >= mem::size_of::<Half>());
@@ -423,45 +428,15 @@ mod imp {
             return result;
         }
 
-        // HACK: Miri cannot track uninitialized bytes on a per byte basis for
-        // partially initialized scalars: https://github.com/rust-lang/rust/issues/69488
-        //
-        // This hack allows miri to properly track the use of uninitialized
-        // bytes. See also tests/uninit.rs that is a test to check if
-        // valgrind/sanitizer/miri can properly detect the use of uninitialized
-        // bytes.
-        //
-        // Note: With or without this hack, atomic load/store of integers
-        // containing uninitialized bytes is technically an undefined behavior.
-        // The only purpose of this hack is to make sure that Miri errors for
-        // atomic load/store of integers containing uninitialized bytes
-        // (which is probably not a problem and uncharted territory at best [1] [2] [3],
-        // and can be detected by `-Zmiri-check-number-validity` [4]),
-        // do not mask Miri errors for the use of uninitialized bytes (which is definitely a problem).
-        // See also tests/padding.rs.
-        //
-        // [1] https://github.com/crossbeam-rs/crossbeam/issues/315
-        // [2] https://github.com/rust-lang/unsafe-code-guidelines/issues/158
-        // [3] https://github.com/rust-lang/unsafe-code-guidelines/issues/71
-        // [4] https://github.com/rust-lang/miri/pull/1904
-        //
-        // rust-lang/rust#69488 affects only CTFE(compile-time function evaluation)/Miri
-        // and atomic operations cannot be called in const context, so our code
-        // is only affected in the case of cfg(miri).
-        if cfg!(miri) {
-            let mut state = load::LoadState::new(result.as_mut_ptr(), src);
-            state.atomic_load_u8(state.remaining());
-            debug_assert_eq!(state.remaining(), 0);
-            return result;
-        }
-
         // Branch 1: If the alignment of `T` is less than usize, but `T` can be read as
         // at least one or more usize, compute the align offset and read it
         // like `(&[AtomicU8], &[AtomicUsize], &[AtomicU8])`.
         if mem::align_of::<T>() < mem::align_of::<AtomicUsize>()
             && mem::size_of::<T>() >= mem::size_of::<usize>() * 4
         {
             let mut state = load::LoadState::new(result.as_mut_ptr(), src);
+            // -Zmiri-symbolic-alignment-check is incompatible with the code that does manual integer arithmetic to ensure alignment.
+            #[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
             #[cfg(not(target_pointer_width = "16"))]
             {
                 // Since the caller guarantees that the pointer is properly aligned,
@@ -702,9 +677,10 @@ mod imp {
                 }
             }
 
+            #[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
+            #[cfg(not(target_pointer_width = "16"))]
             #[cfg_attr(feature = "inline-always", inline(always))]
             #[cfg_attr(not(feature = "inline-always"), inline)]
-            #[cfg(not(target_pointer_width = "16"))]
             pub(super) unsafe fn atomic_store_half(&mut self) {
                 use super::{AtomicHalf, Half};
                 debug_assert!(self.remaining() >= mem::size_of::<Half>());
@@ -769,25 +745,17 @@ mod imp {
             return;
         }
 
-        // HACK: See the `atomic_load` function for the detailed comment.
-        if cfg!(miri) {
-            let mut state = store::StoreState::new(dst, &*val);
-            state.atomic_store_u8(state.remaining());
-            debug_assert_eq!(state.remaining(), 0);
-            mem::forget(guard);
-            return;
-        }
-
         // Branch 1: If the alignment of `T` is less than usize, but `T` can be write as
         // at least one or more usize, compute the align offset and write it
         // like `(&[AtomicU8], &[AtomicUsize], &[AtomicU8])`.
         if mem::align_of::<T>() < mem::align_of::<AtomicUsize>()
             && mem::size_of::<T>() >= mem::size_of::<usize>() * 4
         {
             let mut state = store::StoreState::new(dst, &*val);
+            // See the `atomic_load` function for the detailed comment.
+            #[cfg(not(atomic_memcpy_symbolic_alignment_check_compat))]
             #[cfg(not(target_pointer_width = "16"))]
             {
-                // See the `atomic_load` function for the detailed comment.
                 if mem::align_of::<T>() >= mem::align_of::<Half>() {
                     if dst as usize % mem::size_of::<usize>() == 0 {
                         // SAFETY:

tests/padding.rs

@@ -1,15 +1,16 @@
-// https://github.com/rust-lang/unsafe-code-guidelines/issues/71
-// https://github.com/rust-lang/miri/pull/1904
-//
-// With miri:
-//   MIRIFLAGS='-Zmiri-check-number-validity' cargo miri test --test padding -- --test-threads=1
-
 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)]
@@ -37,6 +38,13 @@ fn enum_padding() {
 
 #[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)]

tests/test.rs

@@ -60,6 +60,17 @@ fn basic_unit() {
     }
 }
 
+#[test]
+fn basic_ptr() {
+    unsafe {
+        let x = UnsafeCell::<*mut u8>::new(ptr::null_mut());
+        assert!(atomic_load(x.get(), Ordering::Relaxed).assume_init().is_null());
+        let mut v = 0u8;
+        atomic_store(x.get(), &mut v, Ordering::Relaxed);
+        assert!(!atomic_load(x.get(), Ordering::Relaxed).assume_init().is_null());
+    }
+}
+
 #[cfg(not(feature = "no-panic"))]
 #[track_caller]
 fn assert_panic<T: std::fmt::Debug>(f: impl FnOnce() -> T) -> std::string::String {