Skip to content
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

backport: move new c abi abort behavior behind feature gate #84722

Merged

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Apr 29, 2021

This is a backport of PR #84158 to the beta branch.

The original T-compiler plan was to revert PR #76570 in its entirety, as was attempted in PR #84672. But the revert did not go smoothly (details below).

Therefore, we are backporting PR #84158 instead, which was our established backup plan if a revert did not go smoothly.

I have manually confirmed that this backport fixes the luajit issue described on issue #83541

Click for details as to why revert of PR #76570 did not go smoothly.

It turns out that Miri had been subsequently updated to reflect changes to rustc_target that landed in PR #76570. This meant that the attempt to land PR #84672 broke Miri builds.

Normally we allow tools to break when landing PR's (and just expect follow-up PR's to fix the tools), but we don't allow it for tools in the run-up to a release.

(We shouldn't be using that uniform policy for all tools. Miri should be allow to break during the week before a release; but currently we cannot express that, due to issue #74709.)

Therefore, its a lot of pain to try to revert PR #76570. And we're going with the backup plan.

Original commit message follows:


Background

In #76570, new ABI strings including C-unwind were introduced. Their behavior is specified in RFC 2945 1.

However, it was reported in the #ffi-unwind stream of the Rust community Zulip that this had altered the way that extern "C" functions behaved even when the c_unwind feature gate was not active. 2

Overview

This makes a small patch to rustc_mir_build::build::should_abort_on_panic, so that the same behavior from before is in place when the c_unwind gate is not active.

rustc_middle::ty::layout::fn_can_unwind is not touched, as the visible behavior should not differ before/after #76570. 3

Footnotes

1.: https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md
2.: https://rust-lang.zulipchat.com/#narrow/stream/210922-project-ffi-unwind/topic/Is.20unwinding.20through.20extern.20C.20UB.3F/near/230112325
3.: https://github.com/rust-lang/rust/pull/76570/files#diff-b0320c2b8868f325d83c027fc5d71732636e9763551e35895488f30fe057c6e9L2599-R2617

 ### Background

    In rust-lang#76570, new ABI strings including `C-unwind` were introduced.
    Their behavior is specified in RFC 2945 [1].

    However, it was reported in the #ffi-unwind stream of the Rust
    community Zulip that this had altered the way that `extern "C"`
    functions behaved even when the `c_unwind` feature gate was not
    active. [2]

 ### Overview

    This makes a small patch to
    `rustc_mir_build::build::should_abort_on_panic`, so that the same
    behavior from before is in place when the `c_unwind` gate is not
    active.

    `rustc_middle::ty::layout::fn_can_unwind` is not touched, as the
    visible behavior should not differ before/after rust-lang#76570. [3]

 ### Footnotes

 [1]: https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md
 [2]: https://rust-lang.zulipchat.com/#narrow/stream/210922-project-ffi-unwind/topic/Is.20unwinding.20through.20extern.20C.20UB.3F/near/230112325
 [3]: https://github.com/rust-lang/rust/pull/76570/files#diff-b0320c2b8868f325d83c027fc5d71732636e9763551e35895488f30fe057c6e9L2599-R2617
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • Pull requests are usually filed against the master branch for this repo, but this one is against beta. Please double check that you specified the right target!

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 29, 2021
@pnkfelix
Copy link
Member Author

r? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Apr 29, 2021

📌 Commit 8341f42 has been approved by Mark-Simulacrum

@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 Apr 29, 2021
@bors
Copy link
Contributor

bors commented Apr 30, 2021

⌛ Testing commit 8341f42 with merge 9a1dfd2...

@bors
Copy link
Contributor

bors commented Apr 30, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 9a1dfd2 to beta...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 30, 2021
@bors bors merged commit 9a1dfd2 into rust-lang:beta Apr 30, 2021
@rustbot rustbot added this to the 1.52.0 milestone Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants