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

Add compare_exchange, compare_exchange_weak, and fetch_update #7

Merged
merged 1 commit into from
Jun 26, 2022

Conversation

taiki-e
Copy link
Owner

@taiki-e taiki-e commented Jun 22, 2022

This implements atomic CAS operations (compare_exchange, compare_exchange_weak, and fetch_update) on potentially uninitialized integers.

(If this approach is okay) This should be effectively the last requirement to use this crate (as optional deps/experimental features) in crossbeam (and to fix AtomicCell's remaining UBs such as crossbeam-rs/crossbeam#315, crossbeam-rs/crossbeam#748).

Note that this implementation already allows failure ordering stronger than success ordering (rust-lang/rust#98383).


I have not yet documented the pitfall of CAS with values containing uninitialized bytes (EDIT: documented): Comparison of two values containing uninitialized bytes may fail even if they are equivalent as Rust's type, because their contents are not frozen until a pointer to the value containing uninitialized bytes is passed to asm!. (See crossbeam-rs/crossbeam#315 for more)

For example, the following example could be an infinite loop:

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

use atomic_maybe_uninit::AtomicMaybeUninit;

#[derive(Clone, Copy, PartialEq, Eq)]
#[repr(C, align(4))]
struct Test(u8, u16);

unsafe {
    let x = mem::transmute::<_, MaybeUninit<u32>>(Test(0, 0));
    let v = AtomicMaybeUninit::new(x);
    while v
        .compare_exchange(
            mem::transmute::<_, MaybeUninit<u32>>(Test(0, 0)),
            mem::transmute::<_, MaybeUninit<u32>>(Test(1, 0)),
            Ordering::AcqRel,
            Ordering::Acquire,
        )
        .is_err()
    {}
}

To work around this problem, you need to use a helper like this function in crossbeam.

unsafe fn atomic_compare_exchange(
    v: &AtomicMaybeUninit<u32>,
    mut current: Test,
    new: Test,
) -> Result<Test, Test> {
    let mut current_raw = mem::transmute::<_, MaybeUninit<u32>>(current);
    let new_raw = mem::transmute::<_, MaybeUninit<u32>>(new);
    loop {
        match v.compare_exchange_weak(current_raw, new_raw, Ordering::AcqRel, Ordering::Acquire)
        {
            Ok(_) => break Ok(current),
            Err(previous_raw) => {
                let previous = mem::transmute_copy(&previous_raw);

                if !Test::eq(&previous, &current) {
                    break Err(previous);
                }

                // The compare-exchange operation has failed and didn't store `new`. The
                // failure is either spurious, or `previous` was semantically equal to
                // `current` but not byte-equal. Let's retry with `previous` as the new
                // `current`.
                current = previous;
                current_raw = previous_raw;
            }
        }
    }
}

unsafe {
    let x = mem::transmute::<_, MaybeUninit<u32>>(Test(0, 0));
    let v = AtomicMaybeUninit::new(x);
    while atomic_compare_exchange(&v, Test(0, 0), Test(1, 0)).is_err() {}
}

@taiki-e taiki-e added O-arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state O-riscv Target: RISC-V architecture O-x86 Target: x86/x64 processors O-mips Target: MIPS processors O-powerpc Target: PowerPC processors O-s390x Target: SystemZ processors (s390x) labels Jun 22, 2022
@taiki-e
Copy link
Owner Author

taiki-e commented Jun 22, 2022

cc @RalfJung

@taiki-e taiki-e force-pushed the cmpxchg branch 2 times, most recently from e07803b to 752bdf0 Compare June 25, 2022 16:52
@taiki-e taiki-e marked this pull request as ready for review June 25, 2022 17:01
@taiki-e taiki-e merged commit 4417d05 into main Jun 26, 2022
@taiki-e taiki-e deleted the cmpxchg branch June 26, 2022 03:35
@RalfJung
Copy link
Contributor

I am not sure what to say about this, assembly code is outside my realm of knowledge.^^

@RalfJung
Copy link
Contributor

Also with my Miri maintainer hat on, I have to say that I am a bit worried about fundamental ecosystem crates starting to use asm!. Miri will not be able to run any code that uses such crates.

@taiki-e
Copy link
Owner Author

taiki-e commented Jun 27, 2022

Miri will not be able to run any code that uses such crates.

I'm aware of this issue and tend to avoid inline assembly calls when Miri or Sanitizer is used. (e.g., this code calls unstable core_arch function instead of asm on cfg(miri))

I plan to do a similar with crossbeam, but, there is currently no Miri-compatible (sound) way to do the equivalent, so we would probably need to keep the current implementation with conditions. (Of cause, the result would be a situation where soundness issues exist only when Miri or Sanitizer is used.)

In the future, if AtomicMaybeUninit is added to core, or at least MaybeUninit<{integer}> is allowed in some of core::intrinsics::atomic_*, it can be used in this crate when Miri or Sanitizer is used. (If the latter is acceptable, I have interest in working on it.)

@RalfJung
Copy link
Contributor

RalfJung commented Jun 27, 2022

there is currently no Miri-compatible (sound) way to do the equivalent

That is indeed the point: there is no Rust-compatible way of doing that, as in, no way to do this within the confines of the Rust language. (That core::arch intrinsic will not work for Miri, either.)

The only way to fix that is go do a proper language extension (this is not just a libs thing, it is a T-lang thing), and add support for this in Rust itself. I am not sure what the best API for this looks like, and this touches on a number of tricky subjects. (For example, Rust by design currently does not have freeze or allow comparing uninit data. Allowing this here would be a Big Deal. So while atomic load/store/swap are fine, compare_exchange operations for uninit data are a lot more tricky.) I'd be happy to work with you on that.

@taiki-e taiki-e added the O-aarch64 Target: Armv8-A, Armv8-R, or later processors in AArch64 mode label Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-aarch64 Target: Armv8-A, Armv8-R, or later processors in AArch64 mode O-arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state O-mips Target: MIPS processors O-powerpc Target: PowerPC processors O-riscv Target: RISC-V architecture O-s390x Target: SystemZ processors (s390x) O-x86 Target: x86/x64 processors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants