Skip to content

Commit 9e6a606

Browse files
krishna-veerareddykrishna-veerareddy
committed
Add lint to detect usage of invalid atomic ordering
Detect usage of invalid atomic ordering modes such as `Ordering::{Release, AcqRel}` in atomic loads and `Ordering::{Acquire, AcqRel}` in atomic stores.
1 parent 62ff639 commit 9e6a606

File tree

7 files changed

+700
-2
lines changed

7 files changed

+700
-2
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1134,6 +1134,7 @@ Released 2018-09-13
11341134
[`integer_division`]: https://rust-lang.github.io/rust-clippy/master/index.html#integer_division
11351135
[`into_iter_on_array`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_array
11361136
[`into_iter_on_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#into_iter_on_ref
1137+
[`invalid_atomic_ordering`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_atomic_ordering
11371138
[`invalid_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_ref
11381139
[`invalid_regex`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_regex
11391140
[`invalid_upcast_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_upcast_comparisons

‎README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
88

9-
[There are 344 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
9+
[There are 345 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
1010

1111
We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
1212

‎clippy_lints/src/atomic_ordering.rs

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
use crate::utils::{match_def_path, span_help_and_lint};
2+
use if_chain::if_chain;
3+
use rustc::declare_lint_pass;
4+
use rustc::hir::def_id::DefId;
5+
use rustc::hir::*;
6+
use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
7+
use rustc::ty;
8+
use rustc_session::declare_tool_lint;
9+
10+
declare_clippy_lint! {
11+
/// **What it does:** Checks for usage of invalid atomic
12+
/// ordering in Atomic*::{load, store} calls.
13+
///
14+
/// **Why is this bad?** Using an invalid atomic ordering
15+
/// will cause a panic at run-time.
16+
///
17+
/// **Known problems:** None.
18+
///
19+
/// **Example:**
20+
/// ```rust,ignore
21+
/// # use std::sync::atomic::{AtomicBool, Ordering};
22+
///
23+
/// let x = AtomicBool::new(true);
24+
///
25+
/// let _ = x.load(Ordering::Release);
26+
/// let _ = x.load(Ordering::AcqRel);
27+
///
28+
/// x.store(false, Ordering::Acquire);
29+
/// x.store(false, Ordering::AcqRel);
30+
/// ```
31+
pub INVALID_ATOMIC_ORDERING,
32+
correctness,
33+
"lint usage of invalid atomic ordering in atomic load/store calls"
34+
}
35+
36+
declare_lint_pass!(AtomicOrdering => [INVALID_ATOMIC_ORDERING]);
37+
38+
const ATOMIC_TYPES: [&str; 12] = [
39+
"AtomicBool",
40+
"AtomicI8",
41+
"AtomicI16",
42+
"AtomicI32",
43+
"AtomicI64",
44+
"AtomicIsize",
45+
"AtomicPtr",
46+
"AtomicU8",
47+
"AtomicU16",
48+
"AtomicU32",
49+
"AtomicU64",
50+
"AtomicUsize",
51+
];
52+
53+
fn type_is_atomic(cx: &LateContext<'_, '_>, expr: &Expr<'_>) -> bool {
54+
if let ty::Adt(&ty::AdtDef { did, .. }, _) = cx.tables.expr_ty(expr).kind {
55+
ATOMIC_TYPES
56+
.iter()
57+
.any(|ty| match_def_path(cx, did, &["core", "sync", "atomic", ty]))
58+
} else {
59+
false
60+
}
61+
}
62+
63+
fn match_ordering_def_path(cx: &LateContext<'_, '_>, did: DefId, orderings: &[&str]) -> bool {
64+
orderings
65+
.iter()
66+
.any(|ordering| match_def_path(cx, did, &["core", "sync", "atomic", "Ordering", ordering]))
67+
}
68+
69+
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AtomicOrdering {
70+
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
71+
if_chain! {
72+
if let ExprKind::MethodCall(ref method_path, _, args) = &expr.kind;
73+
let method = method_path.ident.name.as_str();
74+
if type_is_atomic(cx, &args[0]);
75+
if method == "load" || method == "store";
76+
let ordering_arg = if method == "load" { &args[1] } else { &args[2] };
77+
if let ExprKind::Path(ref ordering_qpath) = ordering_arg.kind;
78+
if let Some(ordering_def_id) = cx.tables.qpath_res(ordering_qpath, ordering_arg.hir_id).opt_def_id();
79+
then {
80+
if method == "load" &&
81+
match_ordering_def_path(cx, ordering_def_id, &["Release", "AcqRel"]) {
82+
span_help_and_lint(
83+
cx,
84+
INVALID_ATOMIC_ORDERING,
85+
ordering_arg.span,
86+
"atomic loads cannot have `Release` and `AcqRel` ordering",
87+
"consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`"
88+
);
89+
} else if method == "store" &&
90+
match_ordering_def_path(cx, ordering_def_id, &["Acquire", "AcqRel"]) {
91+
span_help_and_lint(
92+
cx,
93+
INVALID_ATOMIC_ORDERING,
94+
ordering_arg.span,
95+
"atomic stores cannot have `Acquire` and `AcqRel` ordering",
96+
"consider using ordering modes `Release`, `SeqCst` or `Relaxed`"
97+
);
98+
}
99+
}
100+
}
101+
}
102+
}

‎clippy_lints/src/lib.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,7 @@ pub mod arithmetic;
165165
pub mod as_conversions;
166166
pub mod assertions_on_constants;
167167
pub mod assign_ops;
168+
pub mod atomic_ordering;
168169
pub mod attrs;
169170
pub mod bit_mask;
170171
pub mod blacklisted_name;
@@ -466,6 +467,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
466467
&assertions_on_constants::ASSERTIONS_ON_CONSTANTS,
467468
&assign_ops::ASSIGN_OP_PATTERN,
468469
&assign_ops::MISREFACTORED_ASSIGN_OP,
470+
&atomic_ordering::INVALID_ATOMIC_ORDERING,
469471
&attrs::DEPRECATED_CFG_ATTR,
470472
&attrs::DEPRECATED_SEMVER,
471473
&attrs::EMPTY_LINE_AFTER_OUTER_ATTR,
@@ -984,6 +986,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
984986
store.register_early_pass(|| box as_conversions::AsConversions);
985987
store.register_early_pass(|| box utils::internal_lints::ProduceIce);
986988
store.register_late_pass(|| box let_underscore::LetUnderscore);
989+
store.register_late_pass(|| box atomic_ordering::AtomicOrdering);
987990

988991
store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
989992
LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@@ -1089,6 +1092,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
10891092
LintId::of(&assertions_on_constants::ASSERTIONS_ON_CONSTANTS),
10901093
LintId::of(&assign_ops::ASSIGN_OP_PATTERN),
10911094
LintId::of(&assign_ops::MISREFACTORED_ASSIGN_OP),
1095+
LintId::of(&atomic_ordering::INVALID_ATOMIC_ORDERING),
10921096
LintId::of(&attrs::DEPRECATED_CFG_ATTR),
10931097
LintId::of(&attrs::DEPRECATED_SEMVER),
10941098
LintId::of(&attrs::UNKNOWN_CLIPPY_LINTS),
@@ -1509,6 +1513,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf
15091513

15101514
store.register_group(true, "clippy::correctness", Some("clippy_correctness"), vec![
15111515
LintId::of(&approx_const::APPROX_CONSTANT),
1516+
LintId::of(&atomic_ordering::INVALID_ATOMIC_ORDERING),
15121517
LintId::of(&attrs::DEPRECATED_SEMVER),
15131518
LintId::of(&attrs::USELESS_ATTRIBUTE),
15141519
LintId::of(&bit_mask::BAD_BIT_MASK),

‎src/lintlist/mod.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ pub use lint::Lint;
66
pub use lint::LINT_LEVELS;
77

88
// begin lint list, do not remove this comment, it’s used in `update_lints`
9-
pub const ALL_LINTS: [Lint; 344] = [
9+
pub const ALL_LINTS: [Lint; 345] = [
1010
Lint {
1111
name: "absurd_extreme_comparisons",
1212
group: "correctness",
@@ -833,6 +833,13 @@ pub const ALL_LINTS: [Lint; 344] = [
833833
deprecation: None,
834834
module: "methods",
835835
},
836+
Lint {
837+
name: "invalid_atomic_ordering",
838+
group: "correctness",
839+
desc: "lint usage of invalid atomic ordering in atomic load/store calls",
840+
deprecation: None,
841+
module: "atomic_ordering",
842+
},
836843
Lint {
837844
name: "invalid_regex",
838845
group: "correctness",

‎tests/ui/atomic_ordering.rs

Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,196 @@
1+
#![warn(clippy::invalid_atomic_ordering)]
2+
3+
use std::sync::atomic::{
4+
AtomicBool, AtomicI16, AtomicI32, AtomicI64, AtomicI8, AtomicIsize, AtomicPtr, AtomicU16, AtomicU32, AtomicU64,
5+
AtomicU8, AtomicUsize, Ordering,
6+
};
7+
8+
fn main() {
9+
// `AtomicBool` test cases
10+
let x = AtomicBool::new(true);
11+
12+
// Allowed load ordering modes
13+
let _ = x.load(Ordering::Acquire);
14+
let _ = x.load(Ordering::SeqCst);
15+
let _ = x.load(Ordering::Relaxed);
16+
17+
// Disallowed load ordering modes
18+
let _ = x.load(Ordering::Release);
19+
let _ = x.load(Ordering::AcqRel);
20+
21+
// Allowed store ordering modes
22+
x.store(false, Ordering::Release);
23+
x.store(false, Ordering::SeqCst);
24+
x.store(false, Ordering::Relaxed);
25+
26+
// Disallowed store ordering modes
27+
x.store(false, Ordering::Acquire);
28+
x.store(false, Ordering::AcqRel);
29+
30+
// `AtomicI8` test cases
31+
let x = AtomicI8::new(0);
32+
33+
let _ = x.load(Ordering::Acquire);
34+
let _ = x.load(Ordering::SeqCst);
35+
let _ = x.load(Ordering::Relaxed);
36+
let _ = x.load(Ordering::Release);
37+
let _ = x.load(Ordering::AcqRel);
38+
39+
x.store(1, Ordering::Release);
40+
x.store(1, Ordering::SeqCst);
41+
x.store(1, Ordering::Relaxed);
42+
x.store(1, Ordering::Acquire);
43+
x.store(1, Ordering::AcqRel);
44+
45+
// `AtomicI16` test cases
46+
let x = AtomicI16::new(0);
47+
48+
let _ = x.load(Ordering::Acquire);
49+
let _ = x.load(Ordering::SeqCst);
50+
let _ = x.load(Ordering::Relaxed);
51+
let _ = x.load(Ordering::Release);
52+
let _ = x.load(Ordering::AcqRel);
53+
54+
x.store(1, Ordering::Release);
55+
x.store(1, Ordering::SeqCst);
56+
x.store(1, Ordering::Relaxed);
57+
x.store(1, Ordering::Acquire);
58+
x.store(1, Ordering::AcqRel);
59+
60+
// `AtomicI32` test cases
61+
let x = AtomicI32::new(0);
62+
63+
let _ = x.load(Ordering::Acquire);
64+
let _ = x.load(Ordering::SeqCst);
65+
let _ = x.load(Ordering::Relaxed);
66+
let _ = x.load(Ordering::Release);
67+
let _ = x.load(Ordering::AcqRel);
68+
69+
x.store(1, Ordering::Release);
70+
x.store(1, Ordering::SeqCst);
71+
x.store(1, Ordering::Relaxed);
72+
x.store(1, Ordering::Acquire);
73+
x.store(1, Ordering::AcqRel);
74+
75+
// `AtomicI64` test cases
76+
let x = AtomicI64::new(0);
77+
78+
let _ = x.load(Ordering::Acquire);
79+
let _ = x.load(Ordering::SeqCst);
80+
let _ = x.load(Ordering::Relaxed);
81+
let _ = x.load(Ordering::Release);
82+
let _ = x.load(Ordering::AcqRel);
83+
84+
x.store(1, Ordering::Release);
85+
x.store(1, Ordering::SeqCst);
86+
x.store(1, Ordering::Relaxed);
87+
x.store(1, Ordering::Acquire);
88+
x.store(1, Ordering::AcqRel);
89+
90+
// `AtomicIsize` test cases
91+
let x = AtomicIsize::new(0);
92+
93+
let _ = x.load(Ordering::Acquire);
94+
let _ = x.load(Ordering::SeqCst);
95+
let _ = x.load(Ordering::Relaxed);
96+
let _ = x.load(Ordering::Release);
97+
let _ = x.load(Ordering::AcqRel);
98+
99+
x.store(1, Ordering::Release);
100+
x.store(1, Ordering::SeqCst);
101+
x.store(1, Ordering::Relaxed);
102+
x.store(1, Ordering::Acquire);
103+
x.store(1, Ordering::AcqRel);
104+
105+
// `AtomicPtr` test cases
106+
let ptr = &mut 5;
107+
let other_ptr = &mut 10;
108+
let x = AtomicPtr::new(ptr);
109+
110+
let _ = x.load(Ordering::Acquire);
111+
let _ = x.load(Ordering::SeqCst);
112+
let _ = x.load(Ordering::Relaxed);
113+
let _ = x.load(Ordering::Release);
114+
let _ = x.load(Ordering::AcqRel);
115+
116+
x.store(other_ptr, Ordering::Release);
117+
x.store(other_ptr, Ordering::SeqCst);
118+
x.store(other_ptr, Ordering::Relaxed);
119+
x.store(other_ptr, Ordering::Acquire);
120+
x.store(other_ptr, Ordering::AcqRel);
121+
122+
// `AtomicU8` test cases
123+
let x = AtomicU8::new(0);
124+
125+
let _ = x.load(Ordering::Acquire);
126+
let _ = x.load(Ordering::SeqCst);
127+
let _ = x.load(Ordering::Relaxed);
128+
let _ = x.load(Ordering::Release);
129+
let _ = x.load(Ordering::AcqRel);
130+
131+
x.store(1, Ordering::Release);
132+
x.store(1, Ordering::SeqCst);
133+
x.store(1, Ordering::Relaxed);
134+
x.store(1, Ordering::Acquire);
135+
x.store(1, Ordering::AcqRel);
136+
137+
// `AtomicU16` test cases
138+
let x = AtomicU16::new(0);
139+
140+
let _ = x.load(Ordering::Acquire);
141+
let _ = x.load(Ordering::SeqCst);
142+
let _ = x.load(Ordering::Relaxed);
143+
let _ = x.load(Ordering::Release);
144+
let _ = x.load(Ordering::AcqRel);
145+
146+
x.store(1, Ordering::Release);
147+
x.store(1, Ordering::SeqCst);
148+
x.store(1, Ordering::Relaxed);
149+
x.store(1, Ordering::Acquire);
150+
x.store(1, Ordering::AcqRel);
151+
152+
// `AtomicU32` test cases
153+
let x = AtomicU32::new(0);
154+
155+
let _ = x.load(Ordering::Acquire);
156+
let _ = x.load(Ordering::SeqCst);
157+
let _ = x.load(Ordering::Relaxed);
158+
let _ = x.load(Ordering::Release);
159+
let _ = x.load(Ordering::AcqRel);
160+
161+
x.store(1, Ordering::Release);
162+
x.store(1, Ordering::SeqCst);
163+
x.store(1, Ordering::Relaxed);
164+
x.store(1, Ordering::Acquire);
165+
x.store(1, Ordering::AcqRel);
166+
167+
// `AtomicU64` test cases
168+
let x = AtomicU64::new(0);
169+
170+
let _ = x.load(Ordering::Acquire);
171+
let _ = x.load(Ordering::SeqCst);
172+
let _ = x.load(Ordering::Relaxed);
173+
let _ = x.load(Ordering::Release);
174+
let _ = x.load(Ordering::AcqRel);
175+
176+
x.store(1, Ordering::Release);
177+
x.store(1, Ordering::SeqCst);
178+
x.store(1, Ordering::Relaxed);
179+
x.store(1, Ordering::Acquire);
180+
x.store(1, Ordering::AcqRel);
181+
182+
// `AtomicUsize` test cases
183+
let x = AtomicUsize::new(0);
184+
185+
let _ = x.load(Ordering::Acquire);
186+
let _ = x.load(Ordering::SeqCst);
187+
let _ = x.load(Ordering::Relaxed);
188+
let _ = x.load(Ordering::Release);
189+
let _ = x.load(Ordering::AcqRel);
190+
191+
x.store(1, Ordering::Release);
192+
x.store(1, Ordering::SeqCst);
193+
x.store(1, Ordering::Relaxed);
194+
x.store(1, Ordering::Acquire);
195+
x.store(1, Ordering::AcqRel);
196+
}

0 commit comments

Comments
 (0)