Auto merge of rust-lang#79261 - faern:deprecate-compare-and-swap, r=A…

…manieu

Deprecate atomic compare_and_swap method

Finish implementing [RFC 1443](https://github.com/rust-lang/rfcs/blob/master/text/1443-extended-compare-and-swap.md) (rust-lang/rfcs#1443).

It was decided to deprecate `compare_and_swap` [back in Rust 1.12 already](rust-lang#31767 (comment)). I can't find any info about that decision being reverted. My understanding is just that it has been forgotten. If there has been a decision on keeping `compare_and_swap` then it's hard to find, and even if this PR does not go through it can act as a place where people can find out about the decision being reverted.

Atomic operations are hard to understand, very hard. And it does not help that there are multiple similar methods to do compare and swap with. They are so similar that for a reader it might be hard to understand the difference. This PR aims to make that simpler by finally deprecating `compare_and_swap` which is essentially just a more limited version of `compare_exchange`. The documentation is also updated (according to the RFC text) to explain the differences a bit better.

Even if we decide to not deprecate `compare_and_swap`. I still think the documentation for the atomic operations should be improved to better describe their differences and similarities. And the documentation can be written nicer than the PR currently proposes, but I wanted to start somewhere. Most of it is just copied from the RFC.

The documentation for `compare_exchange` and `compare_exchange_weak` indeed describe how they work! The problem is that they are more complex and harder to understand than `compare_and_swap`. So for someone who does not fully grasp this they might fall back to using `compare_and_swap`. Making the documentation outline the similarities and differences might build a bridge for people so they can cross over to the more powerful and sometimes more efficient operations.

The conversions I do to avoid the `std` internal deprecation errors are very straight forward `compare_and_swap -> compare_exchange` changes where the orderings are just using the mapping in the new documentation. Only in one place did I use `compare_exchange_weak`. This can probably be improved further. But the goal here was not for those operations to be perfect. Just to not get worse and to allow the deprecation to happen.

library/core/src/sync/atomic.rs

@@ -464,6 +464,23 @@ impl AtomicBool {
     /// **Note:** This method is only available on platforms that support atomic
     /// operations on `u8`.
     ///
+    /// # Migrating to `compare_exchange` and `compare_exchange_weak`
+    ///
+    /// `compare_and_swap` is equivalent to `compare_exchange` with the following mapping for
+    /// memory orderings:
+    ///
+    /// Original | Success | Failure
+    /// -------- | ------- | -------
+    /// Relaxed  | Relaxed | Relaxed
+    /// Acquire  | Acquire | Acquire
+    /// Release  | Release | Relaxed
+    /// AcqRel   | AcqRel  | Acquire
+    /// SeqCst   | SeqCst  | SeqCst
+    ///
+    /// `compare_exchange_weak` is allowed to fail spuriously even when the comparison succeeds,
+    /// which allows the compiler to generate better assembly code when the compare and swap
+    /// is used in a loop.
+    ///
     /// # Examples
     ///
     /// ```
@@ -479,6 +496,10 @@ impl AtomicBool {
     /// ```
     #[inline]
     #[stable(feature = "rust1", since = "1.0.0")]
+    #[rustc_deprecated(
+        since = "1.50.0",
+        reason = "Use `compare_exchange` or `compare_exchange_weak` instead"
+    )]
     #[cfg(target_has_atomic = "8")]
     pub fn compare_and_swap(&self, current: bool, new: bool, order: Ordering) -> bool {
         match self.compare_exchange(current, new, order, strongest_failure_ordering(order)) {
@@ -493,9 +514,10 @@ impl AtomicBool {
     /// the previous value. On success this value is guaranteed to be equal to `current`.
     ///
     /// `compare_exchange` takes two [`Ordering`] arguments to describe the memory
-    /// ordering of this operation. The first describes the required ordering if the
-    /// operation succeeds while the second describes the required ordering when the
-    /// operation fails. Using [`Acquire`] as success ordering makes the store part
+    /// ordering of this operation. `success` describes the required ordering for the
+    /// read-modify-write operation that takes place if the comparison with `current` succeeds.
+    /// `failure` describes the required ordering for the load operation that takes place when
+    /// the comparison fails. Using [`Acquire`] as success ordering makes the store part
     /// of this operation [`Relaxed`], and using [`Release`] makes the successful load
     /// [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`]
     /// and must be equivalent to or weaker than the success ordering.
@@ -525,6 +547,7 @@ impl AtomicBool {
     /// ```
     #[inline]
     #[stable(feature = "extended_compare_and_swap", since = "1.10.0")]
+    #[doc(alias = "compare_and_swap")]
     #[cfg(target_has_atomic = "8")]
     pub fn compare_exchange(
         &self,
@@ -550,9 +573,10 @@ impl AtomicBool {
     /// previous value.
     ///
     /// `compare_exchange_weak` takes two [`Ordering`] arguments to describe the memory
-    /// ordering of this operation. The first describes the required ordering if the
-    /// operation succeeds while the second describes the required ordering when the
-    /// operation fails. Using [`Acquire`] as success ordering makes the store part
+    /// ordering of this operation. `success` describes the required ordering for the
+    /// read-modify-write operation that takes place if the comparison with `current` succeeds.
+    /// `failure` describes the required ordering for the load operation that takes place when
+    /// the comparison fails. Using [`Acquire`] as success ordering makes the store part
     /// of this operation [`Relaxed`], and using [`Release`] makes the successful load
     /// [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`]
     /// and must be equivalent to or weaker than the success ordering.
@@ -578,6 +602,7 @@ impl AtomicBool {
     /// ```
     #[inline]
     #[stable(feature = "extended_compare_and_swap", since = "1.10.0")]
+    #[doc(alias = "compare_and_swap")]
     #[cfg(target_has_atomic = "8")]
     pub fn compare_exchange_weak(
         &self,
@@ -1066,6 +1091,23 @@ impl<T> AtomicPtr<T> {
     /// **Note:** This method is only available on platforms that support atomic
     /// operations on pointers.
     ///
+    /// # Migrating to `compare_exchange` and `compare_exchange_weak`
+    ///
+    /// `compare_and_swap` is equivalent to `compare_exchange` with the following mapping for
+    /// memory orderings:
+    ///
+    /// Original | Success | Failure
+    /// -------- | ------- | -------
+    /// Relaxed  | Relaxed | Relaxed
+    /// Acquire  | Acquire | Acquire
+    /// Release  | Release | Relaxed
+    /// AcqRel   | AcqRel  | Acquire
+    /// SeqCst   | SeqCst  | SeqCst
+    ///
+    /// `compare_exchange_weak` is allowed to fail spuriously even when the comparison succeeds,
+    /// which allows the compiler to generate better assembly code when the compare and swap
+    /// is used in a loop.
+    ///
     /// # Examples
     ///
     /// ```
@@ -1080,6 +1122,10 @@ impl<T> AtomicPtr<T> {
     /// ```
     #[inline]
     #[stable(feature = "rust1", since = "1.0.0")]
+    #[rustc_deprecated(
+        since = "1.50.0",
+        reason = "Use `compare_exchange` or `compare_exchange_weak` instead"
+    )]
     #[cfg(target_has_atomic = "ptr")]
     pub fn compare_and_swap(&self, current: *mut T, new: *mut T, order: Ordering) -> *mut T {
         match self.compare_exchange(current, new, order, strongest_failure_ordering(order)) {
@@ -1094,9 +1140,10 @@ impl<T> AtomicPtr<T> {
     /// the previous value. On success this value is guaranteed to be equal to `current`.
     ///
     /// `compare_exchange` takes two [`Ordering`] arguments to describe the memory
-    /// ordering of this operation. The first describes the required ordering if the
-    /// operation succeeds while the second describes the required ordering when the
-    /// operation fails. Using [`Acquire`] as success ordering makes the store part
+    /// ordering of this operation. `success` describes the required ordering for the
+    /// read-modify-write operation that takes place if the comparison with `current` succeeds.
+    /// `failure` describes the required ordering for the load operation that takes place when
+    /// the comparison fails. Using [`Acquire`] as success ordering makes the store part
     /// of this operation [`Relaxed`], and using [`Release`] makes the successful load
     /// [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`]
     /// and must be equivalent to or weaker than the success ordering.
@@ -1157,9 +1204,10 @@ impl<T> AtomicPtr<T> {
     /// previous value.
     ///
     /// `compare_exchange_weak` takes two [`Ordering`] arguments to describe the memory
-    /// ordering of this operation. The first describes the required ordering if the
-    /// operation succeeds while the second describes the required ordering when the
-    /// operation fails. Using [`Acquire`] as success ordering makes the store part
+    /// ordering of this operation. `success` describes the required ordering for the
+    /// read-modify-write operation that takes place if the comparison with `current` succeeds.
+    /// `failure` describes the required ordering for the load operation that takes place when
+    /// the comparison fails. Using [`Acquire`] as success ordering makes the store part
     /// of this operation [`Relaxed`], and using [`Release`] makes the successful load
     /// [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`]
     /// and must be equivalent to or weaker than the success ordering.
@@ -1604,6 +1652,23 @@ happens, and using [`Release`] makes the load part [`Relaxed`].
 **Note**: This method is only available on platforms that support atomic
 operations on [`", $s_int_type, "`](", $int_ref, ").
 
+# Migrating to `compare_exchange` and `compare_exchange_weak`
+
+`compare_and_swap` is equivalent to `compare_exchange` with the following mapping for
+memory orderings:
+
+Original | Success | Failure
+-------- | ------- | -------
+Relaxed  | Relaxed | Relaxed
+Acquire  | Acquire | Acquire
+Release  | Release | Relaxed
+AcqRel   | AcqRel  | Acquire
+SeqCst   | SeqCst  | SeqCst
+
+`compare_exchange_weak` is allowed to fail spuriously even when the comparison succeeds,
+which allows the compiler to generate better assembly code when the compare and swap
+is used in a loop.
+
 # Examples
 
 ```
@@ -1619,6 +1684,10 @@ assert_eq!(some_var.load(Ordering::Relaxed), 10);
 ```"),
                 #[inline]
                 #[$stable]
+                #[rustc_deprecated(
+                    since = "1.50.0",
+                    reason = "Use `compare_exchange` or `compare_exchange_weak` instead")
+                ]
                 #[$cfg_cas]
                 pub fn compare_and_swap(&self,
                                         current: $int_type,
@@ -1643,9 +1712,10 @@ containing the previous value. On success this value is guaranteed to be equal t
 `current`.
 
 `compare_exchange` takes two [`Ordering`] arguments to describe the memory
-ordering of this operation. The first describes the required ordering if the
-operation succeeds while the second describes the required ordering when the
-operation fails. Using [`Acquire`] as success ordering makes the store part
+ordering of this operation. `success` describes the required ordering for the
+read-modify-write operation that takes place if the comparison with `current` succeeds.
+`failure` describes the required ordering for the load operation that takes place when
+the comparison fails. Using [`Acquire`] as success ordering makes the store part
 of this operation [`Relaxed`], and using [`Release`] makes the successful load
 [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`]
 and must be equivalent to or weaker than the success ordering.
@@ -1695,9 +1765,10 @@ platforms. The return value is a result indicating whether the new value was
 written and containing the previous value.
 
 `compare_exchange_weak` takes two [`Ordering`] arguments to describe the memory
-ordering of this operation. The first describes the required ordering if the
-operation succeeds while the second describes the required ordering when the
-operation fails. Using [`Acquire`] as success ordering makes the store part
+ordering of this operation. `success` describes the required ordering for the
+read-modify-write operation that takes place if the comparison with `current` succeeds.
+`failure` describes the required ordering for the load operation that takes place when
+the comparison fails. Using [`Acquire`] as success ordering makes the store part
 of this operation [`Relaxed`], and using [`Release`] makes the successful load
 [`Relaxed`]. The failure ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`]
 and must be equivalent to or weaker than the success ordering.

library/core/tests/atomic.rs

@@ -4,11 +4,11 @@ use core::sync::atomic::*;
 #[test]
 fn bool_() {
     let a = AtomicBool::new(false);
-    assert_eq!(a.compare_and_swap(false, true, SeqCst), false);
-    assert_eq!(a.compare_and_swap(false, true, SeqCst), true);
+    assert_eq!(a.compare_exchange(false, true, SeqCst, SeqCst), Ok(false));
+    assert_eq!(a.compare_exchange(false, true, SeqCst, SeqCst), Err(true));
 
     a.store(false, SeqCst);
-    assert_eq!(a.compare_and_swap(false, true, SeqCst), false);
+    assert_eq!(a.compare_exchange(false, true, SeqCst, SeqCst), Ok(false));
 }
 
 #[test]

library/std/src/sync/mpsc/blocking.rs

@@ -36,7 +36,11 @@ pub fn tokens() -> (WaitToken, SignalToken) {
 
 impl SignalToken {
     pub fn signal(&self) -> bool {
-        let wake = !self.inner.woken.compare_and_swap(false, true, Ordering::SeqCst);
+        let wake = self
+            .inner
+            .woken
+            .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst)
+            .is_ok();
         if wake {
             self.inner.thread.unpark();
         }

library/std/src/sync/mpsc/oneshot.rs

@@ -129,7 +129,7 @@ impl<T> Packet<T> {
             let ptr = unsafe { signal_token.cast_to_usize() };
 
             // race with senders to enter the blocking state
-            if self.state.compare_and_swap(EMPTY, ptr, Ordering::SeqCst) == EMPTY {
+            if self.state.compare_exchange(EMPTY, ptr, Ordering::SeqCst, Ordering::SeqCst).is_ok() {
                 if let Some(deadline) = deadline {
                     let timed_out = !wait_token.wait_max_until(deadline);
                     // Try to reset the state
@@ -161,7 +161,12 @@ impl<T> Packet<T> {
                 // the state changes under our feet we'd rather just see that state
                 // change.
                 DATA => {
-                    self.state.compare_and_swap(DATA, EMPTY, Ordering::SeqCst);
+                    let _ = self.state.compare_exchange(
+                        DATA,
+                        EMPTY,
+                        Ordering::SeqCst,
+                        Ordering::SeqCst,
+                    );
                     match (&mut *self.data.get()).take() {
                         Some(data) => Ok(data),
                         None => unreachable!(),
@@ -264,7 +269,10 @@ impl<T> Packet<T> {
 
             // If we've got a blocked thread, then use an atomic to gain ownership
             // of it (may fail)
-            ptr => self.state.compare_and_swap(ptr, EMPTY, Ordering::SeqCst),
+            ptr => self
+                .state
+                .compare_exchange(ptr, EMPTY, Ordering::SeqCst, Ordering::SeqCst)
+                .unwrap_or_else(|x| x),
         };
 
         // Now that we've got ownership of our state, figure out what to do

library/std/src/sync/mpsc/shared.rs

@@ -385,8 +385,15 @@ impl<T> Packet<T> {
         self.port_dropped.store(true, Ordering::SeqCst);
         let mut steals = unsafe { *self.steals.get() };
         while {
-            let cnt = self.cnt.compare_and_swap(steals, DISCONNECTED, Ordering::SeqCst);
-            cnt != DISCONNECTED && cnt != steals
+            match self.cnt.compare_exchange(
+                steals,
+                DISCONNECTED,
+                Ordering::SeqCst,
+                Ordering::SeqCst,
+            ) {
+                Ok(_) => false,
+                Err(old) => old != DISCONNECTED,
+            }
         } {
             // See the discussion in 'try_recv' for why we yield
             // control of this thread.

library/std/src/sync/mpsc/stream.rs

@@ -322,12 +322,15 @@ impl<T> Packet<T> {
         // (because there is a bounded number of senders).
         let mut steals = unsafe { *self.queue.consumer_addition().steals.get() };
         while {
-            let cnt = self.queue.producer_addition().cnt.compare_and_swap(
+            match self.queue.producer_addition().cnt.compare_exchange(
                 steals,
                 DISCONNECTED,
                 Ordering::SeqCst,
-            );
-            cnt != DISCONNECTED && cnt != steals
+                Ordering::SeqCst,
+            ) {
+                Ok(_) => false,
+                Err(old) => old != DISCONNECTED,
+            }
         } {
             while self.queue.pop().is_some() {
                 steals += 1;

library/std/src/sync/once.rs

@@ -65,7 +65,7 @@
 //       must do so with Release ordering to make the result available.
 //     - `wait` inserts `Waiter` nodes as a pointer in `state_and_queue`, and
 //       needs to make the nodes available with Release ordering. The load in
-//       its `compare_and_swap` can be Relaxed because it only has to compare
+//       its `compare_exchange` can be Relaxed because it only has to compare
 //       the atomic, not to read other data.
 //     - `WaiterQueue::Drop` must see the `Waiter` nodes, so it must load
 //       `state_and_queue` with Acquire ordering.
@@ -110,7 +110,7 @@ use crate::thread::{self, Thread};
 /// ```
 #[stable(feature = "rust1", since = "1.0.0")]
 pub struct Once {
-    // `state_and_queue` is actually an a pointer to a `Waiter` with extra state
+    // `state_and_queue` is actually a pointer to a `Waiter` with extra state
     // bits, so we add the `PhantomData` appropriately.
     state_and_queue: AtomicUsize,
     _marker: marker::PhantomData<*const Waiter>,
@@ -395,12 +395,13 @@ impl Once {
                 }
                 POISONED | INCOMPLETE => {
                     // Try to register this thread as the one RUNNING.
-                    let old = self.state_and_queue.compare_and_swap(
+                    let exchange_result = self.state_and_queue.compare_exchange(
                         state_and_queue,
                         RUNNING,
                         Ordering::Acquire,
+                        Ordering::Acquire,
                     );
-                    if old != state_and_queue {
+                    if let Err(old) = exchange_result {
                         state_and_queue = old;
                         continue;
                     }
@@ -452,8 +453,13 @@ fn wait(state_and_queue: &AtomicUsize, mut current_state: usize) {
 
         // Try to slide in the node at the head of the linked list, making sure
         // that another thread didn't just replace the head of the linked list.
-        let old = state_and_queue.compare_and_swap(current_state, me | RUNNING, Ordering::Release);
-        if old != current_state {
+        let exchange_result = state_and_queue.compare_exchange(
+            current_state,
+            me | RUNNING,
+            Ordering::Release,
+            Ordering::Relaxed,
+        );
+        if let Err(old) = exchange_result {
             current_state = old;
             continue;
         }

library/std/src/sys/sgx/abi/mod.rs

@@ -36,20 +36,20 @@ unsafe extern "C" fn tcs_init(secondary: bool) {
     }
 
     // Try to atomically swap UNINIT with BUSY. The returned state can be:
-    match RELOC_STATE.compare_and_swap(UNINIT, BUSY, Ordering::Acquire) {
+    match RELOC_STATE.compare_exchange(UNINIT, BUSY, Ordering::Acquire, Ordering::Acquire) {
         // This thread just obtained the lock and other threads will observe BUSY
-        UNINIT => {
+        Ok(_) => {
             reloc::relocate_elf_rela();
             RELOC_STATE.store(DONE, Ordering::Release);
         }
         // We need to wait until the initialization is done.
-        BUSY => {
+        Err(BUSY) => {
             while RELOC_STATE.load(Ordering::Acquire) == BUSY {
                 core::hint::spin_loop();
             }
         }
         // Initialization is done.
-        DONE => {}
+        Err(DONE) => {}
         _ => unreachable!(),
     }
 }

library/std/src/sys/sgx/waitqueue/spin_mutex.rs

@@ -42,7 +42,7 @@ impl<T> SpinMutex<T> {
 
     #[inline(always)]
     pub fn try_lock(&self) -> Option<SpinMutexGuard<'_, T>> {
-        if !self.lock.compare_and_swap(false, true, Ordering::Acquire) {
+        if self.lock.compare_exchange(false, true, Ordering::Acquire, Ordering::Acquire).is_ok() {
             Some(SpinMutexGuard { mutex: self })
         } else {
             None