Skip to content
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

panic runtime and C-unwind documentation #1226

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

BatmanAoD
Copy link
Member

@BatmanAoD BatmanAoD commented May 29, 2022

Tracking issue: rust-lang/rust#74990

@BatmanAoD BatmanAoD marked this pull request as ready for review May 30, 2022 17:35
@BatmanAoD
Copy link
Member Author

BatmanAoD commented May 30, 2022

Hm... not sure how to fix the links to the newly-introduced page. Is there an index page I need to edit?

Edit: I think I found it

@ehuss ehuss added the S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository label Jun 22, 2022
src/items/functions.md Outdated Show resolved Hide resolved
behavior when unwinding out of a function.

In the table below, "Unforced foreign unwind" refers to something like a C++
exception; the table indicates the behavior when entering a Rust stackframe via
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
exception; the table indicates the behavior when entering a Rust stackframe via
exception; the table indicates the effect of entering a Rust stackframe via

This term 'behavior' suggested to me that something would happen right away, whereas it seems like the table is actually describing what happens if a panic occurs later on?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah... I meant that the table describes the behavior when the unwind itself "enters" a Rust stackframe. That's probably not the clearest way to phrase 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.

The RFC wording is:

the behavior of an unwinding operation reaching each type of ABI boundary (function declaration or definition).

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'm replacing the original change with the RFC text; please let me know if it's okay now.

src/behavior-considered-undefined.md Outdated Show resolved Hide resolved
Comment on lines 219 to 224
| panic runtime | ABI | `panic`-unwind | Unforced foreign unwind |
| -------------- | ------------ | ------------------------------------- | ----------------------- |
| `panic=unwind` | `"C-unwind"` | unwind | unwind |
| `panic=unwind` | `"C"` | abort | UB |
| `panic=abort` | `"C-unwind"` | `panic!` aborts | abort |
| `panic=abort` | `"C"` | `panic!` aborts (no unwinding occurs) | UB |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| panic runtime | ABI | `panic`-unwind | Unforced foreign unwind |
| -------------- | ------------ | ------------------------------------- | ----------------------- |
| `panic=unwind` | `"C-unwind"` | unwind | unwind |
| `panic=unwind` | `"C"` | abort | UB |
| `panic=abort` | `"C-unwind"` | `panic!` aborts | abort |
| `panic=abort` | `"C"` | `panic!` aborts (no unwinding occurs) | UB |
| panic runtime | ABI | `panic`-unwind | Unforced foreign unwind |
| -------------- | ------------ | ------------------------------------- | ----------------------- |
| `panic=unwind` | `"C-unwind"` | unwind | unwind |
| `panic=unwind` | `"C"` | abort if unwinding reaches the function | UB if unwinding reaches the function |
| `panic=abort` | `"C-unwind"` | aborts immediately (no unwinding occurs) | abort if unwinding reaches the function |
| `panic=abort` | `"C"` | aborts immediately (no unwinding occurs) | UB if unwinding reaches the function |

I found this a bit confusing. I believe there are subtle differences in terms of where the aborts occur and so forth. I have tried to clarify above, but I think it may be worth further clarifying.

It may also be worth adding some (perhaps non-normative) discussion of implementation:

  • When compiling a function F with panic=unwind and extern "C", the compiler inserts unwinding guards for Rust panics that trigger an abort when unwinding reaches F.

I am also be misunderstanding what's going on. I was a bit surprised to see "UB" for unforced-foreign-unwind with C=unwind. I guess that this table is combining two scenarios:

  • what happens when you call a C++ function declared as extern "C", and it unwinds (UB, we haven't compiled any guards)
  • what happens when an extern "C" Rust function invokes some C++ function that throws (probably, in practice, an abort, but perhaps we have simplified to call it UB?)

Is that right?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's only UB for a foreign function declared as extern "C" to unwind.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nbdd0121 what happens when an extern "C" Rust function unwinds? I believe we insert an abort guard, but this table doesn't clarify that, right? Or maybe I don't understand what it's trying to convey. I'm imagining a scenario like

extern "C-unwind" fn throws();

extern "C" fn rust_fn() {
    throws(); // unwinds
}

In this case, I presume you get an abort -- and I think we guarantee that? But the way I read this table, it would be listed as UB.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm....I don't know if the panic abort guard would currently catch and abort in that case, or if it relies on the personality function to only abort on true Rust panics. I agree that the behavior in the table as-written is UB.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nbdd0121 what happens when an extern "C" Rust function unwinds? I believe we insert an abort guard, but this table doesn't clarify that, right? Or maybe I don't understand what it's trying to convey. I'm imagining a scenario like

extern "C-unwind" fn throws();

extern "C" fn rust_fn() {
    throws(); // unwinds
}

In this case, I presume you get an abort -- and I think we guarantee that? But the way I read this table, it would be listed as UB.

Unwinding out from extern "C" functions (defined in either Rust or foreign language) is UB.
In the case you listed, we insert guard to prevent unwinding from actually leaving a Rust extern "C" functions, therefore the function does not unwind, so UB is prevented; in this case we never unwinds out from a extern "C" Rust functions.

If you define a extern "C-unwind" Rust function and transmute it to extern "C" and then call it, it's not UB if unwinding does not happen, and it's UB if unwinding happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nikomatsakis With the change to the verbiage above, explaining that the table entries are specifically describing behavior at function boundaries, do you still want to make a change here?

Copy link

Choose a reason for hiding this comment

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

Please check whether the notes I suggested to add under the table are correct.

src/linkage.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
@BatmanAoD
Copy link
Member Author

Sorry for the delay; I think I've addressed all comments.

@BatmanAoD
Copy link
Member Author

@tmandry @nikomatsakis I'm not sure you saw my comments & changes last week, but I think this is ready for re-review.

@nbdd0121
Copy link
Contributor

Could you squash the commits?

@BatmanAoD
Copy link
Member Author

@nbdd0121 Can that be done on merge? I've heard that GitHub sometimes has trouble with PR branches that receive force-pushes.

The choice of ABI, together with the [panic mode][panic-modes], determines the
behavior when unwinding out of a function.

In the table below, "Foreign unwind (unforced)" refers to something like a C++
Copy link
Member

Choose a reason for hiding this comment

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

It feels a little strange to call out "Foreign unwind (unforced)" but make no mention of presumably forced unwinding as a distinct section. Is that UB? How does one determine whether unwinding is forced or unforced?

Copy link
Member

Choose a reason for hiding this comment

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

Forced unwinding happens on for example pthread_cancel. It is like unforced unwinding except that you can't catch it and depending in the platform cleanup may or may not happen. If I recall correctly a forced unwind passing through a rust stack frame which has locals to drop on unwinding is UB, but passing through a rust stack frame without any locals to drop hasn't been decided as UB or not yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yeah, this is a bit of an oddity; the phrasing & column header are from RFC 2945, but since we're not actually specifying behavior for forced unwind yet, I forgot to include an explanation.

Forced unwinding is pthread_cancel in glibc and longjmp in Windows. I'm not aware of any other uses. So for the purpose of defining language behavior, we effectively want to say "here are the circumstances in which phtread_cancel and longjmp are safe on any platform", and that will imply the behavior of forced unwind on platforms where it's used.

I'm not sure whether it's preferable to explain here what forced unwinding is or just remove "unforced" from the text for now. I think the latter is probably okay, since the explanation of "something like a C++ exception" already implies that this isn't talking about forced unwinding.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should specify as precisely as possible the exceptions we are including (and aren't including) - "like C++" doesn't feel like the right level of detail here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Mark-Simulacrum @bjorn3 @Amanieu @nbdd0121

Do you have any specific suggestions for how to define "native unforced unwinding" here, in an appropriate level of detail? I believe the salient points to capture are:

  • It's determined by the platform (compiler backend + OS + libc implementation if applicable + architecture + ...?)
  • It specifically doesn't include "forced unwinding", terminology that we didn't invent but which...seems kind of hard to find a good reference for? Everything I know about forced unwinding was told to me as part of this project, I think mostly by one of you.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine to just say that this is defined per target and is usually whatever C++ uses for unwinding on that target. And we can just omit any mention of forced unwinding.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Mark-Simulacrum Are you okay with Amanieu's suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

Defined per target is fine, but I feel like we would still want at least some targets to have actual definitions. That definition can be "we match C++ behavior".

Copy link
Member Author

Choose a reason for hiding this comment

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

@Mark-Simulacrum @Amanieu How about:

Native unwinding is defined per-target. On targets that support throwing and catching C++ exceptions, it refers to the mechanism used to implement this feature.

src/linkage.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/panic.md Outdated Show resolved Hide resolved
src/panic.md Outdated Show resolved Hide resolved
src/panic.md Outdated Show resolved Hide resolved
src/panic.md Outdated Show resolved Hide resolved
src/panic.md Outdated Show resolved Hide resolved
src/panic.md Outdated Show resolved Hide resolved
src/panic.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
@BatmanAoD
Copy link
Member Author

I think I've resolved all open questions and concerns. Is there anything else needed from me at the moment?

| `panic=abort` | `"C"` | `panic!` aborts (no unwinding occurs) | [Undefined Behavior] |

[panic-modes]: ../panic.md#panic-runtimes
[Undefined Behavior]: ../behavior-considered-undefined.md

Copy link

Choose a reason for hiding this comment

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

What happens if you unwind through a C++ stack frame compiled with -fno-exceptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Very much UB, just as it would be to throw a C++ exception into stack frames compiled that way.

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've added a bullet point to the "behavior considered undefined" list to that effect, though I'm not sure the wording is ideal.

@nbdd0121
Copy link
Contributor

This really needs rebasing now.

BatmanAoD and others added 11 commits August 26, 2023 12:49
PR suggestion: `panic!` with `panic=abort` doesn't care what the ABI is

Co-authored-by: Tyler Mandry <tmandry@gmail.com>
PR suggestion: Take focus off of "unforced"

Co-authored-by: Tyler Mandry <tmandry@gmail.com>
PR suggestion: not all ABIs have `-unwind`

Co-authored-by: Tyler Mandry <tmandry@gmail.com>
Links and 'note's

Co-authored-by: Eric Huss <eric@huss.org>
Co-authored-by: Eric Huss <eric@huss.org>
@BatmanAoD
Copy link
Member Author

@nbdd0121 Done!

@BatmanAoD
Copy link
Member Author

@tmandry two changes since your review:

  • I added the new ABIs to items/external-blocks
  • I added that catching an exception or unwind from the "wrong" language is UB

@BatmanAoD
Copy link
Member Author

@tmandry @ehuss can this be merged, since the partial stabilization was a while ago now?

* `"aapcs-unwind"`
* `"win64-unwind"`
* `"sysv64-unwind"`
* `"system-unwind"`
Copy link

Choose a reason for hiding this comment

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

Suggested change
* `"system-unwind"`

"system-unwind" is not a "platform-specific ABI string" in this sense.

Comment on lines +219 to +220
`stdcall` or any other ABI supported by the language
implementation.
Copy link

Choose a reason for hiding this comment

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

Suggested change
`stdcall` or any other ABI supported by the language
implementation.
`stdcall` or any other ABI (other than `Rust`) supported by
the language implementation.

| -------------- | ------------ | ------------------------------------- | ----------------------- |
| `panic=unwind` | `"C-unwind"` | unwind | unwind |
| `panic=unwind` | `"C"` | abort | [Undefined Behavior] |
| `panic=abort` | `"C-unwind"` | `panic!` aborts (no unwinding occurs) | abort |
Copy link

@daira daira May 28, 2024

Choose a reason for hiding this comment

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

For a foreign unwind (unforced) in this case: when does it abort? It can only abort when the unwind reaches a Rust frame, right?

That should be specified; it matters that side effects in the relevant foreign code (e.g. in foreign-language catch handlers, destructors, etc.) will occur before the abort.

Suggested change
| `panic=abort` | `"C-unwind"` | `panic!` aborts (no unwinding occurs) | abort |
| `panic=abort` | `"C-unwind"` | `panic!` aborts (no unwinding occurs) | abort (*2) |

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, I don't think it can be reliably specified "when" a abort occurs, but I need to reread the comments here.

Certainly if an unwind is caught before it would encounter a Rust frame, Rust is unaffected. I can make that explicit in the docs.

| `panic=unwind` | `"C"` | abort | [Undefined Behavior] |
| `panic=abort` | `"C-unwind"` | `panic!` aborts (no unwinding occurs) | abort |
| `panic=abort` | `"C"` | `panic!` aborts (no unwinding occurs) | [Undefined Behavior] |

Copy link

@daira daira May 28, 2024

Choose a reason for hiding this comment

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

Suggested change
(*1) At the point when a panic would otherwise propagate from a function
*defined in Rust* and declared using `extern "C"` (or another non-unwinding ABI),
an abort is guaranteed even with `panic=unwind`.
(*2) The abort only occurs if and when the unwind reaches a Rust frame. That is, side
effects resulting from execution of foreign code before the unwinding reaches a Rust
frame (e.g. in foreign-language catch constructs or destructors) will still occur.
It has been [proposed] that the cases marked [Undefined Behavior] should always cause
an abort in a future version of Rust. This proposal has not so far been approved or
implemented, therefore correct code should not rely on an abort in these cases (except
as noted above for (*1)).
Note that an unwind that occurs entirely within the foreign code without reaching a
Rust frame, is not [Undefined Behavior] just because the function was called via a
function declaration or pointer declared with a non-unwinding ABI. For example, the
`"C"` or `"system"` ABIs can safely be used to declare foreign functions that only use
exceptions internally, if all other requirements of those ABIs are met.
[proposed]: https://github.com/rust-lang/rust/issues/115285

For (*1), I am relying on rust-lang/rust#52652 (comment) .

For (*2), an alternative would be to make the point at which the abort occurs for panic=abort unspecified, except that it must occur at the latest when the unwind reaches a Rust frame. But I don't think it is necessary to weaken the spec in that way given the intended implementation.

another ABI that permits unwinding) from a runtime that does not support
unwinding, such as code compiled with GCC or Clang using `-fno-exceptions`
* Catching a Rust `panic` in non Rust code (for instance `catch (...)` in C++)
* Catching a non-Rust unwind (such as a C++ exception) with `catch_unwind`
Copy link

@daira daira May 28, 2024

Choose a reason for hiding this comment

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

Does this have to be UB? It would be safer to wrap the exception from the non-Rust unwind when it reaches Rust, so that catch_unwind just works.

After all, the point of catch_unwind is to catch all possible unwinding, and the main point of using unwinding ABIs is to make unwinding from foreign code into Rust safe. So if the combination of these features is not safe, then it arguably creates a new footgun.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, for now, it does need to be undefined, and it may be that we need to introduce a new catch mechanism that is more universal (this bothers me, too). There are implementation complexities I can't recall at the moment. I can take a look later in our Zulip channel and/or project-group notes to recall the specifics.

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 agree that "C-unwind" and catch_unwind sound like they should work together, and in conjunction they're a footgun. 😔

Copy link

@daira daira May 29, 2024

Choose a reason for hiding this comment

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

What is it about how catch_unwind is implemented that makes this undefined? Note that catch_unwind doesn't allow you to get the "thrown" object associated with the unwind, and so naively I would have thought that it doesn't matter what language the unwind originates in; we're not trying to interpret an object thrown from another language.

I don't know whether the "thrown" object is used to implement the payload from a panic, but in any case, there seems to be specific provision for interoperating between different languages in the Itanium C++ exception ABI (which I understand was adopted by other platforms). So it should be possible for catch_unwind to distinguish an unwind originating from another language from a Rust panic, if it needs to.

(But yes, it should be documented as UB for the time being.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the existing Zulip thread: https://rust-lang.zulipchat.com/#narrow/stream/210922-project-ffi-unwind/topic/Allowing.20catch_unwind.20for.20foreign.20exceptions/near/284872034

Complications, based on reviewing that thread:

  • Catching a C++ exception is, for some reason I don't quite understand, UB on Itanium unless a specific C++ standard library function is called.
  • catch_unwind does actually allow you to get the "thrown" object (it's the error variant of the result), but presumably we could do something to prevent users from actually downcasting from &dyn Any to get the foreign exception object. (And yes, even with a pure Rust panic, you can throw objects other than &str, using std::panic::panic_any.)

| panic runtime | ABI | `panic`-unwind | Foreign unwind (unforced) |
| -------------- | ------------ | ------------------------------------- | ----------------------- |
| `panic=unwind` | `"C-unwind"` | unwind | unwind |
| `panic=unwind` | `"C"` | abort | [Undefined Behavior] |
Copy link

@daira daira May 28, 2024

Choose a reason for hiding this comment

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

Suggested change
| `panic=unwind` | `"C"` | abort | [Undefined Behavior] |
| `panic=unwind` | `"C"` | abort | [Undefined Behavior] (*1) |

Co-authored-by: Daira-Emma Hopwood <daira@jacaranda.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet