-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uplift the invalid_atomic_ordering
lint from clippy to rustc
#79654
Changes from 6 commits
5d8c4be
5d59e7a
4a9aaa0
e517634
ace2c5d
ee38c89
a02f7b7
844329c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,18 +4,20 @@ use rustc_attr as attr; | |
use rustc_data_structures::fx::FxHashSet; | ||
use rustc_errors::Applicability; | ||
use rustc_hir as hir; | ||
use rustc_hir::{is_range_literal, ExprKind, Node}; | ||
use rustc_hir::def_id::DefId; | ||
use rustc_hir::{is_range_literal, Expr, ExprKind, Node}; | ||
use rustc_index::vec::Idx; | ||
use rustc_middle::ty::layout::{IntegerExt, SizeSkeleton}; | ||
use rustc_middle::ty::subst::SubstsRef; | ||
use rustc_middle::ty::{self, AdtKind, Ty, TyCtxt, TypeFoldable}; | ||
use rustc_span::source_map; | ||
use rustc_span::symbol::sym; | ||
use rustc_span::{Span, DUMMY_SP}; | ||
use rustc_span::{Span, Symbol, DUMMY_SP}; | ||
use rustc_target::abi::Abi; | ||
use rustc_target::abi::{Integer, LayoutOf, TagEncoding, VariantIdx, Variants}; | ||
use rustc_target::spec::abi::Abi as SpecAbi; | ||
|
||
use if_chain::if_chain; | ||
use std::cmp; | ||
use std::ops::ControlFlow; | ||
use tracing::debug; | ||
|
@@ -1358,3 +1360,216 @@ impl<'tcx> LateLintPass<'tcx> for VariantSizeDifferences { | |
} | ||
} | ||
} | ||
|
||
declare_lint! { | ||
/// The `invalid_atomic_ordering` lint detects using passing an `Ordering` | ||
/// to an atomic operation that does not support that ordering. | ||
/// | ||
/// ### Example | ||
/// | ||
/// ```rust,compile_fail | ||
/// # use core::sync::atomic::{AtomicU8, Ordering}; | ||
/// let atom = AtomicU8::new(0); | ||
/// let value = atom.load(Ordering::Release); | ||
/// # let _ = value; | ||
/// ``` | ||
/// | ||
/// {{produces}} | ||
/// | ||
/// ### Explanation | ||
/// | ||
/// Some atomic operations are only supported for a subset of the | ||
/// `atomic::Ordering` variants. Passing an unsupported variant will cause | ||
/// an unconditional panic at runtime, which is detected by this lint. | ||
/// | ||
/// This lint will trigger in the following cases: (where `AtomicType` is an | ||
/// atomic type from `core::sync::atomic`, such as `AtomicBool`, | ||
/// `AtomicPtr`, `AtomicUsize`, or any of the other integer atomics). | ||
/// | ||
/// - Passing `Ordering::Acquire` or `Ordering::AcqRel` to | ||
/// `AtomicType::store`. | ||
/// | ||
/// - Passing `Ordering::Release` or `Ordering::AcqRel` to | ||
/// `AtomicType::load`. | ||
/// | ||
/// - Passing `Ordering::Relaxed` to `core::sync::atomic::fence` or | ||
/// `core::sync::atomic::compiler_fence`. | ||
/// | ||
/// - Passing `Ordering::Release` or `Ordering::AcqRel` as the failure | ||
/// ordering for any of `AtomicType::compare_exchange`, | ||
/// `AtomicType::compare_exchange_weak`, or `AtomicType::fetch_update`. | ||
/// | ||
/// - Passing in a pair of orderings to `AtomicType::compare_exchange`, | ||
/// `AtomicType::compare_exchange_weak`, or `AtomicType::fetch_update` | ||
/// where the failure ordering is stronger than the success ordering. | ||
INVALID_ATOMIC_ORDERING, | ||
Deny, | ||
"usage of invalid atomic ordering in atomic operations and memory fences" | ||
} | ||
|
||
declare_lint_pass!(InvalidAtomicOrdering => [INVALID_ATOMIC_ORDERING]); | ||
|
||
impl InvalidAtomicOrdering { | ||
fn type_is_atomic(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { | ||
const ATOMIC_TYPES: &[Symbol] = &[ | ||
sym::atomic_bool_type, | ||
sym::atomic_ptr_type, | ||
sym::atomic_usize_type, | ||
sym::atomic_u8_type, | ||
sym::atomic_u16_type, | ||
sym::atomic_u32_type, | ||
sym::atomic_u64_type, | ||
sym::atomic_u128_type, | ||
sym::atomic_isize_type, | ||
sym::atomic_i8_type, | ||
sym::atomic_i16_type, | ||
sym::atomic_i32_type, | ||
sym::atomic_i64_type, | ||
sym::atomic_i128_type, | ||
]; | ||
if let ty::Adt(&ty::AdtDef { did, .. }, _) = cx.typeck_results().expr_ty(expr).kind() { | ||
ATOMIC_TYPES.iter().any(|sym| cx.tcx.is_diagnostic_item(*sym, did)) | ||
} else { | ||
false | ||
} | ||
} | ||
|
||
fn matches_ordering_def_path(cx: &LateContext<'_>, did: DefId, orderings: &[Symbol]) -> bool { | ||
orderings.iter().any(|ordering| { | ||
cx.match_def_path(did, &[sym::core, sym::sync, sym::atomic, sym::Ordering, *ordering]) | ||
}) | ||
} | ||
|
||
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_load_store(cx: &LateContext<'_>, expr: &Expr<'_>) { | ||
if_chain! { | ||
if let ExprKind::MethodCall(ref method_path, _, args, _) = &expr.kind; | ||
let method = method_path.ident.name; | ||
if Self::type_is_atomic(cx, &args[0]); | ||
if method == sym::load || method == sym::store; | ||
let ordering_arg = if method == sym::load { &args[1] } else { &args[2] }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. diagnostic items instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one seems tricky because there's not just one I had thought that diagnostic items wouldn't work here (it didn't seem to, but maybe I'm mistaken) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, they do have to be unique afaik 🤔 I guess using names is fine in that case. Thanks for looking into this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unsure how to solve this but it also seems like less of an issue (since we use rustc_diagnostic_item for the actual atomic type). I don't think it's worth having atomic_store_u8, atomic_store_u16, etc methods all have unique diagnostic items. It feels like you kind of want a As is maybe this is fine though. What's the downside of not using rustc_diagnostic_item here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, someting like The main issue is when moving stuff in std which breaks lints like this. I think this may also cause issues if a user impls a trait like the following, or at least it's less obvious for people reading the code what's the expected behavior here trait Foo {
fn load(self);
} In this PR you can just keep using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add the following as an ui test? // check-pass
use std::sync::atomic::{AtomicUsize, Ordering};
trait Foo {
fn store(self, ordering: Ordering);
}
impl Foo for AtomicUsize {
fn store(self, _ordering: Ordering) {
AtomicUsize::store(&self, 4, Ordering::SeqCst);
}
}
fn main() {
let x = AtomicUsize::new(3);
x.store(Ordering::Acquire);
} |
||
if let ExprKind::Path(ref ordering_qpath) = ordering_arg.kind; | ||
if let Some(ordering_def_id) = cx.qpath_res(ordering_qpath, ordering_arg.hir_id).opt_def_id(); | ||
then { | ||
if method == sym::load && | ||
Self::matches_ordering_def_path(cx, ordering_def_id, &[sym::Release, sym::AcqRel]) { | ||
cx.struct_span_lint(INVALID_ATOMIC_ORDERING, ordering_arg.span, |diag| { | ||
diag.build("atomic loads cannot have `Release` or `AcqRel` ordering") | ||
.help("consider using ordering modes `Acquire`, `SeqCst` or `Relaxed`") | ||
.emit(); | ||
}); | ||
} else if method == sym::store && | ||
Self::matches_ordering_def_path(cx, ordering_def_id, &[sym::Acquire, sym::AcqRel]) { | ||
cx.struct_span_lint(INVALID_ATOMIC_ORDERING, ordering_arg.span, |diag| { | ||
diag.build("atomic stores cannot have `Acquire` or `AcqRel` ordering") | ||
.help("consider using ordering modes `Release`, `SeqCst` or `Relaxed`") | ||
.emit(); | ||
}); | ||
} | ||
} | ||
} | ||
} | ||
|
||
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.qpath_res(func_qpath, func.hir_id).opt_def_id(); | ||
if cx.tcx.is_diagnostic_item(sym::fence, def_id) || | ||
cx.tcx.is_diagnostic_item(sym::compiler_fence, def_id); | ||
if let ExprKind::Path(ref ordering_qpath) = &args[0].kind; | ||
if let Some(ordering_def_id) = cx.qpath_res(ordering_qpath, args[0].hir_id).opt_def_id(); | ||
if Self::matches_ordering_def_path(cx, ordering_def_id, &[sym::Relaxed]); | ||
then { | ||
cx.struct_span_lint(INVALID_ATOMIC_ORDERING, args[0].span, |diag| { | ||
diag.build("memory fences cannot have `Relaxed` ordering") | ||
.help("consider using ordering modes `Acquire`, `Release`, `AcqRel` or `SeqCst`") | ||
.emit(); | ||
}); | ||
} | ||
} | ||
} | ||
|
||
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; | ||
if Self::type_is_atomic(cx, &args[0]); | ||
if method == sym::compare_exchange || method == sym::compare_exchange_weak || method == sym::fetch_update; | ||
let (success_order_arg, failure_order_arg) = if method == sym::fetch_update { | ||
(&args[1], &args[2]) | ||
} else { | ||
(&args[3], &args[4]) | ||
}; | ||
if let Some(fail_ordering_def_id) = Self::opt_ordering_defid(cx, failure_order_arg); | ||
then { | ||
// Helper type holding on to some checking and error reporting data. Has | ||
// - (success ordering, | ||
// - list of failure orderings forbidden by the success order, | ||
// - suggestion message) | ||
type OrdLintInfo = (Symbol, &'static [Symbol], &'static str); | ||
let relaxed: OrdLintInfo = (sym::Relaxed, &[sym::SeqCst, sym::Acquire], "ordering mode `Relaxed`"); | ||
let acquire: OrdLintInfo = (sym::Acquire, &[sym::SeqCst], "ordering modes `Acquire` or `Relaxed`"); | ||
let seq_cst: OrdLintInfo = (sym::SeqCst, &[], "ordering modes `Acquire`, `SeqCst` or `Relaxed`"); | ||
let release = (sym::Release, relaxed.1, relaxed.2); | ||
let acqrel = (sym::AcqRel, acquire.1, acquire.2); | ||
let search = [relaxed, acquire, seq_cst, release, acqrel]; | ||
|
||
let success_lint_info = Self::opt_ordering_defid(cx, success_order_arg) | ||
.and_then(|success_ord_def_id| -> Option<OrdLintInfo> { | ||
search | ||
.iter() | ||
.find(|(ordering, ..)| { | ||
cx.match_def_path( | ||
success_ord_def_id, | ||
&[sym::core, sym::sync, sym::atomic, sym::Ordering, *ordering], | ||
) | ||
}) | ||
.copied() | ||
}); | ||
if Self::matches_ordering_def_path(cx, fail_ordering_def_id, &[sym::Release, sym::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; | ||
cx.struct_span_lint(INVALID_ATOMIC_ORDERING, failure_order_arg.span, |diag| { | ||
let msg = format!( | ||
"{}'s failure ordering may not be `Release` or `AcqRel`", | ||
method, | ||
); | ||
diag.build(&msg) | ||
.help(&format!("consider using {} instead", suggested)) | ||
.emit(); | ||
}); | ||
} else if let Some((success_ord, bad_ords_given_success, suggested)) = success_lint_info { | ||
if Self::matches_ordering_def_path(cx, fail_ordering_def_id, bad_ords_given_success) { | ||
cx.struct_span_lint(INVALID_ATOMIC_ORDERING, failure_order_arg.span, |diag| { | ||
let msg = format!( | ||
"{}'s failure ordering may not be stronger than the success ordering of `{}`", | ||
method, | ||
success_ord, | ||
); | ||
diag.build(&msg) | ||
.help(&format!("consider using {} instead", suggested)) | ||
.emit(); | ||
}); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
impl<'tcx> LateLintPass<'tcx> for InvalidAtomicOrdering { | ||
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { | ||
Self::check_atomic_load_store(cx, expr); | ||
Self::check_memory_fence(cx, expr); | ||
Self::check_atomic_compare_exchange(cx, expr); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use diagnostic items instead here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get
on each of these if I try to put it on a variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think (code's still compiling...) can fix this by adding
after this
rust/compiler/rustc_passes/src/diagnostic_items.rs
Line 30 in d7560e8
#[cfg_attr(not(bootstrap), rustc_diagnostic_item = "atomic_ordering_florb")]
It's small enough for me to think that's okay (if it were a more invasive change I'd punt on it), but it's your call.
(I'll update this based on if the tests manage to pass after the rebuild)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to run to work before the build finished, so I pushed it up so CI could sort it out for me. That is: the last two commits are just tentative.