Skip to content

Commit 14ae176

Browse files
author
Jeremy Fitzhardinge
committed
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 2a6197e commit 14ae176

File tree

7 files changed

+485
-0
lines changed

7 files changed

+485
-0
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6650,6 +6650,7 @@ Released 2018-09-13
66506650
[`vec_resize_to_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#vec_resize_to_zero
66516651
[`verbose_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_bit_mask
66526652
[`verbose_file_reads`]: https://rust-lang.github.io/rust-clippy/master/index.html#verbose_file_reads
6653+
[`volatile_composites`]: https://rust-lang.github.io/rust-clippy/master/index.html#volatile_composites
66536654
[`vtable_address_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#vtable_address_comparisons
66546655
[`waker_clone_wake`]: https://rust-lang.github.io/rust-clippy/master/index.html#waker_clone_wake
66556656
[`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
@@ -778,6 +778,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
778778
crate::visibility::NEEDLESS_PUB_SELF_INFO,
779779
crate::visibility::PUB_WITHOUT_SHORTHAND_INFO,
780780
crate::visibility::PUB_WITH_SHORTHAND_INFO,
781+
crate::volatile_composites::VOLATILE_COMPOSITES_INFO,
781782
crate::wildcard_imports::ENUM_GLOB_USE_INFO,
782783
crate::wildcard_imports::WILDCARD_IMPORTS_INFO,
783784
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: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
use clippy_utils::diagnostics::span_lint;
2+
use clippy_utils::sym;
3+
use rustc_hir::{Expr, ExprKind};
4+
use rustc_lint::{LateContext, LateLintPass};
5+
use rustc_middle::ty::{self, Ty};
6+
use rustc_session::declare_lint_pass;
7+
8+
declare_clippy_lint! {
9+
/// ### What it does
10+
///
11+
/// This lint warns when volatile load/store operations
12+
/// (`write_volatile`/`read_volatile`) are applied to composite types.
13+
///
14+
/// ### Why is this bad?
15+
///
16+
/// Volatile operations are typically used with memory mapped IO devices,
17+
/// where the precise number and ordering of load and store instructions is
18+
/// important because they can have side effects. This is well defined for
19+
/// primitive types like `u32`, but less well defined for structures and
20+
/// other composite types. In practice it's implementation defined, and the
21+
/// behavior can be rustc-version dependent.
22+
///
23+
/// As a result, code should only apply `write_volatile`/`read_volatile` to
24+
/// primitive types to be fully well-defined.
25+
///
26+
/// ### Example
27+
/// ```no_run
28+
/// struct MyDevice {
29+
/// addr: usize,
30+
/// count: usize
31+
/// }
32+
///
33+
/// fn start_device(device: *mut MyDevice, addr: usize, count: usize) {
34+
/// device.write_volatile(MyDevice { addr, count })
35+
/// }
36+
/// ```
37+
/// Instead, operate on each primtive field individually:
38+
/// ```no_run
39+
/// struct MyDevice {
40+
/// addr: usize,
41+
/// count: usize
42+
/// }
43+
///
44+
/// fn start_device(device: *mut MyDevice, addr: usize, count: usize) {
45+
/// (&raw mut (*device).addr).write_volatile(addr);
46+
/// (&raw mut (*device).count).write_volatile(count);
47+
/// }
48+
/// ```
49+
#[clippy::version = "1.91.0"]
50+
pub VOLATILE_COMPOSITES,
51+
nursery,
52+
"warn about volatile read/write applied to composite types"
53+
}
54+
declare_lint_pass!(VolatileComposites => [VOLATILE_COMPOSITES]);
55+
56+
// functions:
57+
// core::ptr::{read_volatile,write_volatile}
58+
// methods:
59+
// pointer::{read_volatile,write_volatile}
60+
// NonNull::{read_volatile,write_volatile}
61+
62+
// primitive type:
63+
// unit, [iu]{8,16,32,64,128?}, f{32,64}, thin pointer, usize, isize, bool, char
64+
// C enum with primitive repr
65+
// #[repr(transparent)] wrapper of above
66+
67+
// Zero-sized types are intrinsically safe to use volatile on since they won't
68+
// actually generate *any* loads or stores. But this is also used to skip zero
69+
// fields of #[repr(transparent)] structures.
70+
fn is_zero_sized_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
71+
if let Ok(ty) = cx.tcx.try_normalize_erasing_regions(cx.typing_env(), ty)
72+
&& let Ok(layout) = cx.tcx.layout_of(cx.typing_env().as_query_input(ty))
73+
{
74+
layout.layout.size().bytes() == 0
75+
} else {
76+
false
77+
}
78+
}
79+
80+
// Make sure the raw pointer has no metadata
81+
fn is_narrow_raw_ptr<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
82+
if let ty::RawPtr(_inner, _) = ty.kind() {
83+
ty.pointee_metadata_ty_or_projection(cx.tcx).is_unit()
84+
} else {
85+
false
86+
}
87+
}
88+
89+
// Enum with some fixed representation and no data-carrying variants
90+
fn is_enum_repr_c<'tcx>(_cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
91+
if let ty::Adt(adt_def, _args) = ty.kind()
92+
&& adt_def.is_enum()
93+
&& adt_def.repr().inhibit_struct_field_reordering()
94+
{
95+
adt_def.is_payloadfree()
96+
} else {
97+
false
98+
}
99+
}
100+
101+
// #[repr(transparent)] structures are also OK if the only non-zero
102+
// sized field contains a volatile-safe type
103+
fn is_struct_repr_transparent<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
104+
if let ty::Adt(adt_def, args) = ty.kind()
105+
&& adt_def.is_struct()
106+
&& adt_def.repr().transparent()
107+
&& let [fieldty] = adt_def
108+
.all_fields()
109+
.filter_map(|field| {
110+
let fty = field.ty(cx.tcx, args);
111+
if is_zero_sized_ty(cx, fty) { None } else { Some(fty) }
112+
})
113+
.collect::<Vec<_>>()
114+
.as_slice()
115+
{
116+
is_volatile_safe_ty(cx, *fieldty)
117+
} else {
118+
false
119+
}
120+
}
121+
122+
// SIMD can be useful to get larger atomic loads/stores, though this is still
123+
// pretty machine-dependent.
124+
fn is_simd_repr<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
125+
if let ty::Adt(adt_def, _args) = ty.kind()
126+
&& adt_def.is_struct()
127+
&& adt_def.repr().simd()
128+
{
129+
let (_size, simdty) = ty.simd_size_and_type(cx.tcx);
130+
is_volatile_safe_ty(cx, simdty)
131+
} else {
132+
false
133+
}
134+
}
135+
136+
// We can't know about a generic type, so just let it pass to avoid noise
137+
fn is_generic<'tcx>(_cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
138+
ty.flags().intersects(ty::TypeFlags::HAS_PARAM)
139+
}
140+
141+
fn is_volatile_safe_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
142+
ty.is_primitive()
143+
|| is_narrow_raw_ptr(cx, ty)
144+
|| is_zero_sized_ty(cx, ty)
145+
|| is_enum_repr_c(cx, ty)
146+
|| is_simd_repr(cx, ty)
147+
|| is_struct_repr_transparent(cx, ty)
148+
|| is_generic(cx, ty)
149+
}
150+
151+
fn report_volatile_safe<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, ty: Ty<'tcx>) {
152+
if !is_volatile_safe_ty(cx, ty) {
153+
span_lint(
154+
cx,
155+
VOLATILE_COMPOSITES,
156+
expr.span,
157+
format!("type `{ty}` is not volatile-compatible"),
158+
);
159+
}
160+
}
161+
162+
impl<'tcx> LateLintPass<'tcx> for VolatileComposites {
163+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
164+
// Check our expr is calling a method with pattern matching
165+
match expr.kind {
166+
// Look for method calls to `write_volatile`/`read_volatile`, which
167+
// apply to both raw pointers and std::ptr::NonNull.
168+
ExprKind::MethodCall(name, self_arg, _, _)
169+
if matches!(name.ident.name, sym::read_volatile | sym::write_volatile) =>
170+
{
171+
let self_ty = cx.typeck_results().expr_ty(self_arg);
172+
match self_ty.kind() {
173+
// Raw pointers
174+
ty::RawPtr(innerty, _) => report_volatile_safe(cx, expr, *innerty),
175+
// std::ptr::NonNull
176+
ty::Adt(adt_def, args) if cx.tcx.is_diagnostic_item(sym::NonNull, adt_def.did()) => {
177+
report_volatile_safe(cx, expr, args.type_at(0));
178+
},
179+
_ => (),
180+
}
181+
},
182+
183+
// Also plain function calls to std::ptr::{read,write}_volatile
184+
ExprKind::Call(func, [arg_ptr, ..]) => {
185+
if let ExprKind::Path(ref qpath) = func.kind
186+
&& let Some(def_id) = cx.qpath_res(qpath, func.hir_id).opt_def_id()
187+
&& matches!(
188+
cx.tcx.get_diagnostic_name(def_id),
189+
Some(sym::ptr_read_volatile | sym::ptr_write_volatile)
190+
)
191+
&& let ty::RawPtr(ptrty, _) = cx.typeck_results().expr_ty(arg_ptr).kind()
192+
{
193+
report_volatile_safe(cx, expr, *ptrty);
194+
}
195+
},
196+
_ => {},
197+
}
198+
}
199+
}

clippy_utils/src/sym.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,7 @@ generate! {
260260
read_to_end,
261261
read_to_string,
262262
read_unaligned,
263+
read_volatile,
263264
redundant_imports,
264265
redundant_pub_crate,
265266
regex,
@@ -368,6 +369,7 @@ generate! {
368369
wrapping_offset,
369370
write,
370371
write_unaligned,
372+
write_volatile,
371373
writeln,
372374
zip,
373375
}

0 commit comments

Comments
 (0)