Skip to content

WIP: "C panic" ABI specifier #2753

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.

Sign up for GitHub

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

Closed
wants to merge 18 commits into from
Closed

WIP: "C panic" ABI specifier #2753

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
d252881
Simple RFC with lots of 'TODO's and text from Josh Triplett
Aug 29, 2019
beb20e3
Apply initial changes based on previous discussion
BatmanAoD Aug 29, 2019
4e892b1
Some formatting stuff, remove one TODO
Aug 29, 2019
513a86e
Update text/0000-simple_unwind_annotation.md
BatmanAoD Aug 29, 2019
1d0cc81
Update text/0000-simple_unwind_annotation.md
BatmanAoD Aug 30, 2019
c3588d4
Update text/0000-simple_unwind_annotation.md
BatmanAoD Aug 30, 2019
42b7e2b
Update text/0000-simple_unwind_annotation.md
BatmanAoD Aug 30, 2019
d5b3c95
Update text/0000-simple_unwind_annotation.md
BatmanAoD Aug 30, 2019
b155874
expand motivations and alternatives; emphasize fn pointer behavior
acfoltzer Sep 6, 2019
cb4e853
Merge pull request #2759 from acfoltzer/SimpleUnwindAnnotation
BatmanAoD Sep 7, 2019
e950faa
Apply suggested changes to PR additions
BatmanAoD Sep 7, 2019
dab4e7f
Another suggested change to PR additions
BatmanAoD Sep 7, 2019
233578e
switch from annotation approach to new ABI string approach
acfoltzer Sep 10, 2019
8594404
fix link to related RFC PR
acfoltzer Sep 10, 2019
8c78012
`"C+panic"` -> `"C panic"` per current discussion
acfoltzer Sep 10, 2019
cdd2ab1
Apply suggestions from code review
acfoltzer Sep 10, 2019
0908383
fix up grammar
acfoltzer Sep 10, 2019
62e9a61
Merge pull request #2761 from acfoltzer/SimpleUnwindAnnotation
BatmanAoD Sep 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions text/0000-simple_unwind_annotation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
- 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
Copy link
Contributor

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.

Copy link
Member Author

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.


# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

TODO
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
TODO
The `#[unwind(allowed)]` attribute permits functions with non-Rust ABIs (e.g. `extern "C" fn`) to unwind rather than terminating the process.

(The current verbiage is bad. Please suggest improvements.)

I don't think we've yet reached consensus on whether to include the (allowed). Is it possible for annotations to exist in multiple forms, one that has an argument and one that doesn't? If not, I think we should use (allowed) for forwards compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for annotations to exist in multiple forms, one that has an argument and one that doesn't?

Yes.


# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

TODO: incorporate changes from discussion here:
https://github.com/rust-lang/rust/pull/63909

By default, Rust assumes that an external function imported with extern "C"
cannot unwind, and Rust will abort if a panic would propagate out of a Rust
function exported with extern "C" function. If you specify the #[unwind]
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
function exported with extern "C" function. If you specify the #[unwind]
function with a non-"Rust" ABI ("`extern`") specification. If you specify the #[unwind(allowed)]

This verbiage could still be improved

attribute on an extern "C" function, Rust will instead allow an unwind (such as
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
attribute on an extern "C" function, Rust will instead allow an unwind (such as
attribute on a function with a non-"Rust" ABI, Rust will instead allow an unwind (such as

To be clarified: the annotation can be applied to either function definitions (extern "C" fn...) or function declarations-without-definitions (extern "C" { fn ...; I'm not sure if there's a better term for these declarations).

Without this annotation, what is the behavior supposed to be? I'm pretty sure that for maximum soundness, definitions should always abort and declarations should be assumed not to abort. Should declarations without #[unwind(allowed)] be given automatic "wrapper" functions that would catch unwinding and trigger an abort, or should they simply propagate the exception as normal?

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
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 may require
target-specific options for the non-Rust code, or may not be supported at all.

# Drawbacks
[drawbacks]: #drawbacks

TODO

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives

TODO
Copy link

Choose a reason for hiding this comment

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

Suggested change
TODO
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
Copy link

@jan-hudec jan-hudec Aug 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
TODO
- Since this simple approach does not allow bindings to specify whether they expect callbacks they accept to unwind, we may want to restrict `#[unwind]` to `unsafe` functions. This restriction can be lifted when unwind is made part of the type.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 FnOnce types, doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 #[unwind(allowed)].

@gnzlbg What do you think of this suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

@BatmanAoD If we switch to extern "C panic" fn then this issue goes away, because that will also let us handle function pointers easily.


# Future possibilities
[future-possibilities]: #future-possibilities

TODO

- `unwind(abort)`
- non-"C" ABIs