Make destructors on extern "C" frames to be executed by nbdd0121 · Pull Request #129582 · rust-lang/rust (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

nbdd0121

@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

@rustbot rustbot added the T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

label

Aug 25, 2024

@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

RalfJung

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

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

@traviscross traviscross added T-lang

Relevant to the language team, which will review and decide on the PR/issue.

I-lang-nominated

Nominated for discussion during a lang team meeting.

and removed T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Aug 27, 2024

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

@traviscross

This comment was marked as resolved.

@rfcbot

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

@bors bors added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Sep 30, 2024

@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."

@nbdd0121

Destructor are removed from stack because it's considered UB.

@rustbot

This comment was marked as off-topic.

@nnethercote

@bors

📌 Commit bb53108 has been approved by nnethercote

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Oct 17, 2024

@bors

@bors

@bors bors mentioned this pull request

Oct 17, 2024

@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%)

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request

Jan 22, 2025

@tmeijn

This MR contains the following updates:

Package Update Change
rust minor 1.83.0 -> 1.84.0

MR created with the help of el-capitano/tools/renovate-bot.

Proposed changes to behavior should be submitted there as MRs.

Release Notes

rust-lang/rust (rust)

v1.84.0

Compare Source

==========================

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts

Cargo

Rustdoc

Compatibility Notes

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

♻ Rebasing: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this MR and you won't be reminded about this update again.

This MR has been generated by Renovate Bot.

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Feb 2, 2025

@he32

Pkgsrc changes:

Upstream changes:

Version 1.84.1 (2025-01-30)

Version 1.84.0 (2025-01-09)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts

Cargo

Rustdoc

Compatibility Notes

Labels

A-run-make

Area: port run-make Makefiles to rmake.rs

disposition-merge

This issue / PR is in PFCP or FCP with a disposition to merge it.

finished-final-comment-period

The final comment period is finished for this PR / Issue.

merged-by-bors

This PR was explicitly merged by bors.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-lang

Relevant to the language team, which will review and decide on the PR/issue.