-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Don't leak return value after panic in drop #78373
Conversation
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 7acf3f1b0d7330edcc908662e170dd6182074f8f with merge d610dce8ea41761b41e03160a39fca995dea0df5... |
☀️ Try build successful - checks-actions |
Queued d610dce8ea41761b41e03160a39fca995dea0df5 with parent b9a94c9, future comparison URL. |
Finished benchmarking try commit (d610dce8ea41761b41e03160a39fca995dea0df5): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
Since this adds code, I suspect it's always going to be a regression. I would ignore the incremental benchmarks here, which brings us down to roughly 5% regression on real world codebases. I think given this is a soundness fix (at least arguably) and certainly unintuitive behavior, we should land this (modulo review of the diff itself, which I have not done). |
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.
Unsure if this is still only intended for checking perf, but LGTM; someone else should also take a look though.
r? @pnkfelix |
nominating for discussion at next T-compiler triage meeting. I am inclined to swallow the compiler performance regression, but I figure we will be better off talking about it as a team. Also, I think it will be best to land this early in the release cycle, i.e. soon after the next beta is cut.
|
@bors r+ rollup=never |
📌 Commit 7acf3f1b0d7330edcc908662e170dd6182074f8f has been approved by |
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
27e434f
to
5e71b80
Compare
@bors r=pnkfelix |
0b6ab78
to
4fef391
Compare
@bors r=pnkfelix |
📌 Commit 4fef391 has been approved by |
🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened |
☀️ Test successful - checks-actions |
…ution-via-revert-of-pr-78373, r=matthewjasper Revert 78373 ("dont leak return value after panic in drop") Short term resolution for issue rust-lang#80949. Reopen rust-lang#47949 after this lands. (We plan to fine-tune PR rust-lang#78373 to not run into this problem.)
…ution-via-revert-of-pr-78373, r=matthewjasper Revert 78373 ("dont leak return value after panic in drop") Short term resolution for issue rust-lang#80949. Reopen rust-lang#47949 after this lands. (We plan to fine-tune PR rust-lang#78373 to not run into this problem.)
Fix leaks from panics in destructors Resurrects rust-lang#78373. This avoids the problem with rust-lang#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 rust-lang#47949 r? `@lcnr`
Fix leaks from panics in destructors Resurrects rust-lang#78373. This avoids the problem with rust-lang#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 rust-lang#47949 r? `@lcnr`
Fix leaks from panics in destructors Resurrects rust-lang#78373. This avoids the problem with rust-lang#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 rust-lang#47949 r? `@lcnr`
Fix leaks from panics in destructors Resurrects rust-lang#78373. This avoids the problem with rust-lang#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 rust-lang#47949 r? `@lcnr`
Fix leaks from panics in destructors Resurrects rust-lang#78373. This avoids the problem with rust-lang#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 rust-lang#47949 r? `@lcnr`
Closes #47949