Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Unrolled build for rust-lang#119577
Browse files Browse the repository at this point in the history
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.
rust-timer authored Jan 5, 2024
2 parents 432fffa + df116ec commit 27ef689
Showing 13 changed files with 115 additions and 264 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
@@ -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,
};
@@ -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`.
@@ -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(
@@ -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(_)
@@ -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(
@@ -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(
@@ -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);
@@ -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) {
7 changes: 0 additions & 7 deletions compiler/rustc_mir_transform/src/dest_prop.rs
Original file line number Diff line number Diff line change
@@ -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;
@@ -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);
}
}
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::*;
@@ -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))
@@ -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> {
@@ -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> {
@@ -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"));
@@ -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) {
@@ -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,
),
);
}
}
_ => {}
}

12 changes: 9 additions & 3 deletions compiler/rustc_mir_transform/src/pass_manager.rs
Original file line number Diff line number Diff line change
@@ -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();

@@ -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 {
Original file line number Diff line number Diff line change
@@ -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: {
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
@@ -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()
Original file line number Diff line number Diff line change
@@ -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: {
6 changes: 3 additions & 3 deletions tests/mir-opt/copy-prop/move_projection.rs
Original file line number Diff line number Diff line change
@@ -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()

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.

19 changes: 19 additions & 0 deletions tests/ui/mir/lint/assignment-overlap.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// compile-flags: --crate-type=lib -Zlint-mir -Ztreat-err-as-bug
// build-fail
// failure-status: 101
// dont-check-compiler-stderr
// error-pattern: encountered `Assign` statement with overlapping memory
#![feature(custom_mir, core_intrinsics)]
extern crate core;
use core::intrinsics::mir::*;

#[custom_mir(dialect = "runtime", phase = "optimized")]
pub fn main() {
mir!(
let a: [u8; 1024];
{
a = a;
Return()
}
)
}
23 changes: 23 additions & 0 deletions tests/ui/mir/lint/call-overlap.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// compile-flags: -Zlint-mir -Ztreat-err-as-bug
// build-fail
// failure-status: 101
// dont-check-compiler-stderr
// error-pattern: encountered overlapping memory in `Move` arguments to `Call`
#![feature(custom_mir, core_intrinsics)]
extern crate core;
use core::intrinsics::mir::*;

#[custom_mir(dialect = "runtime", phase = "optimized")]
pub fn main() {
mir!(
let a: [u8; 1024];
{
Call(a = f(Move(a)), ReturnTo(bb1), UnwindUnreachable())
}
bb1 = {
Return()
}
)
}

pub fn f<T: Copy>(a: T) -> T { a }

0 comments on commit 27ef689

Please sign in to comment.