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

Implement RFC 2945: "C-unwind" ABI #76570

Merged
merged 4 commits into from
Mar 10, 2021

Conversation

cratelyn
Copy link

@cratelyn cratelyn commented Sep 10, 2020

Implement RFC 2945: "C-unwind" ABI

This branch implements RFC 2945. The tracking issue for this RFC is #74990.

The feature gate for the issue is #![feature(c_unwind)].

This RFC was created as part of the ffi-unwind project group tracked at rust-lang/lang-team#19.

Changes

Further details will be provided in commit messages, but a high-level overview
of the changes follows:

  • A boolean unwind payload is added to the C, System, Stdcall,
    and Thiscall variants, marking whether unwinding across FFI boundaries is
    acceptable. The cases where each of these variants' unwind member is true
    correspond with the C-unwind, system-unwind, stdcall-unwind, and
    thiscall-unwind ABI strings introduced in RFC 2945 [3].

  • This commit adds a c_unwind feature gate for the new ABI strings.
    Tests for this feature gate are included in src/test/ui/c-unwind/, which
    ensure that this feature gate works correctly for each of the new ABIs.
    A new language features entry in the unstable book is added as well.

  • We adjust the rustc_middle::ty::layout::fn_can_unwind function,
    used to compute whether or not a FnAbi object represents a function that
    should be able to unwind when panic=unwind is in use.

  • Changes are also made to
    rustc_mir_build::build::should_abort_on_panic so that the function ABI is
    used to determind whether it should abort, assuming that the panic=unwind
    strategy is being used, and no explicit unwind attribute was provided.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2020
@cratelyn cratelyn force-pushed the implement-rfc-2945-c-unwind-abi branch from 0a0146d to 8839990 Compare September 10, 2020 13:59
@varkor
Copy link
Member

varkor commented Sep 10, 2020

r? @Amanieu

@rust-highfive rust-highfive assigned Amanieu and unassigned varkor Sep 10, 2020
@bors
Copy link
Contributor

bors commented Sep 10, 2020

☔ The latest upstream changes (presumably #76582) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me so far!

compiler/rustc_target/src/spec/abi.rs Show resolved Hide resolved
@cratelyn cratelyn force-pushed the implement-rfc-2945-c-unwind-abi branch 2 times, most recently from 589f2ea to 9925553 Compare October 23, 2020 22:28
@cratelyn
Copy link
Author

Cross-posting from the #project-ffi-unwind zulip channel:

I am bumping into some platform-specific CI errors that I'll sort out, hopefully on Monday? In any case, that's the shape of the implementation of RFC 2945 🎉 I'll mark it as a formal PR, rather than a draft, once I've appeased CI 😊

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good. One thing I am wondering about is when we call C-unwind functions that are implemented in, well, C. In other words, I think we should have a test for the fastly use case, where we invoke a C function that invokes a Rust function which panics, and we test that this panic propagates up the stack. I'm not sure how easy/hard it will be to write such a test though, including C code might be a pain. At minimum we could have a test for panics propagating through Rust functions with C-unwind ABI (and perhaps the same but where those functions are in an auxiliary crate).

(Cross crate tests can be created by using the // aux-build: comments in tests; I didn't find any docs on this, but you can maybe grep around to see how it's done. You basically write // aux-build: foo.rs and then add a file auxiliary/foo.rs. You can then import the crate foo.)

compiler/rustc_mir_build/src/build/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved
src/test/ui/symbol-names/impl1.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

For C functions to call, I believe https://github.com/rust-lang/rust/blob/master/src/test/auxiliary/rust_test_helpers.c is the place to add it to -- that file is compiled once and then linked with all UI tests I believe. Or, if you need more complex invocations of C compilers or linkers then run-make tests are the way to go.

@nikomatsakis
Copy link
Contributor

Ah, nice. I didn't know about that.

@bors
Copy link
Contributor

bors commented Oct 28, 2020

☔ The latest upstream changes (presumably #75671) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@cratelyn cratelyn force-pushed the implement-rfc-2945-c-unwind-abi branch 2 times, most recently from a7c65d6 to 0bc1754 Compare December 8, 2020 16:43
@cratelyn
Copy link
Author

cratelyn commented Dec 8, 2020

Providing a small update. These two tests are currently failing, because of an undefined reference to rust_eh_personality'`. I'm not quite sure why that's happening, but I'm planning to look into it tomorrow.

failures:
    [ui] ui/allocator/no_std-alloc-error-handler-custom.rs
    [ui] ui/allocator/no_std-alloc-error-handler-default.rs

Aside from that, I want to add the tests described in the comments above, and then this should be in good shape.

@Mark-Simulacrum
Copy link
Member

I've seen both those tests fail locally too when using lld as the linker, so I would check without your PR. It's quite possible that it's unrelated to your work :)

@BatmanAoD
Copy link
Member

Interesting... it may be the case that "C unwind" can't be supported in a no_std environment, because there won't be exception-handling infrastructure available on the target.

@bors
Copy link
Contributor

bors commented Dec 10, 2020

☔ The latest upstream changes (presumably #78837) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@cratelyn
Copy link
Author

Leaving another note here for posterity...

One of the key design goals specified in RFC 2945 is stated as such:

Enable foreign exceptions to propagate through Rust frames: Similarly, we would like to make it possible for C++ code (or other languages) to raise exceptions that will propagate through Rust frames "as if" they were Rust panics (i.e., running destrutors or, in the case of unwind=abort, aborting the program).

This appears to have already been implemented in 5f1a0af, and has test coverage in src/test/run-make-fulldeps/foreign-exceptions. I'll refrain from duplicating these tests, since they already seem to exercise that goal well enough.

@cratelyn cratelyn force-pushed the implement-rfc-2945-c-unwind-abi branch from fde38d6 to 4d2d1d8 Compare December 14, 2020 16:32
@cratelyn cratelyn deleted the implement-rfc-2945-c-unwind-abi branch March 10, 2021 21:40
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Mar 29, 2021
…abi, r=Amanieu

Implement RFC 2945: "C-unwind" ABI

## Implement RFC 2945: "C-unwind" ABI

This branch implements [RFC 2945]. The tracking issue for this RFC is rust-lang#74990.

The feature gate for the issue is `#![feature(c_unwind)]`.

This RFC was created as part of the ffi-unwind project group tracked at rust-lang/lang-team#19.

### Changes

Further details will be provided in commit messages, but a high-level overview
of the changes follows:

* A boolean `unwind` payload is added to the `C`, `System`, `Stdcall`,
and `Thiscall` variants, marking whether unwinding across FFI boundaries is
acceptable. The cases where each of these variants' `unwind` member is true
correspond with the `C-unwind`, `system-unwind`, `stdcall-unwind`, and
`thiscall-unwind` ABI strings introduced in RFC 2945 [3].

* This commit adds a `c_unwind` feature gate for the new ABI strings.
Tests for this feature gate are included in `src/test/ui/c-unwind/`, which
ensure that this feature gate works correctly for each of the new ABIs.
A new language features entry in the unstable book is added as well.

* We adjust the `rustc_middle::ty::layout::fn_can_unwind` function,
used to compute whether or not a `FnAbi` object represents a function that
should be able to unwind when `panic=unwind` is in use.

* Changes are also made to
`rustc_mir_build::build::should_abort_on_panic` so that the function ABI is
used to determind whether it should abort, assuming that the `panic=unwind`
strategy is being used, and no explicit unwind attribute was provided.

[RFC 2945]: https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md
khvzak added a commit to mlua-rs/mlua that referenced this pull request Apr 10, 2021
Motivation behind this change is upcoming breaking change in Rust
compiler v1.52.0 to prevent unwinding across FFI boundaries.
rust-lang/rust#76570
The new functionality requires nightly compiler to declare FFI
functions as "C-unwind".
The fundamental solution is to use C shim to wrap "e" and "m"
Lua functions in pcall.
Additionally define Rust calling convention to trigger lua_error
on Rust behalf.
cratelyn pushed a commit to cratelyn/rust that referenced this pull request Apr 13, 2021
 ### 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
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 14, 2021
…or, r=nikomatsakis

move new c abi abort behavior behind feature gate

*Background*

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

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. <sup>[2]</sup>

*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. <sup>[3]</sup>

---

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

[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
khvzak added a commit to mlua-rs/mlua that referenced this pull request Apr 26, 2021
Motivation behind this change is upcoming breaking change in Rust
compiler v1.52.0 to prevent unwinding across FFI boundaries.
rust-lang/rust#76570
The new functionality requires nightly compiler to declare FFI
functions as "C-unwind".
The fundamental solution is to use C shim to wrap "e" and "m"
Lua functions in pcall.
Additionally define Rust calling convention to trigger lua_error
on Rust behalf.
pnkfelix pushed a commit to pnkfelix/rust that referenced this pull request Apr 29, 2021
 ### 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
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2021
…t-of-84158, r=Mark-Simulacrum

backport: move new c abi abort behavior behind feature gate

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

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

Therefore, we are backporting PR rust-lang#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 rust-lang#83541

<details>

<summary>Click for details as to why revert of PR rust-lang#76570 did not go smoothly.</summary>

It turns out that Miri had been subsequently updated to reflect changes to `rustc_target` that landed in PR rust-lang#76570. This meant that the attempt to land PR rust-lang#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 rust-lang#74709.)

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

</details>

Original commit message follows:

----

 *Background*

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

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. <sup>[2]</sup>

 *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. <sup>[3]</sup>

 ### 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

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