7

Tracking issue for #[cfg(target_has_atomic = ...)] · Issue #32976 · rust-lang/ru...

 2 years ago
source link: https://github.com/rust-lang/rust/issues/32976
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Copy link

Member

sfackler commented on Sep 7, 2016

This is still blocked on some reasonable way to talk about platform-specificity in a more granular way than what we currently support.

Copy link

Member

Manishearth commented on Oct 4, 2016

Is there any reason why AtomicU32 would be hard to stabilize? We need a way to store multi-word bitflags in an atomic thingy. AtomicUsize won't work here since it's too large on 64 bit.

Copy link

Member

sfackler commented on Oct 5, 2016

What do you mean by "too large"?

Copy link

Member

Manishearth commented on Oct 5, 2016

edited

Let's say I have a 64-bit bucket of flags. I need two atomic u32s to represent it. I can't use two usizes because that's too large on 64 bit.

Alternatively, let's say I have a 32-bit bucket of flags. This is on a struct where size matters. I need a u32, not something larger on 64-bit.

I guess I could cfg() it and abstract over it. That solves the first problem but not the second.

Copy link

Member

nagisa commented on Oct 5, 2016

edited

@Manishearth 16-bit-usize targets would give you 16 bit atomic width. 100% savings over 32bit systems! (and potential loss of data)

There isn’t really much problem stabilising any of these IMO, but you’ll still have to CFG on target_has_atomic="32" in your code.

Copy link

Member

Manishearth commented on Oct 5, 2016

16-bit-usize targets would give you 16 bit atomic width.

ugh, these exist, right.

What do I have to do to get these stabilized? I'm okay with the cfgs.

I'm also okay with just u8 being stabilized, as long as I can pack it (not sure if this is possible with atomics)

What do I have to do to get these stabilized?

Any answer on this? cc @alexcrichton

The rfc looks like it's been completely implemented. IMO stabilizing it with target_has_atomic is fine, and we can add further granularity if we need in new rfcs.

Copy link

Member

Author

alexcrichton commented on Dec 16, 2016

@Manishearth state hasn't changed from before. The libs team is essentially waiting for a conclusion on the scenarios story before stabilizing.

Copy link

Member

Manishearth commented on Dec 16, 2016

edited

That discussion has stalled. Last I checked it seemed to mostly have come to consensus on the general idea? Who are we waiting on for a conclusion here? What would the ETA on this be? How can I help?

It also seems like that's mostly orthogonal to this. We can stabilize this using cfgs, and add scenarios later. They seem to be pretty compatible.

(After discussing in IRC, it seems like there have been a lot of recent discussions about this, and the libs team is currently moving on pushing this forward.)

Copy link

Contributor

archshift commented on Jan 15, 2017

Has there been any progress on this since?

Copy link

Contributor

bholley commented on Feb 4, 2017

Just FYI: Per [1], Gecko's Quantum CSS project is no longer blocked on this issue.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1336646

Copy link

Member

nagisa commented on May 7, 2017

We can stabilise AtomicU8 and AtomicI8 the same way we have stabilised AtomicBool – by using Atomic{I/U}Size operations everywhere.

We discussed this in the T-lang meeting today, and though no specific objections to stabilization were discussed, I did have the concern that this feels like it either:

  • does the wrong thing in practice (i.e., does not match libcore's atomic types existing)
  • does precisely the same thing as cfg(accessible) would, in which case this is not particularly useful and seems like not something we should stabilize.

We did not reach any firm conclusions, but my feeling was that we were interested in putting time into cfg accessible rather than into this issue in particular.

That said, if I was wrong in the assumption that this is a 1:1 with cfg(accessible), then we should discuss further.

Removing nomination for now.

Copy link

Contributor

tavianator commented on Nov 5, 2020

The reason I'm asking is that I have recently heard about a paper by concurrency researchers that I would tend to trust, which claimed that this guarantee, which is pretty much the defining characteristic of SeqCst atomic memory ordering on atomic ops, could actually not be fully provided on an architecture as mainstream and concurrency-oriented as POWER.

@HadrienG2 do you have a reference to this paper?

Copy link

Contributor

bstrie commented on Aug 5, 2021

Is there a reason that AtomicU128 is still unavailable on stable?

@bbqsrc This particular issue is currently under the exclusive purview of the lang team, so presumably the libs team doesn't even realize that there's anything libs-related in here. Rather than turn this issue into a joint lang/libs issue, I'd recommend making a new issue for stabilizing AtomicU128 on any supported platforms, which should be tagged with the T-libs label so that the libs team will bring it up for discussion.

Copy link

Contributor

thomcc commented on Dec 10, 2021

That said, if I was wrong in the assumption that this is a 1:1 with cfg(accessible), then we should discuss further.

Sort of, but also sort of not. I think #[cfg(accessible)] is a worse API for this, because it's very easier to check the wrong thing (in this case).

does the wrong thing in practice (i.e., does not match libcore's atomic types existing)

Right, so: target_has_atomic doesn't match libcore's atomic types existing — target_has_atomic_load_store does — target_has_atomic matches libcore's atomic types existing and having RMW/CAS operations.

The distinction is somewhat important, and is the core of my argument (I'm not just nitpicking):

Users are likely to check #[cfg(accessible = core::sync::atomic::AtomicFoo)] rather than realizing that they should check something like #[cfg(accessible = core::sync::atomic::AtomicFoo::compare_exchange)]. This seems extremely easy for someone who doesn't work with this stuff regularly (or does and isn't aware of uncommon targets) to get wrong.

OTOH, the target_has_atomic cfg has the defaults the other way around — by default, the thing with the easy name that sounds like what you want assumes most uses will need RMW/CAS operations. You have to know that the more restricted version exists, which has a longer and more awkward name.

I believe most users wanting atomics will not be experts on the portability story, and will assume that if the type is available, the full API is also available (also, they likely need the RMW ops anyway, as most algorithms involving atomics do). With cfg(accessible) these users will probably have broken code, despite the fact that they went out of their way to handle the case where atomics are not present.

Not to mention, it's probably will feel pretty weird and janky for them to effectively rely on the fact that cfg(accessible = core::sync::atomic::AtomicFoo::compare_exchange) also ensures that the rest of AtomicFoo's API is available. This kind of thing isn't something I'd expect most users to know.

So yeah, IMO target_has_atomic is a harder to misuse API, and does the right thing more of the time, than asking users to manually use cfg(accessible) to do the same thing would be.

thx for coming to my ted talk.


P.S. It's probably neither here nor there, but possibly worth noting: the current situation with cfg(accessible) seems to be that it may not support paths that travel through types, at least initially — this would mean that an equivalent to target_has_atomic_load_store would exist prior to an equivalent to target_has_atomic, which IMO would be bad. That said, cfg(accessible) seems so far away from stabilization that I guess the point is a bit moot anyway.

This has been sitting idle for a long time and really makes the standard library look incomplete.

@tkaitchuck, indeed it does.

I just attempted to make the atomig crate work on embedded targets, only to realize that the target_has_atomic_load_store and target_has_atomic features are unstable. The only workaround I can find uses a build script. Is that really the only solution? That's pretty frustrating. It's been almost 6 years.

Copy link

Member

joshtriplett commented 11 days ago

We discussed this in today's @rust-lang/lang, and we're in favor of stabilizing this.

We'd like to confirm that we'll have documentation (in the Rust documentation for cfg) of exactly what this checks.

Apart from that, let's confirm consensus to stabilize via FCP:

@rfcbot merge

Copy link

rfcbot commented 11 days ago

edited

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

Copy link

Member

scottmcm commented 11 days ago

@rfcbot reviewed

The conversation in the meeting was helpful to me. I was worried that this was covering some complicated list of stuff where we might find another thing later, but since it's all "stuff you can do so long as you have compare-exchange" that seems like a safe definition going forward.

Copy link

Contributor

nikomatsakis commented 11 days ago

@rfcbot reviewed

Copy link

rfcbot commented 11 days ago

bellThis is now entering its final comment period, as per the review above. bell

Copy link

rfcbot commented yesterday

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.


About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK