Mark places as initialized when mutably borrowed #90788

ecstatic-morse · 2021-11-11T00:37:48Z

Fixes the example in #90752, but does not handle some corner cases involving raw pointers and unsafe. See this comment for more information, or the second test.

Although I talked about both MaybeUninitializedPlaces and MaybeInitializedPlaces in #90752, this PR only changes the latter. That's because "maybe uninitialized" is the conservative choice, and marking them as definitely initialized (!maybe_uninitialized) when a mutable borrow is created could lead to problems if addr_of_mut to an uninitialized local is allowed. Additionally, places cannot become uninitialized via a mutable reference, so if a place is definitely initialized, taking a mutable reference to it should not change that.

I think it's correct to ignore interior mutability as nbdd0121 suggests below. Their analysis doesn't work inside of core::cell, which does have access to UnsafeCell's field, but that won't be an issue unless we explicitly instantiate one with an enum within that module.

r? @wesleywiser

nbdd0121 · 2021-11-12T00:21:23Z

I don't think we need to worry about interior mutability, since it is always behind UnsafeCell. As there is no way to access the value field directly, move paths into the value wouldn't be used.

nbdd0121

LGTM overall

nbdd0121 · 2021-11-13T21:00:22Z

src/test/ui/drop/issue-90752-raw-ptr-shenanigans.rs

@@ -0,0 +1,41 @@
+// run-pass


Maybe instead of checking that we are getting the wrong result we should just have ignore-test FIXME(#90788)

If someone (including me) were to change the behavior of this test, they should be made aware.

nbdd0121 · 2021-11-13T21:06:51Z

src/test/ui/drop/issue-90752-raw-ptr-shenanigans.rs

+    // Ideally, we want this...
+    //assert_eq!(*drops.borrow(), &[0, 1]);
+
+    // But the delayed access through the raw pointer confuses drop elaboration,


This is unfortunate. Do you have an idea about how this could be fixed in the future?

We typically handle this is by assuming that any variable which has been mutably borrowed at some point in the CFG could change at any subsequent point. The correctness of that approach doesn't depend on stacked borrows.

We can't implement it directly on top of MaybeInitializedPlaces, however, since it would break code that moves out of a variable after taking a mutable reference to it. We would need two state vectors like #90214 has, preferably only in drop elaboration where the increased precision from tracking variants is worthwhile. I think tracking variants does nothing for the borrow checker since it still has FalseEdges.

ecstatic-morse · 2021-11-18T17:11:25Z

@bors try @rust-timer queue

rust-timer · 2021-11-18T17:11:27Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-11-18T17:11:39Z

⌛ Trying commit 22d937d with merge 4853c802647b3e589b9ce14db5494a242b6f44e4...

bors · 2021-11-18T18:43:40Z

☀️ Try build successful - checks-actions
Build commit: 4853c802647b3e589b9ce14db5494a242b6f44e4 (4853c802647b3e589b9ce14db5494a242b6f44e4)

rust-timer · 2021-11-18T18:43:42Z

Queued 4853c802647b3e589b9ce14db5494a242b6f44e4 with parent 6414e0b, future comparison URL.

rust-timer · 2021-11-18T20:16:23Z

Finished benchmarking commit (4853c802647b3e589b9ce14db5494a242b6f44e4): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

Very large regression in instruction counts (up to 5.4% on incr-unchanged builds of inflate)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

wesleywiser

I'm inclined to merge but I would really appreciate a review from someone more familiar with the drop elaboration code. @ecstatic-morse do you know anyone else who we should ask to review?

wesleywiser · 2021-11-18T21:23:22Z

compiler/rustc_mir_dataflow/src/impls/mod.rs

+            return;
+        }
+
+        for_each_mut_borrow(terminator, location, |place| {


Nit: Can you go ahead and copy the comment from line 322 here as well? Seems like we should try to keep these as similar as possible.

ecstatic-morse · 2021-11-18T22:54:40Z

do you know anyone else who we should ask to review?

The original author (arielb1) is no longer active. You could "ask one of the elders", especially eddyb, who came up with the original idea for #68528 where I introduced the bug. I don't know how much free time they have for Rust reviews, though, and I try to avoid filling up their GitHub notifications.

wesleywiser · 2021-11-23T00:55:02Z

@ecstatic-morse Thanks for resolving this issue so quickly! I think we should probably open a new issue to track the raw pointer bug, but for now let me

@bors r+

Sorry for the delay!

bors · 2021-11-23T00:55:04Z

📌 Commit 22d937d has been approved by wesleywiser

jyn514 · 2021-11-23T14:25:55Z

@bors p=1 - this fixes unsoundness and should land before the beta branch on Friday

bors · 2021-11-23T17:44:37Z

⌛ Testing commit 22d937d with merge 7b3cd07...

bors · 2021-11-23T20:44:27Z

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 7b3cd07 to master...

rust-timer · 2021-11-23T22:49:35Z

Finished benchmarking commit (7b3cd07): comparison url.

Summary: This change led to moderate relevant regressions 😿 in compiler performance.

Moderate regression in instruction counts (up to 0.6% on full builds of match-stress-enum)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

nbdd0121 · 2021-11-24T03:20:25Z

Needs backport

Mark-Simulacrum · 2021-11-30T16:47:08Z

Discussed in T-compiler meeting last week - https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202021-11-24.20.2354818/near/262604287 and decided to forgo a backport to 1.57 (shipping this week). This will be in 1.58, though.

rust-highfive assigned wesleywiser Nov 11, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 11, 2021

ecstatic-morse marked this pull request as draft November 11, 2021 00:38

ecstatic-morse mentioned this pull request Nov 11, 2021

Miscompilation where binding only some fields leaks the others #90752

Closed

This comment has been minimized.

Sign in to view

ecstatic-morse changed the title ~~Mark places as maybe {un,}initialized when mutably borrowed~~ Mark places as initialized when mutably borrowed Nov 11, 2021

ecstatic-morse force-pushed the issue-90752 branch from 07ebe18 to ce67b89 Compare November 11, 2021 01:18

ecstatic-morse force-pushed the issue-90752 branch from ce67b89 to be7944f Compare November 13, 2021 19:03

This comment has been minimized.

Sign in to view

ecstatic-morse force-pushed the issue-90752 branch 2 times, most recently from bd9d8b7 to b8f6ab5 Compare November 13, 2021 19:12

This comment has been minimized.

Sign in to view

ecstatic-morse added 3 commits November 13, 2021 11:16

Mark mutably borrowed places as maybe initialized

bea1bde

Add regression test for rust-lang#90752

d846fe0

Add raw pointer variant of rust-lang#90752 with incorrect behavior

ece0e6a

ecstatic-morse force-pushed the issue-90752 branch from b8f6ab5 to aedb844 Compare November 13, 2021 19:17

Sanity check for move from an uninit variable whose address is taken

22d937d

ecstatic-morse marked this pull request as ready for review November 13, 2021 19:31

This comment has been minimized.

Sign in to view

nbdd0121 mentioned this pull request Nov 13, 2021

Disable precise_enum_drop_elaboration by default #90756

Closed

nbdd0121 approved these changes Nov 13, 2021

View reviewed changes

ecstatic-morse force-pushed the issue-90752 branch from aedb844 to 22d937d Compare November 13, 2021 22:00

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 18, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 18, 2021

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 18, 2021

wesleywiser approved these changes Nov 18, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 23, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 23, 2021

bors merged commit 7b3cd07 into rust-lang:master Nov 23, 2021

rustbot added this to the 1.58.0 milestone Nov 23, 2021

bors mentioned this pull request Nov 23, 2021

Unwinding support for inline assembly #88439

Merged

Mark-Simulacrum added beta-nominated Nominated for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Nov 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark places as initialized when mutably borrowed #90788

Mark places as initialized when mutably borrowed #90788

ecstatic-morse commented Nov 11, 2021 •

edited

Loading

This comment has been minimized.

nbdd0121 commented Nov 12, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nbdd0121 left a comment

nbdd0121 Nov 13, 2021

ecstatic-morse Nov 13, 2021

nbdd0121 Nov 13, 2021

ecstatic-morse Nov 13, 2021 •

edited

Loading

ecstatic-morse commented Nov 18, 2021

rust-timer commented Nov 18, 2021

bors commented Nov 18, 2021

bors commented Nov 18, 2021

rust-timer commented Nov 18, 2021

rust-timer commented Nov 18, 2021

wesleywiser left a comment

wesleywiser Nov 18, 2021

ecstatic-morse commented Nov 18, 2021 •

edited

Loading

wesleywiser commented Nov 23, 2021

bors commented Nov 23, 2021

jyn514 commented Nov 23, 2021

bors commented Nov 23, 2021

bors commented Nov 23, 2021

rust-timer commented Nov 23, 2021

nbdd0121 commented Nov 24, 2021

Mark-Simulacrum commented Nov 30, 2021

Mark places as initialized when mutably borrowed #90788

Mark places as initialized when mutably borrowed #90788

Conversation

ecstatic-morse commented Nov 11, 2021 • edited Loading

This comment has been minimized.

nbdd0121 commented Nov 12, 2021 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nbdd0121 left a comment

Choose a reason for hiding this comment

nbdd0121 Nov 13, 2021

Choose a reason for hiding this comment

ecstatic-morse Nov 13, 2021

Choose a reason for hiding this comment

nbdd0121 Nov 13, 2021

Choose a reason for hiding this comment

ecstatic-morse Nov 13, 2021 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse commented Nov 18, 2021

rust-timer commented Nov 18, 2021

bors commented Nov 18, 2021

bors commented Nov 18, 2021

rust-timer commented Nov 18, 2021

rust-timer commented Nov 18, 2021

wesleywiser left a comment

Choose a reason for hiding this comment

wesleywiser Nov 18, 2021

Choose a reason for hiding this comment

ecstatic-morse commented Nov 18, 2021 • edited Loading

wesleywiser commented Nov 23, 2021

bors commented Nov 23, 2021

jyn514 commented Nov 23, 2021

bors commented Nov 23, 2021

bors commented Nov 23, 2021

rust-timer commented Nov 23, 2021

nbdd0121 commented Nov 24, 2021

Mark-Simulacrum commented Nov 30, 2021

ecstatic-morse commented Nov 11, 2021 •

edited

Loading

nbdd0121 commented Nov 12, 2021 •

edited

Loading

ecstatic-morse Nov 13, 2021 •

edited

Loading

ecstatic-morse commented Nov 18, 2021 •

edited

Loading