Fix leaks from panics in destructors

[matthewjasper]

Resurrects #78373.

This avoids the problem with #80949 by not unscheduling drops of function arguments until after the call (so they still get a drop terminator on the function unwind path).

Closes #47949

r? @lcnr

[rustbot]

Some changes occurred in match lowering

cc @Nadrieril

[lcnr]

@bors try @rust-timer queue

r? @pnkfelix (as you've reviewed #78373 3 years ago :3) am looking through these changes myself but would like you to also take a look

[bors]

⌛ Trying commit 28ac9be with merge 1c34390...

[lcnr]

Went through it and mostly understand this PR, am very much not comfortable enough to approve it myself 😅

Thanks @matthewjasper for picking this up again ❤️ really happy about this

[bors]

☀️ Try build successful - checks-actions
Build commit: 1c34390 (1c343902c7b4004d4dcdeb3cc93fcdd886e21595)

[rust-timer]

Finished benchmarking commit (1c34390): comparison URL.

Overall result: ❌ regressions - 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.

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-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.5%	[0.3%, 12.1%]	245
Regressions ❌ (secondary)	38.2%	[0.3%, 723.1%]	156
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.5%	[0.3%, 12.1%]	245

Max RSS (memory usage)

Results (primary -1.8%, secondary -4.0%)

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)	2.8%	[0.8%, 5.9%]	7
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.7%	[-8.7%, -0.9%]	36
Improvements ✅ (secondary)	-4.0%	[-6.0%, -2.2%]	28
All ❌✅ (primary)	-1.8%	[-8.7%, 5.9%]	43

Cycles

Results (primary 2.4%, secondary 73.0%)

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)	2.4%	[0.9%, 6.3%]	48
Regressions ❌ (secondary)	73.0%	[1.2%, 281.7%]	33
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[0.9%, 6.3%]	48

Binary size

Results (primary 1.1%, secondary 1.3%)

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)	1.1%	[0.0%, 4.9%]	136
Regressions ❌ (secondary)	1.3%	[0.1%, 7.7%]	91
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[-0.1%, 4.9%]	138

Bootstrap: 673.596s -> 675.825s (0.33%)
Artifact size: 318.88 MiB -> 311.92 MiB (-2.19%)

[matthewjasper]

@bors try @rust-timer queue

[lcnr]

(will take a day to check this out locally and fully understand these changes in 1-2 weeks if nobody else tries to review it by then)

It looks like the crater run still detected some ICE?

[matthewjasper]

Yes, the push 3 days ago was a fix and test for that ICE

[saethlin]

I really thought that given some time over the weekend I'd be able to review this, but no, this part of the compiler is too unfamiliar to me.

r? lcnr

[lcnr]

@bors try

[bors]

⌛ Trying commit 3e0db6a with merge b905bf4...

[bors]

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

[lcnr]

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-125923-1/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-125923-2 created and queued.
🤖 Automatically detected try build b905bf4
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-125923-2 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-125923-2 is completed!
📊 0 regressed and 0 fixed (19 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[lcnr]

so the affected code is

fn if_no_else(c: bool) -> impl Sized {
    if c {}
}

where the opaque type gets unified with () and () does not need drop while impl Sized does? 🤔 we probably never emit drop for the return value of an if without an else block or sth?

[lcnr]

this is necessary to avoid ICEing when trying to unschedule the drop of a previous match arm here https://github.com/matthewjasper/rust/blob/3e0db6a36f01a5a8978ced19e0b83ebca5cd103c/compiler/rustc_mir_build/src/build/matches/mod.rs#L480-L484

however, this is not enough as we don't schedule a drop if the destination_ty is an opaque

fn test_me() -> impl Sized {
    match 1 {
        1 => 'ret: {
            break 'ret test_me();
        }
        _ => {},
    };
}

fn main() {}

It works if the destination_ty is () as in this case unschedule_drop is a noop as needs_drop returns false

[lcnr]

I am very confused by expr_into_dest.

Ignoring that opaque types require drop even if their hidden type don't means that the destination of stuff like ExprKind::Borrow would now add a drop, and more importantly, tries to remove a drop in unschedule_drop, expr_into_dest has to schedule the drop for dest to be the last drop in the given scope. If it does not, we just ICE when trying to unschedule the drop for the destination in matches/if else.

I've added an assert that stuff could get unscheduled and it always passes, see a57f57b

[lcnr]

stopped my review for now in case you have thoughts here as my next steps would probably be to attempt to significantly restructure the code to see if it works 😅

I feel like the current setup is very prone to break in the future (and also very sus wrt to opaque types, which is more an opaque type issue than an issue with your impl)

While reviewing I've made some changes to document/rename things which were confusing to me, the important commit is here 4ba5010

[lcnr]

@rustbot author