Merge pull request #71 from madsmtm/c-unwind-feature

madsmtm · web-flow · commit 0ed12c418586 · 2022-06-24T23:05:05.000+02:00
Make calling `throw` no longer UB by using "C-unwind" behind unstable feature
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -154,7 +154,7 @@ jobs:
       # Use --no-fail-fast, except with dinghy
       TESTARGS: ${{ matrix.dinghy && ' ' || '--no-fail-fast' }} ${{ matrix.test-args }}
       FEATURES: ${{ matrix.features || 'malloc,block,exception,catch_all,verify_message' }}
-      UNSTABLE_FEATURES: ${{ matrix.unstable-features || 'unstable-autoreleasesafe' }}
+      UNSTABLE_FEATURES: ${{ matrix.unstable-features || 'unstable-autoreleasesafe,unstable-c-unwind' }}
 
     runs-on: ${{ matrix.os }}
 
diff --git a/objc-sys/CHANGELOG.md b/objc-sys/CHANGELOG.md
@@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ## Unreleased - YYYY-MM-DD
 
+### Added
+* Added `unstable-c-unwind` feature.
+
 
 ## 0.2.0-beta.0 - 2022-06-13
 
diff --git a/objc-sys/Cargo.toml b/objc-sys/Cargo.toml
@@ -49,6 +49,9 @@ unstable-winobjc = ["gnustep-1-8"]
 # Link to ObjFW
 unstable-objfw = []
 
+# Use nightly c_unwind feature
+unstable-c-unwind = []
+
 # Private
 unstable-exception = ["cc"]
 unstable-docsrs = []
diff --git a/objc-sys/src/class.rs b/objc-sys/src/class.rs
@@ -23,6 +23,21 @@ pub struct objc_class {
 /// difference.
 type ivar_layout_type = u8;
 
+// May call `resolveClassMethod:` or `resolveInstanceMethod:`.
+extern_c_unwind! {
+    #[cfg(not(objfw))]
+    pub fn class_getClassMethod(
+        cls: *const objc_class,
+        name: *const objc_selector,
+    ) -> *const objc_method;
+    #[cfg(not(objfw))] // Available in newer versions
+    pub fn class_getInstanceMethod(
+        cls: *const objc_class,
+        name: *const objc_selector,
+    ) -> *const objc_method;
+}
+
+// TODO: Hooks registered with objc_setHook_getClass may be allowed to unwind?
 extern_c! {
     pub fn objc_getClass(name: *const c_char) -> *const objc_class;
     pub fn objc_getRequiredClass(name: *const c_char) -> *const objc_class;
@@ -97,19 +112,9 @@ extern_c! {
     #[cfg(not(objfw))]
     pub fn class_createInstance(cls: *const objc_class, extra_bytes: usize) -> *mut objc_object;
     #[cfg(not(objfw))]
-    pub fn class_getClassMethod(
-        cls: *const objc_class,
-        name: *const objc_selector,
-    ) -> *const objc_method;
-    #[cfg(not(objfw))]
     pub fn class_getClassVariable(cls: *const objc_class, name: *const c_char) -> *const objc_ivar;
     #[cfg(apple)]
     pub fn class_getImageName(cls: *const objc_class) -> *const c_char;
-    #[cfg(not(objfw))] // Available in newer versions
-    pub fn class_getInstanceMethod(
-        cls: *const objc_class,
-        name: *const objc_selector,
-    ) -> *const objc_method;
     pub fn class_getInstanceSize(cls: *const objc_class) -> usize;
     #[cfg(not(objfw))]
     pub fn class_getInstanceVariable(
@@ -140,6 +145,7 @@ extern_c! {
         attributes: *const objc_property_attribute_t,
         attributes_len: c_uint,
     );
+    // TODO: Verify unwinding
     pub fn class_respondsToSelector(cls: *const objc_class, sel: *const objc_selector) -> BOOL;
     #[cfg(not(objfw))]
     pub fn class_setIvarLayout(cls: *mut objc_class, layout: *const ivar_layout_type);
diff --git a/objc-sys/src/exception.rs b/objc-sys/src/exception.rs
@@ -38,18 +38,29 @@ pub type objc_uncaught_exception_handler =
 pub type objc_exception_handler =
     unsafe extern "C" fn(unused: *mut objc_object, context: *mut c_void);
 
-extern_c! {
-    #[cfg(any(gnustep, apple_new))]
-    pub fn objc_begin_catch(exc_buf: *mut c_void) -> *mut objc_object;
-    #[cfg(any(gnustep, apple_new))]
-    pub fn objc_end_catch();
+#[cfg(all(feature = "unstable-exception", not(feature = "unstable-c-unwind")))]
+type TryCatchClosure = extern "C" fn(*mut c_void);
+#[cfg(all(feature = "unstable-exception", feature = "unstable-c-unwind"))]
+type TryCatchClosure = extern "C-unwind" fn(*mut c_void);
+
+extern_c_unwind! {
     /// See [`objc-exception.h`].
     ///
     /// [`objc-exception.h`]: https://github.com/apple-oss-distributions/objc4/blob/objc4-818.2/runtime/objc-exception.h
     pub fn objc_exception_throw(exception: *mut objc_object) -> !;
     #[cfg(apple_new)]
     pub fn objc_exception_rethrow() -> !;
 
+    #[cfg(gnustep)]
+    pub fn objc_exception_rethrow(exc_buf: *mut c_void) -> !;
+}
+
+extern_c! {
+    #[cfg(any(gnustep, apple_new))]
+    pub fn objc_begin_catch(exc_buf: *mut c_void) -> *mut objc_object;
+    #[cfg(any(gnustep, apple_new))]
+    pub fn objc_end_catch();
+
     #[cfg(apple_old)]
     pub fn objc_exception_try_enter(exception_data: *const c_void);
 
@@ -61,9 +72,6 @@ extern_c! {
     // objc_exception_get_functions
     // objc_exception_set_functions
 
-    #[cfg(gnustep)]
-    pub fn objc_exception_rethrow(exc_buf: *mut c_void) -> !;
-
     #[cfg(apple_new)]
     pub fn objc_setExceptionMatcher(f: objc_exception_matcher) -> objc_exception_matcher;
     #[cfg(apple_new)]
@@ -105,7 +113,7 @@ extern_c! {
     /// [manual-asm]: https://gitlab.com/objrs/objrs/-/blob/b4f6598696b3fa622e6fddce7aff281770b0a8c2/src/exception.rs
     #[cfg(feature = "unstable-exception")]
     pub fn rust_objc_sys_0_2_try_catch_exception(
-        f: extern "C" fn(*mut c_void),
+        f: TryCatchClosure,
         context: *mut c_void,
         error: *mut *mut objc_object,
     ) -> c_uchar;
diff --git a/objc-sys/src/lib.rs b/objc-sys/src/lib.rs
@@ -25,9 +25,7 @@
 #![allow(non_upper_case_globals)]
 #![allow(non_snake_case)]
 #![doc(html_root_url = "https://docs.rs/objc-sys/0.2.0-beta.0")]
-
-// TODO: Replace `extern "C"` with `extern "C-unwind"` where applicable.
-// See https://rust-lang.github.io/rfcs/2945-c-unwind-abi.html.
+#![cfg_attr(feature = "unstable-c-unwind", feature(c_unwind))]
 
 // TODO: Remove this and add "no-std" category to Cargo.toml
 // Requires a better solution for C-types in `no_std` crates.
@@ -51,6 +49,7 @@ macro_rules! generate_linking_tests {
             $(#[$m:meta])*
             $v:vis fn $name:ident($($a:ident: $t:ty),* $(,)?) $(-> $r:ty)?;
         )+}
+        mod $test_name:ident;
     } => {
         $(#[$extern_m])*
         extern $abi {$(
@@ -61,10 +60,9 @@ macro_rules! generate_linking_tests {
         $(#[$extern_m])*
         #[allow(deprecated)]
         #[cfg(test)]
-        mod test_linkable {
+        mod $test_name {
             #[allow(unused)]
             use super::*;
-            use std::println;
 
             $(
                 $(#[$m])*
@@ -73,8 +71,12 @@ macro_rules! generate_linking_tests {
                     // Get function pointer to make the linker require the
                     // symbol to be available.
                     let f: unsafe extern $abi fn($($t),*) $(-> $r)? = crate::$name;
+                    // Workaround for https://github.com/rust-lang/rust/pull/92964
+                    #[cfg(feature = "unstable-c-unwind")]
+                    #[allow(clippy::useless_transmute)]
+                    let f: unsafe extern "C" fn() = unsafe { core::mem::transmute(f) };
                     // Execute side-effect to ensure it is not optimized away.
-                    println!("{:p}", f);
+                    std::println!("{:p}", f);
                 }
             )+
         }
@@ -95,6 +97,40 @@ macro_rules! extern_c {
                 $(#[$m])*
                 $v fn $name($($a: $t),*) $(-> $r)?;
             )+}
+            mod test_linkable;
+        }
+    };
+}
+
+// A lot of places may call `+initialize`, but the runtime guards those calls
+// with `@try/@catch` blocks already, so we don't need to mark every function
+// "C-unwind", only certain ones!
+macro_rules! extern_c_unwind {
+    {
+        $(#![$extern_m:meta])*
+        $(
+            $(#[$m:meta])*
+            $v:vis fn $name:ident($($a:ident: $t:ty),* $(,)?) $(-> $r:ty)?;
+        )+
+    } => {
+        #[cfg(not(feature = "unstable-c-unwind"))]
+        generate_linking_tests! {
+            $(#[$extern_m])*
+            extern "C" {$(
+                $(#[$m])*
+                $v fn $name($($a: $t),*) $(-> $r)?;
+            )+}
+            mod test_linkable_unwind;
+        }
+
+        #[cfg(feature = "unstable-c-unwind")]
+        generate_linking_tests! {
+            $(#[$extern_m])*
+            extern "C-unwind" {$(
+                $(#[$m])*
+                $v fn $name($($a: $t),*) $(-> $r)?;
+            )+}
+            mod test_linkable_unwind;
         }
     };
 }
diff --git a/objc-sys/src/message.rs b/objc-sys/src/message.rs
@@ -20,7 +20,12 @@ pub struct objc_super {
     pub super_class: *const objc_class,
 }
 
-extern_c! {
+// All message sending functions should use "C-unwind"!
+//
+// Note that lookup functions won't throw exceptions themselves, but they can
+// call hooks, `resolveClassMethod:` and `resolveInstanceMethod:`, so we have
+// to make those "C-unwind" as well!
+extern_c_unwind! {
     #[cfg(any(gnustep, objfw))]
     pub fn objc_msg_lookup(receiver: *mut objc_object, sel: *const objc_selector) -> IMP;
     #[cfg(objfw)]
diff --git a/objc-sys/src/protocol.rs b/objc-sys/src/protocol.rs
@@ -30,10 +30,12 @@ extern_c! {
     #[cfg(not(objfw))]
     pub fn objc_registerProtocol(proto: *mut objc_protocol);
 
+    // TODO: Verify unwinding
     pub fn protocol_conformsToProtocol(
         proto: *const objc_protocol,
         other: *const objc_protocol,
     ) -> BOOL;
+    // TODO: Verify unwinding
     pub fn protocol_isEqual(proto: *const objc_protocol, other: *const objc_protocol) -> BOOL;
     pub fn protocol_getName(proto: *const objc_protocol) -> *const c_char;
 
diff --git a/objc-sys/src/rc.rs b/objc-sys/src/rc.rs
@@ -14,7 +14,9 @@ use core::ffi::c_void;
 
 use crate::objc_object;
 
-extern_c! {
+// All of these very rarely unwind, but may if the user defined methods
+// `retain`, `release`, `autorelease` or `dealloc` do.
+extern_c_unwind! {
     // Autoreleasepool
     // ObjFW: Defined in `autorelease.h`, not available with libobjfw-rt!
 
diff --git a/objc-sys/src/various.rs b/objc-sys/src/various.rs
@@ -16,10 +16,15 @@ pub struct objc_ivar {
     _p: OpaqueData,
 }
 
+#[cfg(not(feature = "unstable-c-unwind"))]
+type InnerImp = unsafe extern "C" fn();
+#[cfg(feature = "unstable-c-unwind")]
+type InnerImp = unsafe extern "C-unwind" fn();
+
 /// A nullable pointer to the start of a method implementation.
 ///
 /// Not all APIs are guaranteed to take NULL values; read the docs!
-pub type IMP = Option<unsafe extern "C" fn()>;
+pub type IMP = Option<InnerImp>;
 
 // /// Not available on macOS x86.
 // ///
@@ -35,6 +40,12 @@ pub type IMP = Option<unsafe extern "C" fn()>;
 // pub type objc_hook_lazyClassNamer =
 //     unsafe extern "C" fn(cls: *const crate::objc_class) -> *const c_char;
 
+extern_c_unwind! {
+    // Instead of being able to change this, it's a weak symbol on GNUStep.
+    #[cfg(any(apple, objfw))]
+    pub fn objc_enumerationMutation(obj: *mut objc_object);
+}
+
 extern_c! {
     #[cfg(not(objfw))]
     pub fn imp_getBlock(imp: IMP) -> *mut objc_object;
@@ -58,9 +69,6 @@ extern_c! {
     #[cfg(apple)]
     pub fn objc_copyImageNames(out_len: *mut c_uint) -> *mut *const c_char;
 
-    // Instead of being able to change this, it's a weak symbol on GNUStep.
-    #[cfg(any(apple, objfw))]
-    pub fn objc_enumerationMutation(obj: *mut objc_object);
     #[cfg(any(apple, objfw))]
     pub fn objc_setEnumerationMutationHandler(
         handler: Option<unsafe extern "C" fn(obj: *mut objc_object)>,
diff --git a/objc2-encode/Cargo.toml b/objc2-encode/Cargo.toml
@@ -26,6 +26,9 @@ default = ["std"]
 std = ["alloc"]
 alloc = []
 
+# Enables support for the nightly c_unwind feature
+unstable-c-unwind = []
+
 [package.metadata.docs.rs]
 default-target = "x86_64-apple-darwin"
 
diff --git a/objc2-encode/src/encode.rs b/objc2-encode/src/encode.rs
@@ -455,18 +455,23 @@ macro_rules! encode_fn_pointer_impl {
             const ENCODING_REF: Encoding<'static> = Encoding::Pointer(&Self::ENCODING);
         }
     };
-    ($($Arg: ident),+) => {
+    (# $abi:literal; $($Arg: ident),+) => {
         // Normal functions
-        encode_fn_pointer_impl!(@ extern "C" fn($($Arg),+) -> Ret, $($Arg),+ );
-        encode_fn_pointer_impl!(@ unsafe extern "C" fn($($Arg),+) -> Ret, $($Arg),+ );
+        encode_fn_pointer_impl!(@ extern $abi fn($($Arg),+) -> Ret, $($Arg),+ );
+        encode_fn_pointer_impl!(@ unsafe extern $abi fn($($Arg),+) -> Ret, $($Arg),+ );
         // Variadic functions
-        encode_fn_pointer_impl!(@ extern "C" fn($($Arg),+ , ...) -> Ret, $($Arg),+ );
-        encode_fn_pointer_impl!(@ unsafe extern "C" fn($($Arg),+ , ...) -> Ret, $($Arg),+ );
+        encode_fn_pointer_impl!(@ extern $abi fn($($Arg),+ , ...) -> Ret, $($Arg),+ );
+        encode_fn_pointer_impl!(@ unsafe extern $abi fn($($Arg),+ , ...) -> Ret, $($Arg),+ );
     };
-    () => {
+    (# $abi:literal; ) => {
         // No variadic functions with 0 parameters
-        encode_fn_pointer_impl!(@ extern "C" fn() -> Ret, );
-        encode_fn_pointer_impl!(@ unsafe extern "C" fn() -> Ret, );
+        encode_fn_pointer_impl!(@ extern $abi fn() -> Ret, );
+        encode_fn_pointer_impl!(@ unsafe extern $abi fn() -> Ret, );
+    };
+    ($($Arg: ident),*) => {
+        encode_fn_pointer_impl!(# "C"; $($Arg),*);
+        #[cfg(feature = "unstable-c-unwind")]
+        encode_fn_pointer_impl!(# "C-unwind"; $($Arg),*);
     };
 }
 
@@ -607,6 +612,11 @@ mod tests {
             <Option<unsafe extern "C" fn()>>::ENCODING,
             Encoding::Pointer(&Encoding::Unknown)
         );
+        #[cfg(feature = "unstable-c-unwind")]
+        assert_eq!(
+            <extern "C-unwind" fn()>::ENCODING,
+            Encoding::Pointer(&Encoding::Unknown)
+        );
     }
 
     #[test]
diff --git a/objc2-encode/src/lib.rs b/objc2-encode/src/lib.rs
@@ -94,6 +94,7 @@
 #![warn(clippy::ptr_as_ptr)]
 // Update in Cargo.toml as well.
 #![doc(html_root_url = "https://docs.rs/objc2-encode/2.0.0-pre.0")]
+#![cfg_attr(feature = "unstable-c-unwind", feature(c_unwind))]
 
 #[cfg(doctest)]
 #[doc = include_str!("../README.md")]
diff --git a/objc2-foundation/src/array.rs b/objc2-foundation/src/array.rs
@@ -302,6 +302,7 @@ impl<T: Message, O: Ownership> NSMutableArray<T, O> {
 
     #[doc(alias = "sortUsingFunction:context:")]
     pub fn sort_by<F: FnMut(&T, &T) -> Ordering>(&mut self, compare: F) {
+        // TODO: "C-unwind"
         extern "C" fn compare_with_closure<U, F: FnMut(&U, &U) -> Ordering>(
             obj1: &U,
             obj2: &U,
diff --git a/objc2-foundation/src/geometry.rs b/objc2-foundation/src/geometry.rs
@@ -315,6 +315,7 @@ mod tests {
     fn test_partial_eq() {
         use objc2::runtime::Bool;
 
+        // Note: No need to use "C-unwind"
         extern "C" {
             fn NSEqualPoints(a: NSPoint, b: NSPoint) -> Bool;
             fn NSEqualSizes(a: NSSize, b: NSSize) -> Bool;
diff --git a/objc2/CHANGELOG.md b/objc2/CHANGELOG.md
@@ -28,11 +28,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 * Added the `"unstable-static-sel"` and `"unstable-static-sel-inlined"`
   feature flags to make the `sel!` macro (and by extension, the `msg_send!`
   macros) faster.
+* Added `"unstable-c-unwind"` feature.
 
 ### Changed
 * **BREAKING:** `Sel` is now required to be non-null, which means that you
   have to ensure that any selectors you receive from method calls are
   non-null before using them.
+* **BREAKING**: `ClassBuilder::root` is now generic over the function pointer,
+  meaning you will have to coerce initializer functions to pointers like in
+  `ClassBuilder::add_method` before you can use it.
 
 ### Removed
 * **BREAKING:** Removed the `Sel::from_ptr` and `Sel::as_ptr` methods.
diff --git a/objc2/Cargo.toml b/objc2/Cargo.toml
diff --git a/objc2/src/declare.rs b/objc2/src/declare.rs
diff --git a/objc2/src/exception.rs b/objc2/src/exception.rs
diff --git a/objc2/src/lib.rs b/objc2/src/lib.rs
diff --git a/objc2/src/message/mod.rs b/objc2/src/message/mod.rs
diff --git a/objc2/src/runtime.rs b/objc2/src/runtime.rs
diff --git a/objc2/src/test_utils.rs b/objc2/src/test_utils.rs
