rust-lang / project-ffi-unwind

Apache License 2.0
36 stars 13 forks source link

"Foreign" exceptions and `catch_unwind`? #35

Open madsmtm opened 3 years ago

madsmtm commented 3 years ago

Objective-C have exceptions similar to C++ exceptions (not very familiar with these, maybe some of this apply there as well?), and catching them require using the objc_begin_catch and objc_end_catch helper functions, similar to Rust calling the internal __rust_panic_cleanup function as part of catch_unwind.

I'd like the ability to create a function similar to catch_unwind, but which catches Objective-C exceptions instead of Rust panics. This would be an unsafe function intended to be used on the FFI boundary just before sending an Objective-C message (equivalent to calling an "C-unwind" function).

In the objc-crate's exception handling this is currently done by letting an Objective-C compiler (like clang) generate a helper function that uses @try and @catch. This adds unnecessary overhead, and is bad for cross-compilation and all that. Using the underlying intrinsic core::intrinsics::try we can avoid the overhead in objc by effectively re-implementing most of catch_unwind, see an example implementation here: https://github.com/SSheldon/rust-objc-exception/pull/11/commits/a351a8a2e6bfbde23a397c1f7602cf4f7ae77dea.

But obviously core::intrinsics::try is an intrinsic, and not intended to be stabilized, so I'd like to discuss a way we could stabilize it, or parts of the catch_unwind implementation, to allow this use-case.

Originally posted as an internals thread.

madsmtm commented 3 years ago

An idea that would accommodate my use-case would be to add something similar to std::panicking::#try, but with the error type being generic and found from a closure:

pub unsafe fn new_try<R, E, F: FnOnce() -> R, FE: FnOnce(*mut u8) -> E>(f: F, fe: FE) -> Result<R, E> {
    // ...

    #[cold]
    unsafe fn cleanup(payload: *mut u8) -> E {
        fe(payload)
    }

    // ...
}

pub unsafe fn r#try<R, F: FnOnce() -> R>(f: F) -> Result<R, Box<dyn Any + Send>> {
    new_try(f, |payload| {
        let obj = unsafe { Box::from_raw(__rust_panic_cleanup(payload)) };
        panic_count::decrease();
        obj
    })
}
BatmanAoD commented 2 years ago

I'm sorry to be the bearer of bad news, but catch_unwind is explicitly UB for anything other than Rust panics, with no plan on our roadmap to change this; and, although it's called out in RFC-2945 as something we'd like to address, I suspect this would be a very uphill battle.

One of the requirements for this group's work has always been to avoid specifying how the exception handling mechanism works. Currently, it's implemented using the "native" exception handling mechanism in LLVM, which in theory would make it possible for catch_unwind to do what you're saying, at least for C++ (and I would hope that since Objective-C is an LLVM-compiled language, it would work for that as well). But I believe the compiler team has discussed other approaches that would be incompatible with the native mechanism.

That said, I do see how this would be useful, and I don't know for sure that specifying it would be too restrictive for the compiler implementation. I'm not sure who has the expertise to discuss this further; @Amanieu perhaps?

BatmanAoD commented 2 years ago

I also don't remember who on the compiler team had alternative ideas/hopes/designs for unwinding.

BatmanAoD commented 1 month ago

catch_unwind is no longer explicitly UB, but it does currently (...and previously) just abort the process.

madsmtm commented 1 month ago

Yeah I've followed https://github.com/rust-lang/rust/pull/128321, it's excellent that it's now specified instead of UB, that solves a lot of my concerns here, so thanks a lot for that!

Now I no longer feel like Rust needs to expose a way to handle custom panics, so I'm going to close this.

The only real remaining issue is that, as you said, that the process just aborts, which is obviously a terrible user experience and hard to debug, but that's a "quality of implementation" issue, and the best way forwards for me here would be to integrate the Objective-C(++) exception catching mechanism in Rust's libunwind itself (please correct mere here if I'm wrong?)

nbdd0121 commented 1 month ago

I think it's still desirable to be able to interact directly with foreign exceptions. Let's use this issue to keep it on radar for the WG.

BatmanAoD commented 1 month ago

In that case, I'm renaming the thread to be a little more general.

purplesyringa commented 1 week ago

I am also interested in this. I'd like to both throw and catch foreign exceptions through Rust frames, at least on a subset of platforms. I'm currently doing this in Itanium ABI EH-conformant platforms by manually invoking _Unwind_RaiseException for throwing and core::intrinsics::catch_unwind for catching. I understand that this is undocumented behavior, but it works well in practice, so getting a stable API would be useful.

BatmanAoD commented 1 week ago

@purplesyringa Thanks for adding a use-case; could you explain a little more about why you need _Unwind_RaiseException rather than throwing and catching Rust panics (possibly with panic_any if you need a payload)? Do you also need the exceptions to be caught by C++ catch?

purplesyringa commented 1 week ago

I'm working on what is effectively an alternative panic/exception implementation for Rust, with a significant focus on performance. As such, going through the default Rust machinery is counterproductive. My main problems are:

To be clear, this is not just theory, and I have seen significant performance improvements by implementing a direct Itanium interface in my usecases. I'm hoping to publish a crate soon, I can send a link here if you're interested.

BatmanAoD commented 1 week ago

Are you calling _Unwind_RaiseException via "normal" FFI (declaring with an extern block and calling that), or some other mechanism?

purplesyringa commented 1 week ago

I'm directly calling the extern "C-unwind" fn _Unwind_RaiseException, yes. You can take a look at the code here: https://github.com/iex-rs/lithium/blob/master/src/backend/itanium.rs