Skip to content

Commit 21fab43

Browse files
borsbors
committed
Auto merge of #104844 - cjgillot:mention-eval-place, r=jackh726,RalfJung
Evaluate place expression in `PlaceMention` #102256 introduces a `PlaceMention(place)` MIR statement which keep trace of `let _ = place` statements from surface rust, but without semantics. This PR proposes to change the behaviour of `let _ =` patterns with respect to the borrow-checker to verify that the bound place is live. Specifically, consider this code: ```rust let _ = { let a = 5; &a }; ``` This passes borrowck without error on stable. Meanwhile, replacing `_` by `_: _` or `_p` errors with "error[E0597]: `a` does not live long enough", [see playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c448d25a7c205dc95a0967fe96bccce8). This PR *does not* change how `_` patterns behave with respect to initializedness: it remains ok to bind a moved-from place to `_`. The relevant test is `tests/ui/borrowck/let_underscore_temporary.rs`. Crater check found no regression. For consistency, this PR changes miri to evaluate the place found in `PlaceMention`, and report eventual dangling pointers found within it. r? `@RalfJung`
2 parents ccb6290 + 2870d26 commit 21fab43

22 files changed

+201
-39
lines changed

‎compiler/rustc_borrowck/src/def_use.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,16 @@ pub fn categorize(context: PlaceContext) -> Option<DefUse> {
5252
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow) |
5353
PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow) |
5454

55+
// `PlaceMention` and `AscribeUserType` both evaluate the place, which must not
56+
// contain dangling references.
57+
PlaceContext::NonUse(NonUseContext::PlaceMention) |
58+
PlaceContext::NonUse(NonUseContext::AscribeUserTy) |
59+
5560
PlaceContext::MutatingUse(MutatingUseContext::AddressOf) |
5661
PlaceContext::NonMutatingUse(NonMutatingUseContext::AddressOf) |
5762
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect) |
5863
PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) |
5964
PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) |
60-
PlaceContext::NonUse(NonUseContext::AscribeUserTy) |
6165
PlaceContext::MutatingUse(MutatingUseContext::Retag) =>
6266
Some(DefUse::Use),
6367

@@ -72,8 +76,6 @@ pub fn categorize(context: PlaceContext) -> Option<DefUse> {
7276
PlaceContext::MutatingUse(MutatingUseContext::Drop) =>
7377
Some(DefUse::Drop),
7478

75-
// This statement exists to help unsafeck. It does not require the place to be live.
76-
PlaceContext::NonUse(NonUseContext::PlaceMention) => None,
7779
// Debug info is neither def nor use.
7880
PlaceContext::NonUse(NonUseContext::VarDebugInfo) => None,
7981

‎compiler/rustc_borrowck/src/invalidation.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
7979
}
8080
// Only relevant for mir typeck
8181
StatementKind::AscribeUserType(..)
82-
// Only relevant for unsafeck
82+
// Only relevant for liveness and unsafeck
8383
| StatementKind::PlaceMention(..)
8484
// Doesn't have any language semantics
8585
| StatementKind::Coverage(..)

‎compiler/rustc_borrowck/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -665,7 +665,7 @@ impl<'cx, 'tcx> rustc_mir_dataflow::ResultsVisitor<'cx, 'tcx> for MirBorrowckCtx
665665
}
666666
// Only relevant for mir typeck
667667
StatementKind::AscribeUserType(..)
668-
// Only relevant for unsafeck
668+
// Only relevant for liveness and unsafeck
669669
| StatementKind::PlaceMention(..)
670670
// Doesn't have any language semantics
671671
| StatementKind::Coverage(..)

‎compiler/rustc_const_eval/src/interpret/step.rs

+8-2
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
113113

114114
Intrinsic(box intrinsic) => self.emulate_nondiverging_intrinsic(intrinsic)?,
115115

116-
// Statements we do not track.
117-
PlaceMention(..) | AscribeUserType(..) => {}
116+
// Evaluate the place expression, without reading from it.
117+
PlaceMention(box place) => {
118+
let _ = self.eval_place(*place)?;
119+
}
120+
121+
// This exists purely to guide borrowck lifetime inference, and does not have
122+
// an operational effect.
123+
AscribeUserType(..) => {}
118124

119125
// Currently, Miri discards Coverage statements. Coverage statements are only injected
120126
// via an optional compile time MIR pass and have no side effects. Since Coverage

‎compiler/rustc_const_eval/src/transform/validate.rs

+1-8
Original file line numberDiff line numberDiff line change
@@ -802,14 +802,6 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
802802
}
803803
}
804804
}
805-
StatementKind::PlaceMention(..) => {
806-
if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {
807-
self.fail(
808-
location,
809-
"`PlaceMention` should have been removed after drop lowering phase",
810-
);
811-
}
812-
}
813805
StatementKind::AscribeUserType(..) => {
814806
if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {
815807
self.fail(
@@ -919,6 +911,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
919911
StatementKind::StorageDead(_)
920912
| StatementKind::Coverage(_)
921913
| StatementKind::ConstEvalCounter
914+
| StatementKind::PlaceMention(..)
922915
| StatementKind::Nop => {}
923916
}
924917

‎compiler/rustc_interface/src/tests.rs

+1
Original file line numberDiff line numberDiff line change
@@ -768,6 +768,7 @@ fn test_unstable_options_tracking_hash() {
768768
tracked!(merge_functions, Some(MergeFunctions::Disabled));
769769
tracked!(mir_emit_retag, true);
770770
tracked!(mir_enable_passes, vec![("DestProp".to_string(), false)]);
771+
tracked!(mir_keep_place_mention, true);
771772
tracked!(mir_opt_level, Some(4));
772773
tracked!(move_size_limit, Some(4096));
773774
tracked!(mutable_noalias, false);

‎compiler/rustc_middle/src/mir/syntax.rs

+2-3
Original file line numberDiff line numberDiff line change
@@ -331,9 +331,8 @@ pub enum StatementKind<'tcx> {
331331
/// This is especially useful for `let _ = PLACE;` bindings that desugar to a single
332332
/// `PlaceMention(PLACE)`.
333333
///
334-
/// When executed at runtime this is a nop.
335-
///
336-
/// Disallowed after drop elaboration.
334+
/// When executed at runtime, this computes the given place, but then discards
335+
/// it without doing a load. It is UB if the place is not pointing to live memory.
337336
PlaceMention(Box<Place<'tcx>>),
338337

339338
/// Encodes a user's type ascription. These need to be preserved

‎compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs

-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ impl<'tcx> MirPass<'tcx> for CleanupPostBorrowck {
2424
for statement in basic_block.statements.iter_mut() {
2525
match statement.kind {
2626
StatementKind::AscribeUserType(..)
27-
| StatementKind::PlaceMention(..)
2827
| StatementKind::Assign(box (_, Rvalue::Ref(_, BorrowKind::Shallow, _)))
2928
| StatementKind::FakeRead(..) => statement.make_nop(),
3029
_ => (),

‎compiler/rustc_mir_transform/src/dead_store_elimination.rs

+2-3
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,10 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS
5454
| StatementKind::Coverage(_)
5555
| StatementKind::Intrinsic(_)
5656
| StatementKind::ConstEvalCounter
57+
| StatementKind::PlaceMention(_)
5758
| StatementKind::Nop => (),
5859

59-
StatementKind::FakeRead(_)
60-
| StatementKind::PlaceMention(_)
61-
| StatementKind::AscribeUserType(_, _) => {
60+
StatementKind::FakeRead(_) | StatementKind::AscribeUserType(_, _) => {
6261
bug!("{:?} not found in this MIR phase!", &statement.kind)
6362
}
6463
}

‎compiler/rustc_mir_transform/src/dest_prop.rs

+3-4
Original file line numberDiff line numberDiff line change
@@ -582,10 +582,9 @@ impl WriteInfo {
582582
| StatementKind::Nop
583583
| StatementKind::Coverage(_)
584584
| StatementKind::StorageLive(_)
585-
| StatementKind::StorageDead(_) => (),
586-
StatementKind::FakeRead(_)
587-
| StatementKind::AscribeUserType(_, _)
588-
| StatementKind::PlaceMention(_) => {
585+
| StatementKind::StorageDead(_)
586+
| StatementKind::PlaceMention(_) => (),
587+
StatementKind::FakeRead(_) | StatementKind::AscribeUserType(_, _) => {
589588
bug!("{:?} not found in this MIR phase", statement)
590589
}
591590
}

‎compiler/rustc_mir_transform/src/lib.rs

+6-2
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ mod add_retag;
4848
mod check_const_item_mutation;
4949
mod check_packed_ref;
5050
pub mod check_unsafety;
51+
mod remove_place_mention;
5152
// This pass is public to allow external drivers to perform MIR cleanup
5253
pub mod cleanup_post_borrowck;
5354
mod const_debuginfo;
@@ -460,8 +461,11 @@ fn run_runtime_lowering_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
460461

461462
/// Returns the sequence of passes that do the initial cleanup of runtime MIR.
462463
fn run_runtime_cleanup_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
463-
let passes: &[&dyn MirPass<'tcx>] =
464-
&[&lower_intrinsics::LowerIntrinsics, &simplify::SimplifyCfg::ElaborateDrops];
464+
let passes: &[&dyn MirPass<'tcx>] = &[
465+
&lower_intrinsics::LowerIntrinsics,
466+
&remove_place_mention::RemovePlaceMention,
467+
&simplify::SimplifyCfg::ElaborateDrops,
468+
];
465469

466470
pm::run_passes(tcx, body, passes, Some(MirPhase::Runtime(RuntimePhase::PostCleanup)));
467471

‎compiler/rustc_mir_transform/src/remove_place_mention.rs

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
//! This pass removes `PlaceMention` statement, which has no effect at codegen.
2+
3+
use crate::MirPass;
4+
use rustc_middle::mir::*;
5+
use rustc_middle::ty::TyCtxt;
6+
7+
pub struct RemovePlaceMention;
8+
9+
impl<'tcx> MirPass<'tcx> for RemovePlaceMention {
10+
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
11+
!sess.opts.unstable_opts.mir_keep_place_mention
12+
}
13+
14+
fn run_pass(&self, _: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
15+
trace!("Running RemovePlaceMention on {:?}", body.source);
16+
for data in body.basic_blocks.as_mut_preserves_cfg() {
17+
data.statements.retain(|statement| match statement.kind {
18+
StatementKind::PlaceMention(..) | StatementKind::Nop => false,
19+
_ => true,
20+
})
21+
}
22+
}
23+
}

‎compiler/rustc_session/src/options.rs

+3
Original file line numberDiff line numberDiff line change
@@ -1558,6 +1558,9 @@ options! {
15581558
"use like `-Zmir-enable-passes=+DestProp,-InstCombine`. Forces the specified passes to be \
15591559
enabled, overriding all other checks. Passes that are not specified are enabled or \
15601560
disabled by other flags as usual."),
1561+
mir_keep_place_mention: bool = (false, parse_bool, [TRACKED],
1562+
"keep place mention MIR statements, interpreted e.g., by miri; implies -Zmir-opt-level=0 \
1563+
(default: no)"),
15611564
#[rustc_lint_opt_deny_field_access("use `Session::mir_opt_level` instead of this field")]
15621565
mir_opt_level: Option<usize> = (None, parse_opt_number, [TRACKED],
15631566
"MIR optimization level (0-4; default: 1 in non optimized builds and 2 in optimized builds)"),

‎src/tools/clippy/tests/ui/option_if_let_else.fixed

+2-2
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> {
2525
fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
2626
let _ = string.map_or(0, |s| s.len());
2727
let _ = num.as_ref().map_or(&0, |s| s);
28-
let _ = num.as_mut().map_or(&mut 0, |s| {
28+
let _ = num.as_mut().map_or(&0, |s| {
2929
*s += 1;
3030
s
3131
});
@@ -34,7 +34,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
3434
s += 1;
3535
s
3636
});
37-
let _ = num.as_mut().map_or(&mut 0, |s| {
37+
let _ = num.as_mut().map_or(&0, |s| {
3838
*s += 1;
3939
s
4040
});

‎src/tools/clippy/tests/ui/option_if_let_else.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
3333
*s += 1;
3434
s
3535
} else {
36-
&mut 0
36+
&0
3737
};
3838
let _ = if let Some(ref s) = num { s } else { &0 };
3939
let _ = if let Some(mut s) = num {
@@ -46,7 +46,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
4646
*s += 1;
4747
s
4848
} else {
49-
&mut 0
49+
&0
5050
};
5151
}
5252

‎src/tools/clippy/tests/ui/option_if_let_else.stderr

+4-4
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,13 @@ LL | let _ = if let Some(s) = &mut num {
3030
LL | | *s += 1;
3131
LL | | s
3232
LL | | } else {
33-
LL | | &mut 0
33+
LL | | &0
3434
LL | | };
3535
| |_____^
3636
|
3737
help: try
3838
|
39-
LL ~ let _ = num.as_mut().map_or(&mut 0, |s| {
39+
LL ~ let _ = num.as_mut().map_or(&0, |s| {
4040
LL + *s += 1;
4141
LL + s
4242
LL ~ });
@@ -76,13 +76,13 @@ LL | let _ = if let Some(ref mut s) = num {
7676
LL | | *s += 1;
7777
LL | | s
7878
LL | | } else {
79-
LL | | &mut 0
79+
LL | | &0
8080
LL | | };
8181
| |_____^
8282
|
8383
help: try
8484
|
85-
LL ~ let _ = num.as_mut().map_or(&mut 0, |s| {
85+
LL ~ let _ = num.as_mut().map_or(&0, |s| {
8686
LL + *s += 1;
8787
LL + s
8888
LL ~ });

‎src/tools/miri/src/lib.rs

+1
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ pub const MIRI_DEFAULT_ARGS: &[&str] = &[
130130
"-Zalways-encode-mir",
131131
"-Zextra-const-ub-checks",
132132
"-Zmir-emit-retag",
133+
"-Zmir-keep-place-mention",
133134
"-Zmir-opt-level=0",
134135
"-Zmir-enable-passes=-CheckAlignment",
135136
];

‎src/tools/miri/tests/fail/dangling_pointers/dangling_pointer_deref_underscore.rs

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// Make sure we find these even with many checks disabled.
2+
//@compile-flags: -Zmiri-disable-alignment-check -Zmiri-disable-stacked-borrows -Zmiri-disable-validation
3+
4+
fn main() {
5+
let p = {
6+
let b = Box::new(42);
7+
&*b as *const i32
8+
};
9+
unsafe {
10+
let _ = *p; //~ ERROR: dereferenced after this allocation got freed
11+
}
12+
panic!("this should never print");
13+
}

‎src/tools/miri/tests/fail/dangling_pointers/dangling_pointer_deref_underscore.stderr

+15
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
error: Undefined Behavior: pointer to ALLOC was dereferenced after this allocation got freed
2+
--> $DIR/dangling_pointer_deref_underscore.rs:LL:CC
3+
|
4+
LL | let _ = *p;
5+
| ^^ pointer to ALLOC was dereferenced after this allocation got freed
6+
|
7+
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
8+
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
9+
= note: BACKTRACE:
10+
= note: inside `main` at $DIR/dangling_pointer_deref_underscore.rs:LL:CC
11+
12+
note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
13+
14+
error: aborting due to previous error
15+

‎tests/ui/borrowck/let_underscore_temporary.rs

+29-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// check-pass
1+
// check-fail
22

33
fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
44
let _ = if let Some(s) = *string { s.len() } else { 0 };
@@ -8,6 +8,7 @@ fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
88
s
99
} else {
1010
&mut 0
11+
//~^ ERROR temporary value dropped while borrowed
1112
};
1213
let _ = if let Some(ref s) = num { s } else { &0 };
1314
let _ = if let Some(mut s) = num {
@@ -21,6 +22,33 @@ fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
2122
s
2223
} else {
2324
&mut 0
25+
//~^ ERROR temporary value dropped while borrowed
26+
};
27+
}
28+
29+
fn let_ascribe(string: &Option<&str>, mut num: Option<i32>) {
30+
let _: _ = if let Some(s) = *string { s.len() } else { 0 };
31+
let _: _ = if let Some(s) = &num { s } else { &0 };
32+
let _: _ = if let Some(s) = &mut num {
33+
*s += 1;
34+
s
35+
} else {
36+
&mut 0
37+
//~^ ERROR temporary value dropped while borrowed
38+
};
39+
let _: _ = if let Some(ref s) = num { s } else { &0 };
40+
let _: _ = if let Some(mut s) = num {
41+
s += 1;
42+
s
43+
} else {
44+
0
45+
};
46+
let _: _ = if let Some(ref mut s) = num {
47+
*s += 1;
48+
s
49+
} else {
50+
&mut 0
51+
//~^ ERROR temporary value dropped while borrowed
2452
};
2553
}
2654

0 commit comments

Comments
 (0)