From 0e504d22114cb0d1ebff5adf9af7196f300c895e Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 2 Mar 2022 22:47:17 +0100 Subject: [PATCH 1/6] Add `Id::retain_autoreleased` An optimized version of `Id::retain` for when the previous caller returns an autoreleased value --- objc2-foundation/src/array.rs | 6 +-- objc2-foundation/src/dictionary.rs | 4 +- objc2-foundation/src/enumerator.rs | 2 +- objc2-foundation/src/object.rs | 2 +- objc2-foundation/src/process_info.rs | 4 +- objc2-foundation/src/thread.rs | 6 +-- objc2/CHANGELOG.md | 2 + objc2/benches/autorelease.rs | 5 +-- objc2/src/rc/id.rs | 35 +++++++++++++++- objc2/tests/id_retain_autoreleased.rs | 60 +++++++++++++++++++++++++++ 10 files changed, 110 insertions(+), 16 deletions(-) create mode 100644 objc2/tests/id_retain_autoreleased.rs diff --git a/objc2-foundation/src/array.rs b/objc2-foundation/src/array.rs index ad36a9852..1cda52b5e 100644 --- a/objc2-foundation/src/array.rs +++ b/objc2-foundation/src/array.rs @@ -140,7 +140,7 @@ impl NSArray { pub fn get_retained(&self, index: usize) -> Id { let obj = self.get(index).unwrap(); // SAFETY: The object is originally shared (see `where` bound). - unsafe { Id::retain(obj as *const T as *mut T).unwrap_unchecked() } + unsafe { Id::retain_autoreleased(obj as *const T as *mut T).unwrap_unchecked() } } pub fn to_shared_vec(&self) -> Vec> { @@ -247,7 +247,7 @@ impl NSMutableArray { pub fn replace(&mut self, index: usize, obj: Id) -> Id { let old_obj = unsafe { let obj = self.get(index).unwrap(); - Id::retain(obj as *const T as *mut T).unwrap_unchecked() + Id::retain_autoreleased(obj as *const T as *mut T).unwrap_unchecked() }; unsafe { let _: () = msg_send![ @@ -262,7 +262,7 @@ impl NSMutableArray { #[doc(alias = "removeObjectAtIndex:")] pub fn remove(&mut self, index: usize) -> Id { let obj = if let Some(obj) = self.get(index) { - unsafe { Id::retain(obj as *const T as *mut T).unwrap_unchecked() } + unsafe { Id::retain_autoreleased(obj as *const T as *mut T).unwrap_unchecked() } } else { panic!("removal index should be < len"); }; diff --git a/objc2-foundation/src/dictionary.rs b/objc2-foundation/src/dictionary.rs index fcb39a0d3..072b3190c 100644 --- a/objc2-foundation/src/dictionary.rs +++ b/objc2-foundation/src/dictionary.rs @@ -103,7 +103,7 @@ impl NSDictionary { pub fn keys_array(&self) -> Id, Shared> { unsafe { let keys = msg_send![self, allKeys]; - Id::retain(keys).unwrap() + Id::retain_autoreleased(keys).unwrap() } } @@ -130,7 +130,7 @@ impl NSDictionary { pub fn into_values_array(dict: Id) -> Id, Shared> { unsafe { let vals = msg_send![dict, allValues]; - Id::retain(vals).unwrap() + Id::retain_autoreleased(vals).unwrap() } } } diff --git a/objc2-foundation/src/enumerator.rs b/objc2-foundation/src/enumerator.rs index 60422c1a8..927f116af 100644 --- a/objc2-foundation/src/enumerator.rs +++ b/objc2-foundation/src/enumerator.rs @@ -23,7 +23,7 @@ impl<'a, T: Message> NSEnumerator<'a, T> { /// ownership. pub unsafe fn from_ptr(ptr: *mut Object) -> Self { Self { - id: unsafe { Id::retain(ptr) }.unwrap(), + id: unsafe { Id::retain_autoreleased(ptr) }.unwrap(), item: PhantomData, } } diff --git a/objc2-foundation/src/object.rs b/objc2-foundation/src/object.rs index e913741dc..22d3df361 100644 --- a/objc2-foundation/src/object.rs +++ b/objc2-foundation/src/object.rs @@ -24,7 +24,7 @@ impl NSObject { unsafe { let result: *mut NSString = msg_send![self, description]; // TODO: Verify that description always returns a non-null string - Id::retain(result).unwrap() + Id::retain_autoreleased(result).unwrap() } } diff --git a/objc2-foundation/src/process_info.rs b/objc2-foundation/src/process_info.rs index 3abb37008..dcd8c5a99 100644 --- a/objc2-foundation/src/process_info.rs +++ b/objc2-foundation/src/process_info.rs @@ -22,11 +22,11 @@ impl NSProcessInfo { // currentThread is @property(strong), what does that mean? let obj: *mut Self = unsafe { msg_send![Self::class(), processInfo] }; // TODO: Always available? - unsafe { Id::retain(obj).unwrap() } + unsafe { Id::retain_autoreleased(obj).unwrap() } } pub fn process_name(&self) -> Id { let obj: *mut NSString = unsafe { msg_send![Self::class(), processName] }; - unsafe { Id::retain(obj).unwrap() } + unsafe { Id::retain_autoreleased(obj).unwrap() } } } diff --git a/objc2-foundation/src/thread.rs b/objc2-foundation/src/thread.rs index 66a401873..6ea3393d1 100644 --- a/objc2-foundation/src/thread.rs +++ b/objc2-foundation/src/thread.rs @@ -20,7 +20,7 @@ impl NSThread { // TODO: currentThread is @property(strong), what does that mean? let obj: *mut Self = unsafe { msg_send![Self::class(), currentThread] }; // TODO: Always available? - unsafe { Id::retain(obj).unwrap() } + unsafe { Id::retain_autoreleased(obj).unwrap() } } /// Returns the [`NSThread`] object representing the main thread. @@ -29,7 +29,7 @@ impl NSThread { let obj: *mut Self = unsafe { msg_send![Self::class(), mainThread] }; // The main thread static may not have been initialized // This can at least fail in GNUStep! - unsafe { Id::retain(obj).expect("Could not retrieve main thread.") } + unsafe { Id::retain_autoreleased(obj).expect("Could not retrieve main thread.") } } /// Returns `true` if the thread is the main thread. @@ -41,7 +41,7 @@ impl NSThread { /// The name of the thread. pub fn name(&self) -> Option> { let obj: *mut NSString = unsafe { msg_send![self, name] }; - unsafe { Id::retain(obj) } + unsafe { Id::retain_autoreleased(obj) } } } diff --git a/objc2/CHANGELOG.md b/objc2/CHANGELOG.md index b6b83c452..d66b305b0 100644 --- a/objc2/CHANGELOG.md +++ b/objc2/CHANGELOG.md @@ -13,6 +13,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). * Added `Bool::as_bool` (more descriptive name than `Bool::is_true`). * Added convenience method `Id::as_ptr`. * The `objc2-encode` dependency is now exposed as `objc2::encode`. +* Added `Id::retain_autoreleased` to allow following Cocoas memory management + rules more efficiently. ### Changed * **BREAKING**: Changed signature of `Id::new` and `Id::retain` from diff --git a/objc2/benches/autorelease.rs b/objc2/benches/autorelease.rs index f220ab88f..b4cabfde6 100644 --- a/objc2/benches/autorelease.rs +++ b/objc2/benches/autorelease.rs @@ -1,7 +1,6 @@ use core::ffi::c_void; use std::mem::ManuallyDrop; -use objc2::ffi; use objc2::rc::{autoreleasepool, Id, Shared}; use objc2::runtime::{Class, Object, Sel}; use objc2::{class, msg_send, sel}; @@ -94,8 +93,7 @@ fn autoreleased_nsstring() -> *mut Object { } fn retain_autoreleased(obj: *mut Object) -> Id { - let obj = unsafe { ffi::objc_retainAutoreleasedReturnValue(obj.cast()) }; - unsafe { Id::new(obj.cast()).unwrap_unchecked() } + unsafe { Id::retain_autoreleased(obj.cast()).unwrap_unchecked() } } fn autoreleased_nsdata_pool_cleanup() -> *mut Object { @@ -133,6 +131,7 @@ macro_rules! main_with_warmup { )+ } + // Required to get DYLD to resolve the stubs on x86_64 fn warmup() { $( warmup_fns::$f(); diff --git a/objc2/src/rc/id.rs b/objc2/src/rc/id.rs index c2790f89a..8c680dc38 100644 --- a/objc2/src/rc/id.rs +++ b/objc2/src/rc/id.rs @@ -210,6 +210,9 @@ impl Id { /// some API, and you would like to ensure that the object stays around /// so that you can work with it. /// + /// If said API is a normal Objective-C method, you probably want to use + /// [`Id::retain_autoreleased`] instead. + /// /// This is rarely used to construct owned [`Id`]s, see [`Id::new`] for /// that. /// @@ -249,6 +252,37 @@ impl Id { unsafe { Self::new(res as *mut T) } } + /// Retains a previously autoreleased object pointer. + /// + /// This is useful when calling Objective-C methods that return + /// autoreleased objects, see [Cocoa's Memory Management Policy][mmRules]. + /// + /// This has exactly the same semantics as [`Id::retain`], except it can + /// sometimes avoid putting the object into the autorelease pool, possibly + /// yielding increased speed and reducing memory pressure. + /// + /// Note: This relies heavily on being inlined right after [`msg_send!`], + /// be careful not accidentally require instructions between these. + /// + /// [mmRules]: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html + /// + /// # Safety + /// + /// Same as [`Id::retain`]. + #[doc(alias = "objc_retainAutoreleasedReturnValue")] + #[inline(always)] + pub unsafe fn retain_autoreleased(ptr: *mut T) -> Option> { + let ptr = ptr as *mut ffi::objc_object; + // SAFETY: Same as `retain`, `objc_retainAutoreleasedReturnValue` is + // just an optimization. + let res = unsafe { ffi::objc_retainAutoreleasedReturnValue(ptr) }; + debug_assert_eq!( + res, ptr, + "objc_retainAutoreleasedReturnValue did not return the same pointer" + ); + unsafe { Self::new(res as *mut T) } + } + #[inline] fn autorelease_inner(self) -> *mut T { // Note that this (and the actual `autorelease`) is not an associated @@ -267,7 +301,6 @@ impl Id { res as *mut T } - // TODO: objc_retainAutoreleasedReturnValue // TODO: objc_autoreleaseReturnValue // TODO: objc_retainAutorelease // TODO: objc_retainAutoreleaseReturnValue diff --git a/objc2/tests/id_retain_autoreleased.rs b/objc2/tests/id_retain_autoreleased.rs new file mode 100644 index 000000000..39688e4cd --- /dev/null +++ b/objc2/tests/id_retain_autoreleased.rs @@ -0,0 +1,60 @@ +use std::ffi::c_void; + +use objc2::rc::{autoreleasepool, Id, Shared}; +use objc2::runtime::Object; +use objc2::{class, msg_send}; + +fn retain_count(obj: &Object) -> usize { + unsafe { msg_send![obj, retainCount] } +} + +fn create_data(bytes: &[u8]) -> Id { + let bytes_ptr = bytes.as_ptr() as *const c_void; + unsafe { + // All code between the `msg_send!` and the `retain_autoreleased` must + // be able to be optimized away for this to work. + let obj: *mut Object = msg_send![ + class!(NSData), + dataWithBytes: bytes_ptr, + length: bytes.len(), + ]; + Id::retain_autoreleased(obj).unwrap() + } +} + +#[test] +fn test_retain_autoreleased() { + #[cfg(gnustep)] + unsafe { + objc2::__gnustep_hack::get_class_to_force_linkage() + }; + + #[cfg(apple)] + #[link(name = "Foundation", kind = "framework")] + extern "C" {} + + autoreleasepool(|_| { + // Run once to allow DYLD to resolve the symbol stubs. + // Required for making `retain_autoreleased` work on x86_64. + let _data = create_data(b"12"); + + // When compiled in release mode / with optimizations enabled, + // subsequent usage of `retain_autoreleased` will succeed in retaining + // the autoreleased value! + let expected = if cfg!(all(debug_assertions, not(gnustep))) { + 2 + } else { + 1 + }; + + let data = create_data(b"34"); + assert_eq!(retain_count(&data), expected); + + let data = create_data(b"56"); + assert_eq!(retain_count(&data), expected); + + // Here we manually clean up the autorelease, so it will always be 1. + let data = autoreleasepool(|_| create_data(b"78")); + assert_eq!(retain_count(&data), 1); + }); +} From 24f6ffdbbd0c893ebeb374029821cd2fbf209d15 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 2 Mar 2022 23:05:50 +0100 Subject: [PATCH 2/6] Add assembly tests for Id::retain_autoreleased --- .../expected/armv7-apple-ios.s | 1 - .../test_retain_autoreleased/Cargo.toml | 13 ++++++++++++ .../expected/aarch64-apple-darwin.s | 11 ++++++++++ .../expected/aarch64-apple-ios-sim.s | 15 ++++++++++++++ .../expected/aarch64-apple-ios.s | 15 ++++++++++++++ .../expected/armv7-apple-ios.s | 13 ++++++++++++ .../expected/armv7s-apple-ios.s | 13 ++++++++++++ .../expected/i386-apple-ios.s | 20 +++++++++++++++++++ .../expected/i686-apple-darwin.s | 20 +++++++++++++++++++ .../expected/x86_64-apple-darwin.s | 13 ++++++++++++ .../expected/x86_64-apple-ios.s | 13 ++++++++++++ .../assembly/test_retain_autoreleased/lib.rs | 11 ++++++++++ tests/src/bin/test_assembly.rs | 8 ++++++-- 13 files changed, 163 insertions(+), 3 deletions(-) create mode 100644 tests/assembly/test_retain_autoreleased/Cargo.toml create mode 100644 tests/assembly/test_retain_autoreleased/expected/aarch64-apple-darwin.s create mode 100644 tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios-sim.s create mode 100644 tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios.s create mode 100644 tests/assembly/test_retain_autoreleased/expected/armv7-apple-ios.s create mode 100644 tests/assembly/test_retain_autoreleased/expected/armv7s-apple-ios.s create mode 100644 tests/assembly/test_retain_autoreleased/expected/i386-apple-ios.s create mode 100644 tests/assembly/test_retain_autoreleased/expected/i686-apple-darwin.s create mode 100644 tests/assembly/test_retain_autoreleased/expected/x86_64-apple-darwin.s create mode 100644 tests/assembly/test_retain_autoreleased/expected/x86_64-apple-ios.s create mode 100644 tests/assembly/test_retain_autoreleased/lib.rs diff --git a/tests/assembly/test_msg_send_zero_cost/expected/armv7-apple-ios.s b/tests/assembly/test_msg_send_zero_cost/expected/armv7-apple-ios.s index f9b43ae49..fe45b82cc 100644 --- a/tests/assembly/test_msg_send_zero_cost/expected/armv7-apple-ios.s +++ b/tests/assembly/test_msg_send_zero_cost/expected/armv7-apple-ios.s @@ -1,5 +1,4 @@ .section __TEXT,__text,regular,pure_instructions - .ios_version_min 7, 0 .syntax unified .globl _handle .p2align 2 diff --git a/tests/assembly/test_retain_autoreleased/Cargo.toml b/tests/assembly/test_retain_autoreleased/Cargo.toml new file mode 100644 index 000000000..0a7caa2f6 --- /dev/null +++ b/tests/assembly/test_retain_autoreleased/Cargo.toml @@ -0,0 +1,13 @@ +[package] +name = "test_retain_autoreleased" +version = "0.1.0" +edition = "2018" +publish = false + +[lib] +path = "lib.rs" + +[dependencies] +objc2 = { path = "../../../objc2" } +objc-sys = { path = "../../../objc-sys" } +block-sys = { path = "../../../block-sys" } diff --git a/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-darwin.s b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-darwin.s new file mode 100644 index 000000000..a65869a06 --- /dev/null +++ b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-darwin.s @@ -0,0 +1,11 @@ + .section __TEXT,__text,regular,pure_instructions + .globl _handle + .p2align 2 +_handle: + stp x29, x30, [sp, #-16]! + mov x29, sp + bl _objc_msgSend + ldp x29, x30, [sp], #16 + b _objc_retainAutoreleasedReturnValue + +.subsections_via_symbols diff --git a/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios-sim.s b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios-sim.s new file mode 100644 index 000000000..e0cb4520a --- /dev/null +++ b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios-sim.s @@ -0,0 +1,15 @@ + .section __TEXT,__text,regular,pure_instructions + .globl _handle + .p2align 2 +_handle: + stp x29, x30, [sp, #-16]! + mov x29, sp + bl _objc_msgSend + ldp x29, x30, [sp], #16 + b _objc_retainAutoreleasedReturnValue + +; Stripped __LLVM section + +; Stripped __LLVM section + +.subsections_via_symbols diff --git a/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios.s b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios.s new file mode 100644 index 000000000..e0cb4520a --- /dev/null +++ b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios.s @@ -0,0 +1,15 @@ + .section __TEXT,__text,regular,pure_instructions + .globl _handle + .p2align 2 +_handle: + stp x29, x30, [sp, #-16]! + mov x29, sp + bl _objc_msgSend + ldp x29, x30, [sp], #16 + b _objc_retainAutoreleasedReturnValue + +; Stripped __LLVM section + +; Stripped __LLVM section + +.subsections_via_symbols diff --git a/tests/assembly/test_retain_autoreleased/expected/armv7-apple-ios.s b/tests/assembly/test_retain_autoreleased/expected/armv7-apple-ios.s new file mode 100644 index 000000000..147e682cf --- /dev/null +++ b/tests/assembly/test_retain_autoreleased/expected/armv7-apple-ios.s @@ -0,0 +1,13 @@ + .section __TEXT,__text,regular,pure_instructions + .syntax unified + .globl _handle + .p2align 2 + .code 32 +_handle: + push {r7, lr} + mov r7, sp + bl _objc_msgSend + pop {r7, lr} + b _objc_retainAutoreleasedReturnValue + +.subsections_via_symbols diff --git a/tests/assembly/test_retain_autoreleased/expected/armv7s-apple-ios.s b/tests/assembly/test_retain_autoreleased/expected/armv7s-apple-ios.s new file mode 100644 index 000000000..43c313897 --- /dev/null +++ b/tests/assembly/test_retain_autoreleased/expected/armv7s-apple-ios.s @@ -0,0 +1,13 @@ + .section __TEXT,__text,regular,pure_instructions + .syntax unified + .globl _handle + .p2align 2 + .code 32 +_handle: + push {r7, lr} + mov r7, sp + bl _objc_msgSend + bl _objc_retainAutoreleasedReturnValue + pop {r7, pc} + +.subsections_via_symbols diff --git a/tests/assembly/test_retain_autoreleased/expected/i386-apple-ios.s b/tests/assembly/test_retain_autoreleased/expected/i386-apple-ios.s new file mode 100644 index 000000000..3b9c76dd6 --- /dev/null +++ b/tests/assembly/test_retain_autoreleased/expected/i386-apple-ios.s @@ -0,0 +1,20 @@ + .section __TEXT,__text,regular,pure_instructions + .intel_syntax noprefix + .globl _handle + .p2align 4, 0x90 +_handle: + push ebp + mov ebp, esp + sub esp, 8 + mov eax, dword ptr [ebp + 8] + mov ecx, dword ptr [ebp + 12] + mov dword ptr [esp + 4], ecx + mov dword ptr [esp], eax + call _objc_msgSend + mov dword ptr [esp], eax + call _objc_retainAutoreleasedReturnValue + add esp, 8 + pop ebp + ret + +.subsections_via_symbols diff --git a/tests/assembly/test_retain_autoreleased/expected/i686-apple-darwin.s b/tests/assembly/test_retain_autoreleased/expected/i686-apple-darwin.s new file mode 100644 index 000000000..3b9c76dd6 --- /dev/null +++ b/tests/assembly/test_retain_autoreleased/expected/i686-apple-darwin.s @@ -0,0 +1,20 @@ + .section __TEXT,__text,regular,pure_instructions + .intel_syntax noprefix + .globl _handle + .p2align 4, 0x90 +_handle: + push ebp + mov ebp, esp + sub esp, 8 + mov eax, dword ptr [ebp + 8] + mov ecx, dword ptr [ebp + 12] + mov dword ptr [esp + 4], ecx + mov dword ptr [esp], eax + call _objc_msgSend + mov dword ptr [esp], eax + call _objc_retainAutoreleasedReturnValue + add esp, 8 + pop ebp + ret + +.subsections_via_symbols diff --git a/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-darwin.s b/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-darwin.s new file mode 100644 index 000000000..ed4943a42 --- /dev/null +++ b/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-darwin.s @@ -0,0 +1,13 @@ + .section __TEXT,__text,regular,pure_instructions + .intel_syntax noprefix + .globl _handle + .p2align 4, 0x90 +_handle: + push rbp + mov rbp, rsp + call _objc_msgSend + mov rdi, rax + pop rbp + jmp _objc_retainAutoreleasedReturnValue + +.subsections_via_symbols diff --git a/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-ios.s b/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-ios.s new file mode 100644 index 000000000..ed4943a42 --- /dev/null +++ b/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-ios.s @@ -0,0 +1,13 @@ + .section __TEXT,__text,regular,pure_instructions + .intel_syntax noprefix + .globl _handle + .p2align 4, 0x90 +_handle: + push rbp + mov rbp, rsp + call _objc_msgSend + mov rdi, rax + pop rbp + jmp _objc_retainAutoreleasedReturnValue + +.subsections_via_symbols diff --git a/tests/assembly/test_retain_autoreleased/lib.rs b/tests/assembly/test_retain_autoreleased/lib.rs new file mode 100644 index 000000000..9a722262c --- /dev/null +++ b/tests/assembly/test_retain_autoreleased/lib.rs @@ -0,0 +1,11 @@ +//! Test that `Id::retain_autoreleased` is inlined properly. + +use objc2::rc::{Id, Shared}; +use objc2::runtime::{Object, Sel}; +use objc2::MessageReceiver; + +#[no_mangle] +pub fn handle(obj: &Object, sel: Sel) -> Option> { + let ptr: *mut Object = unsafe { MessageReceiver::send_message(&obj, sel, ()).unwrap() }; + unsafe { Id::retain_autoreleased(ptr) } +} diff --git a/tests/src/bin/test_assembly.rs b/tests/src/bin/test_assembly.rs index 73653571f..ae7549b08 100644 --- a/tests/src/bin/test_assembly.rs +++ b/tests/src/bin/test_assembly.rs @@ -68,7 +68,11 @@ fn main() { let host = env!("TARGET"); for entry in manifest_dir.join("assembly").read_dir().unwrap() { - let package_path = entry.unwrap().path(); + let entry = entry.unwrap(); + if !entry.file_type().unwrap().is_dir() { + continue; + } + let package_path = entry.path(); let package = package_path.file_name().unwrap().to_str().unwrap(); println!("Testing {package}."); @@ -125,7 +129,7 @@ fn main() { let actual = read_assembly(&artifact).unwrap(); if should_overwrite { fs::write(expected_file, actual).unwrap(); - } else if let Ok(expected) = read_assembly(expected_file) { + } else if let Ok(expected) = fs::read_to_string(expected_file) { if expected != actual { eprintln!("\n===Expected===\n{}\n===Actual===\n{}", expected, actual); panic!("Expected and actual did not match."); From e8fa4d2b2244631e68be515d336584d8d2c4d989 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 2 Mar 2022 22:33:10 +0100 Subject: [PATCH 3/6] Add magic nop instructions to Id::retain_autoreleased --- objc2/src/rc/id.rs | 62 +++++++++++++++++++ .../expected/aarch64-apple-darwin.s | 3 + .../expected/aarch64-apple-ios-sim.s | 3 + .../expected/aarch64-apple-ios.s | 3 + .../expected/armv7-apple-ios.s | 3 + .../expected/armv7s-apple-ios.s | 3 + .../expected/i386-apple-ios.s | 5 ++ .../expected/i686-apple-darwin.s | 5 ++ 8 files changed, 87 insertions(+) diff --git a/objc2/src/rc/id.rs b/objc2/src/rc/id.rs index 8c680dc38..7390f243d 100644 --- a/objc2/src/rc/id.rs +++ b/objc2/src/rc/id.rs @@ -272,7 +272,69 @@ impl Id { #[doc(alias = "objc_retainAutoreleasedReturnValue")] #[inline(always)] pub unsafe fn retain_autoreleased(ptr: *mut T) -> Option> { + // Add magic nop instruction to participate in the fast autorelease + // scheme. + // + // See `callerAcceptsOptimizedReturn` in `objc-object.h`: + // https://github.com/apple-oss-distributions/objc4/blob/objc4-838/runtime/objc-object.h#L1209-L1377 + // + // We will unconditionally emit these instructions, even if they end + // up being unused (for example because we're unlucky with inlining, + // some other work is done between the objc_msgSend and this, or the + // runtime version is too old to support it). + // + // It may seem like there should be a better way to do this, but + // emitting raw assembly is exactly what Clang and Swift does: + // swiftc: https://github.com/apple/swift/blob/swift-5.5.3-RELEASE/lib/IRGen/GenObjC.cpp#L148-L173 + // Clang: https://github.com/llvm/llvm-project/blob/889317d47b7f046cf0e68746da8f7f264582fb5b/clang/lib/CodeGen/CGObjC.cpp#L2339-L2373 + // + // Resources: + // - https://www.mikeash.com/pyblog/friday-qa-2011-09-30-automatic-reference-counting.html + // - https://www.galloway.me.uk/2012/02/how-does-objc_retainautoreleasedreturnvalue-work/ + // - https://github.com/gfx-rs/metal-rs/issues/222 + // - https://news.ycombinator.com/item?id=29311736 + // - https://stackoverflow.com/a/23765612 + // + // SAFETY: + // Based on https://doc.rust-lang.org/stable/reference/inline-assembly.html#rules-for-inline-assembly + // + // We don't care about the value of the register (so it's okay to be + // undefined), and its value is preserved. + // + // nomem: No reads or writes to memory are performed (this `mov` + // operates entirely on registers). + // preserves_flags: `mov` doesn't modify any flags. + // nostack: We don't touch the stack. + + // Only worth doing on the Apple runtime. + // Not supported on TARGET_OS_WIN32. + #[cfg(all(apple, not(target_os = "windows")))] + { + // Supported since macOS 10.7. + #[cfg(target_arch = "x86_64")] + {} // x86_64 looks at the next call instruction + + // Supported since macOS 10.8. + #[cfg(target_arch = "arm")] + unsafe { + core::arch::asm!("mov r7, r7", options(nomem, preserves_flags, nostack)) + }; + + // Supported since macOS 10.10. + #[cfg(target_arch = "aarch64")] + unsafe { + core::arch::asm!("mov fp, fp", options(nomem, preserves_flags, nostack)) + }; + + // Supported since macOS 10.12. + #[cfg(target_arch = "x86")] + unsafe { + core::arch::asm!("mov ebp, ebp", options(nomem, preserves_flags, nostack)) + }; + } + let ptr = ptr as *mut ffi::objc_object; + // SAFETY: Same as `retain`, `objc_retainAutoreleasedReturnValue` is // just an optimization. let res = unsafe { ffi::objc_retainAutoreleasedReturnValue(ptr) }; diff --git a/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-darwin.s b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-darwin.s index a65869a06..6982ac9e6 100644 --- a/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-darwin.s +++ b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-darwin.s @@ -5,6 +5,9 @@ _handle: stp x29, x30, [sp, #-16]! mov x29, sp bl _objc_msgSend + ; InlineAsm Start + mov x29, x29 + ; InlineAsm End ldp x29, x30, [sp], #16 b _objc_retainAutoreleasedReturnValue diff --git a/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios-sim.s b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios-sim.s index e0cb4520a..42c336539 100644 --- a/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios-sim.s +++ b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios-sim.s @@ -5,6 +5,9 @@ _handle: stp x29, x30, [sp, #-16]! mov x29, sp bl _objc_msgSend + ; InlineAsm Start + mov x29, x29 + ; InlineAsm End ldp x29, x30, [sp], #16 b _objc_retainAutoreleasedReturnValue diff --git a/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios.s b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios.s index e0cb4520a..42c336539 100644 --- a/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios.s +++ b/tests/assembly/test_retain_autoreleased/expected/aarch64-apple-ios.s @@ -5,6 +5,9 @@ _handle: stp x29, x30, [sp, #-16]! mov x29, sp bl _objc_msgSend + ; InlineAsm Start + mov x29, x29 + ; InlineAsm End ldp x29, x30, [sp], #16 b _objc_retainAutoreleasedReturnValue diff --git a/tests/assembly/test_retain_autoreleased/expected/armv7-apple-ios.s b/tests/assembly/test_retain_autoreleased/expected/armv7-apple-ios.s index 147e682cf..c02cca05f 100644 --- a/tests/assembly/test_retain_autoreleased/expected/armv7-apple-ios.s +++ b/tests/assembly/test_retain_autoreleased/expected/armv7-apple-ios.s @@ -7,6 +7,9 @@ _handle: push {r7, lr} mov r7, sp bl _objc_msgSend + @ InlineAsm Start + mov r7, r7 + @ InlineAsm End pop {r7, lr} b _objc_retainAutoreleasedReturnValue diff --git a/tests/assembly/test_retain_autoreleased/expected/armv7s-apple-ios.s b/tests/assembly/test_retain_autoreleased/expected/armv7s-apple-ios.s index 43c313897..a3821d850 100644 --- a/tests/assembly/test_retain_autoreleased/expected/armv7s-apple-ios.s +++ b/tests/assembly/test_retain_autoreleased/expected/armv7s-apple-ios.s @@ -7,6 +7,9 @@ _handle: push {r7, lr} mov r7, sp bl _objc_msgSend + @ InlineAsm Start + mov r7, r7 + @ InlineAsm End bl _objc_retainAutoreleasedReturnValue pop {r7, pc} diff --git a/tests/assembly/test_retain_autoreleased/expected/i386-apple-ios.s b/tests/assembly/test_retain_autoreleased/expected/i386-apple-ios.s index 3b9c76dd6..b2ebee241 100644 --- a/tests/assembly/test_retain_autoreleased/expected/i386-apple-ios.s +++ b/tests/assembly/test_retain_autoreleased/expected/i386-apple-ios.s @@ -11,6 +11,11 @@ _handle: mov dword ptr [esp + 4], ecx mov dword ptr [esp], eax call _objc_msgSend + ## InlineAsm Start + + mov ebp, ebp + + ## InlineAsm End mov dword ptr [esp], eax call _objc_retainAutoreleasedReturnValue add esp, 8 diff --git a/tests/assembly/test_retain_autoreleased/expected/i686-apple-darwin.s b/tests/assembly/test_retain_autoreleased/expected/i686-apple-darwin.s index 3b9c76dd6..b2ebee241 100644 --- a/tests/assembly/test_retain_autoreleased/expected/i686-apple-darwin.s +++ b/tests/assembly/test_retain_autoreleased/expected/i686-apple-darwin.s @@ -11,6 +11,11 @@ _handle: mov dword ptr [esp + 4], ecx mov dword ptr [esp], eax call _objc_msgSend + ## InlineAsm Start + + mov ebp, ebp + + ## InlineAsm End mov dword ptr [esp], eax call _objc_retainAutoreleasedReturnValue add esp, 8 From 5ea2a9434dba3af3537b8d90475c9f616cacb369 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 2 Mar 2022 22:17:16 +0100 Subject: [PATCH 4/6] Prevent tail-call optimizations from interfering with fast autorelease --- objc2/src/rc/id.rs | 23 +++++++++++++++++++ .../expected/x86_64-apple-darwin.s | 8 ++++++- .../expected/x86_64-apple-ios.s | 8 ++++++- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/objc2/src/rc/id.rs b/objc2/src/rc/id.rs index 7390f243d..702e9bc74 100644 --- a/objc2/src/rc/id.rs +++ b/objc2/src/rc/id.rs @@ -338,6 +338,29 @@ impl Id { // SAFETY: Same as `retain`, `objc_retainAutoreleasedReturnValue` is // just an optimization. let res = unsafe { ffi::objc_retainAutoreleasedReturnValue(ptr) }; + + // Ideally, we'd be able to specify that the above call should never + // be tail-call optimized (become a `jmp` instruction instead of a + // `call`); Rust doesn't really have a way of doing this currently, so + // we just emit a simple `nop` to make such tail-call optimizations + // less likely to occur. + // + // This is brittle! We should find a better solution! + #[cfg(all(apple, not(target_os = "windows"), target_arch = "x86_64"))] + { + // SAFETY: Similar to above. + unsafe { core::arch::asm!("nop", options(nomem, preserves_flags, nostack)) }; + // TODO: Possibly more efficient alternative? + // #![feature(asm_sym)] + // core::arch::asm!( + // "mov rdi, rax", + // "call {}", + // sym objc2::ffi::objc_retainAutoreleasedReturnValue, + // inout("rax") obj, + // clobber_abi("C"), + // ); + } + debug_assert_eq!( res, ptr, "objc_retainAutoreleasedReturnValue did not return the same pointer" diff --git a/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-darwin.s b/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-darwin.s index ed4943a42..bdec662f9 100644 --- a/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-darwin.s +++ b/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-darwin.s @@ -7,7 +7,13 @@ _handle: mov rbp, rsp call _objc_msgSend mov rdi, rax + call _objc_retainAutoreleasedReturnValue + ## InlineAsm Start + + nop + + ## InlineAsm End pop rbp - jmp _objc_retainAutoreleasedReturnValue + ret .subsections_via_symbols diff --git a/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-ios.s b/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-ios.s index ed4943a42..bdec662f9 100644 --- a/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-ios.s +++ b/tests/assembly/test_retain_autoreleased/expected/x86_64-apple-ios.s @@ -7,7 +7,13 @@ _handle: mov rbp, rsp call _objc_msgSend mov rdi, rax + call _objc_retainAutoreleasedReturnValue + ## InlineAsm Start + + nop + + ## InlineAsm End pop rbp - jmp _objc_retainAutoreleasedReturnValue + ret .subsections_via_symbols From d3632491df4a6d937c09bb4f43fa1157a9cb64c0 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Tue, 1 Mar 2022 09:30:35 +0100 Subject: [PATCH 5/6] Fix id_retain_autoreleased test on x86 --- objc2/tests/id_retain_autoreleased.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/objc2/tests/id_retain_autoreleased.rs b/objc2/tests/id_retain_autoreleased.rs index 39688e4cd..22c5941e0 100644 --- a/objc2/tests/id_retain_autoreleased.rs +++ b/objc2/tests/id_retain_autoreleased.rs @@ -11,13 +11,26 @@ fn retain_count(obj: &Object) -> usize { fn create_data(bytes: &[u8]) -> Id { let bytes_ptr = bytes.as_ptr() as *const c_void; unsafe { - // All code between the `msg_send!` and the `retain_autoreleased` must - // be able to be optimized away for this to work. + // let obj: *mut Object = msg_send![ + // class!(NSMutableData), + // dataWithBytes: bytes_ptr, + // length: bytes.len(), + // ]; + // + // On x86 (and perhaps others), dataWithBytes does not tail call + // `autorelease` and hence the return address points into that instead + // of our code, making the fast autorelease scheme fail. + // + // So instead, we call `autorelease` manually here. + let obj: *mut Object = msg_send![class!(NSMutableData), alloc]; let obj: *mut Object = msg_send![ - class!(NSData), - dataWithBytes: bytes_ptr, + obj, + initWithBytes: bytes_ptr, length: bytes.len(), ]; + let obj: *mut Object = msg_send![obj, autorelease]; + // All code between the `msg_send!` and the `retain_autoreleased` must + // be able to be optimized away for this to work. Id::retain_autoreleased(obj).unwrap() } } From e14935147a71f3cb7cf99401db22c3ca0ae503ae Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Tue, 1 Mar 2022 09:31:12 +0100 Subject: [PATCH 6/6] Fix running id_retain_autoreleased in CI --- .github/workflows/ci.yml | 11 ++++++++++- objc2/tests/id_retain_autoreleased.rs | 8 +++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5bd803a34..9f8dba124 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -302,6 +302,13 @@ jobs: args: --features ${{ env.FEATURES }} ${{ env.TESTARGS }} - name: Test in release mode + if: ${{ !matrix.dinghy }} + uses: actions-rs/cargo@v1 + with: + command: test + args: --no-default-features ${{ env.TESTARGS }} --release + + - name: Test in release mode with features if: ${{ !matrix.dinghy }} uses: actions-rs/cargo@v1 with: @@ -346,10 +353,12 @@ jobs: xcrun simctl boot $SIM_ID # Build - cargo dinghy build + cargo dinghy --device=$SIM_ID build # Run tests cargo dinghy --device=$SIM_ID test --no-default-features + cargo dinghy --device=$SIM_ID test --release + # Enable a few features. We're doing it this way because cargo dingy # doesn't support specifying features from a workspace. sed -i -e '/\[features\]/a\ diff --git a/objc2/tests/id_retain_autoreleased.rs b/objc2/tests/id_retain_autoreleased.rs index 22c5941e0..968ef018c 100644 --- a/objc2/tests/id_retain_autoreleased.rs +++ b/objc2/tests/id_retain_autoreleased.rs @@ -54,7 +54,13 @@ fn test_retain_autoreleased() { // When compiled in release mode / with optimizations enabled, // subsequent usage of `retain_autoreleased` will succeed in retaining // the autoreleased value! - let expected = if cfg!(all(debug_assertions, not(gnustep))) { + let expected = if cfg!(gnustep) { + 1 + } else if cfg!(any( + debug_assertions, + feature = "exception", + feature = "verify_message" + )) { 2 } else { 1