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

Added missing atomic operations to atomic types. #10154

Closed
wants to merge 1 commit into from
Closed

Added missing atomic operations to atomic types. #10154

wants to merge 1 commit into from

Conversation

alchemy
Copy link

@alchemy alchemy commented Oct 29, 2013

AtomicPtr now has fetch_add and fetch_sub for pointer arithmetic.
AtomicInt and AtomicUint now have fetch_min, fetch_max and
fetch_and, fetch_nand, fetch_or, fetch_xor for bitwise logical
operations.

Closes #7421

AtomicPtr now has fetch_add and fetch_sub for pointer arithmetic.
AtomicInt and AtomicUint now have fetch_min, fetch_max and
fetch_and, fetch_nand, fetch_or, fetch_xor for bitwise logical
operations.

Closes #7421
@sanxiyn
Copy link
Member

sanxiyn commented Oct 30, 2013

I am worried that this will break ARM build again. See #8151 for details.

@emberian
Copy link
Member

@sanxiyn would cfg-ing it out for ARM be acceptable?

@sanxiyn
Copy link
Member

sanxiyn commented Nov 1, 2013

I would like {max, min, umax, umin} remain unimplemented until either 1. ARM problem is solved, or 2. we have actual uses for them.

@alchemy
Copy link
Author

alchemy commented Nov 1, 2013

I vote for "cfg-ing" them out. In any case, when a consensus will be
reached, i will amend my commit accordingly.
Il 01/nov/2013 03:20 "Seo Sanghyeon" notifications@github.com ha scritto:

I would like {max, min, umax, umin} remain unimplemented until either 1.
ARM problem is solved, or 2. we have actual uses for them.


Reply to this email directly or view it on GitHubhttps://github.com//pull/10154#issuecomment-27543799
.

@alexcrichton
Copy link
Member

It's unfortunate if we advertise ourselves as being a portable library if we have intrinsics that are cfg-ed off. It's true that this is all inside the unstable portions of the library, but I would personally rather see the API the same across all platforms, and just implemented differently on some.

@emberian
Copy link
Member

emberian commented Nov 1, 2013

As long as we expose the LLVM intrinsics, they can be in another crate.

On Fri, Nov 1, 2013 at 12:25 PM, Alex Crichton notifications@github.comwrote:

It's unfortunate if we advertise ourselves as being a portable library if
we have intrinsics that are cfg-ed off. It's true that this is all inside
the unstable portions of the library, but I would personally rather see the
API the same across all platforms, and just implemented differently on some.


Reply to this email directly or view it on GitHubhttps://github.com//pull/10154#issuecomment-27578310
.

@brson
Copy link
Contributor

brson commented Nov 1, 2013

I also do not want to have atomics that can't be supported on some architectures. Let's start building for more recent ARM architectures, decide if we can not support those that don't have these features, and otherwise figure out how to provide the missing polyfills.

@Aatch
Copy link
Contributor

Aatch commented Nov 5, 2013

My suggestion is to leave the intrinsics as-is. Anybody using them directly accepts the risk that they may not work. I agree though that higher-level API should stay as portable as is reasonable (i.e. if you use atomic types on a platform with literally no atomic support, you deserve what you get)

@alchemy
Copy link
Author

alchemy commented Nov 6, 2013

This document from ARM says that modern atomic instructions where introduced with the ARMv6 architecture, which was announced in October 2001.
Here can be found a list of devices based on the different ARM generations (just to have an idea of what kind of devices would be left behind if we decide to support only the newer archs).

@alexcrichton
Copy link
Member

Closing due to inactivity and because this should be able to work on all platforms. Feel free to reopen if you have updates, though!

@thestinger
Copy link
Contributor

I agree that we should be careful about venturing outside of the atomics defined by C11/C++11. These are sure to be widely supported in any future architectures, but I wouldn't be so sure about any extra additions.

@jonhoo
Copy link
Contributor

jonhoo commented Apr 10, 2020

Should this be re-opened following #12949?
These operations are necessary to support atomic pointers with pointer provenance tracking through miri (which, for example, crossbeam needs to be miri compatible).

@RalfJung
Copy link
Member

In terms of portability, I assume if we have AtomicUsize::fetch_xor then we can have it on AtomicPtr as well.

However, @jonhoo talking about provenance honestly makes little sense when XOR is involved.

jonhoo added a commit to jonhoo/crossbeam that referenced this pull request Apr 10, 2020
Pointers stored as `usize` tend to cause miri to lose pointer provenance
tracking, which means we can't take advantage of its checking! See also
the discussion at
rust-lang/miri#940 (comment).

This does not yet compile since `AtomicPtr` does not have `fetch_*`
methods. They were added and then removed from the standard library back
in the day (rust-lang/rust#10154), but I think
the reason they were removed has now been remedied, so they will
hopefully come back again!
@jonhoo
Copy link
Contributor

jonhoo commented Apr 10, 2020

Yeah, I may have been overly eager when suggesting this would give us provenance. Though it is the case that this is currently something that's forcing libraries to use AtomicUsize, which has even less hope of tracking provenance :p

@jonhoo
Copy link
Contributor

jonhoo commented Apr 10, 2020

@RalfJung I posted #71004 because I think this'd be a good thing to add independently of provenance :)

jonhoo added a commit to jonhoo/rust that referenced this pull request Apr 10, 2020
This adds various `fetch_` methods to `AtomicPtr` that are present on
other `Atomic*` types. It does so such that libraries that depend on
atomic operations on pointers do not have to cast those pointers to
`usize` and fiddle around with `AtomicUsize` instead.

Note that this patch currently implements `fetch_add` and `fetch_sub`
without considering the size of the pointer target. This is unlike
regular pointer additions and subtractions. The rationale for this is
that for atomic operations, the user may indeed wish to truly increment
by 1, which is difficult if all deltas are interpreted in increments of
the type's size.

This patch effectively resurrects the change from rust-lang#10154. Based on
rust-lang#12949 (comment),
the rationale for not making the changes at the time no longer hold.
@RalfJung
Copy link
Member

These operations are necessary to support atomic pointers with pointer provenance tracking through miri (which, for example, crossbeam needs to be miri compatible).

See rust-lang/miri#1318 for the Miri-side issue.

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

Successfully merging this pull request may close these issues.

Add remaining operations to atomic types.
9 participants