-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Implement RFC 2945: "C-unwind" ABI #76570
Conversation
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. |
0a0146d
to
8839990
Compare
r? @Amanieu |
☔ 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:
|
There was a problem hiding this 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!
589f2ea
to
9925553
Compare
Cross-posting from the #project-ffi-unwind zulip channel:
|
There was a problem hiding this 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
.)
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. |
Ah, nice. I didn't know about that. |
☔ 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:
|
a7c65d6
to
0bc1754
Compare
Providing a small update. These two tests are currently failing, because of an
Aside from that, I want to add the tests described in the comments above, and then this should be in good shape. |
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 :) |
Interesting... it may be the case that |
☔ 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:
|
Leaving another note here for posterity... One of the key design goals specified in RFC 2945 is stated as such:
This appears to have already been implemented in 5f1a0af, and has test coverage in |
fde38d6
to
4d2d1d8
Compare
…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
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.
### 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
…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
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.
### 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
…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
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 theC
,System
,Stdcall
,and
Thiscall
variants, marking whether unwinding across FFI boundaries isacceptable. The cases where each of these variants'
unwind
member is truecorrespond with the
C-unwind
,system-unwind
,stdcall-unwind
, andthiscall-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/
, whichensure 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 thatshould 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 isused to determind whether it should abort, assuming that the
panic=unwind
strategy is being used, and no explicit unwind attribute was provided.