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

Stabilize the `integer_atomics` feature: Atomic{I,U}{8,16,32,64} #56753

Open
alexcrichton opened this Issue Dec 12, 2018 · 13 comments

Comments

Projects
None yet
5 participants
@alexcrichton
Copy link
Member

alexcrichton commented Dec 12, 2018

RFC: rust-lang/rfcs#1543
Tracking issue: #32976
Documentation: https://doc.rust-lang.org/nightly/std/sync/atomic/

This issue is a proposal to stabilize the integer_atomics feature as-is on nightly, modulo some documentation updates. Specifically, for each atomic type, this is proposed:

impl AtomicU8 {
    pub const fn new(x: u8) -> AtomicU8;
    pub fn get_mut(&mut self) -> &mut u8;
    pub fn into_inner(self) -> u8;
    pub fn load(&self, Ordering) -> u8;
    pub fn store(&self, Ordering) -> u8;
    pub fn swap(&self, val: u8, Ordering) -> u8;
    pub fn compare_and_swap(&self, u8, u8, Ordering) -> u8;
    pub fn compare_exchange(&self, u8, u8, Ordering, Ordering) -> Result<u8, u8>;
    pub fn compare_exchange_weak(&self, u8, u8, Ordering, Ordering) -> Result<u8, u8>;
    pub fn fetch_add(&self, u8, Ordering) -> u8;
    pub fn fetch_sub(&self, u8, Ordering) -> u8;
    pub fn fetch_and(&self, u8, Ordering) -> u8;
    pub fn fetch_nand(&self, u8, Ordering) -> u8;
    pub fn fetch_or(&self, u8, Ordering) -> u8;
    pub fn fetch_xor(&self, u8, Ordering) -> u8;
}

Portability

One of the main points on the RFC and tracking issue is the portability of these types. @alexcrichton has compiled this table of known platform support, and the conclusions we've drawn with the libs team are:

  • The AtomicUsize type is already "sketchily" supported. It's not present on armv5te (non-linux) and has a stripped down API on thumv6m
  • LLVM emulates small-size atomics with larger atomics (aka 1 byte atomics with 4 byte atomics) on platforms like MIPS. We're unclear on whether it's even possible to safely define these same operations on crates.io, as LLVM might deduce it's undefined behavior. It's sound, however, for LLVM's codegen backend to implement atomics like this.
  • AtomicU32 and below are basically universally supported across all platforms that have any atomics at all.
  • AtomicU64 actually has a good showing for support as well, except on mips/powerpc platforms.

It's proposed in this stabilization that we stabilize all APIs as-is, knowing that these apis are in a "portable location" yet are not as maximally portable as, say, Add for u8. It's intended that platforms which don't support these types simply won't provide the types, as they do today. For example armv5te will continue to have none of these types. (for those following these issues, **it's not proposed that target_has_atomic is stabilized as part of this proposal).

While this is a departure from the norms of the rest of the standard library, it's hopefully the most pragmatic decision here.

What about u128?

Some very recent (aka 20 minutes before this issue was opened) shows that AtomicU128 can be supported on x86_64 with the LLVM cx16 feature activated and aarch64 platforms. All other platforms look to require intrinsics one way or another. This proposal doesn't propose stabilizing these just yet, but the thinking is that a stable/safe std::arch::aarch64::AtomicU128 could be provided while a stable/unsafe std::arch::x86_64::AtomicU128 could be provided documenting the requirement of the target feature (exact name of the target feature TBD)

TODO checklist before stabilization:

  • FCP
  • Update documentation to reflect that AtomicU8/16/etc may be implemented in terms of 4-byte atomics
  • Update documentation to reflect that these types are not maximally portable. Architectures that want to be supported should be tested. MIPS and PowerPC do not support AtomicU64, and "more esoteric" platforms like armv5te, thumv6m, etc, have little-to-no support.
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 12, 2018

@rfcbot fcp merge

Proposing a formal stabilization here, lots of words are above!

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 12, 2018

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 12, 2018

@rfcbot concern target_has_atomic

Why not stabilize this cfg as well? For code that does want to be compatible with those esoteric platforms while using those types, it seems that a cfg-based fallback would be useful.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 12, 2018

I'm not currently proposing it for two main reasons, the first being that it can always be stabilized later and it's more conservative to start out without it. Other than that though I'd like to try to see what the experience is without these cfgs and how bad it is to be portable across these platforms. I'm not sure that it's a great name for a cfg anyway because it doesn't cover thumv6m's case of "they exist but have few methods".

@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Dec 12, 2018

I'm not too happy with target_has_atomic = "cas" which was added in #51953. I feel that target_has_atomic should only be enabled if full atomic support is available for a particular size. A separate cfg taget_has_atomic_load_store should be used for minimal load/store support like on thumbv6m.

Maybe we could defer stabilizing atomics on platforms like thumbv6m that only have partial support?

@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Dec 12, 2018

cc #54250

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 29, 2018

I don't think we should stabilize target_has_atomic especially when it seems like it'd be subsumed by #[cfg(accessible(...)] (see #32976 (comment)). In any case, adding a new cfg is a T-Lang decision.

Really, stabilizing these atomics is also a decision that T-Lang should be in on since from what I can tell the abstract machine is affected; but given that we already have similar operations for AtomicUsize I'm happy to defer to T-Libs on that in this specific instance.

@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Jan 1, 2019

I think it's fine if we stabilize without target_has_atomic for now. For my current projects I only need AtomicU8 and AtomicU32, which are supported pretty much everywhere.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 7, 2019

ping @sfackler, @Kimundi, @SimonSapin, it'd be great to make some progress on this!

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 7, 2019

@rfcbot resolve target_has_atomic

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 7, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 7, 2019

std: Stabilize fixed-width integer atomics
This commit stabilizes the `Atomic{I,U}{8,16,32,64}` APIs in the
`std::sync::atomic` and `core::sync::atomic` modules. Proposed in rust-lang#56753
and tracked in rust-lang#32976 this feature has been unstable for quite some time
and is hopefully ready to go over the finish line now!

The API is being stabilized as-is. The API of `AtomicU8` and friends
mirrors that of `AtomicUsize`. A list of changes made here are:

* A portability documentation section has been added to describe the
  current state of affairs.
* Emulation of smaller-size atomics with larger-size atomics has been
  documented.
* As an added bonus, `ATOMIC_*_INIT` is now scheduled for deprecation
  across the board in 1.34.0 now that `const` functions can be invoked
  in statics.

Note that the 128-bit atomic types are omitted from this stabilization
explicitly. They have far less platform support than the other atomic
types, and will likely require further discussion about their best
location.

Closes rust-lang#32976
Closes rust-lang#56753

@alexcrichton alexcrichton referenced a pull request that will close this issue Jan 7, 2019

Open

std: Stabilize fixed-width integer atomics #57425

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 7, 2019

I've posted a stabilization PR at #57425 for when FCP is done

alexcrichton added a commit to alexcrichton/rust that referenced this issue Jan 8, 2019

std: Stabilize fixed-width integer atomics
This commit stabilizes the `Atomic{I,U}{8,16,32,64}` APIs in the
`std::sync::atomic` and `core::sync::atomic` modules. Proposed in rust-lang#56753
and tracked in rust-lang#32976 this feature has been unstable for quite some time
and is hopefully ready to go over the finish line now!

The API is being stabilized as-is. The API of `AtomicU8` and friends
mirrors that of `AtomicUsize`. A list of changes made here are:

* A portability documentation section has been added to describe the
  current state of affairs.
* Emulation of smaller-size atomics with larger-size atomics has been
  documented.
* As an added bonus, `ATOMIC_*_INIT` is now scheduled for deprecation
  across the board in 1.34.0 now that `const` functions can be invoked
  in statics.

Note that the 128-bit atomic types are omitted from this stabilization
explicitly. They have far less platform support than the other atomic
types, and will likely require further discussion about their best
location.

Closes rust-lang#32976
Closes rust-lang#56753
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 17, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

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