Make destructors on extern "C" frames to be executed

[nbdd0121]

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:

• Tracking Issue for "C-unwind ABI", RFC 2945 #74990

[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 #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 (#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:

• panic in a no-unwind function leads to not dropping local variables #123231 (comment)

@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 (#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 #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?

[rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry
• @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.

[bors]

📌 Commit 8f63b6a has been approved by nnethercote

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8f63b6a with merge 52c2521...

[bors]

💔 Test failed - checks-actions

[nbdd0121]

The failing test tests/run-make/longjmp-across-rust looks like UB to me.

[RalfJung]

Added in #48572.

It probably needs C-unwind?

longjmp across frames without destructors is IIRC intended to work.

[nbdd0121]

Yeah, but there is a destructor here, albeit empty.

[RalfJung]

Oh. Yeah that's UB indeed.
Maybe adjust the test to remove the destructor?

Cc @alexcrichton just FYI, a test you added many years ago turns out to have UB... not sure if there's any context there that is still relevant.

[alexcrichton]

It's ok times change and we learn things, I think it's a better move to consider unwinding/longjmp over rust frames with destructors as UB rather than "something that should work", so sounds like it's a good test to remove/update 👍

[BatmanAoD]

Removing the test is correct; longjmp over Rust frames with destructors is always UB, per RFC 2945. Such frames are not Plane Old Frames, and:

This RFC specifies that, regardless of the platform or the ABI string ("C" or "C-unwind"), any platform features that may rely on forced unwinding will always be considered undefined behavior if they cross non-POFs.

longjmp is one such platform feature that "may rely on forced unwinding."

[nnethercote]

@bors r+

[bors]

📌 Commit bb53108 has been approved by nnethercote

It is now in the queue for this repository.

[bors]

⌛ Testing commit bb53108 with merge 06d261d...

[bors]

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing 06d261d to master...

[rust-timer]

Finished benchmarking commit (06d261d): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 5.4%)

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)	-	-	0
Regressions ❌ (secondary)	5.4%	[5.2%, 5.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 2.3%, secondary 4.5%)

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.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	4.5%	[4.5%, 4.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.3%	[2.3%, 2.3%]	1

Binary size

Results (primary 0.1%, secondary 0.2%)

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)	0.1%	[0.0%, 0.2%]	12
Regressions ❌ (secondary)	0.2%	[0.1%, 0.2%]	38
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.2%]	12

Bootstrap: 781.077s -> 780.529s (-0.07%)
Artifact size: 333.74 MiB -> 333.89 MiB (0.04%)