rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.57k stars 12.62k forks source link

Tracking Issue for arithmetic and certain bitwise ops on `AtomicPtr` #99108

Open WaffleLapkin opened 2 years ago

WaffleLapkin commented 2 years ago

Feature gate: #![feature(strict_provenance_atomic_ptr)]

This is a tracking issue for arithmetic and certain bitwise operations on AtomicPtr. As part of the strict provenance experiment #95228.

This feature adds arithmetic (add, sub) and bitwise (or, end, xor) atomic operations for AtomicPtr in order to replace uses of AtomicUsize-but-actually-a-pointer to preserve provenance information for the compiler and remove usize->ptr casts from existing code.

Arithmetic ops behave as the their non-atomic wrapping versions. Bitwise ops behave as .map_addr(|x| x op val).

Public API

// core::sync::atomic

impl<T> AtomicPtr<T> {
    pub fn fetch_ptr_add(&self, val: usize, order: Ordering) -> *mut T;
    pub fn fetch_ptr_sub(&self, val: usize, order: Ordering) -> *mut T;

    pub fn fetch_byte_add(&self, val: usize, order: Ordering) -> *mut T;
    pub fn fetch_byte_sub(&self, val: usize, order: Ordering) -> *mut T;

    pub fn fetch_or(&self, val: usize, order: Ordering) -> *mut T;
    pub fn fetch_and(&self, val: usize, order: Ordering) -> *mut T;
    pub fn fetch_xor(&self, val: usize, order: Ordering) -> *mut T;
}

Steps / History

Unresolved Questions

RalfJung commented 2 years ago

In terms of implementation details, I think we should also improve the types at which we call the atomic_add and other core atomic intrinsics. Right now we pass them two pointers and then the backend has to know that the left one is actually a pointer and we care about its provenance, and the right one isn't. Ideally we'd give a pointer as first argument and an integer as second argument.

WaffleLapkin commented 2 years ago

"The problem" with making rhs usize is that all atomic intrinsics are currently defined as *mut T -> T -> T, for example: https://github.com/rust-lang/rust/blob/8a392a5992fda3f041726fc85e42569497dfd753/library/core/src/intrinsics.rs#L111

Making the second argument of a different type will probably require compiler changes.

RalfJung commented 2 years ago

Indeed, and I think those compiler changes should be made. :)

WaffleLapkin commented 2 years ago

I think ptr in method names (fetch_ptr_add and fetch_ptr_sub) is confusing, we are not adding/subtracting a pointer but instead just add/sub in units of T. IMO it's too similar to sub_ptr.

ibraheemdev commented 7 months ago

I think the only open question about this API is whether there is a better name for fetch_byte_{add,sub}/fetch_ptr_{add,sub} (https://github.com/rust-lang/libs-team/issues/126#issuecomment-1549797854)? Can we move forward towards stabilization? If those methods are still up for debate, fetch_{or,and,xor} are pretty straightforward additions (and probably the more common operations) so can probably be stabilized.

tgross35 commented 7 months ago

Why not just fetch_wrapping_{add, sub} rather than fetch_ptr_sub? ptr in the name seems redundant, and I would expect fetch_add to be the equivalent of <*const T>::add (not <*const T>::wrapping_add)

WaffleLapkin commented 7 months ago

@tgross35 note that all atomic operations are wrapping, so wrapping is probably even more redundant than ptr 😅

I don't really like _ptr_ either, but it does disambiguate that it's in units of T, as opposed to fetch_byte_add.