-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Make destructors on extern "C"
frames to be executed
#129582
base: master
Are you sure you want to change the base?
Conversation
r? @nnethercote rustbot has assigned @nnethercote. Use |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Any ideas for how we could measure the cost of this change in real-world code? |
What is actually supposed to be causing a cost in this change, other than generating slightly more landing pads? |
Mostly size cost (landing pads). I guess it doesn't hurt to do a perf run? |
Are there that many extern C function to cause a serious size cost? Could you have a test that checks for the expected runtime behavior? |
Am I correct in thinking that there will be no cost if built with panic=abort, it will still abort immediately? |
This PR does not affect panic=abort since panic=abort has no unwind paths anyway |
It is concerning that CI just passes with hardly any test changes. Could you add a ui run-pass test with the example from #123231? You can model it after And maybe we should have a mir-opt test as well so that one can see the |
If we want to go ahead with this, what do we do with beta (about to become 1.81 real soon)? I doubt we want to land a change like this so late in the cycle. @Mark-Simulacrum can we still land a PR that reverts |
Can we document the current behaviour (prefix of frames) for 1.81 in the relnotes and then we further specify that it's all-or-nothing in 1.82? |
Thanks for the pointer. I'll add that when I have time. |
In fact you can just edit |
I don't think there's any blockers to landing PRs on beta doing anything at this point, modulo T-compiler signoff. Reverting things to status quo on stable feels pretty safe to me. |
@rustbot +T-lang +I-lang-nominated Over here: @RalfJung proposes a particular semantic that is implemented by this PR:
Having followed the thread, I agree this makes the most sense, so I propose... @rfcbot fcp merge With respect to what to do about Rust 1.81, I probably think it's fine to ship as-is as long as we're clear about the limits of the guarantees that we're making, but I'm also open to a revert if people think that makes more sense. Edit 2024-08-28: In our meeting, we also clarified as part of this FCP that we may later add new unwinding mode with different behavior, but that those may have to take into account code that relies on the behavior of the existing modes. |
I think the only way this would make sense is if it includes the change to the status quo unwinding behavior, does it? |
I don't understand the question. On the current beta (soon to be 1.81), the example in #123231 changes behavior compared to 1.80. Formally speaking, in 1.80 the example was UB, and in 1.81 it is well-defined. Practically speaking, in 1.80 "Noisy Drop" was printed, and in 1.81 it is not. I don't know if anyone still relied on the 1.80 behavior, ideally everyone who used to rely on this should have migrated to the "C-unwind" ABI. This patch makes it so that "Noisy Drop" is printed again. Assuming that we want to land this PR, the question is: do we want to have two changes here, where 1.81 makes the behavior non-UB but includes early-abort in some cases and then 1.82 or 1.83 puts the abort always in the place we want it, or do we want to avoid such back-and-forth? |
This comment was marked as resolved.
This comment was marked as resolved.
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
@rfcbot reviewed My preferred semantics: Given panic=abort, no destructors run. Given panic=unwind, it may abort immediately, but it may also run destructors up until the extern "C" boundary (or other such "no unwind" boundary). Future panic modes may behave differently, that will be adjudicated when they are added. None of this permits the current behavior of aborting before the destructors of the extern "C" function. I believe this is Ralf's proposal and also what we are FCP'ing, so ✅ |
👍 for niko's description in #129582 (comment) |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
I don't know anything about this stuff, but given how many people have chimed in with comments, and the fact that the lang-team has approved it, I plan to rubber-stamp this PR once the FCP is over. If anybody has opinions about the code changes, please weigh in (or even take review assignment) before then. Thanks. |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
☔ The latest upstream changes (presumably #130165) made this pull request unmergeable. Please resolve the merge conflicts. |
This would make the example in #123231 print "Noisy Drop". I didn't mark this as fixing the issue because the behaviour is yet to be spec'ed.
Tracking: