Add missing atomic operations to AtomicPtr

[jonhoo]

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 #10154. Based on
#12949 (comment),
the rationale for not making the changes at the time no longer hold.

[rust-highfive]

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

[Centril]

cc @RalfJung

[RalfJung]

Honestly I don't think this really helps for crossbeam-rs/crossbeam#464.

And it seems rather inconsistent to have fetch_xor on pointers when pointer do not implement the BitXor trait (and likewise for the other operations).

[jonhoo]

Yeah, adding these is more to allow libraries that need atomic ops on pointers not have to use AtomicUsize than to support miri. I agree that it's inconsistent given that pointers do not implement BitXor, though it seems to me (perhaps naively) that the fix there would be to make non-atomic pointers implement BitXor as well?

[shepmaster]

Reassigning as this is above my paygrade...

r? @sfackler

[RalfJung]

though it seems to me (perhaps naively) that the fix there would be to make non-atomic pointers implement BitXor as well?

Maybe. OTOH, these cannot actually be implemented without internally casting to usize and doing the bit operations there, so in terms of making int-ptr-casts explicit to the user, that would be a step backwards.

[jonhoo]

in terms of making int-ptr-casts explicit to the user, that would be a step backwards.

Hmm, yeah, that's a good point. I honestly don't know what the "right thing" to do here is. It seems weird to force users to turn pointers into numbers to add to them atomically, but maybe it does make sense to force them to do it to do things like XORs? It's a strange line to draw though I think.

One question that comes to mind for me here is whether there is an advantage to rustc to know that a bitwise XOR/AND/OR is happening specifically to a pointer. If we make the user do the cast, then the compiler sort of loses insight into that fact, whereas if we do the cast internally, then at least we know that's what's going on. I don't know if that buys us anything, at least at the moment, but keeping that internally in the compiler seems like it gives the compiler strictly more information?

[RalfJung]

keeping that internally in the compiler seems like it gives the compiler strictly more information?

Well, it could. But I currently don't see anything we could do with that information, amongst the various proposals for int-ptr-cast semantics that I have seen or worked on (excluding the clearly insane ones...).

[jonhoo]

I guess the question then is whether we then continue to keep the conversion explicit, or whether we make the APIs ready for some such improvement potentially coming down the line, to try and pre-emptively make people use the "more semantically rich" API.

[joelpalmer]

Ping from Triage: Any update on this or movement offline, @jonhoo?

[jonhoo]

I think fetch_add and fetch_sub are probably pretty uncontroversial changes here, but they also may not add much. fetch_xor and friends are trickier. I honestly don't know. I defer to @RalfJung on whether this is a worthwhile direction

[RalfJung]

I don't think Xor, BitAnd etc on pointers are meaningful, they basically can only be explained via "cast to integer, do thing, cast back".

For add and sub, I agree these could be meaningful. I am not sure what the motivation for adding them is, though -- what is the problem this is solving? This got triggered by Miri having issues with leak detecting when atomic operations are performed on pointers (rust-lang/miri#1350), but the PR doesn't really help much with that. It would have to replace some more ptr <-> int casts by transmutes (#70765), which is sketchy at best. Ideally LLVM would provide atomic operations on pointers directly so we wouldn't have to hack around the lack of that in Rust.

Besides, the fact that they work in bytes when usually our pointer arithmetic works in units of T is quite the footgun. Also, which of the core::ptr methods do they correspond to -- the ones with or without inbounds requirement?

[jonhoo]

I would say that adding these methods is not to explicitly address a problem, but rather to remove a restriction wherein users currently have to cast pointers to usize in order to perform atomic operations on them. The underlying hope being that this introduces additional semantic information that may be useful later.

[sfackler]

Why would this divergence from normal pointers be preferable?

[jonhoo]

Ah, sorry, I wrote this in the PR/commit, but not in the docs:

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.

[sfackler]

Why would a user of a pointer-that-is-atomic want to increment by 1 and a user of a pointer-that-is-not-atomic not want to increment by 1?

[sfackler]

This doesn't seem to have anything to do with the actual sources of unsafety in the block.

[jonhoo]

Ah, that safety comment mirrors that of the other atomic ops, and reflects the reason the atomic_add unsafety is safe. Are you referring to the additional unsafety in transmuting from a usize to a *mut T?

[sfackler]

The unsafe calls in that block are 1. transmuting, and 2. calling the unsafe intrinsic that takes a raw pointer.

[CodesInChaos]

I'd expect the semantics of this to match those of raw pointers, i.e. fetch_{offset, add, sub} must be in bounds and thus are unsafe, while fetch_{offset, add, sub}_wrapping don't require this. The benefit of add/sub seems rather limited, so I'd leave it at offset/offset_wrapping.

If you want to add/sub bytes, the name should call that out, (e.g. fetch_add_bytes). However I can't come up with a usecase for those at all.

In general I'm not convinced that this feature has many use-cases.

[jonhoo]

Okay, it sounds like the building consensus is that this feature is probably not useful or helpful in the near future, and raises more questions than it answers, so I'm going to go ahead and close this. We can always resurrect it should things change down the road. Thanks for the feedback!