-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Fix unwind drop glue for if-then scopes #102394
Conversation
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
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 |
c1c339b
to
4a2c1a1
Compare
@est31 asked for this to be beta-nominated on Zulip. |
Co-authored-by: SafariMonkey <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
@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? |
r? @oli-obk |
@@ -1021,6 +1023,38 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { | |||
cached_drop | |||
} | |||
|
|||
/// This is similar to [diverge_cleanup] except its target is set to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaict diverge_cleanup
could now call this function and pass self.scopes.topmost()
as the target_scope
argument.
r=me with some code deduplication @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 5131e9d with merge 7d853fa063b4f1beb56ab3cb4e2fc8719efee861... |
⌛ Trying commit 565c35a with merge bf8d3a261764632b6a568ff423d2f29942be0e50... |
☀️ Try build successful - checks-actions |
Queued bf8d3a261764632b6a568ff423d2f29942be0e50 with parent 8c71b67, future comparison URL. |
Finished benchmarking commit (bf8d3a261764632b6a568ff423d2f29942be0e50): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking 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 Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Footnotes |
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (c97d02c): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Footnotes |
[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.
cc @est31
Fix #102317
Fix #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 whereStorageDead
must be kept around.