Evaluate place expression in `PlaceMention` #104844

cjgillot · 2022-11-24T19:48:02Z

#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:

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.

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

rustbot · 2022-11-24T19:48:35Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2022-11-26T09:18:10Z

compiler/rustc_const_eval/src/interpret/step.rs

+            // Evaluate the place expression, without reading from it.
+            PlaceMention(box place) | AscribeUserType(box (place, _), _) => {
+                let _ = self.eval_place(*place)?;
+            }


Oh, AscribeUserType also does this? That is slightly surprising.

Is it possible to add tests to Miri that hit both of these cases?

There should always be a PlaceMention before an AscribeUserType, so theoretically AscribeUserType can never fail. So I don't really know if we should check both, or just PlaceMention.

RalfJung · 2022-11-26T09:19:17Z

compiler/rustc_middle/src/mir/syntax.rs

+    /// binding.  This is especially useful for `let _ = PLACE;` bindings that desugar to a single
+    /// `PlaceMention(PLACE)`.
+    ///
+    /// When executed at runtime this is a nop.


This needs to be updated.

Suggested change

/// When executed at runtime this is a nop.

/// 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.

AscribeUserType would probably need something similar, judging from the case you added in Miri?

compiler/rustc_middle/src/mir/syntax.rs

RalfJung · 2022-11-26T09:23:55Z

compiler/rustc_borrowck/src/def_use.rs

+        // `PlaceMention` and `AscribeUserType` both evaluate the place, which must not
+        // contain dangling references.
+        PlaceContext::NonUse(NonUseContext::PlaceMention) |
+        PlaceContext::NonUse(NonUseContext::AscribeUserTy) |


I don't know borrowck, so someone else will have to review this part.
But doesn't this do a check that the place is actually initialized? Just to avoid UB it would be sufficient to check that the place is live, but maybe borrowck does not have such a check.

Checking that the place is initialized will break a lot of code, because we allow binding moved-from places to _. For instance in test src/test/ui/binding/issue-53114-borrow-checks.rs.

Yes an init check is not needed. I just thought DefUse::Use does an init check (since usually when we use a place, we load from it, and it must be init). But it seems I am wrong about that?

compiler/rustc_const_eval/src/interpret/step.rs

RalfJung · 2022-12-21T21:47:18Z

Marking as blocked on #102256

bors · 2023-04-19T05:34:38Z

☔ The latest upstream changes (presumably #110477) made this pull request unmergeable. Please resolve the merge conflicts.

rfcbot · 2023-04-22T00:14:45Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

cjgillot · 2023-04-22T08:32:40Z

@bors r=jackh726,RalfJung

bors · 2023-04-22T08:32:42Z

📌 Commit 2870d26 has been approved by jackh726,RalfJung

It is now in the queue for this repository.

bors · 2023-04-22T09:54:24Z

⌛ Testing commit 2870d26 with merge 21fab43...

bors · 2023-04-22T12:04:05Z

☀️ Test successful - checks-actions
Approved by: jackh726,RalfJung
Pushing 21fab43 to master...

rust-timer · 2023-04-22T13:47:03Z

Finished benchmarking commit (21fab43): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.4%	[3.4%, 3.4%]	1
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.1%	[-2.1%, -2.1%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	-	-	0

…726,RalfJung Evaluate place expression in `PlaceMention` rust-lang#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`

…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

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/rust#102256 ~rust-lang/rust#104844 Fixes rust-lang/rust#99180 rust-lang/rust#53114

rustbot assigned RalfJung Nov 24, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 24, 2022

cjgillot marked this pull request as ready for review November 24, 2022 19:48

RalfJung reviewed Nov 26, 2022

View reviewed changes

compiler/rustc_middle/src/mir/syntax.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 26, 2022

View reviewed changes

cjgillot force-pushed the mention-eval-place branch from cb4b8f8 to 4316315 Compare November 26, 2022 14:14

RalfJung reviewed Nov 26, 2022

View reviewed changes

compiler/rustc_const_eval/src/interpret/step.rs Outdated Show resolved Hide resolved