Fix unwind drop glue for if-then scopes #102394

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.

pietroalbini · 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

compiler/rustc_mir_build/src/build/scope.rs

src/test/ui/let-else/issue-102317.rs

Co-authored-by: SafariMonkey <[email protected]>

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

compiler/rustc_mir_build/src/build/scope.rs

@@ -1021,6 1023,38 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 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...

est31 · 2022-10-05T14:38:42Z

@bors try

bors · 2022-10-05T14:49:28Z

⌛ Trying commit 565c35a with merge bf8d3a261764632b6a568ff423d2f29942be0e50...

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

compiler/rustc_mir_build/src/build/block.rs

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 ↩

[beta] backports * Avoid duplicating StorageLive in let-else rust-lang#101894 * Re-add HRTB implied static bug note rust-lang#101924 * Revert "Copy stage0 binaries into stage0-sysroot" rust-lang#101942 * implied_bounds: deal with inference vars rust-lang#102016 * fix ConstProp handling of written_only_inside_own_block_locals rust-lang#102045 * Fix wrongly refactored Lift impl rust-lang#102088 * Fix a typo “pararmeter” in error message rust-lang#102119 * Deny associated type bindings within associated type bindings rust-lang#102338 * Continue migration of CSS themes rust-lang#101934 * Fix search result colors rust-lang#102369 * Fix unwind drop glue for if-then scopes rust-lang#102394 * Revert "Use getentropy when possible on all Apple platforms" rust-lang#102693 * Fix associated type bindings with anon const in GAT position rust-lang#102336 * Revert perf-regression 101620 rust-lang#102064 * `EscapeAscii` is not an `ExactSizeIterator` rust-lang#99880

…-obk Fix unwind drop glue for if-then scopes cc `@est31` Fix rust-lang#102317 Fix rust-lang#99852 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 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

pietroalbini 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

compiler/rustc_mir_build/src/build/scope.rs Outdated Show resolved Hide resolved

tmandry reviewed Sep 29, 2022

View reviewed changes

src/test/ui/let-else/issue-102317.rs Outdated Show resolved Hide resolved

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

apply suggestion

fb52dc7

Co-authored-by: SafariMonkey <[email protected]>

use build-pass for the test

5131e9d

dingxiangfei2009 requested review from tmandry and SafariMonkey 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

oli-obk approved these changes Oct 5, 2022

View reviewed changes

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

compiler/rustc_mir_build/src/build/block.rs Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unwind drop glue for if-then scopes #102394

Fix unwind drop glue for if-then scopes #102394

dingxiangfei2009 commented Sep 28, 2022 •

edited

Loading

rust-highfive commented Sep 28, 2022

est31 commented Sep 28, 2022

rustbot commented Sep 28, 2022

pietroalbini commented Sep 28, 2022

apiraino commented Sep 29, 2022

SafariMonkey left a comment

est31 commented Sep 30, 2022

est31 commented Oct 4, 2022

dingxiangfei2009 commented Oct 5, 2022

oli-obk Oct 5, 2022

oli-obk commented Oct 5, 2022

rust-timer commented Oct 5, 2022

bors commented Oct 5, 2022

est31 commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 5, 2022

rust-timer commented Oct 5, 2022

oli-obk commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 6, 2022

Fix unwind drop glue for if-then scopes #102394

Fix unwind drop glue for if-then scopes #102394

Conversation

dingxiangfei2009 commented Sep 28, 2022 • edited Loading

rust-highfive commented Sep 28, 2022

est31 commented Sep 28, 2022

rustbot commented Sep 28, 2022

pietroalbini commented Sep 28, 2022

apiraino commented Sep 29, 2022

SafariMonkey left a comment

Choose a reason for hiding this comment

est31 commented Sep 30, 2022

est31 commented Oct 4, 2022

dingxiangfei2009 commented Oct 5, 2022

oli-obk Oct 5, 2022

Choose a reason for hiding this comment

oli-obk commented Oct 5, 2022

rust-timer commented Oct 5, 2022

bors commented Oct 5, 2022

est31 commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 5, 2022

rust-timer commented Oct 5, 2022

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

oli-obk commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 6, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

dingxiangfei2009 commented Sep 28, 2022 •

edited

Loading