Skip to content

Commit 6211599

Browse files
thomccthomcc
committed
Extend invalid_atomic_ordering to detect misuse of compare_exchange{,_weak}
1 parent 8b54f1e commit 6211599

File tree

3 files changed

+423
-2
lines changed

3 files changed

+423
-2
lines changed

‎clippy_lints/src/atomic_ordering.rs

Lines changed: 80 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use rustc_session::{declare_lint_pass, declare_tool_lint};
88

99
declare_clippy_lint! {
1010
/// **What it does:** Checks for usage of invalid atomic
11-
/// ordering in atomic loads/stores and memory fences.
11+
/// ordering in atomic loads/stores/exchanges and memory fences
1212
///
1313
/// **Why is this bad?** Using an invalid atomic ordering
1414
/// will cause a panic at run-time.
@@ -29,10 +29,13 @@ declare_clippy_lint! {
2929
///
3030
/// atomic::fence(Ordering::Relaxed);
3131
/// atomic::compiler_fence(Ordering::Relaxed);
32+
///
33+
/// let _ = x.compare_exchange(false, false, Ordering::Relaxed, Ordering::SeqCst);
34+
/// let _ = x.compare_exchange_weak(false, true, Ordering::SeqCst, Ordering::Release);
3235
/// ```
3336
pub INVALID_ATOMIC_ORDERING,
3437
correctness,
35-
"usage of invalid atomic ordering in atomic loads/stores and memory fences"
38+
"usage of invalid atomic ordering in atomic loads/stores/exchanges ane memory fences"
3639
}
3740

3841
declare_lint_pass!(AtomicOrdering => [INVALID_ATOMIC_ORDERING]);
@@ -127,9 +130,84 @@ fn check_memory_fence(cx: &LateContext<'_>, expr: &Expr<'_>) {
127130
}
128131
}
129132

133+
fn opt_ordering_defid(cx: &LateContext<'_>, ord_arg: &Expr<'_>) -> Option<DefId> {
134+
if let ExprKind::Path(ref ord_qpath) = ord_arg.kind {
135+
cx.qpath_res(ord_qpath, ord_arg.hir_id).opt_def_id()
136+
} else {
137+
None
138+
}
139+
}
140+
fn check_atomic_compare_exchange(cx: &LateContext<'_>, expr: &Expr<'_>) {
141+
if_chain! {
142+
if let ExprKind::MethodCall(ref method_path, _, args, _) = &expr.kind;
143+
let method = method_path.ident.name.as_str();
144+
if type_is_atomic(cx, &args[0]);
145+
if method == "compare_exchange" || method == "compare_exchange_weak";
146+
let failure_order_arg = &args[4];
147+
if let Some(fail_ordering_def_id) = opt_ordering_defid(cx, failure_order_arg);
148+
then {
149+
// Helper type holding on to some checking and error reporting data. Has
150+
// - (success ordering name,
151+
// - list of failure orderings forbidden by the success order,
152+
// - suggestion message)
153+
type OrdLintInfo = (&'static str, &'static [&'static str], &'static str);
154+
let relaxed: OrdLintInfo = ("Relaxed", &["SeqCst", "Acquire"], "ordering mode `Relaxed`");
155+
let acquire: OrdLintInfo = ("Acquire", &["SeqCst"], "ordering modes `Acquire` or `Relaxed`");
156+
let seq_cst: OrdLintInfo = ("SeqCst", &[], "ordering modes `Acquire`, `SeqCst` or `Relaxed`");
157+
let release = ("Release", relaxed.1, relaxed.2);
158+
let acqrel = ("AcqRel", acquire.1, acquire.2);
159+
let search = [relaxed, acquire, seq_cst, release, acqrel];
160+
161+
let success_lint_info = opt_ordering_defid(cx, &args[3])
162+
.and_then(|success_ord_def_id| -> Option<OrdLintInfo> {
163+
search
164+
.iter()
165+
.find(|(ordering, ..)| {
166+
match_def_path(cx, success_ord_def_id,
167+
&["core", "sync", "atomic", "Ordering", ordering])
168+
})
169+
.copied()
170+
});
171+
172+
if match_ordering_def_path(cx, fail_ordering_def_id, &["Release", "AcqRel"]) {
173+
// If we don't know the success order is, use what we'd suggest
174+
// if it were maximally permissive.
175+
let suggested = success_lint_info.unwrap_or(seq_cst).2;
176+
span_lint_and_help(
177+
cx,
178+
INVALID_ATOMIC_ORDERING,
179+
failure_order_arg.span,
180+
&format!(
181+
"{}'s failure ordering may not be `Release` or `AcqRel`",
182+
method,
183+
),
184+
None,
185+
&format!("consider using {} instead", suggested),
186+
);
187+
} else if let Some((success_ord_name, bad_ords_given_success, suggested)) = success_lint_info {
188+
if match_ordering_def_path(cx, fail_ordering_def_id, bad_ords_given_success) {
189+
span_lint_and_help(
190+
cx,
191+
INVALID_ATOMIC_ORDERING,
192+
failure_order_arg.span,
193+
&format!(
194+
"{}'s failure ordering may not stronger than the success ordering of `{}`",
195+
method,
196+
success_ord_name,
197+
),
198+
None,
199+
&format!("consider using {} instead", suggested),
200+
);
201+
}
202+
}
203+
}
204+
}
205+
}
206+
130207
impl<'tcx> LateLintPass<'tcx> for AtomicOrdering {
131208
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
132209
check_atomic_load_store(cx, expr);
133210
check_memory_fence(cx, expr);
211+
check_atomic_compare_exchange(cx, expr);
134212
}
135213
}

‎tests/ui/atomic_ordering_exchange.rs

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
#![warn(clippy::invalid_atomic_ordering)]
2+
3+
use std::sync::atomic::{AtomicUsize, Ordering};
4+
5+
fn main() {
6+
// `compare_exchange` (not weak) testing
7+
let x = AtomicUsize::new(0);
8+
9+
// Allowed ordering combos
10+
let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::Relaxed);
11+
let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::Acquire);
12+
let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::Relaxed);
13+
let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::Relaxed);
14+
let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::Acquire);
15+
let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::Relaxed);
16+
let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::Relaxed);
17+
let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::Acquire);
18+
let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::SeqCst);
19+
20+
// AcqRel is always forbidden as a failure ordering
21+
let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::AcqRel);
22+
let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::AcqRel);
23+
let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::AcqRel);
24+
let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::AcqRel);
25+
let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::AcqRel);
26+
27+
// Release is always forbidden as a failure ordering
28+
let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::Release);
29+
let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::Release);
30+
let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::Release);
31+
let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::Release);
32+
let _ = x.compare_exchange(0, 0, Ordering::SeqCst, Ordering::Release);
33+
34+
// Release success order forbids failure order of Acquire or SeqCst
35+
let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::Acquire);
36+
let _ = x.compare_exchange(0, 0, Ordering::Release, Ordering::SeqCst);
37+
38+
// Relaxed success order also forbids failure order of Acquire or SeqCst
39+
let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::SeqCst);
40+
let _ = x.compare_exchange(0, 0, Ordering::Relaxed, Ordering::Acquire);
41+
42+
// Acquire/AcqRel forbids failure order of SeqCst
43+
let _ = x.compare_exchange(0, 0, Ordering::Acquire, Ordering::SeqCst);
44+
let _ = x.compare_exchange(0, 0, Ordering::AcqRel, Ordering::SeqCst);
45+
46+
// compare_exchange_weak tests
47+
48+
// Allowed ordering combos
49+
let _ = x.compare_exchange_weak(0, 0, Ordering::Relaxed, Ordering::Relaxed);
50+
let _ = x.compare_exchange_weak(0, 0, Ordering::Acquire, Ordering::Acquire);
51+
let _ = x.compare_exchange_weak(0, 0, Ordering::Acquire, Ordering::Relaxed);
52+
let _ = x.compare_exchange_weak(0, 0, Ordering::Release, Ordering::Relaxed);
53+
let _ = x.compare_exchange_weak(0, 0, Ordering::AcqRel, Ordering::Acquire);
54+
let _ = x.compare_exchange_weak(0, 0, Ordering::AcqRel, Ordering::Relaxed);
55+
let _ = x.compare_exchange_weak(0, 0, Ordering::SeqCst, Ordering::Relaxed);
56+
let _ = x.compare_exchange_weak(0, 0, Ordering::SeqCst, Ordering::Acquire);
57+
let _ = x.compare_exchange_weak(0, 0, Ordering::SeqCst, Ordering::SeqCst);
58+
59+
// AcqRel is always forbidden as a failure ordering
60+
let _ = x.compare_exchange_weak(0, 0, Ordering::Relaxed, Ordering::AcqRel);
61+
let _ = x.compare_exchange_weak(0, 0, Ordering::Acquire, Ordering::AcqRel);
62+
let _ = x.compare_exchange_weak(0, 0, Ordering::Release, Ordering::AcqRel);
63+
let _ = x.compare_exchange_weak(0, 0, Ordering::AcqRel, Ordering::AcqRel);
64+
let _ = x.compare_exchange_weak(0, 0, Ordering::SeqCst, Ordering::AcqRel);
65+
66+
// Release is always forbidden as a failure ordering
67+
let _ = x.compare_exchange_weak(0, 0, Ordering::Relaxed, Ordering::Release);
68+
let _ = x.compare_exchange_weak(0, 0, Ordering::Acquire, Ordering::Release);
69+
let _ = x.compare_exchange_weak(0, 0, Ordering::Release, Ordering::Release);
70+
let _ = x.compare_exchange_weak(0, 0, Ordering::AcqRel, Ordering::Release);
71+
let _ = x.compare_exchange_weak(0, 0, Ordering::SeqCst, Ordering::Release);
72+
73+
// Release success order forbids failure order of Acquire or SeqCst
74+
let _ = x.compare_exchange_weak(0, 0, Ordering::Release, Ordering::Acquire);
75+
let _ = x.compare_exchange_weak(0, 0, Ordering::Release, Ordering::SeqCst);
76+
77+
// Relaxed success order also forbids failure order of Acquire or SeqCst
78+
let _ = x.compare_exchange_weak(0, 0, Ordering::Relaxed, Ordering::SeqCst);
79+
let _ = x.compare_exchange_weak(0, 0, Ordering::Relaxed, Ordering::Acquire);
80+
81+
// Acquire/AcqRel forbids failure order of SeqCst
82+
let _ = x.compare_exchange_weak(0, 0, Ordering::Acquire, Ordering::SeqCst);
83+
let _ = x.compare_exchange_weak(0, 0, Ordering::AcqRel, Ordering::SeqCst);
84+
}

0 commit comments

Comments
 (0)