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

Allow partially moved values in match #103208

Merged
merged 6 commits into from
Oct 27, 2023
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
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);
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
}

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 @@ -1837,6 +1845,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 @@ -2338,6 +2347,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
Loading