Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AtomicU8 is stable (and supported by the MSRV) #744

Open
steffahn opened this issue Sep 7, 2021 · 3 comments
Open

AtomicU8 is stable (and supported by the MSRV) #744

steffahn opened this issue Sep 7, 2021 · 3 comments

Comments

@steffahn
Copy link

steffahn commented Sep 7, 2021

Apparently, crossbeam has MSRV 1.36, which includes AtomicU8. Hence the following comment is outdated:

// 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.
let val = ptr::read_volatile(src);

Considering that this comment legitimizes code that is officially UB, I propose that this should be fixed, if possible.

@nico-abram
Copy link

This very naive patch passes the tests:

-                let val = ptr::read_volatile(src);
+                let mut val = std::mem::MaybeUninit::uninit();
+                let mut val_bytes = val.as_mut_ptr() as *mut u8;
+                let mut src_bytes = src as *mut core::sync::atomic::AtomicU8;
+                let src_end = src_bytes.offset(std::mem::size_of::<T>() as isize);
+                let val = unsafe {
+                    while src_bytes != src_end {
+                            val_bytes.write((*src_bytes).load(Ordering::Relaxed));
+                            val_bytes = val_bytes.offset(1);
+                            src_bytes = src_bytes.offset(1);
+                    }
+                    val.assume_init()
+                };

We could try using larger loads like thomcc's tearcell does: https:/thomcc/tearor/blob/c558f6105230a49bb79b8afe989194e7656fdda7/src/lib.rs#L122-L162 instead of a naive byte by byte copy.

However, when I asked for review in the community discord server's #dark-arts a problem with padding bytes was mentioned: reading uninit/padding memory with AtomicU8 is not documented to not be UB. No way to fix this without a new language primitive or the language allowing these reads for atomics. For example, for this program

use std::mem::MaybeUninit;
use std::sync::atomic::{AtomicU8, Ordering};

fn main() {
    #[repr(align(2))]
    #[derive(Debug)]
    struct WithPadding {
        _d: u8,
    }
    let mut x = WithPadding { _d: 3u8 };
    let x_p = std::ptr::addr_of_mut!(x);
    let mut y = MaybeUninit::<WithPadding>::uninit();
    unsafe {
        *y.as_mut_ptr().cast::<u8>() = (*x_p.cast::<AtomicU8>()).load(Ordering::Relaxed);
        *y.as_mut_ptr().cast::<u8>().offset(1) =
            (*x_p.cast::<AtomicU8>().offset(1)).load(Ordering::Relaxed);

        dbg!(y.assume_init());
        dbg!(std::mem::size_of::<WithPadding>());
    }
}

MIRI currently gives the following error (With -Zmiri-check-number-validity)

error: Undefined Behavior: type validation failed: encountered uninitialized bytes, but expected initialized plain (non-pointer) bytes
    --> D:\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\sync\atomic.rs:2570:24
     |
2570 |             Relaxed => intrinsics::atomic_load_relaxed(dst),
     |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes, but expected initialized plain (non-pointer) bytes

@taiki-e
Copy link
Member

taiki-e commented Apr 25, 2022

I have investigated the problem of such before (atomic-memcpy) and there are two main problems with the approach that uses AtomicU8.

  • reads and writes as AtomicU8 slice are very slow compared to volatile reads/writes.
  • safety requirements for transmuting to/from integers must also be met since copying is via integers.

For the first problem, p1478r1 has an idea that seems to handle it well, and atomic-memcpy implements a variant of it. (In some cases, it can generate assembly equivalent to volatile read generates. See atomic-memcpy's readme and the implementation comments for more).

The second problem actually contains (as far as I know) 2 to 3 problems.

  • ptr2int2ptr transmute (not cast)
  • partial copy of pointers
  • copy of partially or fully uninitialized integers as initialized integers

ptr2int2ptr transmute was previously used as a hack to work around ptr2int2ptr cast, but this has recently changed and Miri now reports this case as UB.
The known fix to ptr2int2ptr transmute problem is to transmute pointers to/from MaybeUninit. Of course, this way does not work when using std atomic types.

Even assuming that ptr2int2ptr transmute is not a problem, the approach using AtomicU8 may cause problems with partial copy of pointers. (IIRC, Miri currently reports "unsupported operation: unable to turn pointer into raw bytes" error on this case)
This is usually not a problem for atomic-memcpy's approach, which reads and writes as slices of AtomicUsize at most depending on the alignment. (except for repr(packed) and similar things)

The uninitialized byte problem can never be resolved when using std atomic types.


Based on the above, I believe that the feature to read and write MaybeUninit<u{8,size}> atomically would fix most of the remaining problems. I can think of 3 solutions:

I have implemented the third (atomic-maybe-uninit), and I think it needs "audits from someone familiar with these platform-specific codes", "more platform support", and "methods optimized for the case like atomic-memcpy" (inline assembly prevents some optimizations) when use it in crossbeam.

EDIT: Of course, using inline assembly in atomic-memcpy or crossbeam is also one of the solutions. Actually, I tried that approach first (taiki-e/atomic-memcpy#6) and then decided to implement it as a separate crate.

@KamilaBorowska
Copy link

ptr2int2ptr transmute problem could be mostly solved (as long no unaligned pointers are stored in #[repr(packed)] structures) by using AtomicPtr when a given memory location could potentially store a pointer - it's perfectly fine to store non-pointer values in a pointer. Not that it helps that much with other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants