Skip to content

Commit 894cc61

Browse files
matthiaskrgrmatthiaskrgr
authored
Rollup merge of rust-lang#119577 - tmiasko:lint, r=oli-obk
Migrate memory overlap check from validator to lint The check attempts to identify potential undefined behaviour, rather than whether MIR is well-formed. It belongs in the lint not validator. Follow up to changes from rust-lang#119077.
2 parents c29fa7e + df116ec commit 894cc61

13 files changed

+115
-264
lines changed

Diff for: ‎compiler/rustc_const_eval/src/transform/validate.rs

+3-43
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,6 @@ impl<'tcx> MirPass<'tcx> for Validator {
7474
mir_phase,
7575
unwind_edge_count: 0,
7676
reachable_blocks: traversal::reachable_as_bitset(body),
77-
place_cache: FxHashSet::default(),
7877
value_cache: FxHashSet::default(),
7978
can_unwind,
8079
};
@@ -106,7 +105,6 @@ struct CfgChecker<'a, 'tcx> {
106105
mir_phase: MirPhase,
107106
unwind_edge_count: usize,
108107
reachable_blocks: BitSet<BasicBlock>,
109-
place_cache: FxHashSet<PlaceRef<'tcx>>,
110108
value_cache: FxHashSet<u128>,
111109
// If `false`, then the MIR must not contain `UnwindAction::Continue` or
112110
// `TerminatorKind::Resume`.
@@ -294,19 +292,6 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
294292

295293
fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
296294
match &statement.kind {
297-
StatementKind::Assign(box (dest, rvalue)) => {
298-
// FIXME(JakobDegen): Check this for all rvalues, not just this one.
299-
if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
300-
// The sides of an assignment must not alias. Currently this just checks whether
301-
// the places are identical.
302-
if dest == src {
303-
self.fail(
304-
location,
305-
"encountered `Assign` statement with overlapping memory",
306-
);
307-
}
308-
}
309-
}
310295
StatementKind::AscribeUserType(..) => {
311296
if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {
312297
self.fail(
@@ -341,7 +326,8 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
341326
self.fail(location, format!("explicit `{kind:?}` is forbidden"));
342327
}
343328
}
344-
StatementKind::StorageLive(_)
329+
StatementKind::Assign(..)
330+
| StatementKind::StorageLive(_)
345331
| StatementKind::StorageDead(_)
346332
| StatementKind::Intrinsic(_)
347333
| StatementKind::Coverage(_)
@@ -404,10 +390,7 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
404390
}
405391

406392
// The call destination place and Operand::Move place used as an argument might be
407-
// passed by a reference to the callee. Consequently they must be non-overlapping
408-
// and cannot be packed. Currently this simply checks for duplicate places.
409-
self.place_cache.clear();
410-
self.place_cache.insert(destination.as_ref());
393+
// passed by a reference to the callee. Consequently they cannot be packed.
411394
if is_within_packed(self.tcx, &self.body.local_decls, *destination).is_some() {
412395
// This is bad! The callee will expect the memory to be aligned.
413396
self.fail(
@@ -418,10 +401,8 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
418401
),
419402
);
420403
}
421-
let mut has_duplicates = false;
422404
for arg in args {
423405
if let Operand::Move(place) = arg {
424-
has_duplicates |= !self.place_cache.insert(place.as_ref());
425406
if is_within_packed(self.tcx, &self.body.local_decls, *place).is_some() {
426407
// This is bad! The callee will expect the memory to be aligned.
427408
self.fail(
@@ -434,16 +415,6 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
434415
}
435416
}
436417
}
437-
438-
if has_duplicates {
439-
self.fail(
440-
location,
441-
format!(
442-
"encountered overlapping memory in `Move` arguments to `Call` terminator: {:?}",
443-
terminator.kind,
444-
),
445-
);
446-
}
447418
}
448419
TerminatorKind::Assert { target, unwind, .. } => {
449420
self.check_edge(location, *target, EdgeKind::Normal);
@@ -1112,17 +1083,6 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
11121083
)
11131084
}
11141085
}
1115-
// FIXME(JakobDegen): Check this for all rvalues, not just this one.
1116-
if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
1117-
// The sides of an assignment must not alias. Currently this just checks whether
1118-
// the places are identical.
1119-
if dest == src {
1120-
self.fail(
1121-
location,
1122-
"encountered `Assign` statement with overlapping memory",
1123-
);
1124-
}
1125-
}
11261086
}
11271087
StatementKind::AscribeUserType(..) => {
11281088
if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {

Diff for: ‎compiler/rustc_mir_transform/src/dest_prop.rs

-7
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,6 @@
133133
134134
use std::collections::hash_map::{Entry, OccupiedEntry};
135135

136-
use crate::simplify::remove_dead_blocks;
137136
use crate::MirPass;
138137
use rustc_data_structures::fx::FxHashMap;
139138
use rustc_index::bit_set::BitSet;
@@ -241,12 +240,6 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation {
241240
apply_merges(body, tcx, &merges, &merged_locals);
242241
}
243242

244-
if round_count != 0 {
245-
// Merging can introduce overlap between moved arguments and/or call destination in an
246-
// unreachable code, which validator considers to be ill-formed.
247-
remove_dead_blocks(body);
248-
}
249-
250243
trace!(round_count);
251244
}
252245
}

Diff for: ‎compiler/rustc_mir_transform/src/lint.rs

+48-14
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
//! This pass statically detects code which has undefined behaviour or is likely to be erroneous.
22
//! It can be used to locate problems in MIR building or optimizations. It assumes that all code
33
//! can be executed, so it has false positives.
4+
use rustc_data_structures::fx::FxHashSet;
45
use rustc_index::bit_set::BitSet;
56
use rustc_middle::mir::visit::{PlaceContext, Visitor};
67
use rustc_middle::mir::*;
@@ -11,7 +12,6 @@ use rustc_mir_dataflow::{Analysis, ResultsCursor};
1112
use std::borrow::Cow;
1213

1314
pub fn lint_body<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, when: String) {
14-
let reachable_blocks = traversal::reachable_as_bitset(body);
1515
let always_live_locals = &always_storage_live_locals(body);
1616

1717
let maybe_storage_live = MaybeStorageLive::new(Cow::Borrowed(always_live_locals))
@@ -24,17 +24,19 @@ pub fn lint_body<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, when: String) {
2424
.iterate_to_fixpoint()
2525
.into_results_cursor(body);
2626

27-
Lint {
27+
let mut lint = Lint {
2828
tcx,
2929
when,
3030
body,
3131
is_fn_like: tcx.def_kind(body.source.def_id()).is_fn_like(),
3232
always_live_locals,
33-
reachable_blocks,
3433
maybe_storage_live,
3534
maybe_storage_dead,
35+
places: Default::default(),
36+
};
37+
for (bb, data) in traversal::reachable(body) {
38+
lint.visit_basic_block_data(bb, data);
3639
}
37-
.visit_body(body);
3840
}
3941

4042
struct Lint<'a, 'tcx> {
@@ -43,9 +45,9 @@ struct Lint<'a, 'tcx> {
4345
body: &'a Body<'tcx>,
4446
is_fn_like: bool,
4547
always_live_locals: &'a BitSet<Local>,
46-
reachable_blocks: BitSet<BasicBlock>,
4748
maybe_storage_live: ResultsCursor<'a, 'tcx, MaybeStorageLive<'a>>,
4849
maybe_storage_dead: ResultsCursor<'a, 'tcx, MaybeStorageDead<'a>>,
50+
places: FxHashSet<PlaceRef<'tcx>>,
4951
}
5052

5153
impl<'a, 'tcx> Lint<'a, 'tcx> {
@@ -67,7 +69,7 @@ impl<'a, 'tcx> Lint<'a, 'tcx> {
6769

6870
impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> {
6971
fn visit_local(&mut self, local: Local, context: PlaceContext, location: Location) {
70-
if self.reachable_blocks.contains(location.block) && context.is_use() {
72+
if context.is_use() {
7173
self.maybe_storage_dead.seek_after_primary_effect(location);
7274
if self.maybe_storage_dead.get().contains(local) {
7375
self.fail(location, format!("use of local {local:?}, which has no storage here"));
@@ -76,28 +78,38 @@ impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> {
7678
}
7779

7880
fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
79-
match statement.kind {
80-
StatementKind::StorageLive(local) => {
81-
if self.reachable_blocks.contains(location.block) {
82-
self.maybe_storage_live.seek_before_primary_effect(location);
83-
if self.maybe_storage_live.get().contains(local) {
81+
match &statement.kind {
82+
StatementKind::Assign(box (dest, rvalue)) => {
83+
if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
84+
// The sides of an assignment must not alias. Currently this just checks whether
85+
// the places are identical.
86+
if dest == src {
8487
self.fail(
8588
location,
86-
format!("StorageLive({local:?}) which already has storage here"),
89+
"encountered `Assign` statement with overlapping memory",
8790
);
8891
}
8992
}
9093
}
94+
StatementKind::StorageLive(local) => {
95+
self.maybe_storage_live.seek_before_primary_effect(location);
96+
if self.maybe_storage_live.get().contains(*local) {
97+
self.fail(
98+
location,
99+
format!("StorageLive({local:?}) which already has storage here"),
100+
);
101+
}
102+
}
91103
_ => {}
92104
}
93105

94106
self.super_statement(statement, location);
95107
}
96108

97109
fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) {
98-
match terminator.kind {
110+
match &terminator.kind {
99111
TerminatorKind::Return => {
100-
if self.is_fn_like && self.reachable_blocks.contains(location.block) {
112+
if self.is_fn_like {
101113
self.maybe_storage_live.seek_after_primary_effect(location);
102114
for local in self.maybe_storage_live.get().iter() {
103115
if !self.always_live_locals.contains(local) {
@@ -111,6 +123,28 @@ impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> {
111123
}
112124
}
113125
}
126+
TerminatorKind::Call { args, destination, .. } => {
127+
// The call destination place and Operand::Move place used as an argument might be
128+
// passed by a reference to the callee. Consequently they must be non-overlapping.
129+
// Currently this simply checks for duplicate places.
130+
self.places.clear();
131+
self.places.insert(destination.as_ref());
132+
let mut has_duplicates = false;
133+
for arg in args {
134+
if let Operand::Move(place) = arg {
135+
has_duplicates |= !self.places.insert(place.as_ref());
136+
}
137+
}
138+
if has_duplicates {
139+
self.fail(
140+
location,
141+
format!(
142+
"encountered overlapping memory in `Move` arguments to `Call` terminator: {:?}",
143+
terminator.kind,
144+
),
145+
);
146+
}
147+
}
114148
_ => {}
115149
}
116150

Diff for: ‎compiler/rustc_mir_transform/src/pass_manager.rs

+9-3
Original file line numberDiff line numberDiff line change
@@ -109,14 +109,15 @@ fn run_passes_inner<'tcx>(
109109
phase_change: Option<MirPhase>,
110110
validate_each: bool,
111111
) {
112-
let lint = tcx.sess.opts.unstable_opts.lint_mir & !body.should_skip();
113-
let validate = validate_each & tcx.sess.opts.unstable_opts.validate_mir & !body.should_skip();
114112
let overridden_passes = &tcx.sess.opts.unstable_opts.mir_enable_passes;
115113
trace!(?overridden_passes);
116114

117115
let prof_arg = tcx.sess.prof.enabled().then(|| format!("{:?}", body.source.def_id()));
118116

119117
if !body.should_skip() {
118+
let validate = validate_each & tcx.sess.opts.unstable_opts.validate_mir;
119+
let lint = tcx.sess.opts.unstable_opts.lint_mir;
120+
120121
for pass in passes {
121122
let name = pass.name();
122123

@@ -162,7 +163,12 @@ fn run_passes_inner<'tcx>(
162163
body.pass_count = 0;
163164

164165
dump_mir_for_phase_change(tcx, body);
165-
if validate || new_phase == MirPhase::Runtime(RuntimePhase::Optimized) {
166+
167+
let validate =
168+
(validate_each & tcx.sess.opts.unstable_opts.validate_mir & !body.should_skip())
169+
|| new_phase == MirPhase::Runtime(RuntimePhase::Optimized);
170+
let lint = tcx.sess.opts.unstable_opts.lint_mir & !body.should_skip();
171+
if validate {
166172
validate_body(tcx, body, format!("after phase change to {}", new_phase.name()));
167173
}
168174
if lint {

Diff for: ‎tests/mir-opt/copy-prop/custom_move_arg.f.CopyProp.panic-unwind.diff

+4-4
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,14 @@
88

99
bb0: {
1010
- _2 = _1;
11-
- _0 = opaque::<NotCopy>(move _1) -> [return: bb1, unwind continue];
12-
+ _0 = opaque::<NotCopy>(_1) -> [return: bb1, unwind continue];
11+
- _0 = opaque::<NotCopy>(move _1) -> [return: bb1, unwind unreachable];
12+
+ _0 = opaque::<NotCopy>(_1) -> [return: bb1, unwind unreachable];
1313
}
1414

1515
bb1: {
1616
- _3 = move _2;
17-
- _0 = opaque::<NotCopy>(_3) -> [return: bb2, unwind continue];
18-
+ _0 = opaque::<NotCopy>(_1) -> [return: bb2, unwind continue];
17+
- _0 = opaque::<NotCopy>(_3) -> [return: bb2, unwind unreachable];
18+
+ _0 = opaque::<NotCopy>(_1) -> [return: bb2, unwind unreachable];
1919
}
2020

2121
bb2: {

Diff for: ‎tests/mir-opt/copy-prop/custom_move_arg.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,15 @@ use core::intrinsics::mir::*;
1010
struct NotCopy(bool);
1111

1212
// EMIT_MIR custom_move_arg.f.CopyProp.diff
13-
#[custom_mir(dialect = "analysis", phase = "post-cleanup")]
13+
#[custom_mir(dialect = "runtime")]
1414
fn f(_1: NotCopy) {
1515
mir!({
1616
let _2 = _1;
17-
Call(RET = opaque(Move(_1)), ReturnTo(bb1), UnwindContinue())
17+
Call(RET = opaque(Move(_1)), ReturnTo(bb1), UnwindUnreachable())
1818
}
1919
bb1 = {
2020
let _3 = Move(_2);
21-
Call(RET = opaque(_3), ReturnTo(bb2), UnwindContinue())
21+
Call(RET = opaque(_3), ReturnTo(bb2), UnwindUnreachable())
2222
}
2323
bb2 = {
2424
Return()

Diff for: ‎tests/mir-opt/copy-prop/move_projection.f.CopyProp.panic-unwind.diff

+3-3
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,13 @@
99
bb0: {
1010
- _2 = _1;
1111
- _3 = move (_2.0: u8);
12-
- _0 = opaque::<Foo>(move _1) -> [return: bb1, unwind continue];
12+
- _0 = opaque::<Foo>(move _1) -> [return: bb1, unwind unreachable];
1313
+ _3 = (_1.0: u8);
14-
+ _0 = opaque::<Foo>(_1) -> [return: bb1, unwind continue];
14+
+ _0 = opaque::<Foo>(_1) -> [return: bb1, unwind unreachable];
1515
}
1616

1717
bb1: {
18-
_0 = opaque::<u8>(move _3) -> [return: bb2, unwind continue];
18+
_0 = opaque::<u8>(move _3) -> [return: bb2, unwind unreachable];
1919
}
2020

2121
bb2: {

Diff for: ‎tests/mir-opt/copy-prop/move_projection.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -11,17 +11,17 @@ fn opaque(_: impl Sized) -> bool { true }
1111

1212
struct Foo(u8);
1313

14-
#[custom_mir(dialect = "analysis", phase = "post-cleanup")]
14+
#[custom_mir(dialect = "runtime")]
1515
fn f(a: Foo) -> bool {
1616
mir!(
1717
{
1818
let b = a;
1919
// This is a move out of a copy, so must become a copy of `a.0`.
2020
let c = Move(b.0);
21-
Call(RET = opaque(Move(a)), ReturnTo(bb1), UnwindContinue())
21+
Call(RET = opaque(Move(a)), ReturnTo(bb1), UnwindUnreachable())
2222
}
2323
bb1 = {
24-
Call(RET = opaque(Move(c)), ReturnTo(ret), UnwindContinue())
24+
Call(RET = opaque(Move(c)), ReturnTo(ret), UnwindUnreachable())
2525
}
2626
ret = {
2727
Return()

0 commit comments

Comments
 (0)