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

32-bit platforms do not have AtomicU64, AtomicI64 #532

Closed
bunnie opened this issue Apr 1, 2022 · 6 comments
Closed

32-bit platforms do not have AtomicU64, AtomicI64 #532

bunnie opened this issue Apr 1, 2022 · 6 comments

Comments

@bunnie
Copy link

bunnie commented Apr 1, 2022

Hi there! We were just testing bincode 2.0.0.-rc.1 on a RV32-IMAC platform, and encountered an issue where by default 64-bit atomics are included in the Atomic feature set.

Unfortunately, 32-bit platforms structurally lack a single-instruction 64-bit atomic. It would be nice to have a feature that could specify if the platform is 32 bit or 64 bit, and in the case of a 32-bit platform the 64-bit Atomics are not used.

On the other hand, 32-bit platforms are a rather niche edge case these days, and I didn't check to see how hard it would be to support this feature. If it's hard, no worries, we can work around it.

Thank you for your consideration!

@VictorKoenders
Copy link
Contributor

VictorKoenders commented Apr 1, 2022

For bincode 2 we really wanted to support embedded devices, so it makes complete sense to be able to run bincode on 32 bit platforms.

We should probably add:

  • #[cfg(target_pointer_width = 64)] to AtomicI64 and AtomicU64
  • #[cfg(any(target_pointer_width = 32, target_pointer_width = 64))] to AtomicI32 and AtomicU32

target_pointer_width docs

Some other options are:

  • Adding an atomic32 feature flag, but this seems cumbersome if we end up with a million flags
  • Do feature detection in build.rs like serde does, but I think cfg flags will be a better solution for now

@xobs
Copy link
Contributor

xobs commented Apr 1, 2022

It seems as though some 32-bit platforms do have AtomicU64 and AtomicI64. For example, i686-pc-windows-msvc and armv7-unknown-linux-gnueabi appear to have it.

Annoyingly, armv5te-unknown-linux-gnueabi does not have it, and it has identical configuration flags to armv7-unknown-linux-gnueabi meaning there is no way to know if a platform has it by looking at the target_arch cfg variable.

@VictorKoenders
Copy link
Contributor

It looks like target_has_atomic does exactly what we want. It has been stabilized in this pr on feb 10th. I'm guessing that will be in the next stable version of rust.

On nightly this already works:

#[cfg(target_has_atomic = "64")]
impl Encode for AtomicU64 {
    fn encode<E: crate::enc::Encoder>(
        &self,
        encoder: &mut E,
    ) -> Result<(), crate::error::EncodeError> {
        self.load(Ordering::SeqCst).encode(encoder)
    }
}

#[cfg(target_has_atomic = "64")]
impl Decode for AtomicU64 {
    fn decode<D: crate::de::Decoder>(decoder: &mut D) -> Result<Self, crate::error::DecodeError> {
        Ok(AtomicU64::new(Decode::decode(decoder)?))
    }
}

Rust 1.60 should be released next week. I'd suggest waiting a week to see if it gets stabilized and we'll use target_has_atomic if it is, otherwise we'll merge #533

@xobs
Copy link
Contributor

xobs commented Apr 1, 2022

That's a really good find! It will be included in 1.60:

[18:17:35] E:/Code/bincode> rustc +beta --target armv7-unknown-linux-gnueabi --print cfg
debug_assertions
panic="unwind"
target_arch="arm"
target_endian="little"
target_env="gnu"
target_family="unix"
target_has_atomic="16"
target_has_atomic="32"
target_has_atomic="64"
target_has_atomic="8"
target_has_atomic="ptr"
target_os="linux"
target_pointer_width="32"
target_vendor="unknown"
unix
[18:17:39] E:/Code/bincode>

I'll rework #533 to take advantage of that feature.

xobs added a commit to xobs/bincode that referenced this issue Apr 1, 2022
Not all platforms support AtomicI64 or AtomicU64. Use the
`target_has_atomic` config flag to determine whether to include these.

This fixes bincode-org#532.

Signed-off-by: Sean Cross <[email protected]>
@xobs
Copy link
Contributor

xobs commented Apr 1, 2022

I've updated #533 which uses the new features. I only added the check for 64-bit atomics. It does bump the MSRV up to 1.60, in case that's a thing you worry about.

@VictorKoenders
Copy link
Contributor

I've added #534 to check for different platforms, and #535 as a tracking issue for the platforms that fail. Unfortunately cross does not have a riscv32imac platform, this might be a good thing to add after 534 is merged.

It does bump the MSRV up to 1.60

We don't have a MSRV yet until we release 2.0, so we can bump it freely for now.

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

No branches or pull requests

3 participants