Skip to content

Commit

Permalink
Migrate memory overlap check from validator to lint
Browse files Browse the repository at this point in the history
The check attempts to identify potential undefined behaviour, rather
than whether MIR is well-formed. It belongs in the lint not validator.
  • Loading branch information
tmiasko committed Jan 4, 2024
1 parent 6e9a4e5 commit 86c8f0b
Show file tree
Hide file tree
Showing 8 changed files with 85 additions and 237 deletions.
46 changes: 3 additions & 43 deletions compiler/rustc_const_eval/src/transform/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ impl<'tcx> MirPass<'tcx> for Validator {
mir_phase,
unwind_edge_count: 0,
reachable_blocks: traversal::reachable_as_bitset(body),
place_cache: FxHashSet::default(),
value_cache: FxHashSet::default(),
can_unwind,
};
Expand Down Expand Up @@ -106,7 +105,6 @@ struct CfgChecker<'a, 'tcx> {
mir_phase: MirPhase,
unwind_edge_count: usize,
reachable_blocks: BitSet<BasicBlock>,
place_cache: FxHashSet<PlaceRef<'tcx>>,
value_cache: FxHashSet<u128>,
// If `false`, then the MIR must not contain `UnwindAction::Continue` or
// `TerminatorKind::Resume`.
Expand Down Expand Up @@ -294,19 +292,6 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {

fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
match &statement.kind {
StatementKind::Assign(box (dest, rvalue)) => {
// FIXME(JakobDegen): Check this for all rvalues, not just this one.
if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
// The sides of an assignment must not alias. Currently this just checks whether
// the places are identical.
if dest == src {
self.fail(
location,
"encountered `Assign` statement with overlapping memory",
);
}
}
}
StatementKind::AscribeUserType(..) => {
if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {
self.fail(
Expand Down Expand Up @@ -341,7 +326,8 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
self.fail(location, format!("explicit `{kind:?}` is forbidden"));
}
}
StatementKind::StorageLive(_)
StatementKind::Assign(..)
| StatementKind::StorageLive(_)
| StatementKind::StorageDead(_)
| StatementKind::Intrinsic(_)
| StatementKind::Coverage(_)
Expand Down Expand Up @@ -404,10 +390,7 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
}

// The call destination place and Operand::Move place used as an argument might be
// passed by a reference to the callee. Consequently they must be non-overlapping
// and cannot be packed. Currently this simply checks for duplicate places.
self.place_cache.clear();
self.place_cache.insert(destination.as_ref());
// passed by a reference to the callee. Consequently they cannot be packed.
if is_within_packed(self.tcx, &self.body.local_decls, *destination).is_some() {
// This is bad! The callee will expect the memory to be aligned.
self.fail(
Expand All @@ -418,10 +401,8 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
),
);
}
let mut has_duplicates = false;
for arg in args {
if let Operand::Move(place) = arg {
has_duplicates |= !self.place_cache.insert(place.as_ref());
if is_within_packed(self.tcx, &self.body.local_decls, *place).is_some() {
// This is bad! The callee will expect the memory to be aligned.
self.fail(
Expand All @@ -434,16 +415,6 @@ impl<'a, 'tcx> Visitor<'tcx> for CfgChecker<'a, 'tcx> {
}
}
}

if has_duplicates {
self.fail(
location,
format!(
"encountered overlapping memory in `Move` arguments to `Call` terminator: {:?}",
terminator.kind,
),
);
}
}
TerminatorKind::Assert { target, unwind, .. } => {
self.check_edge(location, *target, EdgeKind::Normal);
Expand Down Expand Up @@ -1112,17 +1083,6 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
)
}
}
// FIXME(JakobDegen): Check this for all rvalues, not just this one.
if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
// The sides of an assignment must not alias. Currently this just checks whether
// the places are identical.
if dest == src {
self.fail(
location,
"encountered `Assign` statement with overlapping memory",
);
}
}
}
StatementKind::AscribeUserType(..) => {
if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {
Expand Down
7 changes: 0 additions & 7 deletions compiler/rustc_mir_transform/src/dest_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@

use std::collections::hash_map::{Entry, OccupiedEntry};

use crate::simplify::remove_dead_blocks;
use crate::MirPass;
use rustc_data_structures::fx::FxHashMap;
use rustc_index::bit_set::BitSet;
Expand Down Expand Up @@ -241,12 +240,6 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation {
apply_merges(body, tcx, &merges, &merged_locals);
}

if round_count != 0 {
// Merging can introduce overlap between moved arguments and/or call destination in an
// unreachable code, which validator considers to be ill-formed.
remove_dead_blocks(body);
}

trace!(round_count);
}
}
Expand Down
43 changes: 40 additions & 3 deletions compiler/rustc_mir_transform/src/lint.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! This pass statically detects code which has undefined behaviour or is likely to be erroneous.
//! It can be used to locate problems in MIR building or optimizations. It assumes that all code
//! can be executed, so it has false positives.
use rustc_data_structures::fx::FxHashSet;
use rustc_index::bit_set::BitSet;
use rustc_middle::mir::visit::{PlaceContext, Visitor};
use rustc_middle::mir::*;
Expand Down Expand Up @@ -31,6 +32,7 @@ pub fn lint_body<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, when: String) {
always_live_locals,
maybe_storage_live,
maybe_storage_dead,
places: Default::default(),
};
for (bb, data) in traversal::reachable(body) {
lint.visit_basic_block_data(bb, data);
Expand All @@ -45,6 +47,7 @@ struct Lint<'a, 'tcx> {
always_live_locals: &'a BitSet<Local>,
maybe_storage_live: ResultsCursor<'a, 'tcx, MaybeStorageLive<'a>>,
maybe_storage_dead: ResultsCursor<'a, 'tcx, MaybeStorageDead<'a>>,
places: FxHashSet<PlaceRef<'tcx>>,
}

impl<'a, 'tcx> Lint<'a, 'tcx> {
Expand Down Expand Up @@ -75,10 +78,22 @@ impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> {
}

fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
match statement.kind {
match &statement.kind {
StatementKind::Assign(box (dest, rvalue)) => {
if let Rvalue::Use(Operand::Copy(src) | Operand::Move(src)) = rvalue {
// The sides of an assignment must not alias. Currently this just checks whether
// the places are identical.
if dest == src {
self.fail(
location,
"encountered `Assign` statement with overlapping memory",
);
}
}
}
StatementKind::StorageLive(local) => {
self.maybe_storage_live.seek_before_primary_effect(location);
if self.maybe_storage_live.get().contains(local) {
if self.maybe_storage_live.get().contains(*local) {
self.fail(
location,
format!("StorageLive({local:?}) which already has storage here"),
Expand All @@ -92,7 +107,7 @@ impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> {
}

fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) {
match terminator.kind {
match &terminator.kind {
TerminatorKind::Return => {
if self.is_fn_like {
self.maybe_storage_live.seek_after_primary_effect(location);
Expand All @@ -108,6 +123,28 @@ impl<'a, 'tcx> Visitor<'tcx> for Lint<'a, 'tcx> {
}
}
}
TerminatorKind::Call { args, destination, .. } => {
// The call destination place and Operand::Move place used as an argument might be
// passed by a reference to the callee. Consequently they must be non-overlapping.
// Currently this simply checks for duplicate places.
self.places.clear();
self.places.insert(destination.as_ref());
let mut has_duplicates = false;
for arg in args {
if let Operand::Move(place) = arg {
has_duplicates |= !self.places.insert(place.as_ref());
}
}
if has_duplicates {
self.fail(
location,
format!(
"encountered overlapping memory in `Move` arguments to `Call` terminator: {:?}",
terminator.kind,
),
);
}
}
_ => {}
}

Expand Down

This file was deleted.

This file was deleted.

20 changes: 0 additions & 20 deletions tests/mir-opt/dest-prop/unreachable.rs

This file was deleted.

Loading

0 comments on commit 86c8f0b

Please sign in to comment.