Auto merge of #104844 - cjgillot:mention-eval-place, r=jackh726,RalfJung

bors · bors · commit 21fab435da99 · 2023-04-22T09:54:21.000Z
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`
diff --git a/compiler/rustc_borrowck/src/def_use.rs b/compiler/rustc_borrowck/src/def_use.rs
@@ -52,12 +52,16 @@ pub fn categorize(context: PlaceContext) -> Option<DefUse> {
         PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow) |
         PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow) |
 
+        // `PlaceMention` and `AscribeUserType` both evaluate the place, which must not
+        // contain dangling references.
+        PlaceContext::NonUse(NonUseContext::PlaceMention) |
+        PlaceContext::NonUse(NonUseContext::AscribeUserTy) |
+
         PlaceContext::MutatingUse(MutatingUseContext::AddressOf) |
         PlaceContext::NonMutatingUse(NonMutatingUseContext::AddressOf) |
         PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect) |
         PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) |
         PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) |
-        PlaceContext::NonUse(NonUseContext::AscribeUserTy) |
         PlaceContext::MutatingUse(MutatingUseContext::Retag) =>
             Some(DefUse::Use),
 
@@ -72,8 +76,6 @@ pub fn categorize(context: PlaceContext) -> Option<DefUse> {
         PlaceContext::MutatingUse(MutatingUseContext::Drop) =>
             Some(DefUse::Drop),
 
-        // This statement exists to help unsafeck. It does not require the place to be live.
-        PlaceContext::NonUse(NonUseContext::PlaceMention) => None,
         // Debug info is neither def nor use.
         PlaceContext::NonUse(NonUseContext::VarDebugInfo) => None,
 
diff --git a/compiler/rustc_borrowck/src/invalidation.rs b/compiler/rustc_borrowck/src/invalidation.rs
@@ -79,7 +79,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
             }
             // Only relevant for mir typeck
             StatementKind::AscribeUserType(..)
-            // Only relevant for unsafeck
+            // Only relevant for liveness and unsafeck
             | StatementKind::PlaceMention(..)
             // Doesn't have any language semantics
             | StatementKind::Coverage(..)
diff --git a/compiler/rustc_borrowck/src/lib.rs b/compiler/rustc_borrowck/src/lib.rs
@@ -665,7 +665,7 @@ impl<'cx, 'tcx> rustc_mir_dataflow::ResultsVisitor<'cx, 'tcx> for MirBorrowckCtx
             }
             // Only relevant for mir typeck
             StatementKind::AscribeUserType(..)
-            // Only relevant for unsafeck
+            // Only relevant for liveness and unsafeck
             | StatementKind::PlaceMention(..)
             // Doesn't have any language semantics
             | StatementKind::Coverage(..)
diff --git a/compiler/rustc_const_eval/src/interpret/step.rs b/compiler/rustc_const_eval/src/interpret/step.rs
@@ -113,8 +113,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
 
             Intrinsic(box intrinsic) => self.emulate_nondiverging_intrinsic(intrinsic)?,
 
-            // Statements we do not track.
-            PlaceMention(..) | AscribeUserType(..) => {}
+            // Evaluate the place expression, without reading from it.
+            PlaceMention(box place) => {
+                let _ = self.eval_place(*place)?;
+            }
+
+            // This exists purely to guide borrowck lifetime inference, and does not have
+            // an operational effect.
+            AscribeUserType(..) => {}
 
             // Currently, Miri discards Coverage statements. Coverage statements are only injected
             // via an optional compile time MIR pass and have no side effects. Since Coverage
diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs
@@ -802,14 +802,6 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
                     }
                 }
             }
-            StatementKind::PlaceMention(..) => {
-                if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {
-                    self.fail(
-                        location,
-                        "`PlaceMention` should have been removed after drop lowering phase",
-                    );
-                }
-            }
             StatementKind::AscribeUserType(..) => {
                 if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {
                     self.fail(
@@ -919,6 +911,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
             StatementKind::StorageDead(_)
             | StatementKind::Coverage(_)
             | StatementKind::ConstEvalCounter
+            | StatementKind::PlaceMention(..)
             | StatementKind::Nop => {}
         }
 
diff --git a/compiler/rustc_interface/src/tests.rs b/compiler/rustc_interface/src/tests.rs
@@ -768,6 +768,7 @@ fn test_unstable_options_tracking_hash() {
     tracked!(merge_functions, Some(MergeFunctions::Disabled));
     tracked!(mir_emit_retag, true);
     tracked!(mir_enable_passes, vec![("DestProp".to_string(), false)]);
+    tracked!(mir_keep_place_mention, true);
     tracked!(mir_opt_level, Some(4));
     tracked!(move_size_limit, Some(4096));
     tracked!(mutable_noalias, false);
diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs
@@ -331,9 +331,8 @@ pub enum StatementKind<'tcx> {
     /// This is especially useful for `let _ = PLACE;` bindings that desugar to a single
     /// `PlaceMention(PLACE)`.
     ///
-    /// When executed at runtime this is a nop.
-    ///
-    /// Disallowed after drop elaboration.
+    /// When executed at runtime, this computes the given place, but then discards
+    /// it without doing a load. It is UB if the place is not pointing to live memory.
     PlaceMention(Box<Place<'tcx>>),
 
     /// Encodes a user's type ascription. These need to be preserved
diff --git a/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs b/compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs
@@ -24,7 +24,6 @@ impl<'tcx> MirPass<'tcx> for CleanupPostBorrowck {
             for statement in basic_block.statements.iter_mut() {
                 match statement.kind {
                     StatementKind::AscribeUserType(..)
-                    | StatementKind::PlaceMention(..)
                     | StatementKind::Assign(box (_, Rvalue::Ref(_, BorrowKind::Shallow, _)))
                     | StatementKind::FakeRead(..) => statement.make_nop(),
                     _ => (),
diff --git a/compiler/rustc_mir_transform/src/dead_store_elimination.rs b/compiler/rustc_mir_transform/src/dead_store_elimination.rs
@@ -54,11 +54,10 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS
                 | StatementKind::Coverage(_)
                 | StatementKind::Intrinsic(_)
                 | StatementKind::ConstEvalCounter
+                | StatementKind::PlaceMention(_)
                 | StatementKind::Nop => (),
 
-                StatementKind::FakeRead(_)
-                | StatementKind::PlaceMention(_)
-                | StatementKind::AscribeUserType(_, _) => {
+                StatementKind::FakeRead(_) | StatementKind::AscribeUserType(_, _) => {
                     bug!("{:?} not found in this MIR phase!", &statement.kind)
                 }
             }
diff --git a/compiler/rustc_mir_transform/src/dest_prop.rs b/compiler/rustc_mir_transform/src/dest_prop.rs
@@ -582,10 +582,9 @@ impl WriteInfo {
             | StatementKind::Nop
             | StatementKind::Coverage(_)
             | StatementKind::StorageLive(_)
-            | StatementKind::StorageDead(_) => (),
-            StatementKind::FakeRead(_)
-            | StatementKind::AscribeUserType(_, _)
-            | StatementKind::PlaceMention(_) => {
+            | StatementKind::StorageDead(_)
+            | StatementKind::PlaceMention(_) => (),
+            StatementKind::FakeRead(_) | StatementKind::AscribeUserType(_, _) => {
                 bug!("{:?} not found in this MIR phase", statement)
             }
         }
diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs
@@ -48,6 +48,7 @@ mod add_retag;
 mod check_const_item_mutation;
 mod check_packed_ref;
 pub mod check_unsafety;
+mod remove_place_mention;
 // This pass is public to allow external drivers to perform MIR cleanup
 pub mod cleanup_post_borrowck;
 mod const_debuginfo;
@@ -460,8 +461,11 @@ fn run_runtime_lowering_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
 
 /// Returns the sequence of passes that do the initial cleanup of runtime MIR.
 fn run_runtime_cleanup_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
-    let passes: &[&dyn MirPass<'tcx>] =
-        &[&lower_intrinsics::LowerIntrinsics, &simplify::SimplifyCfg::ElaborateDrops];
+    let passes: &[&dyn MirPass<'tcx>] = &[
+        &lower_intrinsics::LowerIntrinsics,
+        &remove_place_mention::RemovePlaceMention,
+        &simplify::SimplifyCfg::ElaborateDrops,
+    ];
 
     pm::run_passes(tcx, body, passes, Some(MirPhase::Runtime(RuntimePhase::PostCleanup)));
 
diff --git a/compiler/rustc_mir_transform/src/remove_place_mention.rs b/compiler/rustc_mir_transform/src/remove_place_mention.rs
@@ -0,0 +1,23 @@
+//! This pass removes `PlaceMention` statement, which has no effect at codegen.
+
+use crate::MirPass;
+use rustc_middle::mir::*;
+use rustc_middle::ty::TyCtxt;
+
+pub struct RemovePlaceMention;
+
+impl<'tcx> MirPass<'tcx> for RemovePlaceMention {
+    fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
+        !sess.opts.unstable_opts.mir_keep_place_mention
+    }
+
+    fn run_pass(&self, _: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
+        trace!("Running RemovePlaceMention on {:?}", body.source);
+        for data in body.basic_blocks.as_mut_preserves_cfg() {
+            data.statements.retain(|statement| match statement.kind {
+                StatementKind::PlaceMention(..) | StatementKind::Nop => false,
+                _ => true,
+            })
+        }
+    }
+}
diff --git a/compiler/rustc_session/src/options.rs b/compiler/rustc_session/src/options.rs
@@ -1558,6 +1558,9 @@ options! {
         "use like `-Zmir-enable-passes=+DestProp,-InstCombine`. Forces the specified passes to be \
         enabled, overriding all other checks. Passes that are not specified are enabled or \
         disabled by other flags as usual."),
+    mir_keep_place_mention: bool = (false, parse_bool, [TRACKED],
+        "keep place mention MIR statements, interpreted e.g., by miri; implies -Zmir-opt-level=0 \
+        (default: no)"),
     #[rustc_lint_opt_deny_field_access("use `Session::mir_opt_level` instead of this field")]
     mir_opt_level: Option<usize> = (None, parse_opt_number, [TRACKED],
         "MIR optimization level (0-4; default: 1 in non optimized builds and 2 in optimized builds)"),
diff --git a/src/tools/clippy/tests/ui/option_if_let_else.fixed b/src/tools/clippy/tests/ui/option_if_let_else.fixed
@@ -25,7 +25,7 @@ fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> {
 fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
     let _ = string.map_or(0, |s| s.len());
     let _ = num.as_ref().map_or(&0, |s| s);
-    let _ = num.as_mut().map_or(&mut 0, |s| {
+    let _ = num.as_mut().map_or(&0, |s| {
         *s += 1;
         s
     });
@@ -34,7 +34,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
         s += 1;
         s
     });
-    let _ = num.as_mut().map_or(&mut 0, |s| {
+    let _ = num.as_mut().map_or(&0, |s| {
         *s += 1;
         s
     });
diff --git a/src/tools/clippy/tests/ui/option_if_let_else.rs b/src/tools/clippy/tests/ui/option_if_let_else.rs
@@ -33,7 +33,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
         *s += 1;
         s
     } else {
-        &mut 0
+        &0
     };
     let _ = if let Some(ref s) = num { s } else { &0 };
     let _ = if let Some(mut s) = num {
@@ -46,7 +46,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
         *s += 1;
         s
     } else {
-        &mut 0
+        &0
     };
 }
 
diff --git a/src/tools/clippy/tests/ui/option_if_let_else.stderr b/src/tools/clippy/tests/ui/option_if_let_else.stderr
@@ -30,13 +30,13 @@ LL |       let _ = if let Some(s) = &mut num {
 LL | |         *s += 1;
 LL | |         s
 LL | |     } else {
-LL | |         &mut 0
+LL | |         &0
 LL | |     };
    | |_____^
    |
 help: try
    |
-LL ~     let _ = num.as_mut().map_or(&mut 0, |s| {
+LL ~     let _ = num.as_mut().map_or(&0, |s| {
 LL +         *s += 1;
 LL +         s
 LL ~     });
@@ -76,13 +76,13 @@ LL |       let _ = if let Some(ref mut s) = num {
 LL | |         *s += 1;
 LL | |         s
 LL | |     } else {
-LL | |         &mut 0
+LL | |         &0
 LL | |     };
    | |_____^
    |
 help: try
    |
-LL ~     let _ = num.as_mut().map_or(&mut 0, |s| {
+LL ~     let _ = num.as_mut().map_or(&0, |s| {
 LL +         *s += 1;
 LL +         s
 LL ~     });
diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs
@@ -130,6 +130,7 @@ pub const MIRI_DEFAULT_ARGS: &[&str] = &[
     "-Zalways-encode-mir",
     "-Zextra-const-ub-checks",
     "-Zmir-emit-retag",
+    "-Zmir-keep-place-mention",
     "-Zmir-opt-level=0",
     "-Zmir-enable-passes=-CheckAlignment",
 ];
diff --git a/src/tools/miri/tests/fail/dangling_pointers/dangling_pointer_deref_underscore.rs b/src/tools/miri/tests/fail/dangling_pointers/dangling_pointer_deref_underscore.rs
@@ -0,0 +1,13 @@
+// Make sure we find these even with many checks disabled.
+//@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 {
+        let _ = *p; //~ ERROR: dereferenced after this allocation got freed
+    }
+    panic!("this should never print");
+}
diff --git a/src/tools/miri/tests/fail/dangling_pointers/dangling_pointer_deref_underscore.stderr b/src/tools/miri/tests/fail/dangling_pointers/dangling_pointer_deref_underscore.stderr
@@ -0,0 +1,15 @@
+error: Undefined Behavior: pointer to ALLOC was dereferenced after this allocation got freed
+  --> $DIR/dangling_pointer_deref_underscore.rs:LL:CC
+   |
+LL |         let _ = *p;
+   |                 ^^ pointer to ALLOC was dereferenced after this allocation got freed
+   |
+   = 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_underscore.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to previous error
+
diff --git a/tests/ui/borrowck/let_underscore_temporary.rs b/tests/ui/borrowck/let_underscore_temporary.rs
@@ -1,4 +1,4 @@
-// check-pass
+// check-fail
 
 fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
     let _ = if let Some(s) = *string { s.len() } else { 0 };
@@ -8,6 +8,7 @@ fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
         s
     } else {
         &mut 0
+        //~^ ERROR temporary value dropped while borrowed
     };
     let _ = if let Some(ref s) = num { s } else { &0 };
     let _ = if let Some(mut s) = num {
@@ -21,6 +22,33 @@ fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
         s
     } else {
         &mut 0
+        //~^ ERROR temporary value dropped while borrowed
+    };
+}
+
+fn let_ascribe(string: &Option<&str>, mut num: Option<i32>) {
+    let _: _ = if let Some(s) = *string { s.len() } else { 0 };
+    let _: _ = if let Some(s) = &num { s } else { &0 };
+    let _: _ = if let Some(s) = &mut num {
+        *s += 1;
+        s
+    } else {
+        &mut 0
+        //~^ ERROR temporary value dropped while borrowed
+    };
+    let _: _ = if let Some(ref s) = num { s } else { &0 };
+    let _: _ = if let Some(mut s) = num {
+        s += 1;
+        s
+    } else {
+        0
+    };
+    let _: _ = if let Some(ref mut s) = num {
+        *s += 1;
+        s
+    } else {
+        &mut 0
+        //~^ ERROR temporary value dropped while borrowed
     };
 }
 
diff --git a/tests/ui/borrowck/let_underscore_temporary.stderr b/tests/ui/borrowck/let_underscore_temporary.stderr
diff --git a/tests/ui/span/send-is-not-static-std-sync-2.stderr b/tests/ui/span/send-is-not-static-std-sync-2.stderr

Original file line number	Diff line number	Diff line change
`@@ -79,7 +79,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {`
`79`	`79`	`}`
`80`	`80`	`// Only relevant for mir typeck`
`81`	`81`	`StatementKind::AscribeUserType(..)`
`82`		`- // Only relevant for unsafeck`
	`82`	`+ // Only relevant for liveness and unsafeck`
`83`	`83`	`\| StatementKind::PlaceMention(..)`
`84`	`84`	`// Doesn't have any language semantics`
`85`	`85`	`\| StatementKind::Coverage(..)`
Original file line number	Diff line number	Diff line change
`@@ -665,7 +665,7 @@ impl<'cx, 'tcx> rustc_mir_dataflow::ResultsVisitor<'cx, 'tcx> for MirBorrowckCtx`
`665`	`665`	`}`
`666`	`666`	`// Only relevant for mir typeck`
`667`	`667`	`StatementKind::AscribeUserType(..)`
`668`		`- // Only relevant for unsafeck`
	`668`	`+ // Only relevant for liveness and unsafeck`
`669`	`669`	`\| StatementKind::PlaceMention(..)`
`670`	`670`	`// Doesn't have any language semantics`
`671`	`671`	`\| StatementKind::Coverage(..)`
Original file line number	Diff line number	Diff line change
`@@ -802,14 +802,6 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {`
`802`	`802`	`}`
`803`	`803`	`}`
`804`	`804`	`}`
`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`		`- }`
`813`	`805`	`StatementKind::AscribeUserType(..) => {`
`814`	`806`	`if self.mir_phase >= MirPhase::Runtime(RuntimePhase::Initial) {`
`815`	`807`	`self.fail(`
`@@ -919,6 +911,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {`
`919`	`911`	`StatementKind::StorageDead(_)`
`920`	`912`	`\| StatementKind::Coverage(_)`
`921`	`913`	`\| StatementKind::ConstEvalCounter`
	`914`	`+ \| StatementKind::PlaceMention(..)`
`922`	`915`	`\| StatementKind::Nop => {}`
`923`	`916`	`}`
`924`	`917`
Original file line number	Diff line number	Diff line change
`@@ -54,11 +54,10 @@ pub fn eliminate<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, borrowed: &BitS`
`54`	`54`	`\| StatementKind::Coverage(_)`
`55`	`55`	`\| StatementKind::Intrinsic(_)`
`56`	`56`	`\| StatementKind::ConstEvalCounter`
	`57`	`+ \| StatementKind::PlaceMention(_)`
`57`	`58`	`\| StatementKind::Nop => (),`
`58`	`59`
`59`		`- StatementKind::FakeRead(_)`
`60`		`- \| StatementKind::PlaceMention(_)`
`61`		`- \| StatementKind::AscribeUserType(_, _) => {`
	`60`	`+ StatementKind::FakeRead(_) \| StatementKind::AscribeUserType(_, _) => {`
`62`	`61`	`bug!("{:?} not found in this MIR phase!", &statement.kind)`
`63`	`62`	`}`
`64`	`63`	`}`
Original file line number	Diff line number	Diff line change
`@@ -582,10 +582,9 @@ impl WriteInfo {`
`582`	`582`	`\| StatementKind::Nop`
`583`	`583`	`\| StatementKind::Coverage(_)`
`584`	`584`	`\| 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(_, _) => {`
`589`	`588`	`bug!("{:?} not found in this MIR phase", statement)`
`590`	`589`	`}`
`591`	`590`	`}`