Fix unwind drop glue for if-then scopes by dingxiangfei2009 · Pull Request #102394 · rust-lang/rust

dingxiangfei2009 · 2022-09-28T09:05:31Z

This PR fixes the drop glue for unwinding from a panic originated in a drop while breaking out for the else block in an if-then scope.
MIR validation does not fail for the synchronous versions of the test program, because StorageDead statements are skipped over in the unwinding process. It is only becoming a problem when it is inside a generator where StorageDead must be kept around.

rust-highfive · 2022-09-28T09:05:35Z

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

est31 · 2022-09-28T09:18:57Z

Thanks @dingxiangfei2009 for the fast fix! It's indeed the same issue as @Nilstrieb said.

Beta-nominating as it fixes an ICE in a feature stabilized on beta: #102317

@rustbot label beta-nominated

rustbot · 2022-09-28T09:18:59Z

Error: Label beta-nominated can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

emilyalbini · 2022-09-28T16:43:43Z

@est31 asked for this to be beta-nominated on Zulip.

apiraino · 2022-09-29T14:45:48Z

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

Co-authored-by: SafariMonkey <charlton.rodda@gmail.com>

SafariMonkey

Thanks for accepting my suggestion! I see you requested re-review from me. This was a drive-by suggestion, and I don't feel qualified to give a general review, so I think someone else should do that.

est31 · 2022-09-30T17:48:11Z

Yeah Rust doesn't really use the github builtin review system. Instead it uses bors. Already now bors has assigned a reviewer to this PR.

est31 · 2022-10-04T18:45:02Z

@dingxiangfei2009 maybe it would be a good idea to r? @oli-obk on this as they have approved your previous PRs, and jackh726 has stated that it's not their area of expertise?

dingxiangfei2009 · 2022-10-05T02:36:09Z

r? @oli-obk

oli-obk · 2022-10-05T07:34:57Z

        cached_drop
    }

+    /// This is similar to [diverge_cleanup] except its target is set to


Afaict diverge_cleanup could now call this function and pass self.scopes.topmost() as the target_scope argument.

oli-obk · 2022-10-05T07:35:25Z

r=me with some code deduplication

@bors try @rust-timer queue

rust-timer · 2022-10-05T07:35:38Z

Awaiting bors try build completion.

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

bors · 2022-10-05T07:36:13Z

⌛ Trying commit 5131e9d with merge 7d853fa063b4f1beb56ab3cb4e2fc8719efee861...

bors · 2022-10-05T16:17:49Z

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

rust-timer · 2022-10-05T16:26:01Z

Queued bf8d3a261764632b6a568ff423d2f29942be0e50 with parent 8c71b67, future comparison URL.

rust-timer · 2022-10-05T17:47:28Z

Finished benchmarking commit (bf8d3a261764632b6a568ff423d2f29942be0e50): comparison URL.

Overall result: ❌ regressions - no action needed

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 may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -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)	1.3%	[1.3%, 1.3%]	1
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.3%	[1.3%, 1.3%]	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
Regressions ❌ (secondary)	3.7%	[2.2%, 4.6%]	3
Improvements ✅ (primary)	-4.1%	[-4.1%, -4.1%]	1
Improvements ✅ (secondary)	-2.3%	[-4.3%, -1.2%]	4
All ❌✅ (primary)	-4.1%	[-4.1%, -4.1%]	1

Cycles

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

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

oli-obk · 2022-10-05T18:39:29Z

@bors r+

bors · 2022-10-05T18:39:30Z

📌 Commit 565c35a has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-10-05T20:47:42Z

⌛ Testing commit 565c35a with merge c97d02c...

bors · 2022-10-05T23:41:57Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c97d02c to master...

rust-timer · 2022-10-06T01:58:54Z

Finished benchmarking commit (c97d02c): 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)	7.0%	[7.0%, 7.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.3%, -2.0%]	2
All ❌✅ (primary)	-	-	0

Cycles

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

the arithmetic mean of the percent change ↩
number of relevant changes ↩

rust-highfive assigned jackh726 Sep 28, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 28, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 28, 2022

fix unwind drop glue for if-then scopes

4a2c1a1

dingxiangfei2009 force-pushed the issue-102317 branch from c1c339b to 4a2c1a1 Compare September 28, 2022 10:45

emilyalbini added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 28, 2022

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 29, 2022

SafariMonkey reviewed Sep 29, 2022

View reviewed changes

Comment thread compiler/rustc_mir_build/src/build/scope.rs Outdated

tmandry reviewed Sep 29, 2022

View reviewed changes

Comment thread src/test/ui/let-else/issue-102317.rs Outdated

dingxiangfei2009 and others added 2 commits September 30, 2022 21:03

apply suggestion

fb52dc7

Co-authored-by: SafariMonkey <charlton.rodda@gmail.com>

use build-pass for the test

5131e9d

dingxiangfei2009 requested review from SafariMonkey and tmandry and removed request for tmandry September 30, 2022 13:38

SafariMonkey reviewed Sep 30, 2022

View reviewed changes

rust-highfive assigned oli-obk and unassigned jackh726 Oct 5, 2022

oli-obk reviewed Oct 5, 2022

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 5, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 5, 2022

bors removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 5, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 5, 2022

bors merged commit c97d02c into rust-lang:master Oct 5, 2022

rustbot added this to the 1.66.0 milestone Oct 5, 2022

cuviper mentioned this pull request Oct 6, 2022

[beta] backports #102676

Merged

cuviper reviewed Oct 6, 2022

View reviewed changes

Comment thread compiler/rustc_mir_build/src/build/block.rs

cuviper modified the milestones: 1.66.0, 1.65.0 Oct 6, 2022

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 6, 2022

dingxiangfei2009 deleted the issue-102317 branch October 6, 2022 09:44

est31 mentioned this pull request Oct 15, 2022

Broken MIR with if let chain #99938

Closed

Nadrieril mentioned this pull request Apr 1, 2024

ice: broken mir with let_chains #122871

Closed

est31 mentioned this pull request Nov 10, 2024

Stabilize let chains in the 2024 edition #132833

Merged

9 tasks

Uh oh!

Uh oh!

Conversation

dingxiangfei2009 commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Sep 28, 2022

Uh oh!

est31 commented Sep 28, 2022

Uh oh!

rustbot commented Sep 28, 2022

Uh oh!

emilyalbini commented Sep 28, 2022

Uh oh!

apiraino commented Sep 29, 2022

Uh oh!

Uh oh!

Uh oh!

SafariMonkey left a comment

Choose a reason for hiding this comment

Uh oh!

est31 commented Sep 30, 2022

Uh oh!

est31 commented Oct 4, 2022

Uh oh!

dingxiangfei2009 commented Oct 5, 2022

Uh oh!

oli-obk Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Oct 5, 2022

Uh oh!

rust-timer commented Oct 5, 2022

Uh oh!

bors commented Oct 5, 2022

Uh oh!

bors commented Oct 5, 2022

Uh oh!

rust-timer commented Oct 5, 2022

Uh oh!

rust-timer commented Oct 5, 2022

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

oli-obk commented Oct 5, 2022

Uh oh!

bors commented Oct 5, 2022

Uh oh!

bors commented Oct 5, 2022

Uh oh!

bors commented Oct 5, 2022

Uh oh!

Uh oh!

rust-timer commented Oct 6, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

dingxiangfei2009 commented Sep 28, 2022 •

edited

Loading