-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
WIP: "C panic" ABI specifier #2753
Changes from 8 commits
d252881
beb20e3
4e892b1
513a86e
1d0cc81
c3588d4
42b7e2b
d5b3c95
b155874
cb4e853
e950faa
dab4e7f
233578e
8594404
8c78012
cdd2ab1
0908383
62e9a61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,78 @@ | ||||||||||||
- Feature Name: `simple_unwind_attribute` | ||||||||||||
- Start Date: 2019-08-29 | ||||||||||||
- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) | ||||||||||||
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) | ||||||||||||
|
||||||||||||
# Summary | ||||||||||||
[summary]: #summary | ||||||||||||
|
||||||||||||
Provides an annotation to permit functions with explicit ABI specifications | ||||||||||||
(such as `extern "C"`) to unwind | ||||||||||||
|
||||||||||||
# Motivation | ||||||||||||
[motivation]: #motivation | ||||||||||||
|
||||||||||||
TODO | ||||||||||||
|
||||||||||||
- soundness & optimization | ||||||||||||
- libjpeg/mozjpeg | ||||||||||||
|
||||||||||||
# Guide-level explanation | ||||||||||||
[guide-level-explanation]: #guide-level-explanation | ||||||||||||
|
||||||||||||
The `#[unwind(allowed)]` attribute permits functions with non-Rust ABIs (e.g. `extern "C" fn`) to unwind rather than terminating the process. | ||||||||||||
|
||||||||||||
# Reference-level explanation | ||||||||||||
[reference-level-explanation]: #reference-level-explanation | ||||||||||||
|
||||||||||||
By default, Rust assumes that an external function imported with `extern "C" { | ||||||||||||
... }` (or another ABI other than `"Rust"`) cannot unwind, and Rust will abort if a panic would propagate out of a | ||||||||||||
Rust function with a non-"Rust" ABI ("`extern "ABI" fn`") specification. If you specify | ||||||||||||
BatmanAoD marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
the `#[unwind(allowed)]` attribute on a function with a non-"Rust" ABI, Rust | ||||||||||||
will instead allow an unwind (such as a panic) to proceed through that function | ||||||||||||
boundary using Rust's normal unwind mechanism. This may potentially allow Rust | ||||||||||||
code to call non-Rust code that calls back into Rust code, and then allow a | ||||||||||||
panic to propagate from Rust to Rust across the non-Rust code. | ||||||||||||
|
||||||||||||
The Rust unwind mechanism is intentionally not specified here. Catching a Rust | ||||||||||||
BatmanAoD marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
panic in Rust code compiled with a different Rust toolchain or options is not | ||||||||||||
specified. Catching a Rust panic in another language or vice versa is not | ||||||||||||
specified. The Rust unwind may or may not run non-Rust destructors as it | ||||||||||||
unwinds. Propagating a Rust panic through non-Rust code is unspecified; | ||||||||||||
implementations that define the behavior may require target-specific options | ||||||||||||
for the non-Rust code, or this feature may not be supported at all. | ||||||||||||
BatmanAoD marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
|
||||||||||||
BatmanAoD marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
# Drawbacks | ||||||||||||
[drawbacks]: #drawbacks | ||||||||||||
|
||||||||||||
- Only works as long as Rust uses the same unwinding mechanism as C++. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually think it's reasonable to cite as a disadvantage that the C++ mechanism is the only one that currently works and should be expected to continue working after this RFC. The actual layout of the panic should not matter, because this RFC does not permit foreign code to I don't think the behavior should be explicitly undefined for foreign exceptions in Rust code; as long as the unwinding mechanisms have ABI compatibility and the Rust code doesn't try to catch the exception, this should be just as safe as the other way around. I'm okay with including "this will make changing the ABI harder", but I don't think the proposed phrasing is appropriate. |
||||||||||||
- Does not allow external library bindings to specify whether callbacks they accept are expected to unwind. | ||||||||||||
BatmanAoD marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
|
||||||||||||
# Rationale and alternatives | ||||||||||||
[rationale-and-alternatives]: #rationale-and-alternatives | ||||||||||||
|
||||||||||||
BatmanAoD marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
The goal is to enable the current use-cases for unwinding panics through foreign code while providing safer behaviour towards bindings not expecting that, with minimal changes to the language. | ||||||||||||
|
||||||||||||
We should try not to prevent the RFC 2699 or other later RFC from resolving the above disadvantages on top of this one. | ||||||||||||
|
||||||||||||
- https://github.com/rust-lang/rfcs/pull/2699 | ||||||||||||
|
||||||||||||
# Prior art | ||||||||||||
[prior-art]: #prior-art | ||||||||||||
|
||||||||||||
TODO | ||||||||||||
|
||||||||||||
# Unresolved questions | ||||||||||||
[unresolved-questions]: #unresolved-questions | ||||||||||||
|
||||||||||||
TODO | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this already addressed by your point above that "function pointers shall all be considered unwind and no nounwind function pointer shall be introduced"? I think that statement applies to all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ....actually, this should probably be mentioned in the "alternatives" section, now that the primary proposal recommends treating all function pointers as @gnzlbg What do you think of this suggestion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @BatmanAoD If we switch to |
||||||||||||
|
||||||||||||
# Future possibilities | ||||||||||||
[future-possibilities]: #future-possibilities | ||||||||||||
|
||||||||||||
TODO | ||||||||||||
BatmanAoD marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
|
||||||||||||
- https://github.com/rust-lang/rfcs/pull/2699 | ||||||||||||
|
||||||||||||
- `unwind(abort)` | ||||||||||||
- non-"C" ABIs |
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.
Note that AFAIK
mozjpeg
does not need this. A 200 LOC diff removes the UB from that library, without a performance impact.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.
I (or whoever fleshes this section out) can mention that. These are just items to mention when this section is written.