Auto merge of #6025 - thomcc:compare_exchange_atomics, r=flip1995

Extend invalid_atomic_ordering for compare_exchange{,_weak} and fetch_update

changelog: The invalid_atomic_ordering lint can now detect misuse of `compare_exchange`, `compare_exchange_weak`, and `fetch_update`.

---

I was surprised not to find an issue or existing support here, since these are the functions which are always hardest to get the ordering right on for me (as the allowed orderings for `fail` depend on the `success` parameter).

I believe this lint now covers all atomic methods which care about their ordering now, but I could be wrong.

Hopefully I didn't forget to do anything for the PR!

Author: bors

clippy_lints/src/atomic_ordering.rs

@@ -8,7 +8,8 @@ use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 declare_clippy_lint! {
     /// **What it does:** Checks for usage of invalid atomic
-    /// ordering in atomic loads/stores and memory fences.
+    /// ordering in atomic loads/stores/exchanges/updates and
+    /// memory fences.
     ///
     /// **Why is this bad?** Using an invalid atomic ordering
     /// will cause a panic at run-time.
@@ -17,22 +18,35 @@ declare_clippy_lint! {
     ///
     /// **Example:**
     /// ```rust,no_run
-    /// # use std::sync::atomic::{self, AtomicBool, Ordering};
+    /// # use std::sync::atomic::{self, AtomicU8, Ordering};
     ///
-    /// let x = AtomicBool::new(true);
+    /// let x = AtomicU8::new(0);
     ///
+    /// // Bad: `Release` and `AcqRel` cannot be used for `load`.
     /// let _ = x.load(Ordering::Release);
     /// let _ = x.load(Ordering::AcqRel);
     ///
-    /// x.store(false, Ordering::Acquire);
-    /// x.store(false, Ordering::AcqRel);
+    /// // Bad: `Acquire` and `AcqRel` cannot be used for `store`.
+    /// x.store(1, Ordering::Acquire);
+    /// x.store(2, Ordering::AcqRel);
     ///
+    /// // Bad: `Relaxed` cannot be used as a fence's ordering.
     /// atomic::fence(Ordering::Relaxed);
     /// atomic::compiler_fence(Ordering::Relaxed);
+    ///
+    /// // Bad: `Release` and `AcqRel` are both always invalid
+    /// // for the failure ordering (the last arg).
+    /// let _ = x.compare_exchange(1, 2, Ordering::SeqCst, Ordering::Release);
+    /// let _ = x.compare_exchange_weak(2, 3, Ordering::AcqRel, Ordering::AcqRel);
+    ///
+    /// // Bad: The failure ordering is not allowed to be
+    /// // stronger than the success order, and `SeqCst` is
+    /// // stronger than `Relaxed`.
+    /// let _ = x.fetch_update(Ordering::Relaxed, Ordering::SeqCst, |val| Some(val + val));
     /// ```
     pub INVALID_ATOMIC_ORDERING,
     correctness,
-    "usage of invalid atomic ordering in atomic loads/stores and memory fences"
+    "usage of invalid atomic ordering in atomic operations and memory fences"
 }
 
 declare_lint_pass!(AtomicOrdering => [INVALID_ATOMIC_ORDERING]);
@@ -127,9 +141,89 @@ fn check_memory_fence(cx: &LateContext<'_>, expr: &Expr<'_>) {
     }
 }
 
+fn opt_ordering_defid(cx: &LateContext<'_>, ord_arg: &Expr<'_>) -> Option<DefId> {
+    if let ExprKind::Path(ref ord_qpath) = ord_arg.kind {
+        cx.qpath_res(ord_qpath, ord_arg.hir_id).opt_def_id()
+    } else {
+        None
+    }
+}
+
+fn check_atomic_compare_exchange(cx: &LateContext<'_>, expr: &Expr<'_>) {
+    if_chain! {
+        if let ExprKind::MethodCall(ref method_path, _, args, _) = &expr.kind;
+        let method = method_path.ident.name.as_str();
+        if type_is_atomic(cx, &args[0]);
+        if method == "compare_exchange" || method == "compare_exchange_weak" || method == "fetch_update";
+        let (success_order_arg, failure_order_arg) = if method == "fetch_update" {
+            (&args[1], &args[2])
+        } else {
+            (&args[3], &args[4])
+        };
+        if let Some(fail_ordering_def_id) = opt_ordering_defid(cx, failure_order_arg);
+        then {
+            // Helper type holding on to some checking and error reporting data. Has
+            // - (success ordering name,
+            // - list of failure orderings forbidden by the success order,
+            // - suggestion message)
+            type OrdLintInfo = (&'static str, &'static [&'static str], &'static str);
+            let relaxed: OrdLintInfo = ("Relaxed", &["SeqCst", "Acquire"], "ordering mode `Relaxed`");
+            let acquire: OrdLintInfo = ("Acquire", &["SeqCst"], "ordering modes `Acquire` or `Relaxed`");
+            let seq_cst: OrdLintInfo = ("SeqCst", &[], "ordering modes `Acquire`, `SeqCst` or `Relaxed`");
+            let release = ("Release", relaxed.1, relaxed.2);
+            let acqrel = ("AcqRel", acquire.1, acquire.2);
+            let search = [relaxed, acquire, seq_cst, release, acqrel];
+
+            let success_lint_info = opt_ordering_defid(cx, success_order_arg)
+                .and_then(|success_ord_def_id| -> Option<OrdLintInfo> {
+                    search
+                        .iter()
+                        .find(|(ordering, ..)| {
+                            match_def_path(cx, success_ord_def_id,
+                                &["core", "sync", "atomic", "Ordering", ordering])
+                        })
+                        .copied()
+                });
+
+            if match_ordering_def_path(cx, fail_ordering_def_id, &["Release", "AcqRel"]) {
+                // If we don't know the success order is, use what we'd suggest
+                // if it were maximally permissive.
+                let suggested = success_lint_info.unwrap_or(seq_cst).2;
+                span_lint_and_help(
+                    cx,
+                    INVALID_ATOMIC_ORDERING,
+                    failure_order_arg.span,
+                    &format!(
+                        "{}'s failure ordering may not be `Release` or `AcqRel`",
+                        method,
+                    ),
+                    None,
+                    &format!("consider using {} instead", suggested),
+                );
+            } else if let Some((success_ord_name, bad_ords_given_success, suggested)) = success_lint_info {
+                if match_ordering_def_path(cx, fail_ordering_def_id, bad_ords_given_success) {
+                    span_lint_and_help(
+                        cx,
+                        INVALID_ATOMIC_ORDERING,
+                        failure_order_arg.span,
+                        &format!(
+                            "{}'s failure ordering may not be stronger than the success ordering of `{}`",
+                            method,
+                            success_ord_name,
+                        ),
+                        None,
+                        &format!("consider using {} instead", suggested),
+                    );
+                }
+            }
+        }
+    }
+}
+
 impl<'tcx> LateLintPass<'tcx> for AtomicOrdering {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         check_atomic_load_store(cx, expr);
         check_memory_fence(cx, expr);
+        check_atomic_compare_exchange(cx, expr);
     }
 }

src/lintlist/mod.rs

@@ -930,7 +930,7 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
     Lint {
         name: "invalid_atomic_ordering",
         group: "correctness",
-        desc: "usage of invalid atomic ordering in atomic loads/stores and memory fences",
+        desc: "usage of invalid atomic ordering in atomic operations and memory fences",
         deprecation: None,
         module: "atomic_ordering",
     },

tests/ui/atomic_ordering_exchange.rs

@@ -0,0 +1,45 @@
+#![warn(clippy::invalid_atomic_ordering)]
+
+use std::sync::atomic::{AtomicUsize, Ordering};
+
+fn main() {
+    // `compare_exchange` (not weak) testing
+    let x = AtomicUsize::new(0);
+
+    // Allowed ordering combos
+    let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::Relaxed);
+    let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::Acquire);
+    let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::Relaxed);
+    let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::Relaxed);
+    let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::Acquire);
+    let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::Relaxed);
+    let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::Relaxed);
+    let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::Acquire);
+    let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::SeqCst);
+
+    // AcqRel is always forbidden as a failure ordering
+    let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::AcqRel);
+    let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::AcqRel);
+    let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::AcqRel);
+    let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::AcqRel);
+    let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::AcqRel);
+
+    // Release is always forbidden as a failure ordering
+    let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::Release);
+    let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::Release);
+    let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::Release);
+    let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::Release);
+    let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::Release);
+
+    // Release success order forbids failure order of Acquire or SeqCst
+    let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::Acquire);
+    let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::SeqCst);
+
+    // Relaxed success order also forbids failure order of Acquire or SeqCst
+    let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::SeqCst);
+    let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::Acquire);
+
+    // Acquire/AcqRel forbids failure order of SeqCst
+    let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::SeqCst);
+    let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::SeqCst);
+}

tests/ui/atomic_ordering_exchange.stderr

@@ -0,0 +1,131 @@
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:21:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::AcqRel);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::invalid-atomic-ordering` implied by `-D warnings`
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:22:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::AcqRel);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:23:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::AcqRel);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:24:56
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::AcqRel);
+   |                                                        ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:25:56
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::AcqRel);
+   |                                                        ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:28:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::Release);
+   |                                                         ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:29:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::Release);
+   |                                                         ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:30:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::Release);
+   |                                                         ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:31:56
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::Release);
+   |                                                        ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be `Release` or `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:32:56
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::Release);
+   |                                                        ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire`, `SeqCst` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be stronger than the success ordering of `Release`
+  --> $DIR/atomic_ordering_exchange.rs:35:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::Acquire);
+   |                                                         ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be stronger than the success ordering of `Release`
+  --> $DIR/atomic_ordering_exchange.rs:36:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::SeqCst);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be stronger than the success ordering of `Relaxed`
+  --> $DIR/atomic_ordering_exchange.rs:39:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::SeqCst);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be stronger than the success ordering of `Relaxed`
+  --> $DIR/atomic_ordering_exchange.rs:40:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::Acquire);
+   |                                                         ^^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering mode `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be stronger than the success ordering of `Acquire`
+  --> $DIR/atomic_ordering_exchange.rs:43:57
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::SeqCst);
+   |                                                         ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: compare_exchange's failure ordering may not be stronger than the success ordering of `AcqRel`
+  --> $DIR/atomic_ordering_exchange.rs:44:56
+   |
+LL |     let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::SeqCst);
+   |                                                        ^^^^^^^^^^^^^^^^
+   |
+   = help: consider using ordering modes `Acquire` or `Relaxed` instead
+
+error: aborting due to 16 previous errors
+

tests/ui/atomic_ordering_exchange_weak.rs

@@ -0,0 +1,47 @@
+#![warn(clippy::invalid_atomic_ordering)]
+
+use std::sync::atomic::{AtomicPtr, Ordering};
+
+fn main() {
+    let ptr = &mut 5;
+    let ptr2 = &mut 10;
+    // `compare_exchange_weak` testing
+    let x = AtomicPtr::new(ptr);
+
+    // Allowed ordering combos
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Relaxed, Ordering::Relaxed);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Acquire, Ordering::Acquire);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Acquire, Ordering::Relaxed);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Release, Ordering::Relaxed);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::AcqRel, Ordering::Acquire);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::AcqRel, Ordering::Relaxed);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::SeqCst, Ordering::Relaxed);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::SeqCst, Ordering::Acquire);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::SeqCst, Ordering::SeqCst);
+
+    // AcqRel is always forbidden as a failure ordering
+    let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Relaxed, Ordering::AcqRel);
+    let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Acquire, Ordering::AcqRel);
+    let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Release, Ordering::AcqRel);
+    let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::AcqRel, Ordering::AcqRel);
+    let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::SeqCst, Ordering::AcqRel);
+
+    // Release is always forbidden as a failure ordering
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Relaxed, Ordering::Release);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Acquire, Ordering::Release);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Release, Ordering::Release);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::AcqRel, Ordering::Release);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::SeqCst, Ordering::Release);
+
+    // Release success order forbids failure order of Acquire or SeqCst
+    let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Release, Ordering::Acquire);
+    let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Release, Ordering::SeqCst);
+
+    // Relaxed success order also forbids failure order of Acquire or SeqCst
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Relaxed, Ordering::SeqCst);
+    let _ = x.compare_exchange_weak(ptr, ptr2, Ordering::Relaxed, Ordering::Acquire);
+
+    // Acquire/AcqRel forbids failure order of SeqCst
+    let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::Acquire, Ordering::SeqCst);
+    let _ = x.compare_exchange_weak(ptr2, ptr, Ordering::AcqRel, Ordering::SeqCst);
+}