Fix Rust panics in exception::catch

Author: madsmtm

crates/objc2-exception-helper/CHANGELOG.md

@@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ### Fixed
 * Fixed the symbol name to include the correct SemVer version of the crate.
+* Fixed the ABI of `try_catch` to be `extern "C-unwind"`.
+* Clarified that `try_catch` does not catch Rust panics.
 
 
 ## 0.1.0 - 2024-06-02

crates/objc2-exception-helper/src/lib.rs

@@ -20,8 +20,9 @@ use core::ffi::c_void;
 
 type TryCatchClosure = extern "C-unwind" fn(*mut c_void);
 
-// `try_catch` is deliberately `extern "C"`, we just prevented the unwind.
-extern "C" {
+// `try_catch` is `extern "C-unwind"`, since it does not use `@catch (...)`,
+// but instead let unhandled exceptions pass through.
+extern "C-unwind" {
     /// Call the given function inside an Objective-C `@try/@catch` block.
     ///
     /// Defined in `src/try_catch.m` and compiled in `build.rs`.
@@ -37,6 +38,12 @@ extern "C" {
     /// (using mechanisms like core::intrinsics::r#try) is not an option!
     ///
     /// [manual-asm]: https://gitlab.com/objrs/objrs/-/blob/b4f6598696b3fa622e6fddce7aff281770b0a8c2/src/exception.rs
+    ///
+    ///
+    /// # Panics
+    ///
+    /// This panics / continues unwinding if the unwind is not triggered by an
+    /// Objective-C exception (i.e. it was triggered by Rust/C++/...).
     #[link_name = "objc2_exception_helper_0_1_try_catch"]
     pub fn try_catch(f: TryCatchClosure, context: *mut c_void, error: *mut *mut c_void) -> u8;
 }

crates/objc2-exception-helper/src/try_catch.m

@@ -17,7 +17,27 @@ unsigned char objc2_exception_helper_0_1_try_catch(void (*f)(void *), void *cont
     } @catch (id exception) {
         // The exception is retained while inside the `@catch` block, but is
         // not guaranteed to be so outside of it; so hence we must do it here!
+        //
+        // Code throwing an exception know that they don't hold sole access to
+        // that object any more, so it's certainly safe to retain.
         *error = objc_retain(exception);
+        // NOTE: The above `retain` can throw, so an extra landing pad will be
+        // inserted here for that case.
         return 1;
     }
+    // NOTE: @catch(...) exists, but it only works reliably in 64-bit. On
+    // 32-bit, an exception may continue past that frame (I think).
+    // https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Exceptions/Articles/Exceptions64Bit.html
+    //
+    // Given that there is no way for us to reliably catch all Rust/C++/...
+    // exceptions here, the best choice is to consistently let them continue
+    // unwinding up the stack frame.
+    //
+    // Besides, not re-throwing Rust exceptions is not currently supported,
+    // so we'd just get an `abort` if we _did_ try to handle it:
+    // https://github.com/rust-lang/rust/blob/80d0d927d5069b67cc08c0c65b48e7b6e0cdeeb5/library/std/src/panicking.rs#L51-L58
+    //
+    // In any case, this _is_ the behaviour we want, to just catch Objective-C
+    // exceptions, C++/Rust/... exceptions are better handled with
+    // `std::panic::catch_unwind`.
 }

crates/objc2/src/exception.rs

@@ -261,6 +261,18 @@ unsafe fn try_no_ret<F: FnOnce()>(closure: F) -> Result<(), Option<Retained<Exce
     let context = context.cast();
 
     let mut exception = ptr::null_mut();
+    // SAFETY: The function pointer and context are valid.
+    //
+    // The exception catching itself is sound on the Rust side, because we
+    // correctly use `extern "C-unwind"`. Objective-C does not completely
+    // specify how foreign unwinds are handled, though they do have the
+    // `@catch (...)` construct intended for catching C++ exceptions, so it is
+    // likely that they intend to support Rust panics (and it works in
+    // practice).
+    //
+    // See also:
+    // https://github.com/rust-lang/rust/pull/128321
+    // https://github.com/rust-lang/reference/pull/1226
     let success = unsafe { objc2_exception_helper::try_catch(f, context, &mut exception) };
 
     if success == 0 {
@@ -269,12 +281,8 @@ unsafe fn try_no_ret<F: FnOnce()>(closure: F) -> Result<(), Option<Retained<Exce
         // SAFETY:
         // The exception is always a valid object or NULL.
         //
-        // Since we do a retain inside `extern/exception.m`, the object has
-        // +1 retain count.
-        //
-        // Code throwing an exception know that they don't hold sole access to
-        // that object any more, so even if the type was originally mutable,
-        // it is okay to create a new `Retained` to it here.
+        // Since we do a retain in `objc2_exception_helper/src/try_catch.m`,
+        // the object has +1 retain count.
         Err(unsafe { Retained::from_raw(exception.cast()) })
     }
 }
@@ -406,4 +414,14 @@ mod tests {
         let result = catch_unwind(|| throw(obj));
         let _ = result.unwrap_err();
     }
+
+    #[test]
+    #[should_panic = "test"]
+    #[cfg_attr(
+        all(target_os = "macos", target_arch = "x86", panic = "unwind"),
+        ignore = "panic won't start on 32-bit / w. fragile runtime, it'll just abort, since the runtime uses setjmp/longjump unwinding"
+    )]
+    fn does_not_catch_panic() {
+        let _ = unsafe { catch(|| panic!("test")) };
+    }
 }