Skip to content

Commit

Permalink
Auto merge of rust-lang#103208 - cjgillot:match-fake-read, r=oli-obk,…
Browse files Browse the repository at this point in the history
…RalfJung

Allow partially moved values in match

This PR attempts to unify the behaviour between `let _ = PLACE`, `let _: TY = PLACE;` and `match PLACE { _ => {} }`.
The logical conclusion is that the `match` version should not check for uninitialised places nor check that borrows are still live.

The `match PLACE {}` case is handled by keeping a `FakeRead` in the unreachable fallback case to verify that `PLACE` has a legal value.

Schematically, `match PLACE { arms }` in surface rust becomes in MIR:
```rust
PlaceMention(PLACE)
match PLACE {
  // Decision tree for the explicit arms
  arms,
  // An extra fallback arm
  _ => {
    FakeRead(ForMatchedPlace, PLACE);
    unreachable
  }
}
```

`match *borrow { _ => {} }` continues to check that `*borrow` is live, but does not read the value.
`match *borrow {}` both checks that `*borrow` is live, and fake-reads the value.

Continuation of ~rust-lang#102256 ~rust-lang#104844

Fixes rust-lang#99180 rust-lang#53114
  • Loading branch information
bors committed Oct 27, 2023
2 parents 10143e7 + 479fb4b commit 59bb950
Show file tree
Hide file tree
Showing 64 changed files with 394 additions and 506 deletions.
68 changes: 39 additions & 29 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
/// [ 0. Pre-match ]
/// |
/// [ 1. Evaluate Scrutinee (expression being matched on) ]
/// [ (fake read of scrutinee) ]
/// [ (PlaceMention of scrutinee) ]
/// |
/// [ 2. Decision tree -- check discriminants ] <--------+
/// | |
Expand All @@ -184,7 +184,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
///
/// We generate MIR in the following steps:
///
/// 1. Evaluate the scrutinee and add the fake read of it ([Builder::lower_scrutinee]).
/// 1. Evaluate the scrutinee and add the PlaceMention of it ([Builder::lower_scrutinee]).
/// 2. Create the decision tree ([Builder::lower_match_tree]).
/// 3. Determine the fake borrows that are needed from the places that were
/// matched against and create the required temporaries for them
Expand Down Expand Up @@ -223,6 +223,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let fake_borrow_temps = self.lower_match_tree(
block,
scrutinee_span,
&scrutinee_place,
match_start_span,
match_has_guard,
&mut candidates,
Expand All @@ -238,34 +239,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
)
}

/// Evaluate the scrutinee and add the fake read of it.
/// Evaluate the scrutinee and add the PlaceMention for it.
fn lower_scrutinee(
&mut self,
mut block: BasicBlock,
scrutinee: &Expr<'tcx>,
scrutinee_span: Span,
) -> BlockAnd<PlaceBuilder<'tcx>> {
let scrutinee_place_builder = unpack!(block = self.as_place_builder(block, scrutinee));
// Matching on a `scrutinee_place` with an uninhabited type doesn't
// generate any memory reads by itself, and so if the place "expression"
// contains unsafe operations like raw pointer dereferences or union
// field projections, we wouldn't know to require an `unsafe` block
// around a `match` equivalent to `std::intrinsics::unreachable()`.
// See issue #47412 for this hole being discovered in the wild.
//
// HACK(eddyb) Work around the above issue by adding a dummy inspection
// of `scrutinee_place`, specifically by applying `ReadForMatch`.
//
// NOTE: ReadForMatch also checks that the scrutinee is initialized.
// This is currently needed to not allow matching on an uninitialized,
// uninhabited value. If we get never patterns, those will check that
// the place is initialized, and so this read would only be used to
// check safety.
let cause_matched_place = FakeReadCause::ForMatchedPlace(None);
let source_info = self.source_info(scrutinee_span);

if let Some(scrutinee_place) = scrutinee_place_builder.try_to_place(self) {
self.cfg.push_fake_read(block, source_info, cause_matched_place, scrutinee_place);
let source_info = self.source_info(scrutinee_span);
self.cfg.push_place_mention(block, source_info, scrutinee_place);
}

block.and(scrutinee_place_builder)
Expand Down Expand Up @@ -304,6 +288,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
&mut self,
block: BasicBlock,
scrutinee_span: Span,
scrutinee_place_builder: &PlaceBuilder<'tcx>,
match_start_span: Span,
match_has_guard: bool,
candidates: &mut [&mut Candidate<'pat, 'tcx>],
Expand Down Expand Up @@ -331,6 +316,33 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// otherwise block. Match checking will ensure this is actually
// unreachable.
let source_info = self.source_info(scrutinee_span);

// Matching on a `scrutinee_place` with an uninhabited type doesn't
// generate any memory reads by itself, and so if the place "expression"
// contains unsafe operations like raw pointer dereferences or union
// field projections, we wouldn't know to require an `unsafe` block
// around a `match` equivalent to `std::intrinsics::unreachable()`.
// See issue #47412 for this hole being discovered in the wild.
//
// HACK(eddyb) Work around the above issue by adding a dummy inspection
// of `scrutinee_place`, specifically by applying `ReadForMatch`.
//
// NOTE: ReadForMatch also checks that the scrutinee is initialized.
// This is currently needed to not allow matching on an uninitialized,
// uninhabited value. If we get never patterns, those will check that
// the place is initialized, and so this read would only be used to
// check safety.
let cause_matched_place = FakeReadCause::ForMatchedPlace(None);

if let Some(scrutinee_place) = scrutinee_place_builder.try_to_place(self) {
self.cfg.push_fake_read(
otherwise_block,
source_info,
cause_matched_place,
scrutinee_place,
);
}

self.cfg.terminate(otherwise_block, source_info, TerminatorKind::Unreachable);
}

Expand Down Expand Up @@ -599,13 +611,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}

_ => {
let place_builder = unpack!(block = self.as_place_builder(block, initializer));

if let Some(place) = place_builder.try_to_place(self) {
let source_info = self.source_info(initializer.span);
self.cfg.push_place_mention(block, source_info, place);
}

let place_builder =
unpack!(block = self.lower_scrutinee(block, initializer, initializer.span));
self.place_into_pattern(block, &irrefutable_pat, place_builder, true)
}
}
Expand All @@ -622,6 +629,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let fake_borrow_temps = self.lower_match_tree(
block,
irrefutable_pat.span,
&initializer,
irrefutable_pat.span,
false,
&mut [&mut candidate],
Expand Down Expand Up @@ -1841,6 +1849,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let fake_borrow_temps = self.lower_match_tree(
block,
pat.span,
&expr_place_builder,
pat.span,
false,
&mut [&mut guard_candidate, &mut otherwise_candidate],
Expand Down Expand Up @@ -2342,6 +2351,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let fake_borrow_temps = this.lower_match_tree(
block,
initializer_span,
&scrutinee,
pattern.span,
false,
&mut [&mut candidate, &mut wildcard],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Make sure we find these even with many checks disabled.
//@compile-flags: -Zmiri-disable-alignment-check -Zmiri-disable-stacked-borrows -Zmiri-disable-validation

#![allow(unreachable_code)]
#![feature(never_type)]

fn main() {
let p = {
let b = Box::new(42);
&*b as *const i32 as *const !
};
unsafe {
match *p {} //~ ERROR: entering unreachable code
}
panic!("this should never print");
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error: Undefined Behavior: entering unreachable code
--> $DIR/dangling_pointer_deref_match_never.rs:LL:CC
|
LL | match *p {}
| ^^ entering unreachable code
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: BACKTRACE:
= note: inside `main` at $DIR/dangling_pointer_deref_match_never.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

10 changes: 10 additions & 0 deletions src/tools/miri/tests/fail/never_match_never.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// This should fail even without validation
//@compile-flags: -Zmiri-disable-validation

#![feature(never_type)]
#![allow(unreachable_code)]

fn main() {
let ptr: *const (i32, !) = &0i32 as *const i32 as *const _;
unsafe { match (*ptr).1 {} } //~ ERROR: entering unreachable code
}
15 changes: 15 additions & 0 deletions src/tools/miri/tests/fail/never_match_never.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error: Undefined Behavior: entering unreachable code
--> $DIR/never_match_never.rs:LL:CC
|
LL | unsafe { match (*ptr).1 {} }
| ^^^^^^^^ entering unreachable code
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: BACKTRACE:
= note: inside `main` at $DIR/never_match_never.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// A `_` binding in a match is a nop, so we do not detect that the pointer is dangling.
//@compile-flags: -Zmiri-disable-alignment-check -Zmiri-disable-stacked-borrows -Zmiri-disable-validation

fn main() {
let p = {
let b = Box::new(42);
&*b as *const i32
};
unsafe {
match *p {
_ => {}
}
}
}
17 changes: 17 additions & 0 deletions src/tools/miri/tests/pass/union-uninhabited-match-underscore.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
fn main() {
#[derive(Copy, Clone)]
enum Void {}
union Uninit<T: Copy> {
value: T,
uninit: (),
}
unsafe {
let x: Uninit<Void> = Uninit { uninit: () };
match x.value {
// rustc warns about un unreachable pattern,
// but is wrong in unsafe code.
#[allow(unreachable_patterns)]
_ => println!("hi from the void!"),
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hi from the void!
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ fn b::{closure#0}(_1: Pin<&mut {async fn body@$DIR/async_await.rs:15:18: 18:2}>,

bb3: {
StorageDead(_5);
PlaceMention(_4);
nop;
(((*(_1.0: &mut {async fn body@$DIR/async_await.rs:15:18: 18:2})) as variant#3).0: {async fn body@$DIR/async_await.rs:12:14: 12:16}) = move _4;
goto -> bb4;
Expand Down Expand Up @@ -162,6 +163,7 @@ fn b::{closure#0}(_1: Pin<&mut {async fn body@$DIR/async_await.rs:15:18: 18:2}>,
bb7: {
StorageDead(_13);
StorageDead(_10);
PlaceMention(_9);
_16 = discriminant(_9);
switchInt(move _16) -> [0: bb10, 1: bb8, otherwise: bb9];
}
Expand Down Expand Up @@ -223,6 +225,7 @@ fn b::{closure#0}(_1: Pin<&mut {async fn body@$DIR/async_await.rs:15:18: 18:2}>,

bb15: {
StorageDead(_22);
PlaceMention(_21);
nop;
(((*(_1.0: &mut {async fn body@$DIR/async_await.rs:15:18: 18:2})) as variant#4).0: {async fn body@$DIR/async_await.rs:12:14: 12:16}) = move _21;
goto -> bb16;
Expand Down Expand Up @@ -258,6 +261,7 @@ fn b::{closure#0}(_1: Pin<&mut {async fn body@$DIR/async_await.rs:15:18: 18:2}>,
bb19: {
StorageDead(_29);
StorageDead(_26);
PlaceMention(_25);
_32 = discriminant(_25);
switchInt(move _32) -> [0: bb21, 1: bb20, otherwise: bb9];
}
Expand Down
2 changes: 1 addition & 1 deletion tests/mir-opt/building/issue_101867.main.built.after.mir
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ fn main() -> () {
FakeRead(ForLet(None), _1);
AscribeUserType(_1, o, UserTypeProjection { base: UserType(1), projs: [] });
StorageLive(_5);
FakeRead(ForMatchedPlace(None), _1);
PlaceMention(_1);
_6 = discriminant(_1);
switchInt(move _6) -> [1: bb4, otherwise: bb3];
}
Expand Down
2 changes: 1 addition & 1 deletion tests/mir-opt/building/issue_49232.main.built.after.mir
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ fn main() -> () {
StorageLive(_2);
StorageLive(_3);
_3 = const true;
FakeRead(ForMatchedPlace(None), _3);
PlaceMention(_3);
switchInt(_3) -> [0: bb3, otherwise: bb4];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ fn test_complex() -> () {
}

bb1: {
FakeRead(ForMatchedPlace(None), _2);
PlaceMention(_2);
_3 = discriminant(_2);
switchInt(move _3) -> [0: bb2, otherwise: bb3];
}
Expand Down Expand Up @@ -151,7 +151,7 @@ fn test_complex() -> () {
}

bb25: {
FakeRead(ForMatchedPlace(None), _12);
PlaceMention(_12);
_13 = discriminant(_12);
switchInt(move _13) -> [1: bb27, otherwise: bb26];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ fn full_tested_match() -> () {
StorageLive(_1);
StorageLive(_2);
_2 = Option::<i32>::Some(const 42_i32);
FakeRead(ForMatchedPlace(None), _2);
PlaceMention(_2);
_3 = discriminant(_2);
switchInt(move _3) -> [0: bb1, 1: bb2, otherwise: bb4];
}
Expand All @@ -45,6 +45,7 @@ fn full_tested_match() -> () {
}

bb4: {
FakeRead(ForMatchedPlace(None), _2);
unreachable;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ fn full_tested_match2() -> () {
StorageLive(_1);
StorageLive(_2);
_2 = Option::<i32>::Some(const 42_i32);
FakeRead(ForMatchedPlace(None), _2);
PlaceMention(_2);
_3 = discriminant(_2);
switchInt(move _3) -> [0: bb1, 1: bb2, otherwise: bb4];
}
Expand All @@ -51,6 +51,7 @@ fn full_tested_match2() -> () {
}

bb4: {
FakeRead(ForMatchedPlace(None), _2);
unreachable;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ fn main() -> () {
StorageLive(_1);
StorageLive(_2);
_2 = Option::<i32>::Some(const 1_i32);
FakeRead(ForMatchedPlace(None), _2);
PlaceMention(_2);
_4 = discriminant(_2);
switchInt(move _4) -> [1: bb2, otherwise: bb1];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ fn match_bool(_1: bool) -> usize {
let mut _0: usize;

bb0: {
FakeRead(ForMatchedPlace(None), _1);
PlaceMention(_1);
switchInt(_1) -> [0: bb2, otherwise: bb1];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
fn unreachable_box() -> ! {
let mut _0: !;
let _1: std::boxed::Box<Never>;
let mut _2: *const Never;
scope 1 {
debug x => _1;
}
Expand All @@ -13,7 +14,9 @@
bb0: {
StorageLive(_1);
- _1 = const 1_usize as std::boxed::Box<Never> (Transmute);
- _2 = (((_1.0: std::ptr::Unique<Never>).0: std::ptr::NonNull<Never>).0: *const Never);
+ _1 = const Box::<Never>(Unique::<Never> {{ pointer: NonNull::<Never> {{ pointer: {0x1 as *const Never} }}, _marker: PhantomData::<Never> }}, std::alloc::Global);
+ _2 = const {0x1 as *const Never};
unreachable;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
fn unreachable_box() -> ! {
let mut _0: !;
let _1: std::boxed::Box<Never>;
let mut _2: *const Never;
scope 1 {
debug x => _1;
}
Expand All @@ -13,7 +14,9 @@
bb0: {
StorageLive(_1);
- _1 = const 1_usize as std::boxed::Box<Never> (Transmute);
- _2 = (((_1.0: std::ptr::Unique<Never>).0: std::ptr::NonNull<Never>).0: *const Never);
+ _1 = const Box::<Never>(Unique::<Never> {{ pointer: NonNull::<Never> {{ pointer: {0x1 as *const Never} }}, _marker: PhantomData::<Never> }}, std::alloc::Global);
+ _2 = const {0x1 as *const Never};
unreachable;
}
}
Expand Down
Loading

0 comments on commit 59bb950

Please sign in to comment.