Skip to content
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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3831,6 +3831,7 @@ dependencies = [
name = "rustc_lint"
version = "0.0.0"
dependencies = [
"if_chain",
"rustc_ast",
"rustc_ast_pretty",
"rustc_attr",
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_lint/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ version = "0.0.0"
edition = "2018"

[dependencies]
if_chain = "1.0"
tracing = "0.1"
unicode-security = "0.0.5"
rustc_middle = { path = "../rustc_middle" }
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ macro_rules! late_lint_passes {
DropTraitConstraints: DropTraitConstraints,
TemporaryCStringAsPtr: TemporaryCStringAsPtr,
PanicFmt: PanicFmt,
InvalidAtomicOrdering: InvalidAtomicOrdering,
]
);
};
Expand Down
214 changes: 212 additions & 2 deletions compiler/rustc_lint/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1358,3 +1360,211 @@ 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(cx: &LateContext<'_>, did: DefId, orderings: &[Symbol]) -> bool {
orderings.iter().any(|ordering| cx.tcx.is_diagnostic_item(*ordering, did))
}

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] };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diagnostic items instead of Symbol equality

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems tricky because there's not just one load method, there's one for AtomicBool::load, one for AtomicU8::load, one for AtomicU16::load, etc.

I had thought that diagnostic items wouldn't work here (it didn't seem to, but maybe I'm mistaken)

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@thomcc thomcc Dec 10, 2020

Choose a reason for hiding this comment

The 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 rustc_diagnostic_tag which would be allowed to have multiple things that it applies to.

As is maybe this is fine though. What's the downside of not using rustc_diagnostic_item here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, someting like rustc_doagnostic_tag is what I would want here 👍

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 sym::load for now

Copy link
Contributor

Choose a reason for hiding this comment

The 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(cx, ordering_def_id, &[sym::atomic_ordering_release, sym::atomic_ordering_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(cx, ordering_def_id, &[sym::atomic_ordering_acquire, sym::atomic_ordering_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(cx, ordering_def_id, &[sym::atomic_ordering_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::atomic_ordering_relaxed, &[sym::atomic_ordering_seqcst, sym::atomic_ordering_acquire], "ordering mode `Relaxed`");
let acquire: OrdLintInfo = (sym::atomic_ordering_acquire, &[sym::atomic_ordering_seqcst], "ordering modes `Acquire` or `Relaxed`");
let seq_cst: OrdLintInfo = (sym::atomic_ordering_seqcst, &[], "ordering modes `Acquire`, `SeqCst` or `Relaxed`");
let release = (sym::atomic_ordering_release, relaxed.1, relaxed.2);
let acqrel = (sym::atomic_ordering_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.tcx.is_diagnostic_item(*ordering, success_ord_def_id)
})
.copied()
});
if Self::matches_ordering(cx, fail_ordering_def_id, &[sym::atomic_ordering_seqcst, sym::atomic_ordering_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(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);
}
}
6 changes: 6 additions & 0 deletions compiler/rustc_passes/src/diagnostic_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ struct DiagnosticItemCollector<'tcx> {
impl<'v, 'tcx> ItemLikeVisitor<'v> for DiagnosticItemCollector<'tcx> {
fn visit_item(&mut self, item: &hir::Item<'_>) {
self.observe_item(&item.attrs, item.hir_id);

if let hir::ItemKind::Enum(e, _) = &item.kind {
for variant in e.variants {
self.observe_item(variant.attrs, variant.id);
}
}
}

fn visit_trait_item(&mut self, trait_item: &hir::TraitItem<'_>) {
Expand Down
28 changes: 28 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,26 @@ symbols! {
assume_init,
async_await,
async_closure,
atomic,
atomic_bool_type,
atomic_i128_type,
atomic_i16_type,
atomic_i32_type,
atomic_i64_type,
atomic_i8_type,
atomic_isize_type,
atomic_ordering_acqrel,
atomic_ordering_acquire,
atomic_ordering_relaxed,
atomic_ordering_release,
atomic_ordering_seqcst,
atomic_ptr_type,
atomic_u128_type,
atomic_u16_type,
atomic_u32_type,
atomic_u64_type,
atomic_u8_type,
atomic_usize_type,
atomics,
att_syntax,
attr,
Expand Down Expand Up @@ -350,8 +370,12 @@ symbols! {
coerce_unsized,
cold,
column,
compare_and_swap,
compare_exchange,
compare_exchange_weak,
compile_error,
compiler_builtins,
compiler_fence,
concat,
concat_idents,
conservative_impl_trait,
Expand Down Expand Up @@ -514,6 +538,8 @@ symbols! {
fadd_fast,
fdiv_fast,
feature,
fence,
fetch_update,
ffi,
ffi_const,
ffi_pure,
Expand Down Expand Up @@ -651,6 +677,7 @@ symbols! {
lint_reasons,
literal,
llvm_asm,
load,
local_inner_macros,
log10f32,
log10f64,
Expand Down Expand Up @@ -1074,6 +1101,7 @@ symbols! {
stmt,
stmt_expr_attributes,
stop_after_dataflow,
store,
str,
str_alloc,
string_type,
Expand Down
Loading