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

generator layout: ignore fake borrows #117712

Merged
merged 4 commits into from Nov 9, 2023
Merged

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Nov 8, 2023

fixes #117059

We emit fake shallow borrows in case the scrutinee place uses a Deref and there is a match guard. This is necessary to prevent the match guard from mutating the scrutinee:

// Insert a borrows of prefixes of places that are bound and are
// behind a dereference projection.
//
// These borrows are taken to avoid situations like the following:
//
// match x[10] {
// _ if { x = &[0]; false } => (),
// y => (), // Out of bounds array access!
// }
//
// match *x {
// // y is bound by reference in the guard and then by copy in the
// // arm, so y is 2 in the arm!
// y if { y == 1 && (x = &2) == () } => y,
// _ => 3,
// }

These fake borrows end up impacting the generator witness computation in mir_generator_witnesses, which causes the issue in #117059. This PR now completely ignores fake borrows during this computation. This is sound as thse are always removed after analysis and the actual computation of the generator layout happens afterwards.

Only the second commit impacts behavior, and could be backported by itself.

r? types

@lcnr lcnr added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 8, 2023
@rustbot 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Nov 8, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 8, 2023

This PR changes MIR

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

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

@lcnr lcnr added the I-types-nominated The issue / PR has been nominated for discussion during a types team meeting. label Nov 8, 2023
@BoxyUwU
Copy link
Member

BoxyUwU commented Nov 8, 2023

r? types

@rustbot rustbot added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Nov 8, 2023
@rustbot rustbot assigned jackh726 and unassigned BoxyUwU Nov 8, 2023
@lcnr lcnr removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 8, 2023
@lcnr
Copy link
Contributor Author

lcnr commented Nov 8, 2023

Going to use a t-types fcp again, merging once the FCP period starts without waiting for 10 days.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 8, 2023

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 8, 2023
@rust-log-analyzer

This comment has been minimized.

@jackh726
Copy link
Member

jackh726 commented Nov 8, 2023

r=me when FCP completes

@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor Author

lcnr commented Nov 9, 2023

(checked the box for @oli-obk as they're currently not available)

@aliemjay
Copy link
Member

aliemjay commented Nov 9, 2023

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Nov 9, 2023
@rfcbot
Copy link

rfcbot commented Nov 9, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 9, 2023
@lcnr
Copy link
Contributor Author

lcnr commented Nov 9, 2023

@bors r=jackh726 p=1

@bors
Copy link
Contributor

bors commented Nov 9, 2023

⌛ Testing commit 92267c9 with merge 14575e5...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2023
generator layout: ignore fake borrows

fixes rust-lang#117059

We emit fake shallow borrows in case the scrutinee place uses a `Deref` and there is a match guard. This is necessary to prevent the match guard from mutating the scrutinee: https://github.com/rust-lang/rust/blob/fab1054e1742790c22ccc92a625736d658363677/compiler/rustc_mir_build/src/build/matches/mod.rs#L1250-L1265

These fake borrows end up impacting the generator witness computation in `mir_generator_witnesses`, which causes the issue in rust-lang#117059. This PR now completely ignores fake borrows during this computation. This is sound as thse are always removed after analysis and the actual computation of the generator layout happens afterwards.

Only the second commit impacts behavior, and could be backported by itself.

r? types
@apiraino
Copy link
Contributor

apiraino commented Nov 9, 2023

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 9, 2023
@wesleywiser
Copy link
Member

It is possible to only backport the second commit (after fixing the comment here which belongs in the next commit). However, T-compiler is fine with taking the whole PR as the rest of the rest of the changes are mechanical in nature.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-apple-1 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
SCCACHE_BUCKET=rust-lang-ci-sccache2
SCRIPT=./x.py --stage 2 test --skip tests/ui --skip tests/rustdoc --skip tests/run-make-fulldeps
SHELL=/bin/bash
SHLVL=3
SSH_AUTH_SOCK=/private/tmp/com.apple.launchd.A9bQDgyjTu/Listeners
STATS_EXT=true
STATS_EXTP=https://provjobdsettingscdn.blob.core.windows.net/settings/provjobdsettings-0.5.154/provjobd.data
STATS_RDCL=true
STATS_TIS=mining

@bors
Copy link
Contributor

bors commented Nov 9, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 9, 2023
@lcnr
Copy link
Contributor Author

lcnr commented Nov 9, 2023

@bors retry

seems spurious

@bors 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 9, 2023
@bors
Copy link
Contributor

bors commented Nov 9, 2023

⌛ Testing commit 92267c9 with merge b7583d3...

@bors
Copy link
Contributor

bors commented Nov 9, 2023

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing b7583d3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 9, 2023
@bors bors merged commit b7583d3 into rust-lang:master Nov 9, 2023
12 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 9, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b7583d3): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

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.6% [0.4%, 0.9%] 7
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.6% [-0.7%, -0.5%] 4
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [-0.7%, 0.9%] 11

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.4% [-0.5%, -0.4%] 2
Improvements ✅
(secondary)
-13.6% [-13.8%, -13.4%] 2
All ❌✅ (primary) -0.4% [-0.5%, -0.4%] 2

Binary size

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

Bootstrap: 663.537s -> 662.476s (-0.16%)
Artifact size: 308.81 MiB -> 308.77 MiB (-0.01%)

@cuviper cuviper mentioned this pull request Nov 9, 2023
@cuviper cuviper modified the milestones: 1.75.0, 1.74.0 Nov 9, 2023
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 9, 2023
@lcnr lcnr deleted the expand-coroutine branch November 9, 2023 22:36
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2023
[beta] backports

- dropck_outlives check whether generator witness needs_drop rust-lang#117134
- Make sure that predicates with unmentioned bound vars are still considered global in the old solver rust-lang#117589
- Check binders with bound vars for global bounds that don't hold rust-lang#117637
- generator layout: ignore fake borrows rust-lang#117712

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2023
[beta] backports

- dropck_outlives check whether generator witness needs_drop rust-lang#117134
- Make sure that predicates with unmentioned bound vars are still considered global in the old solver rust-lang#117589
- Check binders with bound vars for global bounds that don't hold rust-lang#117637
- generator layout: ignore fake borrows rust-lang#117712

r? ghost
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 19, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. I-types-nominated The issue / PR has been nominated for discussion during a types team meeting. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-types Relevant to the types team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

regression: change in async capture rules?