From e2e40f2570cc66d3b74a2f474e57763cb75f6029 Mon Sep 17 00:00:00 2001 From: Krishna Sai Veera Reddy Date: Thu, 9 Jan 2020 09:49:15 -0800 Subject: [PATCH 1/2] Detect usage of invalid atomic ordering in memory fences Detect usage of `core::sync::atomic::{fence, compiler_fence}` with `Ordering::Relaxed` and suggest valid alternatives. --- clippy_lints/src/atomic_ordering.rs | 97 +++++++++++++++++++---------- src/lintlist/mod.rs | 2 +- 2 files changed, 65 insertions(+), 34 deletions(-) diff --git a/clippy_lints/src/atomic_ordering.rs b/clippy_lints/src/atomic_ordering.rs index fe06c63a553c9..f5b0b625023f1 100644 --- a/clippy_lints/src/atomic_ordering.rs +++ b/clippy_lints/src/atomic_ordering.rs @@ -9,7 +9,7 @@ use rustc_session::declare_tool_lint; declare_clippy_lint! { /// **What it does:** Checks for usage of invalid atomic - /// ordering in Atomic*::{load, store} calls. + /// ordering in atomic loads/stores and memory fences. /// /// **Why is this bad?** Using an invalid atomic ordering /// will cause a panic at run-time. @@ -18,7 +18,7 @@ declare_clippy_lint! { /// /// **Example:** /// ```rust,no_run - /// # use std::sync::atomic::{AtomicBool, Ordering}; + /// # use std::sync::atomic::{self, AtomicBool, Ordering}; /// /// let x = AtomicBool::new(true); /// @@ -27,10 +27,13 @@ declare_clippy_lint! { /// /// x.store(false, Ordering::Acquire); /// x.store(false, Ordering::AcqRel); + /// + /// atomic::fence(Ordering::Relaxed); + /// atomic::compiler_fence(Ordering::Relaxed); /// ``` pub INVALID_ATOMIC_ORDERING, correctness, - "usage of invalid atomic ordering in atomic load/store calls" + "usage of invalid atomic ordering in atomic loads/stores and memory fences" } declare_lint_pass!(AtomicOrdering => [INVALID_ATOMIC_ORDERING]); @@ -66,37 +69,65 @@ fn match_ordering_def_path(cx: &LateContext<'_, '_>, did: DefId, orderings: &[&s .any(|ordering| match_def_path(cx, did, &["core", "sync", "atomic", "Ordering", ordering])) } -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AtomicOrdering { - fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx 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 == "load" || method == "store"; - let ordering_arg = if method == "load" { &args[1] } else { &args[2] }; - if let ExprKind::Path(ref ordering_qpath) = ordering_arg.kind; - if let Some(ordering_def_id) = cx.tables.qpath_res(ordering_qpath, ordering_arg.hir_id).opt_def_id(); - then { - if method == "load" && - match_ordering_def_path(cx, ordering_def_id, &["Release", "AcqRel"]) { - span_help_and_lint( - cx, - INVALID_ATOMIC_ORDERING, - ordering_arg.span, - "atomic loads cannot have `Release` and `AcqRel` ordering", - "consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`" - ); - } else if method == "store" && - match_ordering_def_path(cx, ordering_def_id, &["Acquire", "AcqRel"]) { - span_help_and_lint( - cx, - INVALID_ATOMIC_ORDERING, - ordering_arg.span, - "atomic stores cannot have `Acquire` and `AcqRel` ordering", - "consider using ordering modes `Release`, `SeqCst` or `Relaxed`" - ); - } +fn check_atomic_load_store(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 == "load" || method == "store"; + let ordering_arg = if method == "load" { &args[1] } else { &args[2] }; + if let ExprKind::Path(ref ordering_qpath) = ordering_arg.kind; + if let Some(ordering_def_id) = cx.tables.qpath_res(ordering_qpath, ordering_arg.hir_id).opt_def_id(); + then { + if method == "load" && + match_ordering_def_path(cx, ordering_def_id, &["Release", "AcqRel"]) { + span_help_and_lint( + cx, + INVALID_ATOMIC_ORDERING, + ordering_arg.span, + "atomic loads cannot have `Release` and `AcqRel` ordering", + "consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`" + ); + } else if method == "store" && + match_ordering_def_path(cx, ordering_def_id, &["Acquire", "AcqRel"]) { + span_help_and_lint( + cx, + INVALID_ATOMIC_ORDERING, + ordering_arg.span, + "atomic stores cannot have `Acquire` and `AcqRel` ordering", + "consider using ordering modes `Release`, `SeqCst` or `Relaxed`" + ); } } } } + +fn check_memory_fence(cx: &LateContext<'_, '_>, expr: &Expr<'_>) { + if_chain! { + if let ExprKind::Call(ref func, ref args) = expr.kind; + if let ExprKind::Path(ref func_qpath) = func.kind; + if let Some(def_id) = cx.tables.qpath_res(func_qpath, func.hir_id).opt_def_id(); + if ["fence", "compiler_fence"] + .iter() + .any(|func| match_def_path(cx, def_id, &["core", "sync", "atomic", func])); + if let ExprKind::Path(ref ordering_qpath) = &args[0].kind; + if let Some(ordering_def_id) = cx.tables.qpath_res(ordering_qpath, args[0].hir_id).opt_def_id(); + if match_ordering_def_path(cx, ordering_def_id, &["Relaxed"]); + then { + span_help_and_lint( + cx, + INVALID_ATOMIC_ORDERING, + args[0].span, + "memory fences cannot have `Relaxed` ordering", + "consider using ordering modes `Acquire`, `Release`, `AcqRel` or `SeqCst`" + ); + } + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AtomicOrdering { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { + check_atomic_load_store(cx, expr); + check_memory_fence(cx, expr); + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index f576bb152de97..b133a79976244 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -836,7 +836,7 @@ pub const ALL_LINTS: [Lint; 346] = [ Lint { name: "invalid_atomic_ordering", group: "correctness", - desc: "usage of invalid atomic ordering in atomic load/store calls", + desc: "usage of invalid atomic ordering in atomic loads/stores and memory fences", deprecation: None, module: "atomic_ordering", }, From 5e058f38f43108d81d6efc98cf7406f28e891ded Mon Sep 17 00:00:00 2001 From: Krishna Sai Veera Reddy Date: Thu, 9 Jan 2020 09:51:13 -0800 Subject: [PATCH 2/2] Add memory fence tests for `invalid_atomic_ordering` --- tests/ui/atomic_ordering_fence.rs | 20 ++++++++++++++++++++ tests/ui/atomic_ordering_fence.stderr | 19 +++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 tests/ui/atomic_ordering_fence.rs create mode 100644 tests/ui/atomic_ordering_fence.stderr diff --git a/tests/ui/atomic_ordering_fence.rs b/tests/ui/atomic_ordering_fence.rs new file mode 100644 index 0000000000000..5ee5182ca051a --- /dev/null +++ b/tests/ui/atomic_ordering_fence.rs @@ -0,0 +1,20 @@ +#![warn(clippy::invalid_atomic_ordering)] + +use std::sync::atomic::{compiler_fence, fence, Ordering}; + +fn main() { + // Allowed fence ordering modes + fence(Ordering::Acquire); + fence(Ordering::Release); + fence(Ordering::AcqRel); + fence(Ordering::SeqCst); + + // Disallowed fence ordering modes + fence(Ordering::Relaxed); + + compiler_fence(Ordering::Acquire); + compiler_fence(Ordering::Release); + compiler_fence(Ordering::AcqRel); + compiler_fence(Ordering::SeqCst); + compiler_fence(Ordering::Relaxed); +} diff --git a/tests/ui/atomic_ordering_fence.stderr b/tests/ui/atomic_ordering_fence.stderr new file mode 100644 index 0000000000000..3ceff27d9ad5e --- /dev/null +++ b/tests/ui/atomic_ordering_fence.stderr @@ -0,0 +1,19 @@ +error: memory fences cannot have `Relaxed` ordering + --> $DIR/atomic_ordering_fence.rs:13:11 + | +LL | fence(Ordering::Relaxed); + | ^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::invalid-atomic-ordering` implied by `-D warnings` + = help: consider using ordering modes `Acquire`, `Release`, `AcqRel` or `SeqCst` + +error: memory fences cannot have `Relaxed` ordering + --> $DIR/atomic_ordering_fence.rs:19:20 + | +LL | compiler_fence(Ordering::Relaxed); + | ^^^^^^^^^^^^^^^^^ + | + = help: consider using ordering modes `Acquire`, `Release`, `AcqRel` or `SeqCst` + +error: aborting due to 2 previous errors +