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

Migrate memory overlap check from validator to lint #119577

Merged
merged 3 commits into from
Jan 5, 2024
Merged
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
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
62 changes: 48 additions & 14 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 All @@ -11,7 +12,6 @@ use rustc_mir_dataflow::{Analysis, ResultsCursor};
use std::borrow::Cow;

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

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

Lint {
let mut lint = Lint {
tcx,
when,
body,
is_fn_like: tcx.def_kind(body.source.def_id()).is_fn_like(),
always_live_locals,
reachable_blocks,
maybe_storage_live,
maybe_storage_dead,
places: Default::default(),
};
for (bb, data) in traversal::reachable(body) {
lint.visit_basic_block_data(bb, data);
}
.visit_body(body);
}

struct Lint<'a, 'tcx> {
Expand All @@ -43,9 +45,9 @@ struct Lint<'a, 'tcx> {
body: &'a Body<'tcx>,
is_fn_like: bool,
always_live_locals: &'a BitSet<Local>,
reachable_blocks: BitSet<BasicBlock>,
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 All @@ -67,7 +69,7 @@ impl<'a, 'tcx> Lint<'a, 'tcx> {

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

fn visit_statement(&mut self, statement: &Statement<'tcx>, location: Location) {
match statement.kind {
StatementKind::StorageLive(local) => {
if self.reachable_blocks.contains(location.block) {
self.maybe_storage_live.seek_before_primary_effect(location);
if self.maybe_storage_live.get().contains(local) {
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,
format!("StorageLive({local:?}) which already has storage here"),
"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) {
self.fail(
location,
format!("StorageLive({local:?}) which already has storage here"),
);
}
}
_ => {}
}

self.super_statement(statement, location);
}

fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) {
match terminator.kind {
match &terminator.kind {
TerminatorKind::Return => {
if self.is_fn_like && self.reachable_blocks.contains(location.block) {
if self.is_fn_like {
self.maybe_storage_live.seek_after_primary_effect(location);
for local in self.maybe_storage_live.get().iter() {
if !self.always_live_locals.contains(local) {
Expand All @@ -111,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
12 changes: 9 additions & 3 deletions compiler/rustc_mir_transform/src/pass_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,15 @@ fn run_passes_inner<'tcx>(
phase_change: Option<MirPhase>,
validate_each: bool,
) {
let lint = tcx.sess.opts.unstable_opts.lint_mir & !body.should_skip();
let validate = validate_each & tcx.sess.opts.unstable_opts.validate_mir & !body.should_skip();
let overridden_passes = &tcx.sess.opts.unstable_opts.mir_enable_passes;
trace!(?overridden_passes);

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

if !body.should_skip() {
let validate = validate_each & tcx.sess.opts.unstable_opts.validate_mir;
let lint = tcx.sess.opts.unstable_opts.lint_mir;

for pass in passes {
let name = pass.name();

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

dump_mir_for_phase_change(tcx, body);
if validate || new_phase == MirPhase::Runtime(RuntimePhase::Optimized) {

let validate =
(validate_each & tcx.sess.opts.unstable_opts.validate_mir & !body.should_skip())
|| new_phase == MirPhase::Runtime(RuntimePhase::Optimized);
let lint = tcx.sess.opts.unstable_opts.lint_mir & !body.should_skip();
if validate {
validate_body(tcx, body, format!("after phase change to {}", new_phase.name()));
}
if lint {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@

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

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

bb2: {
Expand Down
6 changes: 3 additions & 3 deletions tests/mir-opt/copy-prop/custom_move_arg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ use core::intrinsics::mir::*;
struct NotCopy(bool);

// EMIT_MIR custom_move_arg.f.CopyProp.diff
#[custom_mir(dialect = "analysis", phase = "post-cleanup")]
#[custom_mir(dialect = "runtime")]
fn f(_1: NotCopy) {
mir!({
let _2 = _1;
Call(RET = opaque(Move(_1)), ReturnTo(bb1), UnwindContinue())
Call(RET = opaque(Move(_1)), ReturnTo(bb1), UnwindUnreachable())
}
bb1 = {
let _3 = Move(_2);
Call(RET = opaque(_3), ReturnTo(bb2), UnwindContinue())
Call(RET = opaque(_3), ReturnTo(bb2), UnwindUnreachable())
}
bb2 = {
Return()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@
bb0: {
- _2 = _1;
- _3 = move (_2.0: u8);
- _0 = opaque::<Foo>(move _1) -> [return: bb1, unwind continue];
- _0 = opaque::<Foo>(move _1) -> [return: bb1, unwind unreachable];
+ _3 = (_1.0: u8);
+ _0 = opaque::<Foo>(_1) -> [return: bb1, unwind continue];
+ _0 = opaque::<Foo>(_1) -> [return: bb1, unwind unreachable];
}

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

bb2: {
Expand Down
6 changes: 3 additions & 3 deletions tests/mir-opt/copy-prop/move_projection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,17 @@ fn opaque(_: impl Sized) -> bool { true }

struct Foo(u8);

#[custom_mir(dialect = "analysis", phase = "post-cleanup")]
#[custom_mir(dialect = "runtime")]
fn f(a: Foo) -> bool {
mir!(
{
let b = a;
// This is a move out of a copy, so must become a copy of `a.0`.
let c = Move(b.0);
Call(RET = opaque(Move(a)), ReturnTo(bb1), UnwindContinue())
Call(RET = opaque(Move(a)), ReturnTo(bb1), UnwindUnreachable())
}
bb1 = {
Call(RET = opaque(Move(c)), ReturnTo(ret), UnwindContinue())
Call(RET = opaque(Move(c)), ReturnTo(ret), UnwindUnreachable())
}
ret = {
Return()
Expand Down
Loading
Loading