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

Switch from annotation approach to new ABI string approach #2761

Merged

BatmanAoD merged 5 commits into rust-lang:SimpleUnwindAnnotation from acfoltzer:SimpleUnwindAnnotation

Sep 10, 2019

acfoltzer commented Sep 10, 2019

Per discussion in #2753, this switches the RFC to describe an ABI string-based approach, rather than an attribute-based approach.

acfoltzer added 2 commits

September 10, 2019 12:23


switch from annotation approach to new ABI string approach

233578e


fix link to related RFC PR

Member

joshtriplett commented Sep 10, 2019

This looks great to me.

Minor nit: there was a request to use something other than + here, to avoid confusion with C++, and I see no harm in accommodating that. Could you switch it to "C panic" instead?


"C+panic" -> "C panic" per current discussion

8c78012

Member

joshtriplett commented Sep 10, 2019

Thanks!

Once this is merged, I'd be happy to write some of the current TODO sections.

BatmanAoD approved these changes

View reviewed changes

Member

BatmanAoD left a comment

This is great; thank you. I think am going to wait until the current discussion about future-compatibility has reached a bit of a resolution, though, before merging.

text/0000-simple_unwind_annotation.md Outdated

-Calls to function pointers with non-Rust ABIs, as opposed to declared or defined functions,
-currently are permitted to unwind. This RFC does not change this behavior.
+When used on declarations of imported functions (e.g., `extern "C+panic" { fn ... }`), or function

Member

BatmanAoD Sep 10, 2019

Suggested change

When used on declarations of imported functions (e.g., `extern "C+panic" { fn ... }`), or function

When used on declarations of imported functions (e.g., `extern "C panic" { fn ... }`) or function

text/0000-simple_unwind_annotation.md Outdated

-Calls to function pointers with non-Rust ABIs, as opposed to declared or defined functions,
-currently are permitted to unwind. This RFC does not change this behavior.
+When used on declarations of imported functions (e.g., `extern "C+panic" { fn ... }`), or function
+pointers (e.g., `extern "C+panic" fn()`) the `"C+panic"` ABI string means that if the function

Member

BatmanAoD Sep 10, 2019

Suggested change

pointers (e.g., `extern "C+panic" fn()`) the `"C+panic"` ABI string means that if the function

pointers (e.g., `extern "C panic" fn()`), the `"C panic"` ABI string means that if the function

text/0000-simple_unwind_annotation.md Outdated

-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
+Rust function with a non-"Rust" ABI ("`extern "ABI" fn`") specification. Under this RFC,
+functions with the `"C+panic"` ABI string

Member

BatmanAoD Sep 10, 2019

Suggested change

functions with the `"C+panic"` ABI string

functions with the `"C panic"` ABI string

text/0000-simple_unwind_annotation.md Outdated

 implementations that define the behavior may require target-specific options
 for the non-Rust code, or this feature may not be supported at all.
+For the purposes of the type system, `"C+panic"` is considered a totally distinct ABI string from

Member

BatmanAoD Sep 10, 2019

Suggested change

For the purposes of the type system, `"C+panic"` is considered a totally distinct ABI string from

For the purposes of the type system, `"C panic"` is considered a totally distinct ABI string from

text/0000-simple_unwind_annotation.md Outdated

 - Start Date: 2019-08-29
 - RFC PR: [rust-lang/rfcs#0000](https:/rust-lang/rfcs/pull/0000)
 - Rust Issue: [rust-lang/rust#0000](https:/rust-lang/rust/issues/0000)
 # Summary
 [summary]: #summary
-Provides an annotation to permit functions with explicit ABI specifications
-(such as `extern "C"`) to unwind, and affirms that calls to function pointers with explicit ABI specifications may unwind.
+Provides a new ABI string `extern "C+panic"` to denote functions that use the C ABI, but may also

Member

BatmanAoD Sep 10, 2019

Suggested change

Provides a new ABI string `extern "C+panic"` to denote functions that use the C ABI, but may also

Provides a new ABI string `extern "C panic"` to denote functions that use the C ABI, but may also

text/0000-simple_unwind_annotation.md Outdated

@@ @@ -50,21 +50,23 @@ compatible with the unspecified Rust unwinding mechanism. @@
 # Guide-level explanation
 [guide-level-explanation]: #guide-level-explanation
-When used on Rust functions with non-Rust ABIs (e.g. `extern "C" fn`), the `#[unwind(allowed)]` attribute permits unwinding out of the annotated function; without the attribute, the process will automatically be terminated at the function boundary.
+Rust function definitions with the `"C+panic"` ABI string (e.g., `extern "C+panic" fn`) are

Member

BatmanAoD Sep 10, 2019

Suggested change

Rust function definitions with the `"C+panic"` ABI string (e.g., `extern "C+panic" fn`) are

Rust function definitions with the `"C panic"` ABI string (e.g., `extern "C panic" fn`) are

text/0000-simple_unwind_annotation.md Outdated

 implementations that define the behavior may require target-specific options
 for the non-Rust code, or this feature may not be supported at all.
+For the purposes of the type system, `"C+panic"` is considered a totally distinct ABI string from
+`"C"`. While there may be some circumstances where it is sensible to use an `extern "C" fn` in place
+of an `extern "C+panic" fn`, and vice-versa, this introduces questions of subtyping and variance

Member

BatmanAoD Sep 10, 2019

Suggested change

of an `extern "C+panic" fn`, and vice-versa, this introduces questions of subtyping and variance

of an `extern "C panic" fn` (or vice-versa) would be useful, this introduces questions of subtyping and variance

text/0000-simple_unwind_annotation.md Outdated Show resolved Hide resolved

BatmanAoD reviewed

View reviewed changes

text/0000-simple_unwind_annotation.md

-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
+Rust function with a non-"Rust" ABI ("`extern "ABI" fn`") specification. Under this RFC,
+functions with the `"C panic"` ABI string

Member

BatmanAoD Sep 10, 2019

Suggested change

functions with the `"C panic"` ABI string

a function with the `"C panic"` ABI string would

Author

acfoltzer Sep 10, 2019

I goofed up the grammar elsewhere in this sentence; I'll fix

BatmanAoD reviewed

View reviewed changes

text/0000-simple_unwind_annotation.md Outdated Show resolved Hide resolved

BatmanAoD reviewed

View reviewed changes

text/0000-simple_unwind_annotation.md Outdated Show resolved Hide resolved

BatmanAoD reviewed

View reviewed changes

text/0000-simple_unwind_annotation.md Outdated Show resolved Hide resolved

Member

BatmanAoD commented Sep 10, 2019

Wow, the GitHub "suggest changes" mechanism is somewhat less useful for PRs-against-PR-branches.

acfoltzer and others added 2 commits

September 10, 2019 12:42


Apply suggestions from code review

cdd2ab1

Co-Authored-By: Kyle J Strand <[email protected]>


fix up grammar

Author

acfoltzer commented Sep 10, 2019

It's a bit of a mess, particularly when working this concurrently 😂

BatmanAoD merged commit 62e9a61 into rust-lang:SimpleUnwindAnnotation

acfoltzer deleted the SimpleUnwindAnnotation branch

September 10, 2019 22:21

acfoltzer restored the SimpleUnwindAnnotation branch

September 10, 2019 22:21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment