Merge pull request #469 from madsmtm/assembly-improvements

Assembly improvements

Author: madsmtm

Cargo.lock

@@ -1,11 +1,13 @@
 use core::ptr;
+use core::str;
 use core::sync::atomic::{AtomicPtr, Ordering};
+use std::ffi::CStr;
+use std::os::raw::c_char;
 
 use crate::ffi;
 use crate::runtime::{AnyClass, Sel};
 
 /// Allows storing a [`Sel`] in a static and lazily loading it.
-#[doc(hidden)]
 pub struct CachedSel {
     ptr: AtomicPtr<ffi::objc_selector>,
 }
@@ -18,30 +20,38 @@ impl CachedSel {
         }
     }
 
+    // Mark as cold since this should only ever be called once (or maybe twice
+    // if running on multiple threads).
+    #[cold]
+    unsafe fn fetch(&self, name: *const c_char) -> Sel {
+        // The panic inside `Sel::register_unchecked` is unfortunate, but
+        // strict correctness is more important than speed
+
+        // SAFETY: Input is a non-null, NUL-terminated C-string pointer.
+        //
+        // We know this, because we construct it in `sel!` ourselves
+        let sel = unsafe { Sel::register_unchecked(name) };
+        self.ptr
+            .store(sel.as_ptr() as *mut ffi::objc_selector, Ordering::Relaxed);
+        sel
+    }
+
     /// Returns the cached selector. If no selector is yet cached, registers
     /// one with the given name and stores it.
     #[inline]
-    #[doc(hidden)]
     pub unsafe fn get(&self, name: &str) -> Sel {
         // `Relaxed` should be fine since `sel_registerName` is thread-safe.
         let ptr = self.ptr.load(Ordering::Relaxed);
-        unsafe { Sel::from_ptr(ptr) }.unwrap_or_else(|| {
-            // The panic inside `Sel::register_unchecked` is unfortunate, but
-            // strict correctness is more important than speed
-
-            // SAFETY: Input is a non-null, NUL-terminated C-string pointer.
-            //
-            // We know this, because we construct it in `sel!` ourselves
-            let sel = unsafe { Sel::register_unchecked(name.as_ptr().cast()) };
-            self.ptr
-                .store(sel.as_ptr() as *mut ffi::objc_selector, Ordering::Relaxed);
+        if let Some(sel) = unsafe { Sel::from_ptr(ptr) } {
             sel
-        })
+        } else {
+            // SAFETY: Checked by caller
+            unsafe { self.fetch(name.as_ptr().cast()) }
+        }
     }
 }
 
 /// Allows storing a [`AnyClass`] reference in a static and lazily loading it.
-#[doc(hidden)]
 pub struct CachedClass {
     ptr: AtomicPtr<AnyClass>,
 }
@@ -54,19 +64,47 @@ impl CachedClass {
         }
     }
 
+    // Mark as cold since this should only ever be called once (or maybe twice
+    // if running on multiple threads).
+    #[cold]
+    #[track_caller]
+    unsafe fn fetch(&self, name: *const c_char) -> &'static AnyClass {
+        let ptr: *const AnyClass = unsafe { ffi::objc_getClass(name) }.cast();
+        self.ptr.store(ptr as *mut AnyClass, Ordering::Relaxed);
+        if let Some(cls) = unsafe { ptr.as_ref() } {
+            cls
+        } else {
+            // Recover the name from the pointer. We do it like this so that
+            // we don't have to pass the length of the class to this method,
+            // improving binary size.
+            let name = unsafe { CStr::from_ptr(name) };
+            let name = str::from_utf8(name.to_bytes()).unwrap();
+            panic!("class {name} could not be found")
+        }
+    }
+
     /// Returns the cached class. If no class is yet cached, gets one with
     /// the given name and stores it.
     #[inline]
-    #[doc(hidden)]
-    pub unsafe fn get(&self, name: &str) -> Option<&'static AnyClass> {
+    #[track_caller]
+    pub unsafe fn get(&self, name: &str) -> &'static AnyClass {
         // `Relaxed` should be fine since `objc_getClass` is thread-safe.
         let ptr = self.ptr.load(Ordering::Relaxed);
         if let Some(cls) = unsafe { ptr.as_ref() } {
-            Some(cls)
+            cls
         } else {
-            let ptr: *const AnyClass = unsafe { ffi::objc_getClass(name.as_ptr().cast()) }.cast();
-            self.ptr.store(ptr as *mut AnyClass, Ordering::Relaxed);
-            unsafe { ptr.as_ref() }
+            // SAFETY: Checked by caller
+            unsafe { self.fetch(name.as_ptr().cast()) }
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    #[test]
+    #[should_panic = "class NonExistantClass could not be found"]
+    #[cfg(not(feature = "unstable-static-class"))]
+    fn test_not_found() {
+        let _ = crate::class!(NonExistantClass);
+    }
+}

crates/objc2/src/__macro_helpers/cache.rs

@@ -0,0 +1,30 @@
+//! Common selectors.
+//!
+//! These are put here to deduplicate the cached selector, and when using
+//! `unstable-static-sel`, the statics.
+//!
+//! Note that our assembly tests of `unstable-static-sel-inlined` output a GOT
+//! entry for such accesses, but that is just a limitation of our tests - the
+//! actual assembly is as one would expect.
+use crate::runtime::Sel;
+use crate::{__sel_data, __sel_inner};
+
+#[inline]
+pub fn alloc_sel() -> Sel {
+    __sel_inner!(__sel_data!(alloc), "alloc")
+}
+
+#[inline]
+pub fn init_sel() -> Sel {
+    __sel_inner!(__sel_data!(init), "init")
+}
+
+#[inline]
+pub fn new_sel() -> Sel {
+    __sel_inner!(__sel_data!(new), "new")
+}
+
+#[inline]
+pub fn dealloc_sel() -> Sel {
+    __sel_inner!(__sel_data!(dealloc), "dealloc")
+}

crates/objc2/src/__macro_helpers/common_selectors.rs

@@ -13,7 +13,6 @@ use crate::rc::{Allocated, Id};
 use crate::runtime::MethodDescription;
 use crate::runtime::{AnyClass, AnyObject, AnyProtocol, Sel};
 use crate::{Message, MessageArguments, MessageReceiver};
-use crate::{__sel_data, __sel_inner};
 
 pub use core::borrow::{Borrow, BorrowMut};
 pub use core::cell::UnsafeCell;
@@ -29,36 +28,16 @@ pub use core::{compile_error, concat, panic, stringify};
 pub use std::sync::Once;
 
 mod cache;
+mod common_selectors;
 mod declare_class;
 
 pub use self::cache::{CachedClass, CachedSel};
+pub use self::common_selectors::{alloc_sel, dealloc_sel, init_sel, new_sel};
 pub use self::declare_class::{
     assert_mutability_matches_superclass_mutability, MaybeOptionId, MessageRecieveId,
     ValidSubclassMutability,
 };
 
-// Common selectors.
-//
-// These are put here to deduplicate the cached selector, and when using
-// `unstable-static-sel`, the statics.
-//
-// Note that our assembly tests of `unstable-static-sel-inlined` output a GOT
-// entry for such accesses, but that is just a limitation of our tests - the
-// actual assembly is as one would expect.
-
-#[inline]
-pub fn alloc_sel() -> Sel {
-    __sel_inner!(__sel_data!(alloc), "alloc")
-}
-#[inline]
-pub fn init_sel() -> Sel {
-    __sel_inner!(__sel_data!(init), "init")
-}
-#[inline]
-pub fn new_sel() -> Sel {
-    __sel_inner!(__sel_data!(new), "new")
-}
-
 /// Helper for specifying the retain semantics for a given selector family.
 ///
 /// Note that we can't actually check if a method is in a method family; only
@@ -595,6 +574,7 @@ impl ClassProtocolMethodsBuilder<'_, '_> {
         }
     }
 
+    #[inline]
     pub fn __finish(self) {
         #[cfg(all(debug_assertions, feature = "verify"))]
         if let Some(protocol) = self.protocol {

crates/objc2/src/__macro_helpers/mod.rs

@@ -368,7 +368,7 @@ fn test_unreachable() {
 }
 
 #[test]
-#[should_panic = "Failed to add ivar _ivar"]
+#[should_panic = "failed to add ivar _ivar"]
 fn test_duplicate_ivar() {
     declare_class!(
         struct DeclareClassDuplicateIvar {