Make destructors on `extern "C"` frames to be executed

[nbdd0121]

trafficstars

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.

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[tmandry]

Any ideas for how we could measure the cost of this change in real-world code?

[arielb1]

What is actually supposed to be causing a cost in this change, other than generating slightly more landing pads?

[nbdd0121]

Mostly size cost (landing pads). I guess it doesn't hurt to do a perf run?

[arielb1]

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?

[tmandry]

Am I correct in thinking that there will be no cost if built with panic=abort, it will still abort immediately?

[arielb1]

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

[RalfJung]

It is concerning that CI just passes with hardly any test changes. Could you add a ui run-pass test with the example from https://github.com/rust-lang/rust/issues/123231? You can model it after tests/ui/panics/panic-in-cleanup.rs.

And maybe we should have a mir-opt test as well so that one can see the UnwindTerminate terminators being generated.

[RalfJung]

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 feature(c_unwind) stabilization (https://github.com/rust-lang/rust/pull/116088) on the beta branch or is that too disruptive?

[nbdd0121]

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?

[nbdd0121]

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 tests/ui/panics/panic-in-cleanup.rs.

And maybe we should have a mir-opt test as well so that one can see the UnwindTerminate terminators being generated.

Thanks for the pointer. I'll add that when I have time.

[RalfJung]

In fact you can just edit tests/ui/panics/panic-in-ffi.rs to add a type with destructor in panic_in_ffi.

[Mark-Simulacrum]

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.

[traviscross]

@rustbot +T-lang +I-lang-nominated

Over here:

• https://github.com/rust-lang/rust/issues/123231#issuecomment-2312659338

@RalfJung proposes a particular semantic that is implemented by this PR:

I think we should guarantee that we run all destructors during unwind, and leave room for "unwind might fail to initiate and abort immediately instead" to account for 2-phase unwinding. That's reasonably easy to understand. This is currently not the case but #129582 implements that, IIUC.

I'm not aware of any code that would rely on the abort happening "early", i.e. skipping some destructors. On current stable, the destructor in the OP example actually does run, so the proposed guarantee (implemented by #129582) is also closer to the status quo than what happens in current beta.

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.

[tmandry]

@Mark-Simulacrum can we still land a PR that reverts feature(c_unwind) stabilization (https://github.com/rust-lang/rust/pull/116088) on the beta branch or is that too disruptive?

I think the only way this would make sense is if it includes the change to the status quo unwinding behavior, does it?

[RalfJung]

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 https://github.com/rust-lang/rust/issues/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?

[traviscross]

@rfcbot fcp merge

(The bot didn't kick it off the last time. Let's try again.)

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• [ ] @joshtriplett
• [x] @nikomatsakis
• [x] @pnkfelix
• [x] @scottmcm
• [x] @tmandry
• [x] @traviscross

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. See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@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 ✅

[scottmcm]

👍 for niko's description in https://github.com/rust-lang/rust/pull/129582#issuecomment-2315724462 @rfcbot reviewed

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[nnethercote]

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.

[rfcbot]

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.

[bors]

:umbrella: The latest upstream changes (presumably #130165) made this pull request unmergeable. Please resolve the merge conflicts.

[nnethercote]

The FCP ended ~3 weeks ago, so I think this is good to go.

@bors r+

[bors]

:pushpin: Commit 8f63b6a7454c05f512515946a0e9ca7e3b904144 has been approved by nnethercote

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 8f63b6a7454c05f512515946a0e9ca7e3b904144 with merge 52c25217b8e8d43b12887b72240e8f2fd17c3d55...

[rust-log-analyzer]

The job x86_64-msvc failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

---- [run-make] tests\run-make\longjmp-across-rust stdout ----

error: rmake recipe failed to complete
status: exit code: 1
command: "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\test\\run-make\\longjmp-across-rust\\rmake.exe"
--- stderr -------------------------------
command failed at line 17
command failed at line 17
Command { cmd: "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\test\\run-make\\longjmp-across-rust\\rmake_out\\main", stdin_buf: None, stdin: None, stdout: None, stderr: None, drop_bomb: DropBomb { command: "C:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\test\\run-make\\longjmp-across-rust\\rmake_out\\main", defused: true, armed_location: Location { file: "C:\\a\\rust\\rust\\tests\\run-make\\longjmp-across-rust\\rmake.rs", line: 17, col: 5 } }, already_executed: true }
output status: `exit code: 0xc0000409`
=== STDOUT ===


=== STDERR ===
thread 'main' panicked at core\src\panicking.rs:221:5:
---
test result: FAILED. 278 passed; 1 failed; 85 ignored; 0 measured; 0 filtered out; finished in 94.08s

Some tests failed in compiletest suite=run-make mode=run-make host=x86_64-pc-windows-msvc target=x86_64-pc-windows-msvc
Build completed unsuccessfully in 1:39:10
make: *** [Makefile:106: ci-msvc-ps1] Error 1
  network time: Mon, 30 Sep 2024 07:26:48 GMT
##[error]Process completed with exit code 2.
Post job cleanup.
[command]"C:\Program Files\Git\bin\git.exe" version

[bors]

:broken_heart: Test failed - checks-actions