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

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

Closed
1 of 11 tasks
nikomatsakis opened this issue Jul 31, 2020 · 51 comments · Fixed by #116088
Closed
1 of 11 tasks

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

nikomatsakis opened this issue Jul 31, 2020 · 51 comments · Fixed by #116088
Labels
A-abi Area: Concerning the application binary interface (ABI). A-ffi Area: Foreign Function Interface (FFI) B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-c_unwind `#![feature(c_unwind)]` finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-tracking-impl-incomplete Status: The implementation is incomplete. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jul 31, 2020

This is a tracking issue for the RFC "C-unwind ABI" (rust-lang/rfcs#2945).

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.

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Implementation notes

Major provisions in the RFC:

  • Add a C-unwind ABI and system-unwind (I think), we may need more variants.
  • For external functions with the C ABI, we already (I believe) add the "nounwind" LLVM attributes when we build them. We want to continue doing this.
  • For external functions with the C-unwind ABI, we do not want any such attributes.
  • For Rust functions defined with the C ABI, e.g., extern "C" fn foo() { ... }, we want to force them to abort if there is a panic. The MIR helper function should_abort_on_panic already exists and I think will modify the generated code to insert the required shims in such a case. They should also be marked as "nounwind" in LLVM, if they're not already.
  • Rust functions with the "C-unwind" ABI should not abort on panic.
  • Use ABI to guide the "nounwind" attribute on callsites as well.
  • Write suitable codegen tests to check generated LLVM IR.

Unresolved Questions

None. The unresolved questions in the RFC were meant to be solved by future RFCs building on this one.

Implementation history

@nikomatsakis nikomatsakis added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-c_unwind `#![feature(c_unwind)]` labels Jul 31, 2020
@jonas-schievink jonas-schievink added the A-ffi Area: Foreign Function Interface (FFI) label Jul 31, 2020
@cratelyn
Copy link

Hello! I would be interested in helping implement this RFC. 🙋

(cc: @aturon)

@nikomatsakis
Copy link
Contributor Author

Hey @katie-martin-fastly, that's great news! I'm going to assign you to the issue for now:

@rustbot assign @katie-martin-fastly

One thing to mention is that most Rust compiler development discussion takes place on the rust-lang zulip, and this project in particular is chatting in #project-ffi-unwind. I or @Amanieu are probably reasonable people to ping with questions. Also, for general "getting started" tips on building and testing the Rust compiler, the rustc-dev-guide is your friend. Also, rust-analyzer works well for IDE support (e.g., with vscode), though it requires a small bit of configuration since rustc doesn't build with cargo but rather the x.py script. (I couldn't find any documentation on that, I'll ping a few others and see...)

That said, in terms of mentoring and getting started, I think the idea is going to be to adapt an existing mechanism that the compiler offers. We have this attribute #[unwind(allowed)] which allows users to mark functions as permitting unwinding -- that mechanism is basically going to be deprecated and replaced with this new ABI, though I think it'd be best to add the ABI before trying to adapt the unwind attribute code. Still, searching for code related to that attribute will give you a good idea of what needs to change.

I can do more mentoring instructions later but, as a starting point, I updated the head post with some of the major goals of the RFC (see the "Implementation notes") section. Also here are some tips to get you started in terms of reading into the rustc code:

  • The Abi enum lists out all the known ABIs (there may be more similar enums, not sure). We'll want to extend it with C-unwind and perhaps other unwind variants. One way to do this might be to change the C variant to C { unwind: bool } or something like that, or maybe just add a Cunwind, not sure.
  • The UnwindAttr type records what kind of unwind attribute is placed on a particular function (is unwinding allowed? does it force an abort?)
  • The find_unwind_attr function checks for what unwind attribute is placed on a particular function.
  • The should_abort_on_panic function is used to decide when a function ought to, well, abort if a panic occurs (as the name suggests).
  • The [can_unwind] field of this struct seems to be used to set the LLVM attribute "nounwind". I'm not quite sure where it gets set, but I have to stop now because it's Saturday :P so I'll leave that as an exercise to the reader.

@rustbot rustbot self-assigned this Aug 1, 2020
cratelyn pushed a commit to cratelyn/rust that referenced this issue Jan 26, 2021
 ### Overview

    This commit begins the implementation work for RFC 2945. For more
    information, see the rendered RFC [1] and tracking issue [2].

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

 ### Feature Gate and Unstable Book

    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.

 ### Further Work To Be Done

    This commit does not proceed to implement the new unwinding ABIs,
    and is intentionally scoped specifically to *defining* the ABIs and
    their feature flag.

 ### One Note on Test Churn

    This will lead to some test churn, in re-blessing hash tests, as the
    deleted comment in `src/librustc_target/spec/abi.rs` mentioned,
    because we can no longer guarantee the ordering of the `Abi`
    variants.

    While this is a downside, this decision was made bearing in mind
    that RFC 2945 states the following, in the "Other `unwind` Strings"
    section [3]:

    >  More unwind variants of existing ABI strings may be introduced,
    >  with the same semantics, without an additional RFC.

    Adding a new variant for each of these cases, rather than specifying
    a payload for a given ABI, would quickly become untenable, and make
    working with the `Abi` enum prone to mistakes.

    This approach encodes the unwinding information *into* a given ABI,
    to account for the future possibility of other `-unwind` ABI
    strings.

 ### Footnotes

[1]: https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md
[2]: rust-lang#74990
[3]: https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md#other-unwind-abi-strings
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 28, 2021
…945-c-unwind-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
@BatmanAoD
Copy link
Member

@RalfJung @nbdd0121 @nikomatsakis @joshtriplett

What is the next step for the extern "C" change? Do I need to write a separate stabilization report? Gary, would you be willing to write the PR to remove the feature flag, since you made the first stabilization PR?

Since 1.72 will be released this Thursday, it may be too late to make it in time for 1.73, but I'd like to get it in for 1.74 if possible.

@RalfJung
Copy link
Member

RalfJung commented Aug 23, 2023 via email

@nbdd0121
Copy link
Contributor

Gary, would you be willing to write the PR to remove the feature flag, since you made the first stabilization PR?

Sure

@BatmanAoD
Copy link
Member

BatmanAoD commented Aug 26, 2023

Request for Stabilization

This is a request for full stabilization of the c_unwind feature, RFC-2945. The behavior of non-"Rust", non-unwind ABIs (such as extern "C") will be modified to close soundness holes caused by permitting stack-unwinding to cross FFI boundaries that do not support unwinding.

Summary

When using panic=unwind, if a Rust function marked extern "C" panics (and that panic is not caught), the runtime will now abort.

Previously, the runtime would simply attempt to unwind the caller's stack, but the behavior when doing so was undefined, because extern "C" functions are optimized with the assumption that they cannot unwind (i.e. in rustc, they are given the LLVM nounwind annotation).

This affects existing programs. If a program relies on a Rust panic "escaping" from extern "C":

  • It is currently unsound.
  • Once this feature is stabilized, the program will crash when this occurs, whereas previously it may have appeared to work as expected.
  • Replacing extern "x" with extern "x-unwind" will produce the intended behavior without the unsoundness.

The behavior of function calls using extern "C" is unchanged; thus, it is still undefined behavior to call a C++ function that throws an exception using the extern "C" ABI, even when compiling with panic=unwind.

Changes since the RFC

None.

Unresolved questions

This feature does not affect the behavior of pthread_exit or longjmp, so stabilization will neither break existing code using these features nor provide any soundness guarantee.

Tests

"C" guaranteed to abort on panic

This test verifies that an extern "C" function that panic!s will always abort if the panic would otherwise "escape".

Documentation

The documentation PRs for this feature did not distinguish between the initial partially-stabilized behavior and the fully-stabilized behavior, so they already include the modifications to non-unwind ABIs.

I've also opened an issue to discuss adding a brief note about this feature in the Book.

@BatmanAoD
Copy link
Member

Opened a separate issue (#115285) at @joshtriplett's request, to enable re-using the FCP bot.

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 10, 2023
Stabilise `c_unwind`

Fix rust-lang#74990
Fix rust-lang#115285

Marking as draft PR for now due to `compiler_builtins` issues

r? `@Amanieu`
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 26, 2023
Stabilise `c_unwind`

Fix rust-lang#74990
Fix rust-lang#115285

Marking as draft PR for now due to `compiler_builtins` issues

r? `@Amanieu`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 7, 2024
Stabilise `c_unwind`

Fix rust-lang#74990
Fix rust-lang#115285 (that's also where FCP is happening)

Marking as draft PR for now due to `compiler_builtins` issues

r? `@Amanieu`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 7, 2024
Stabilise `c_unwind`

Fix rust-lang#74990
Fix rust-lang#115285 (that's also where FCP is happening)

Marking as draft PR for now due to `compiler_builtins` issues

r? ``@Amanieu``
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 9, 2024
Stabilise `c_unwind`

Fix rust-lang#74990
Fix rust-lang#115285 (that's also where FCP is happening)

Marking as draft PR for now due to `compiler_builtins` issues

r? `@Amanieu`
@bors bors closed this as completed in 1aaab8b Jun 20, 2024
asomers added a commit to asomers/mockall that referenced this issue Jun 22, 2024
Begining within Rust 1.81
(rust-lang/rust#74990) Rust will abort when
attempting to unwind a panic across an "extern C" boundary.  Previously
it was technically UB, but it worked and Mockall relied on it.  Now,
unwinding across such a boundary requires the "extern C-unwind" ABI.
Use that ABI in the generated code.

However, don't use that ABI when mocking a C variadic function.  That
doesn't work, due to rust-lang/rust#126836 .

Fixes #584
asomers added a commit to asomers/mockall that referenced this issue Jun 22, 2024
Begining within Rust 1.81
(rust-lang/rust#74990) Rust will abort when
attempting to unwind a panic across an "extern C" boundary.  Previously
it was technically UB, but it worked and Mockall relied on it.  Now,
unwinding across such a boundary requires the "extern C-unwind" ABI.
Use that ABI in the generated code.

However, don't use that ABI when mocking a C variadic function.  That
doesn't work, due to rust-lang/rust#126836 .

Fixes #584
github-merge-queue bot pushed a commit to stellar/rs-soroban-sdk that referenced this issue Sep 10, 2024
### What

Fixes for Rust 1.81:

1. Update semver-checks to 0.35.0.

2. Don't use the `extern` function for invoking a contract function when
executing tests.

### Why

1. Semver checks is failing to run on the new version of Rust and needs
updating.

2. Rust 1.81.0 changed the behavior of panics on `extern "C"` functions,
such that panics cannot be caught by
[std::panic::catch_unwind](https://doc.rust-lang.org/std/panic/fn.catch_unwind.html).

Contracts expose their functions as an `extern` function that defaults
to `extern "C"`. The test environment catches panics inside contract
functions and resurfaces them as errors by using catch_unwind.

The solution used was to make it so that the test environment never
calls through the extern function, and instead calls through a
non-extern function that does the same work. This seems like the sanest
way to take away any dependence / relationship between the test
environment and the Rust C-ABI so that any future changes to the C-ABI
don't affect the test environment.

An alternative solution would be to mark the contract function as
`extern "C-unwind"` so that it retained unwind functionality, but that
would continue to bind the SDK test environment to behaviour changes in
extern fns.

In the solution in this change the Abi qualifier `"C"` has been added.
When the Abi qualifier is not specified it defaults to `"C"`, but for
the sake of being explicit and removing any doubt from a reader it is
now specified. The Rust reference confirms that `"C"` is the default:
-
https://doc.rust-lang.org/reference/items/functions.html#extern-function-qualifier.
   
   More details on this change to Rust 1.81.0 can be found at:
-
https://blog.rust-lang.org/2024/09/05/Rust-1.81.0.html#abort-on-uncaught-panics-in-extern-c-functions
   - rust-lang/rust#74990
    
   Close #1332
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-abi Area: Concerning the application binary interface (ABI). A-ffi Area: Foreign Function Interface (FFI) B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-c_unwind `#![feature(c_unwind)]` finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-tracking-impl-incomplete Status: The implementation is incomplete. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.