Implement RFC 2945: "C-unwind" ABI by cratelyn · Pull Request #76570 · rust-lang/rust

cratelyn · 2020-09-10T13:19:04Z

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 · 2020-09-10T13:19:07Z

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.

varkor · 2020-09-10T16:41:10Z

r? @Amanieu

bors · 2020-09-10T21:58:20Z

☔ 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

nikomatsakis

Looking good to me so far!

compiler/rustc_target/src/spec/abi.rs

cratelyn · 2020-10-23T23:14:14Z

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 😊

nikomatsakis

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

compiler/rustc_middle/src/ty/layout.rs

src/test/codegen/unwind-abis/system-unwind-abi.rs

src/test/ui/symbol-names/impl1.rs

Mark-Simulacrum · 2020-10-27T15:18:17Z

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 · 2020-10-27T16:21:28Z

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

bors · 2020-10-28T01:51:03Z

☔ 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

compiler/rustc_mir_build/src/build/mod.rs

cratelyn · 2020-12-08T22:29:39Z

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 · 2020-12-08T22:38:10Z

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 · 2020-12-08T22:54:40Z

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 · 2020-12-10T03:17:09Z

☔ 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

compiler/rustc_symbol_mangling/src/v0.rs

src/test/ui/allocator/no_std-alloc-error-handler-custom.rs

cratelyn · 2020-12-12T02:39:48Z

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.

src/test/ui/allocator/no_std-alloc-error-handler-custom.rs

### Changes This commit implements unwind ABI's, specified in RFC 2945. 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. ### Tests Unit tests, checking that the behavior is correct for `C-unwind`, `stdcall-unwind`, `system-unwind`, and `thiscall-unwind`, are included. These alternative `unwind` ABI strings are specified in RFC 2945, in the "_Other `unwind` ABI strings_" section. Additionally, a test case is included to assert that the LLVM IR generated for an external function defined with the `C-unwind` ABI will be appropriately labeled with the `nounwind` LLVM attribute when the `panic=abort` compilation flag is used. ### Ignore Directives This commit uses `ignore-*` directives in two of our `*-unwind` ABI test cases. Specifically, the `stdcall-unwind` and `thiscall-unwind` test cases ignore architectures that do not support `stdcall` and `thiscall`, respectively. These directives are cribbed from `src/test/ui/c-variadic/variadic-ffi-1.rs` for `stdcall`, and `src/test/ui/extern/extern-thiscall.rs` for `thiscall`.

### Integration Tests This commit introduces some new fixtures to the `run-make-fulldeps` test suite. * c-unwind-abi-catch-panic: Exercise unwinding a panic. This catches a panic across an FFI boundary and downcasts it into an integer. * c-unwind-abi-catch-lib-panic: This is similar to the previous `*catch-panic` test, however in this case the Rust code that panics resides in a separate crate. ### Add `rust_eh_personality` to `#[no_std]` alloc tests This commit addresses some test failures that now occur in the following two tests: * no_std-alloc-error-handler-custom.rs * no_std-alloc-error-handler-default.rs Each test now defines a `rust_eh_personality` extern function, in the same manner as shown in the "Writing an executable without stdlib" section of the `lang_items` documentation here: https://doc.rust-lang.org/unstable-book/language-features/lang-items.html#writing-an-executable-without-stdlib Without this change, these tests would fail to compile due to a linking error explaining that there was an "undefined reference to `rust_eh_personality'." ### Updated hash * update 32-bit hash in `impl1` test ### Panics This commit uses `panic!` macro invocations that return a string, rather than using an integer as a panic payload. Doing so avoids the following warnings that were observed during rollup for the `*-msvc-1` targets: ``` warning: panic message is not a string literal --> panic.rs:10:16 | 10 | panic!(x); // That is too big! | ^ | = note: `#[warn(non_fmt_panic)]` on by default = note: this is no longer accepted in Rust 2021 help: add a "{}" format string to Display the message | 10 | panic!("{}", x); // That is too big! | ^^^^^ help: or use std::panic::panic_any instead | 10 | std::panic::panic_any(x); // That is too big! | ^^^^^^^^^^^^^^^^^^^^^ warning: 1 warning emitted ``` See: https://github.com/rust-lang-ci/rust/runs/1992118428 As these errors imply, panicking without a format string will be disallowed in Rust 2021, per rust-lang#78500.

### Add debug assertion to check `AbiDatas` ordering This makes a small alteration to `Abi::index`, so that we include a debug assertion to check that the index we are returning corresponds with the same abi in our data array. This will help prevent ordering bugs in the future, which can manifest in rather strange errors. ### Using exhaustive ABI matches This slightly modifies the changes from our previous commits, favoring exhaustive matches in place of `_ => ...` fall-through arms. This should help with maintenance in the future, when additional ABI's are added, or when existing ABI's are modified. ### List all `-unwind` ABI's in unstable book This updates the `c-unwind` page in the unstable book to list _all_ of the other ABI strings that are introduced by this feature gate. Now, all of the ABI's specified by RFC 2945 are shown. Co-authored-by: Amanieu d'Antras <amanieu@gmail.com> Co-authored-by: Niko Matsakis <niko@alum.mit.edu>

cratelyn · 2021-03-09T19:43:09Z

Just addressed the merge conflicts that had popped up, pending CI this should be ready for another attempt at ye olde rollup 🍬 🎉

Dylan-DPC-zz · 2021-03-09T20:00:25Z

well.. before the 'ye olde rollup it needs to be r+'d first ;)

@bors r=Amanieu

bors · 2021-03-09T20:00:26Z

📌 Commit 05bf037 has been approved by Amanieu

bors · 2021-03-10T16:44:08Z

⌛ Testing commit 05bf037 with merge 17a07d7...

bors · 2021-03-10T19:12:47Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 17a07d7 to master...

rust-highfive · 2021-03-10T19:13:51Z

📣 Toolstate changed by #76570!

Tested on commit 17a07d7.
Direct link to PR: #76570

💔 miri on windows: test-fail → build-fail (cc @RalfJung @oli-obk @eddyb).
💔 miri on linux: test-fail → build-fail (cc @RalfJung @oli-obk @eddyb).

rust-highfive assigned varkor Sep 10, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2020

cratelyn force-pushed the implement-rfc-2945-c-unwind-abi branch from 0a0146d to 8839990 Compare September 10, 2020 13:59

rust-highfive assigned Amanieu and unassigned varkor Sep 10, 2020

nikomatsakis reviewed Sep 24, 2020

View reviewed changes

compiler/rustc_target/src/spec/abi.rs Outdated Show resolved Hide resolved

yuriks mentioned this pull request Sep 25, 2020

Abort instead of unwinding past FFI functions #52652

Closed

nikomatsakis mentioned this pull request Oct 8, 2020

ffi-unwind rust-lang/lang-team#19

Closed

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

nikomatsakis approved these changes Oct 27, 2020

View reviewed changes

gThorondorsen reviewed Dec 5, 2020

View reviewed changes

compiler/rustc_mir_build/src/build/mod.rs Outdated Show resolved Hide resolved

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 commented Dec 10, 2020

View reviewed changes

compiler/rustc_symbol_mangling/src/v0.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Dec 11, 2020

View reviewed changes

src/test/ui/allocator/no_std-alloc-error-handler-custom.rs Outdated Show resolved Hide resolved

cratelyn force-pushed the implement-rfc-2945-c-unwind-abi branch from fde38d6 to 4d2d1d8 Compare December 14, 2020 16:32

bjorn3 reviewed Dec 14, 2020

View reviewed changes

src/test/ui/allocator/no_std-alloc-error-handler-custom.rs Outdated Show resolved Hide resolved

katelyn a. martin and others added 3 commits March 9, 2021 14:38

This was referenced Mar 10, 2021

Allow specifying alignment for functions #81234

Merged

Accept additional user-defined classes in fenced code blocks #79454

Closed

This was referenced Mar 11, 2021

Undefined Behavior in safe code that unwinds out of extern "C" function #63943

Closed

Detect UB due to unwinding past extern "C" functions rust-lang/miri#1740

Closed

Refactor: deduplicate "can this unwind?" logic #65303

Closed

elichai mentioned this pull request Mar 12, 2021

remove panic from C invalid-argument callbacks rust-bitcoin/rust-secp256k1#288

Closed

daira mentioned this pull request Oct 24, 2024

Consider whether to use "C-unwind" calling convention instead of "C" zcash/zcash#6971

Open

cratelyn mentioned this pull request Mar 17, 2021

"C-unwind" ABI is unsound with "-Cpanic=abort" #83116

Closed

lopopolo mentioned this pull request Mar 25, 2021

Implement mrb_raise with panic artichoke/artichoke#35

Open

khvzak mentioned this pull request Mar 27, 2021

illegal hardware instruction when using ffi+luajit on beta and nightly >=2021-03-11 #83541

Closed

rylev mentioned this pull request Mar 31, 2021

Add __CxxFrameHandler3 in panic_abort #83607

Closed

cratelyn mentioned this pull request Apr 13, 2021

move new c abi abort behavior behind feature gate #84158

Merged

ghost mentioned this pull request Apr 23, 2021

Implement calls to exported symbols (#1170) rust-lang/miri#1776

Merged

This was referenced Apr 28, 2021

[beta] Revert c unwind abi PR #84672

Closed

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

Merged

RalfJung mentioned this pull request Jun 14, 2021

rustc: Fill out remaining parts of C-unwind ABI #86155

Merged

Aaron1011 mentioned this pull request Aug 9, 2021

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

Closed

11 tasks

GabrielMajeri mentioned this pull request Sep 7, 2021

Handle panics by unwinding the stack and implement check_event method rust-osdev/uefi-rs#282

Merged

elichai mentioned this pull request Jan 6, 2022

Undefined behavior: the library panics across FFI boundary rust-bitcoin/rust-secp256k1#354

Open

cratelyn mentioned this pull request Mar 6, 2024

c_unwind full stabilization request: change in extern "C" behavior #115285

Closed

xNxExOx mentioned this pull request Mar 15, 2025

Add -unwind versions of the hooks. Hpmason/retour-rs#64

Open

Uh oh!

Conversation

cratelyn commented Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!