From 624dab17ee0ec4fc65766870171b52b093d01e72 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 12 Apr 2023 19:26:28 +0300 Subject: [PATCH 1/2] Improve objc-sys documentation slightly --- crates/objc-sys/CHANGELOG.md | 3 ++ crates/objc-sys/src/constants.rs | 38 ++++++++++++++++++-- crates/objc-sys/src/image_info.rs | 3 ++ crates/objc-sys/src/protocol.rs | 2 -- crates/objc-sys/src/types.rs | 59 ++++++++++++++++++++----------- 5 files changed, 81 insertions(+), 24 deletions(-) diff --git a/crates/objc-sys/CHANGELOG.md b/crates/objc-sys/CHANGELOG.md index c72ad72a5..ae303f0c3 100644 --- a/crates/objc-sys/CHANGELOG.md +++ b/crates/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 +* Improved documentation slightly. + ## 0.3.0 - 2023-02-07 diff --git a/crates/objc-sys/src/constants.rs b/crates/objc-sys/src/constants.rs index 9d2fb02f8..bd92cc9ad 100644 --- a/crates/objc-sys/src/constants.rs +++ b/crates/objc-sys/src/constants.rs @@ -19,12 +19,32 @@ pub const nil: id = 0 as *mut _; /// A quick alias for a [`null_mut`][`core::ptr::null_mut`] class. pub const Nil: *mut objc_class = 0 as *mut _; +/// Policies related to associative references. +/// +/// These are options to [`objc_setAssociatedObject`]. +/// +/// [`objc_setAssociatedObject`]: crate::objc_setAssociatedObject pub type objc_AssociationPolicy = usize; +/// Specifies a weak reference to the associated object. +/// +/// This performs straight assignment, without message sends. pub const OBJC_ASSOCIATION_ASSIGN: objc_AssociationPolicy = 0; +/// Specifies a strong reference to the associated object. +/// +/// The association is not made atomically. pub const OBJC_ASSOCIATION_RETAIN_NONATOMIC: objc_AssociationPolicy = 1; +/// Specifies that the associated object is copied. +/// +/// The association is not made atomically. pub const OBJC_ASSOCIATION_COPY_NONATOMIC: objc_AssociationPolicy = 3; -pub const OBJC_ASSOCIATION_RETAIN: objc_AssociationPolicy = 769; -pub const OBJC_ASSOCIATION_COPY: objc_AssociationPolicy = 771; +/// Specifies a strong reference to the associated object. +/// +/// The association is made atomically. +pub const OBJC_ASSOCIATION_RETAIN: objc_AssociationPolicy = 0o1401; +/// Specifies that the associated object is copied. +/// +/// The association is made atomically. +pub const OBJC_ASSOCIATION_COPY: objc_AssociationPolicy = 0o1403; #[cfg(any(doc, apple))] pub const OBJC_SYNC_SUCCESS: c_int = 0; @@ -36,3 +56,17 @@ pub const OBJC_SYNC_TIMED_OUT: c_int = -2; /// Only relevant before macOS 10.13 #[cfg(any(doc, apple))] pub const OBJC_SYNC_NOT_INITIALIZED: c_int = -3; + +#[cfg(test)] +mod tests { + use super::*; + #[test] + fn test_association_policy() { + assert_eq!(OBJC_ASSOCIATION_RETAIN, 769); + assert_eq!(OBJC_ASSOCIATION_COPY, 771); + + // What the GNUStep headers define + assert_eq!(OBJC_ASSOCIATION_RETAIN, 0x301); + assert_eq!(OBJC_ASSOCIATION_COPY, 0x303); + } +} diff --git a/crates/objc-sys/src/image_info.rs b/crates/objc-sys/src/image_info.rs index f0a8b2931..a05020100 100644 --- a/crates/objc-sys/src/image_info.rs +++ b/crates/objc-sys/src/image_info.rs @@ -1,3 +1,6 @@ +// TODO: Move this to `objc2` once we can detect simulator targets without a +// build script. + /// Note: While `objc2` relies on this, you can freely break this, since it is /// only used behind experimental features (`unstable-static-*`). #[repr(C)] diff --git a/crates/objc-sys/src/protocol.rs b/crates/objc-sys/src/protocol.rs index cef8088e8..47cb77d68 100644 --- a/crates/objc-sys/src/protocol.rs +++ b/crates/objc-sys/src/protocol.rs @@ -30,12 +30,10 @@ extern_c! { #[cfg(any(doc, 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/crates/objc-sys/src/types.rs b/crates/objc-sys/src/types.rs index 8cd4a68ad..c0bcc7a22 100644 --- a/crates/objc-sys/src/types.rs +++ b/crates/objc-sys/src/types.rs @@ -105,14 +105,16 @@ pub type BOOL = inner::BOOL; // // Likewise for NSUInteger. // +// // ## GNUStep / WinObjC // // Defined as intptr_t/uintptr_t, which is exactly the same as isize/usize. // +// // ## ObjFW // -// Doesn't define these, but e.g. `OFString -length` returns size_t, so our -// definitions are should be correct on effectively all targets. +// Doesn't define these, but e.g. -[OFString length] returns size_t, so our +// definitions should be correct on effectively all targets. // // Things might change slightly in the future, see // . @@ -127,15 +129,23 @@ pub type BOOL = inner::BOOL; /// /// [docs]: https://developer.apple.com/documentation/objectivec/nsinteger?language=objc /// +/// /// # Examples /// /// ``` -/// #[repr(isize)] // NSInteger +/// use core::mem::size_of; +/// # use objc_sys::NSInteger; +/// # #[cfg(not_available)] +/// use objc2::ffi::NSInteger; +/// +/// #[repr(isize)] /// pub enum NSComparisonResult { /// NSOrderedAscending = -1, /// NSOrderedSame = 0, /// NSOrderedDescending = 1, /// } +/// +/// assert_eq!(size_of::(), size_of::()); /// ``` pub type NSInteger = isize; @@ -149,36 +159,43 @@ pub type NSInteger = isize; /// /// [docs]: https://developer.apple.com/documentation/objectivec/nsuinteger?language=objc /// +/// /// # Examples /// /// ``` -/// use objc_sys::NSUInteger; -/// // Or: -/// // use objc2::ffi::NSUInteger; -/// // use icrate::Foundation::NSUInteger; +/// # use objc_sys::NSUInteger; +/// # #[cfg(not_available)] +/// use objc2::ffi::NSUInteger; +/// /// extern "C" { /// fn some_external_function() -> NSUInteger; /// } /// ``` /// /// ``` -/// #[repr(usize)] // NSUInteger -/// enum NSRoundingMode { -/// NSRoundPlain = 0, -/// NSRoundDown = 1, -/// NSRoundUp = 2, -/// NSRoundBankers = 3, -/// }; +/// use core::mem::size_of; +/// # use objc_sys::NSUInteger; +/// # #[cfg(not_available)] +/// use objc2::ffi::NSUInteger; +/// +/// #[repr(usize)] +/// enum CLRegionState { +/// Unknown = 0, +/// Inside = 1, +/// Outside = 2, +/// } +/// +/// assert_eq!(size_of::(), size_of::()); /// ``` pub type NSUInteger = usize; -/// The maximum value for an NSInteger. +/// The maximum value for a [`NSInteger`]. pub const NSIntegerMax: NSInteger = NSInteger::MAX; -/// The minimum value for an NSInteger. +/// The minimum value for a [`NSInteger`]. pub const NSIntegerMin: NSInteger = NSInteger::MIN; -/// The maximum value for an NSUInteger. +/// The maximum value for a [`NSUInteger`]. pub const NSUIntegerMax: NSUInteger = NSUInteger::MAX; /// An immutable pointer to a selector. @@ -189,7 +206,9 @@ pub type SEL = *const objc_selector; /// A mutable pointer to an object / instance. /// -/// Type alias provided for convenience. See `objc2::runtime::Object` for a -/// higher level binding, and `objc2::rc::Id` for an easier way of handling -/// objects. +/// Type alias provided for convenience. You'll likely want to use one of: +/// - `icrate::Foundation::NS[...]` for when you know the class of the object +/// you're dealing with. +/// - `objc2::rc::Id` for a proper way of doing memory management. +/// - `objc2::runtime::Object` for a bit safer representation of this. pub type id = *mut objc_object; From 475e7cc4cbef58924889f78670d39ec41fc72160 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Sat, 8 Apr 2023 15:18:58 +0300 Subject: [PATCH 2/2] Fix exception memory management --- crates/objc-sys/build.rs | 14 ++++--- crates/objc-sys/extern/exception.m | 9 ++--- crates/objc2/CHANGELOG.md | 1 + crates/objc2/src/exception.rs | 3 ++ crates/tests/src/exception.rs | 64 +++++++++++++++++------------- 5 files changed, 52 insertions(+), 39 deletions(-) diff --git a/crates/objc-sys/build.rs b/crates/objc-sys/build.rs index 2c58b9329..2494b5da6 100644 --- a/crates/objc-sys/build.rs +++ b/crates/objc-sys/build.rs @@ -210,9 +210,7 @@ fn main() { // - ... // // TODO: -fobjc-weak ? - let mut cc_args = format!( - "-fobjc-arc -fobjc-arc-exceptions -fobjc-exceptions -fobjc-runtime={clang_runtime}" - ); + let mut cc_args = format!("-fobjc-exceptions -fobjc-runtime={clang_runtime}"); if let Runtime::ObjFW(_) = &runtime { // Add compability headers to make `#include ` work. @@ -221,8 +219,6 @@ fn main() { cc_args.push_str(compat_headers.to_str().unwrap()); } - println!("cargo:cc_args={cc_args}"); // DEP_OBJC_[version]_CC_ARGS - if let Runtime::ObjFW(_) = &runtime { // Link to libobjfw-rt println!("cargo:rustc-link-lib=dylib=objfw-rt"); @@ -250,10 +246,18 @@ fn main() { let mut builder = cc::Build::new(); builder.file("extern/exception.m"); + // Compile with exceptions enabled and with the correct runtime, but + // _without ARC_! for flag in cc_args.split(' ') { builder.flag(flag); } builder.compile("librust_objc_sys_0_3_try_catch_exception.a"); } + + // Add this to the `CC` args _after_ we've omitted it when compiling + // `extern/exception.m`. + cc_args.push_str(" -fobjc-arc -fobjc-arc-exceptions"); + + println!("cargo:cc_args={cc_args}"); // DEP_OBJC_[version]_CC_ARGS } diff --git a/crates/objc-sys/extern/exception.m b/crates/objc-sys/extern/exception.m index 4bb7b2707..f2edf9067 100644 --- a/crates/objc-sys/extern/exception.m +++ b/crates/objc-sys/extern/exception.m @@ -12,14 +12,11 @@ unsigned char rust_objc_sys_0_3_try_catch_exception(void (*f)(void *), void *context, id *error) { @try { f(context); - if (error) { - *error = (id)0; // nil - } return 0; } @catch (id exception) { - if (error) { - *error = objc_retain(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! + *error = objc_retain(exception); return 1; } } diff --git a/crates/objc2/CHANGELOG.md b/crates/objc2/CHANGELOG.md index aa134ad48..db00eebe6 100644 --- a/crates/objc2/CHANGELOG.md +++ b/crates/objc2/CHANGELOG.md @@ -68,6 +68,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Fixed * Fixed using autorelease pools on 32bit macOS and older macOS versions. +* Fixed memory leaks in and improved performance of `exception::catch`. ### Removed * **BREAKING**: Removed `rc::SliceId`, since it is trivially implementable diff --git a/crates/objc2/src/exception.rs b/crates/objc2/src/exception.rs index 1e95cc74e..9d5673322 100644 --- a/crates/objc2/src/exception.rs +++ b/crates/objc2/src/exception.rs @@ -237,6 +237,9 @@ unsafe fn try_no_ret(closure: F) -> Result<(), Option // 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 `Id` to it here. diff --git a/crates/tests/src/exception.rs b/crates/tests/src/exception.rs index 3a4d32496..a3da4d782 100644 --- a/crates/tests/src/exception.rs +++ b/crates/tests/src/exception.rs @@ -22,17 +22,8 @@ fn throw_catch_raise_catch() { let reason = NSString::from_str("def"); let exc = NSException::new(&name, Some(&reason), None).unwrap(); - // TODO: Investigate why this is required on GNUStep! - let _exc2 = exc.clone(); - assert_retain_count(&exc, 2); - - // TODO: Investigate this! - let extra_retain = usize::from(cfg!(all( - feature = "apple", - target_os = "macos", - target_arch = "x86" - ))); + assert_retain_count(&exc, 1); let exc = autoreleasepool(|_| { let exc = NSException::into_exception(exc); @@ -40,11 +31,30 @@ fn throw_catch_raise_catch() { let exc = res.unwrap_err().unwrap(); let exc = NSException::from_exception(exc).unwrap(); - assert_retain_count(&exc, 3 + extra_retain); + assert_retain_count(&exc, 1); + exc + }); + + assert_retain_count(&exc, 1); + + let exc = autoreleasepool(|_| { + let inner = || { + autoreleasepool(|pool| { + let exc = Id::autorelease(exc, pool); + unsafe { exc.raise() } + }) + }; + + let res = unsafe { catch(inner) }; + let exc = NSException::from_exception(res.unwrap_err().unwrap()).unwrap(); + + // Undesired: The inner pool _should_ have been drained on unwind, but + // it isn't, see `rc::Pool::drain`. + assert_retain_count(&exc, 2); exc }); - assert_retain_count(&exc, 2 + extra_retain); + assert_retain_count(&exc, 1); assert_eq!(exc.name(), name); assert_eq!(exc.reason().unwrap(), reason); @@ -85,28 +95,26 @@ fn raise_catch() { let exc = NSException::new(&name, Some(&reason), None).unwrap(); assert_retain_count(&exc, 1); - let res = autoreleasepool(|_| { - let res = unsafe { - catch(|| { - if exc.name() == name { - exc.raise(); - } else { - 4 - } - }) + let exc = autoreleasepool(|pool| { + let exc = Id::autorelease(exc, pool); + let inner = || { + if exc.name() == name { + unsafe { exc.raise() }; + } else { + 42 + } }; - assert_retain_count(&exc, 3); + let res = unsafe { catch(inner) }.unwrap_err().unwrap(); + assert_retain_count(&exc, 2); res }); - assert_retain_count(&exc, 2); - - let obj = res.unwrap_err().unwrap(); + assert_retain_count(&exc, 1); - assert_eq!(format!("{obj}"), "def"); + assert_eq!(format!("{exc}"), "def"); assert_eq!( - format!("{obj:?}"), - format!("exception 'abc' reason:def", &*obj) + format!("{exc:?}"), + format!("exception 'abc' reason:def", &*exc) ); }