Skip to content

Commit 1d7c1af

Browse files
Jeremy Fitzhardingejsgf
authored andcommitted
Implement volatile_composites lint
Volatile reads and writes to non-primitive types are not well-defined, and can cause problems. See #15529 for more details.
1 parent 3e218d1 commit 1d7c1af

File tree

7 files changed

+496
-0
lines changed

7 files changed

+496
-0
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6798,6 +6798,7 @@ Released 2018-09-13
67986798
[`vec_resize_to_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_resize_to_zero
67996799
[`verbose_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_bit_mask
68006800
[`verbose_file_reads`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_file_reads
6801+
[`volatile_composites`]: https://rust-lang.github.io/rust-clippy/master/index.html#volatile_composites
68016802
[`vtable_address_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#vtable_address_comparisons
68026803
[`waker_clone_wake`]: https://rust-lang.github.io/rust-clippy/master/index.html#waker_clone_wake
68036804
[`while_float`]: https://rust-lang.github.io/rust-clippy/master/index.html#while_float

clippy_lints/src/declared_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -780,6 +780,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
780780
crate::visibility::NEEDLESS_PUB_SELF_INFO,
781781
crate::visibility::PUB_WITHOUT_SHORTHAND_INFO,
782782
crate::visibility::PUB_WITH_SHORTHAND_INFO,
783+
crate::volatile_composites::VOLATILE_COMPOSITES_INFO,
783784
crate::wildcard_imports::ENUM_GLOB_USE_INFO,
784785
crate::wildcard_imports::WILDCARD_IMPORTS_INFO,
785786
crate::write::PRINTLN_EMPTY_STRING_INFO,

clippy_lints/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -400,6 +400,7 @@ mod useless_conversion;
400400
mod vec;
401401
mod vec_init_then_push;
402402
mod visibility;
403+
mod volatile_composites;
403404
mod wildcard_imports;
404405
mod write;
405406
mod zero_div_zero;
@@ -831,5 +832,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
831832
store.register_late_pass(|_| Box::new(infallible_try_from::InfallibleTryFrom));
832833
store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny));
833834
store.register_late_pass(|_| Box::new(toplevel_ref_arg::ToplevelRefArg));
835+
store.register_late_pass(|_| Box::new(volatile_composites::VolatileComposites));
834836
// add lints here, do not remove this comment, it's used in `new_lint`
835837
}
Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
use clippy_utils::diagnostics::span_lint;
2+
use clippy_utils::sym;
3+
use clippy_utils::ty::is_type_diagnostic_item;
4+
use rustc_hir::{Expr, ExprKind};
5+
use rustc_lint::{LateContext, LateLintPass};
6+
use rustc_middle::ty::layout::LayoutOf;
7+
use rustc_middle::ty::{self, Ty, TypeVisitableExt};
8+
use rustc_session::declare_lint_pass;
9+
10+
declare_clippy_lint! {
11+
/// ### What it does
12+
///
13+
/// This lint warns when volatile load/store operations
14+
/// (`write_volatile`/`read_volatile`) are applied to composite types.
15+
///
16+
/// ### Why is this bad?
17+
///
18+
/// Volatile operations are typically used with memory mapped IO devices,
19+
/// where the precise number and ordering of load and store instructions is
20+
/// important because they can have side effects. This is well defined for
21+
/// primitive types like `u32`, but less well defined for structures and
22+
/// other composite types. In practice it's implementation defined, and the
23+
/// behavior can be rustc-version dependent.
24+
///
25+
/// As a result, code should only apply `write_volatile`/`read_volatile` to
26+
/// primitive types to be fully well-defined.
27+
///
28+
/// ### Example
29+
/// ```no_run
30+
/// struct MyDevice {
31+
/// addr: usize,
32+
/// count: usize
33+
/// }
34+
///
35+
/// fn start_device(device: *mut MyDevice, addr: usize, count: usize) {
36+
/// unsafe {
37+
/// device.write_volatile(MyDevice { addr, count });
38+
/// }
39+
/// }
40+
/// ```
41+
/// Instead, operate on each primtive field individually:
42+
/// ```no_run
43+
/// struct MyDevice {
44+
/// addr: usize,
45+
/// count: usize
46+
/// }
47+
///
48+
/// fn start_device(device: *mut MyDevice, addr: usize, count: usize) {
49+
/// unsafe {
50+
/// (&raw mut (*device).addr).write_volatile(addr);
51+
/// (&raw mut (*device).count).write_volatile(count);
52+
/// }
53+
/// }
54+
/// ```
55+
#[clippy::version = "1.92.0"]
56+
pub VOLATILE_COMPOSITES,
57+
nursery,
58+
"warn about volatile read/write applied to composite types"
59+
}
60+
declare_lint_pass!(VolatileComposites => [VOLATILE_COMPOSITES]);
61+
62+
/// Zero-sized types are intrinsically safe to use volatile on since they won't
63+
/// actually generate *any* loads or stores. But this is also used to skip zero-sized
64+
/// fields of `#[repr(transparent)]` structures.
65+
fn is_zero_sized_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
66+
cx.layout_of(ty).is_ok_and(|layout| layout.is_zst())
67+
}
68+
69+
/// A thin raw pointer or reference.
70+
fn is_narrow_ptr<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
71+
match ty.kind() {
72+
ty::RawPtr(inner, _) | ty::Ref(_, inner, _) => inner.has_trivial_sizedness(cx.tcx, ty::SizedTraitKind::Sized),
73+
_ => false,
74+
}
75+
}
76+
77+
/// Enum with some fixed representation and no data-carrying variants.
78+
fn is_enum_repr_c<'tcx>(_cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
79+
ty.ty_adt_def().is_some_and(|adt_def| {
80+
adt_def.is_enum() && adt_def.repr().inhibit_struct_field_reordering() && adt_def.is_payloadfree()
81+
})
82+
}
83+
84+
/// `#[repr(transparent)]` structures are also OK if the only non-zero
85+
/// sized field contains a volatile-safe type.
86+
fn is_struct_repr_transparent<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
87+
if let ty::Adt(adt_def, args) = ty.kind()
88+
&& adt_def.is_struct()
89+
&& adt_def.repr().transparent()
90+
&& let [fieldty] = adt_def
91+
.all_fields()
92+
.filter_map(|field| {
93+
let fty = field.ty(cx.tcx, args);
94+
if is_zero_sized_ty(cx, fty) { None } else { Some(fty) }
95+
})
96+
.collect::<Vec<_>>()
97+
.as_slice()
98+
{
99+
is_volatile_safe_ty(cx, *fieldty)
100+
} else {
101+
false
102+
}
103+
}
104+
105+
/// SIMD can be useful to get larger single loads/stores, though this is still
106+
/// pretty machine-dependent.
107+
fn is_simd_repr<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
108+
if let ty::Adt(adt_def, _args) = ty.kind()
109+
&& adt_def.is_struct()
110+
&& adt_def.repr().simd()
111+
{
112+
let (_size, simdty) = ty.simd_size_and_type(cx.tcx);
113+
is_volatile_safe_ty(cx, simdty)
114+
} else {
115+
false
116+
}
117+
}
118+
119+
/// Top-level predicate for whether a type is volatile-safe or not.
120+
fn is_volatile_safe_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
121+
ty.is_primitive()
122+
|| is_narrow_ptr(cx, ty)
123+
|| is_zero_sized_ty(cx, ty)
124+
|| is_enum_repr_c(cx, ty)
125+
|| is_simd_repr(cx, ty)
126+
|| is_struct_repr_transparent(cx, ty)
127+
// We can't know about a generic type, so just let it pass to avoid noise
128+
|| ty.has_non_region_param()
129+
}
130+
131+
/// Print diagnostic for volatile read/write on non-volatile-safe types.
132+
fn report_volatile_safe<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, ty: Ty<'tcx>) {
133+
if !is_volatile_safe_ty(cx, ty) {
134+
span_lint(
135+
cx,
136+
VOLATILE_COMPOSITES,
137+
expr.span,
138+
format!("type `{ty}` is not volatile-compatible"),
139+
);
140+
}
141+
}
142+
143+
impl<'tcx> LateLintPass<'tcx> for VolatileComposites {
144+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
145+
// Check our expr is calling a method with pattern matching
146+
match expr.kind {
147+
// Look for method calls to `write_volatile`/`read_volatile`, which
148+
// apply to both raw pointers and std::ptr::NonNull.
149+
ExprKind::MethodCall(name, self_arg, _, _)
150+
if matches!(name.ident.name, sym::read_volatile | sym::write_volatile) =>
151+
{
152+
let self_ty = cx.typeck_results().expr_ty(self_arg);
153+
match self_ty.kind() {
154+
// Raw pointers
155+
ty::RawPtr(innerty, _) => report_volatile_safe(cx, expr, *innerty),
156+
// std::ptr::NonNull
157+
ty::Adt(_, args) if is_type_diagnostic_item(cx, self_ty, sym::NonNull) => {
158+
report_volatile_safe(cx, expr, args.type_at(0));
159+
},
160+
_ => (),
161+
}
162+
},
163+
164+
// Also plain function calls to std::ptr::{read,write}_volatile
165+
ExprKind::Call(func, [arg_ptr, ..]) => {
166+
if let ExprKind::Path(ref qpath) = func.kind
167+
&& let Some(def_id) = cx.qpath_res(qpath, func.hir_id).opt_def_id()
168+
&& matches!(
169+
cx.tcx.get_diagnostic_name(def_id),
170+
Some(sym::ptr_read_volatile | sym::ptr_write_volatile)
171+
)
172+
&& let ty::RawPtr(ptrty, _) = cx.typeck_results().expr_ty_adjusted(arg_ptr).kind()
173+
{
174+
report_volatile_safe(cx, expr, *ptrty);
175+
}
176+
},
177+
_ => {},
178+
}
179+
}
180+
}

clippy_utils/src/sym.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,7 @@ generate! {
264264
read_to_end,
265265
read_to_string,
266266
read_unaligned,
267+
read_volatile,
267268
redundant_imports,
268269
redundant_pub_crate,
269270
regex,
@@ -373,6 +374,7 @@ generate! {
373374
wrapping_offset,
374375
write,
375376
write_unaligned,
377+
write_volatile,
376378
writeln,
377379
zip,
378380
}

0 commit comments

Comments
 (0)