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

Tracking issue for unwind allowed/abort #58760

Open
Mark-Simulacrum opened this Issue Feb 26, 2019 · 8 comments

Comments

Projects
None yet
6 participants
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Feb 26, 2019

See #52652 for the original discussion. There's also some additional discussion here: #48251.

The default was changed to abort-by-default in extern functions in this PR.

This is tracking the stabilization of the #[unwind(allowed)] (and #[unwind(abort)]) attributes.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Feb 26, 2019

I've nominated this for the language team because I think we may want to consider stabilizing this "immediately." These attributes have existed for a long time and I think we're fairly confident we want something like this.

@comex

This comment has been minimized.

Copy link
Contributor

comex commented Feb 26, 2019

Note that this is effectively saying that unwinding through FFI will no longer be undefined behavior in some cases; after all, there would be no point in allowing functions to be marked #[unwind(allowed)] if actually unwinding across them was necessarily undefined behavior.

The question is, which cases?

My understanding of the current implementation is that Rust-triggered unwinding behaves correctly when unwinding across C code, or across C++ code compiled with exceptions disabled, but may have issues when unwinding across C++ code that does use exceptions. That is, on platforms that support unwinding at all; WebAssembly doesn't support it unless you pass a flag to have LLVM manually emulate it, but that's just WebAssembly being broken as usual. :p I have no objection to stabilizing this subset of functionality "immediately", so 👍 on this in general.

However, I’d also like to know how hard it would be to support full interoperability between Rust and C++ unwinding – that is, allowing Rust panics to unwind across C++ code, and allowing C++ exceptions to unwind across Rust code. @alexcrichton, you seem to be the author of most of the unwinding code; can you comment on how difficult this seems from your perspective, or what the biggest obstacle might be?

One issue I know of is on Windows, where instead of using a separate personality function, Rust borrows the C++ one from msvcrt. From libpanic_unwind/seh.rs:

//! 1. The `panic` function calls the standard Windows function
//!    `_CxxThrowException` to throw a C++-like exception, triggering the
//!    unwinding process.
//! 2. All landing pads generated by the compiler use the personality function
//!    `__CxxFrameHandler3`, a function in the CRT, and the unwinding code in
//!    Windows will use this personality function to execute all cleanup code on
//!    the stack.
[..]
//! * Rust has no custom personality function, it is instead *always*
//!   `__CxxFrameHandler3`. Additionally, no extra filtering is performed, so we
//!   end up catching any C++ exceptions that happen to look like the kind we're
//!   throwing. Note that throwing an exception into Rust is undefined behavior
//!   anyway, so this should be fine.

One option would be to use a custom personality function instead of __CxxFrameHandler3, but I guess this is difficult because the implementation has some fairly complex functionality to call the right funclet; it's no more complex than what's already implemented for DWARF, but it is different and not currently implemented. Right?

Another option is to just continue treating Rust panics like C++ exceptions and fix any incompatibilities.

Looking into the implementation more…

With regard to "end up catching any C++ exceptions that happen to look like the kind we're throwing", I believe the relevant comparison is this one, which treat two _TypeDescriptor pointers as equal if (1) they are the same pointer or (2) their names are equal according to strcmp. Currently, Rust uses TypeDescriptors, one named ".PEA_K" and one named ".PA_K" of Rust panics is ".PA_K", strings which were apparently copied from the assembly output of this code because, to quote seh.rs, "I'm not actually sure what they do":

void foo() {
    uint64_t a[2] = {0, 1};
    throw a;
}

Well, .PA_K is just the mangling of unsigned __int64 *, and .PEA_K is the mangling of unsigned __int64 * __ptr64. But this could be any arbitrary string instead, which could be picked to avoid colliding with anything C++ is likely to use – though it should probably be a valid mangling in case some debugging tool tries to demangle it. For example, .?AUFooBar@@ would be the mangled name for a class FooBar; picking some suitably Rust-specific class name should be good enough.

If this is fixed:

  • Unwinding C++ exceptions across Rust should work; Rust will never try to catch them as panics.
  • Similarly, unwinding Rust panics across C++ code that tries to catch a specific type should work, as the type will never match.
  • Unwinding Rust panics across C++ code that uses catch(...) to catch anything would result in C++ catching the panic. It doesn't seem that there's any way to prevent that while still using the C++ personality. This may be undesirable, and would differ from the behavior on other platforms, but catch(...) is a bad idea in general so it's probably not a big deal. However, the panic will not be dropped properly because the _ThrowInfo used by seh.rs does not initialize pnfnUnwind (actually pmfnUnwind, i.e. "pointer to member function"), which is supposed to point to the destructor. This can also be fixed.

Centril added a commit to Centril/rust that referenced this issue Feb 27, 2019

Rollup merge of rust-lang#58762 - petrochenkov:unwind, r=Mark-Simulacrum
Mention `unwind(aborts)` in diagnostics for `#[unwind]`

Simplify input validation for `#[unwind]`, add tests

cc rust-lang#58760
r? @Mark-Simulacrum

Centril added a commit to Centril/rust that referenced this issue Feb 27, 2019

Rollup merge of rust-lang#58762 - petrochenkov:unwind, r=Mark-Simulacrum
Mention `unwind(aborts)` in diagnostics for `#[unwind]`

Simplify input validation for `#[unwind]`, add tests

cc rust-lang#58760
r? @Mark-Simulacrum

Centril added a commit to Centril/rust that referenced this issue Feb 27, 2019

Rollup merge of rust-lang#58761 - Mark-Simulacrum:add-feature-gate-un…
…wind, r=Centril

Add tracking issue for the unwind attribute

cc rust-lang#58760

Centril added a commit to Centril/rust that referenced this issue Feb 27, 2019

Rollup merge of rust-lang#58761 - Mark-Simulacrum:add-feature-gate-un…
…wind, r=Centril

Add tracking issue for the unwind attribute

cc rust-lang#58760
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 27, 2019

@alexcrichton, you seem to be the author of most of the unwinding code; can you comment on how difficult this seems from your perspective, or what the biggest obstacle might be?

From a technical perspective this is pretty feasible, but from a stabilization perspective is historically something we've never wanted to provide. We want the technical freedom to tweak unwinding as we see fit, which means it's not guaranteed to match what C++ does across every single platform.

@koute

This comment has been minimized.

Copy link
Member

koute commented Feb 27, 2019

👍

I have an implementation of very fast local stack unwinding (which we use in our internal LD_PRELOAD-based memory profiler) which uses a shadow stack and trampolines to effectively cache previously unwound stack frames, and I need to clear all of the previously set trampolines when an exception gets thrown or all hell breaks loose. This needs the ability to be able to unwind through an FFI boundary. Since the default behavior was switched to abort-by-default I had to switch to nightly to get the previous behavior; it'd be nice to get this stable again.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Feb 28, 2019

I am denominating this in favor of #58794.

@mjbshaw

This comment was marked as resolved.

Copy link
Contributor

mjbshaw commented Feb 28, 2019

I am denominating this in favor of #58760.

That's infinite recursion there... Did you mean a different issue number?

@Mark-Simulacrum

This comment was marked as resolved.

Copy link
Member Author

Mark-Simulacrum commented Feb 28, 2019

Yes, #58794.

Centril added a commit to Centril/rust that referenced this issue Mar 9, 2019

Rollup merge of rust-lang#58762 - petrochenkov:unwind, r=Mark-Simulacrum
Mention `unwind(aborts)` in diagnostics for `#[unwind]`

Simplify input validation for `#[unwind]`, add tests

cc rust-lang#58760
r? @Mark-Simulacrum

Centril added a commit to Centril/rust that referenced this issue Mar 9, 2019

Rollup merge of rust-lang#58762 - petrochenkov:unwind, r=Mark-Simulacrum
Mention `unwind(aborts)` in diagnostics for `#[unwind]`

Simplify input validation for `#[unwind]`, add tests

cc rust-lang#58760
r? @Mark-Simulacrum

Centril added a commit to Centril/rust that referenced this issue Mar 9, 2019

Rollup merge of rust-lang#58762 - petrochenkov:unwind, r=Mark-Simulacrum
Mention `unwind(aborts)` in diagnostics for `#[unwind]`

Simplify input validation for `#[unwind]`, add tests

cc rust-lang#58760
r? @Mark-Simulacrum
@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Mar 14, 2019

The question is, which cases?

IMO the only case in which we can guarantee this to work properly is if the code on the other side of the FFI is Rust compiled with the exact same toolchain.

allowing C++ exceptions to unwind across Rust code.

I'd prefer if doing that required extern "c++" { ... }.

Currently we assume that the code at the other side of extern { } and extern "C" { } is C code. This code cannot unwind and we could add LLVM nounwind attribute to it by default. The same would not be true for extern "c++" { } code, and if the Rust panic ABI differs from the C++ ABI on the platform (which probably won't be the case, but who knows), we'll need to convert panics from/to C++ when interfacing with that code unless the C++ code is nounwind (noexcept) or the extern "c++" fn Rust functions are nounwind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.