lukaskalbertodt/atomig

Generic and convenient `std` atomics via `Atomic<T>`

atomig's Issues

Support for embedded targets

I see you just recently added support for no_std in #5. Unfortunately, when I tried to use this crate on a thumbv7em target, I discovered it doesn't compile. I started to implement a fix and hoped to open a PR, but my effort revealed that core::sync::atomic is in a pretty sad state at the moment.

To actually implement this, you'll need to separate out the CAS methods from the main PrimitiveAtom trait, adding PrimitiveAtomCas and AtomCas traits. In the standard library, the feature target_has_atomic_load_store is used to gate the basic methods for loading and storing. But you need target_has_atomic as well, if you want the CAS methods. thumbv7em has both for integers up to 32 bits, but thumbv6m, for example, does not have CAS instructions at all.

I went through the library, added those traits, and added cfg flags everywhere, only to find out that the cfg flags are still unstable. This comment is relevant. The notes on portability are relevant as well. I probably should have read them before I tried to implement this.

In any event, I don't see a very viable path forward. So far, the only workaround I've seen is to use a build script. It's pretty frustrating.

More `Atom` implementations

Primitive types and standard library

• f32/f64?
• char?
• Small arrays?
• Small tuples?
• Function pointers?
• Ipv4Addr?

From external crates

• ...

Better support for embedded targets

Some targets do not offer all atomic features used in core::sync::atomic. On those targets, the standard library just does not have specific types or methods. This is problematic for atomig, as those types/methods are referred to anyway, even if a user of atomig would not use them. So in order for atomig to compile on those targets, we need to #[cfg] guard types and methods appropriately.

Unfortunately, the cfg checks that are available on stable are not super powerful yet. There is only target_has_atomic = * where * can be a size like 8, 16, ... and "ptr". This already helped somewhat and made atomig compile for targets like thumbv7em-none-eabi. But other targets need more fine-grained control. For example, compiling for thumbv6m-none-eabi results in no Atom implementations at all. So we are waiting for stuff like:

• target_has_atomic_load_store rust-lang/rust#94039
• ...

Is it possible to `impl<T: Atom> Atom for Option<T>` to be added?

atomig already has impl Atoms for Option<NonNull<T>> and Option<NonZero*> types, but it would be really nice if this could be extended to other Ts that have niches. I've sketched out a solution below that could maybe work, and checked in godbolt that there's no overhead. What do you think?

use core::mem;
use core::ops::AddAssign;
use core::num::NonZeroU64;

pub trait Atom {
    type Repr;

    fn pack(self) -> Self::Repr;
    fn unpack(src: Self::Repr) -> Self;
}

impl<T> Atom for Option<T>
where 
    T: Atom,
    T: TryFrom<T::Repr>,
    T::Repr: Copy,
    T::Repr: From<u8>, // For 0 and 1
    T::Repr: AddAssign,
{
    type Repr = T::Repr;

    fn pack(self) -> Self::Repr {
        let niche = {
            // Guarantee we have a niche.
            assert!(mem::size_of::<Option<T>>() == mem::size_of::<T>());
            let zero = T::Repr::from(0);
            let one = T::Repr::from(1);
            // Since we checked there's a niche, this should terminate (assuming `TryFrom` is normal).
            let mut niche = zero;
            loop {
                if T::try_from(niche).is_err() {
                    break;
                }
                niche += one;
            }
            niche
        };
        self.map(T::pack).unwrap_or(niche)
    }

    fn unpack(src: Self::Repr) -> Self {
        src.try_into().ok()
    }
}

impl Atom for NonZeroU64 {
    type Repr = u64;

    fn pack(self) -> Self::Repr {
        self.get()
    }

    fn unpack(src: Self::Repr) -> Self {
        src.try_into().unwrap()
    }
}

#[no_mangle]
pub fn pack_u64(x: Option<NonZeroU64>) -> u64 {
    x.pack()
}

#[no_mangle]
pub fn unpack_u64(x: u64) -> Option<NonZeroU64> {
    Atom::unpack(x)
}

pub enum Enum {
    A,
    B,
    C,
}

impl Atom for Enum {
    type Repr = u8;

    fn pack(self) -> Self::Repr {
        self as _
    }

    fn unpack(src: Self::Repr) -> Self {
        src.try_into().unwrap()
    }
}

impl TryFrom<u8> for Enum {
    type Error = ();

    fn try_from(value: u8) -> Result<Self, Self::Error> {
        Ok(match value {
            0 => Self::A,
            1 => Self::B,
            2 => Self::C,
            _ => return Err(()),
        })
    }
}

#[no_mangle]
pub fn pack_enum(x: Option<Enum>) -> u8 {
    x.pack()
}

#[no_mangle]
pub fn unpack_enum(x: u8) -> Option<Enum> {
    Atom::unpack(x)
}

Both fn pack_{u64,enum} compile to no-ops, so the niche loop is being fully optimized out. And for fn unpack_{u64,enum} I'm not seeing asm, which I think means they have the same code as fn {NonZeroU64,Enum}::unpack.

The main annoying obstacle I see to this is that there is no impl TryFrom<*mut T> for NonNull<T>, even though there definitely could be (NonNull::new is this). To get around this, some other trait could be added to atomig like trait TryFromRepr that we impl for NonNull, NonZero*, etc., but then users of atomig would have to impl TryFromRepr themselves, and it wouldn't work if crate A, depending on atomig and crate B, tried to use Atomic<Option<b::B>> even if B: TryFrom<b::B::Repr>. Ideally, std would just add the impl TryFrom<*mut T> for NonNull<T>.

The `AtomInteger` trait is not needed for `Atomic::fetch_update`

The function Atomic::fetch_update is only available for types that implement AtomInteger. However, it doesn't actually do any implicit integer operations like, say, fn fetch_add does — it explicitly leaves the update to a Rust closure that is restricted to safe operations on the data types. As such, the restriction is not needed, allowing fetch_update to be used on enum types, for example.

Here's my example that does not work with atomig as it is now:

#[derive(Atom, Copy)]
#[repr(u8)]
enum Enum {
    Off,
    On,
}

fn foo() {
    let t: Atomic<Enum> = Atomic::new(Enum::Off);
    t.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |t0| Some(match t0 {
        Enum::Off => Enum::On,
        Enum::On => Enum::Off,
    }));
}

Support for Option<NonNull<Atomic<T>>

The compiler should optimize this type to a single atomic, so supporting it seems reasonable? Same should hold for std::num:nonzero.