From 5e6f1af2c7094fb692cfc9721831c77905fb718a Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Thu, 14 Nov 2024 15:19:45 +0100 Subject: [PATCH] Fix tests in old runtime / on macOS 10.12 / with panic=abort --- .../src/__macro_helpers/msg_send_retained.rs | 93 ++++++++++++------- crates/objc2/src/exception.rs | 2 +- crates/objc2/src/rc/autorelease.rs | 5 +- crates/objc2/src/runtime/mod.rs | 11 ++- .../tests/protocol_shared_retain_count.rs | 4 + crates/objc2/tests/retain_autoreleased.rs | 49 ---------- crates/objc2/tests/track_caller.rs | 1 + crates/tests/src/exception.rs | 20 +++- .../objc2-core-foundation/src/number.rs | 7 +- .../objc2-foundation/src/tests/dictionary.rs | 8 ++ .../src/tests/mutable_array.rs | 9 +- .../objc2-metal/tests/null_error_handling.rs | 5 + 12 files changed, 121 insertions(+), 93 deletions(-) delete mode 100644 crates/objc2/tests/retain_autoreleased.rs diff --git a/crates/objc2/src/__macro_helpers/msg_send_retained.rs b/crates/objc2/src/__macro_helpers/msg_send_retained.rs index a5e94c6bb..015968c26 100644 --- a/crates/objc2/src/__macro_helpers/msg_send_retained.rs +++ b/crates/objc2/src/__macro_helpers/msg_send_retained.rs @@ -319,7 +319,7 @@ mod tests { use super::*; use crate::rc::{autoreleasepool, Allocated, PartialInit, RcTestObject, ThreadTestData}; - use crate::runtime::{AnyObject, NSObject, NSZone}; + use crate::runtime::{AnyObject, NSObject, NSObjectProtocol, NSZone}; use crate::{class, define_class, extern_methods, msg_send, test_utils, AllocAnyThread}; #[test] @@ -787,34 +787,29 @@ mod tests { let _obj: Retained = unsafe { msg_send![obj, description] }; } - // This is imperfect, but will do for now. - // See also `tests/id_retain_autoreleased.rs`. - // - // Work around https://github.com/rust-lang/rust-clippy/issues/9737: - #[allow(clippy::if_same_then_else)] - const IF_AUTORELEASE_NOT_SKIPPED: usize = if cfg!(feature = "gnustep-1-7") { - 1 - } else if cfg!(target_arch = "x86") { - // x86 autorelease_return is not currently tail-called, so the - // optimization doesn't work on define_class! functions. - 2 - } else if cfg!(target_arch = "aarch64") { - // Currently doesn't work - 2 - } else if cfg!(any(debug_assertions, feature = "catch-all")) { - 2 - } else { - 1 - } - 1; - - // 32-bit ARM unwinding sometimes interferes with the optimization - const IF_AUTORELEASE_NOT_SKIPPED_ARM_HACK: usize = { - if cfg!(all(target_arch = "arm", panic = "unwind")) { - 1 + /// This is imperfect, but will do for now. + const fn autorelease_skipped(self_declared: bool) -> bool { + if cfg!(feature = "gnustep-1-7") { + // GNUStep does the optimization a different way, so it isn't + // optimization-dependent. + true + } else if cfg!(all(target_arch = "arm", panic = "unwind")) { + // 32-bit ARM unwinding sometimes interferes with the optimization + false + } else if self_declared { + // FIXME: Autorelease_return is not currently tail-called, so the + // optimization doesn't work on define_class! functions. + false + } else if cfg!(feature = "catch-all") { + // FIXME: `catch-all` is inserted before we get a chance to retain. + false + } else if cfg!(debug_assertions) { + // `debug_assertions` ~proxy for if optimizations are off. + false } else { - IF_AUTORELEASE_NOT_SKIPPED + true } - }; + } macro_rules! test_error_retained { ($expected:expr, $if_autorelease_not_skipped:expr, $sel:ident, $($obj:tt)*) => { @@ -868,7 +863,7 @@ mod tests { let cls = RcTestObject::class(); test_error_retained!( expected, - IF_AUTORELEASE_NOT_SKIPPED_ARM_HACK, + if autorelease_skipped(true) { 0 } else { 1 }, idAndShouldError, cls ); @@ -879,7 +874,7 @@ mod tests { expected.init += 1; test_error_retained!( expected, - IF_AUTORELEASE_NOT_SKIPPED_ARM_HACK, + if autorelease_skipped(true) { 0 } else { 1 }, idAndShouldError, &obj ); @@ -913,15 +908,49 @@ mod tests { assert!(res.is_some()); expected.alloc += 1; expected.init += 1; - expected.autorelease += IF_AUTORELEASE_NOT_SKIPPED_ARM_HACK; - expected.retain += IF_AUTORELEASE_NOT_SKIPPED_ARM_HACK; + expected.autorelease += if autorelease_skipped(true) { 0 } else { 1 }; + expected.retain += if autorelease_skipped(true) { 0 } else { 1 }; expected.assert_current(); res }); - expected.release += IF_AUTORELEASE_NOT_SKIPPED_ARM_HACK; + expected.release += if autorelease_skipped(true) { 0 } else { 1 }; expected.assert_current(); } + fn create_obj() -> Retained { + let obj = ManuallyDrop::new(NSObject::new()); + unsafe { + let obj: *mut NSObject = 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. + Retained::retain_autoreleased(obj).unwrap() + } + } + + #[test] + fn test_retain_autoreleased() { + autoreleasepool(|_| { + // Run once to allow DYLD to resolve the symbol stubs. + // Required for making `retain_autoreleased` work on x86_64. + let _data = create_obj(); + + // When compiled in release mode / with optimizations enabled, + // subsequent usage of `retain_autoreleased` will succeed in + // retaining the autoreleased value! + let expected = if autorelease_skipped(false) { 1 } else { 2 }; + + let data = create_obj(); + assert_eq!(data.retainCount(), expected); + + let data = create_obj(); + assert_eq!(data.retainCount(), expected); + + // Here we manually clean up the autorelease, so it will always be 1. + let data = autoreleasepool(|_| create_obj()); + assert_eq!(data.retainCount(), 1); + }); + } + #[test] fn msg_send_class() { let cls = NSObject::class(); diff --git a/crates/objc2/src/exception.rs b/crates/objc2/src/exception.rs index dfea8ab1c..0e850871c 100644 --- a/crates/objc2/src/exception.rs +++ b/crates/objc2/src/exception.rs @@ -363,7 +363,7 @@ mod tests { #[test] #[cfg_attr( - all(target_vendor = "apple", target_os = "macos", target_arch = "x86"), + all(target_os = "macos", target_arch = "x86"), ignore = "`NULL` exceptions are invalid on 32-bit / w. fragile runtime" )] fn test_catch_null() { diff --git a/crates/objc2/src/rc/autorelease.rs b/crates/objc2/src/rc/autorelease.rs index 65ba48977..4666a76aa 100644 --- a/crates/objc2/src/rc/autorelease.rs +++ b/crates/objc2/src/rc/autorelease.rs @@ -57,7 +57,7 @@ impl Pool { /// `drop` when possible, so that is what we are going to do. /// /// [clang documentation]: https://clang.llvm.org/docs/AutomaticReferenceCounting.html#autoreleasepool - /// [revision `551.1`]: https://github.com/apple-oss-distributions/objc4/blob/objc4-551.1/runtime/objc-exception.mm#L516 + /// [revision `551.1`]: https://github.com/apple-oss-distributions/objc4/blob/objc4-551.1/runtime/objc-exception.mm#L540 #[inline] unsafe fn drain(self) { #[cfg(all(target_os = "macos", target_arch = "x86"))] @@ -582,8 +582,9 @@ mod tests { } #[test] + #[cfg_attr(panic = "abort", ignore = "requires `catch_unwind`")] #[cfg_attr( - all(target_os = "macos", target_arch = "x86"), + all(target_os = "macos", target_arch = "x86", not(panic = "abort")), ignore = "unwinding through an auto release pool on macOS 32 bit won't pop the pool" )] fn test_unwind_still_autoreleases() { diff --git a/crates/objc2/src/runtime/mod.rs b/crates/objc2/src/runtime/mod.rs index 18b76fd55..8fcedad2b 100644 --- a/crates/objc2/src/runtime/mod.rs +++ b/crates/objc2/src/runtime/mod.rs @@ -1886,8 +1886,15 @@ mod tests { assert_eq!(&*retained, protocol); // Protocols don't implement isKindOfClass: on GNUStep. - if !cfg!(feature = "gnustep-1-7") { - // Protocols are NSObject subclasses. + if cfg!(feature = "gnustep-1-7") { + return; + } + + // In the old runtime, NSObjectProtocol are not NSObject subclasses. + if cfg!(all(target_os = "macos", target_arch = "x86")) { + let _ = retained.downcast::().unwrap_err(); + } else { + // But elsewhere they are. let obj = retained.downcast::().unwrap(); // Test that we can call NSObject methods on protocols. assert_eq!(obj, obj); diff --git a/crates/objc2/tests/protocol_shared_retain_count.rs b/crates/objc2/tests/protocol_shared_retain_count.rs index 41d686459..c3ed4e411 100644 --- a/crates/objc2/tests/protocol_shared_retain_count.rs +++ b/crates/objc2/tests/protocol_shared_retain_count.rs @@ -11,6 +11,10 @@ use objc2::{Message, ProtocolType}; feature = "gnustep-1-7", ignore = "Protocols don't implement isKindOfClass: on GNUStep" )] +#[cfg_attr( + all(target_os = "macos", target_arch = "x86"), + ignore = "protocols are not NSObject subclasses in the old runtime" +)] fn protocol_has_shared_retain_count() { let obj: &AnyObject = ::protocol().unwrap().as_ref(); let obj = obj.downcast_ref::().unwrap(); diff --git a/crates/objc2/tests/retain_autoreleased.rs b/crates/objc2/tests/retain_autoreleased.rs deleted file mode 100644 index c29c67866..000000000 --- a/crates/objc2/tests/retain_autoreleased.rs +++ /dev/null @@ -1,49 +0,0 @@ -use core::mem::ManuallyDrop; - -use objc2::msg_send; -use objc2::rc::{autoreleasepool, Retained}; -use objc2::runtime::{NSObject, NSObjectProtocol}; - -fn create_obj() -> Retained { - let obj = ManuallyDrop::new(NSObject::new()); - unsafe { - let obj: *mut NSObject = 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. - Retained::retain_autoreleased(obj).unwrap() - } -} - -#[test] -fn test_retain_autoreleased() { - autoreleasepool(|_| { - // Run once to allow DYLD to resolve the symbol stubs. - // Required for making `retain_autoreleased` work on x86_64. - let _data = create_obj(); - - // When compiled in release mode / with optimizations enabled, - // subsequent usage of `retain_autoreleased` will succeed in retaining - // the autoreleased value! - #[allow(clippy::if_same_then_else)] - let expected = if cfg!(feature = "gnustep-1-7") { - 1 - } else if cfg!(all(target_arch = "arm", panic = "unwind")) { - // 32-bit ARM unwinding interferes with the optimization - 2 - } else if cfg!(any(debug_assertions, feature = "catch-all")) { - 2 - } else { - 1 - }; - - let data = create_obj(); - assert_eq!(data.retainCount(), expected); - - let data = create_obj(); - assert_eq!(data.retainCount(), expected); - - // Here we manually clean up the autorelease, so it will always be 1. - let data = autoreleasepool(|_| create_obj()); - assert_eq!(data.retainCount(), 1); - }); -} diff --git a/crates/objc2/tests/track_caller.rs b/crates/objc2/tests/track_caller.rs index 61ae58e2e..52a2d6c8e 100644 --- a/crates/objc2/tests/track_caller.rs +++ b/crates/objc2/tests/track_caller.rs @@ -75,6 +75,7 @@ impl Drop for PanicChecker { } #[test] +#[cfg_attr(panic = "abort", ignore = "requires `catch_unwind`")] fn test_track_caller() { let checker = PanicChecker::new(); diff --git a/crates/tests/src/exception.rs b/crates/tests/src/exception.rs index ddcee0018..0a314d763 100644 --- a/crates/tests/src/exception.rs +++ b/crates/tests/src/exception.rs @@ -26,11 +26,19 @@ fn throw_catch_raise_catch() { let exc = res.unwrap_err().unwrap(); let exc = NSException::from_exception(exc).unwrap(); - assert_eq!(exc.retainCount(), 1); + if cfg!(all(target_os = "macos", target_arch = "x86")) { + assert_eq!(exc.retainCount(), 2); + } else { + assert_eq!(exc.retainCount(), 1); + } exc }); - assert_eq!(exc.retainCount(), 1); + if cfg!(all(target_os = "macos", target_arch = "x86")) { + assert_eq!(exc.retainCount(), 2); + } else { + assert_eq!(exc.retainCount(), 1); + } let exc = autoreleasepool(|_| { let res = catch(|| { @@ -42,9 +50,11 @@ fn throw_catch_raise_catch() { 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_eq!(exc.retainCount(), 2); + if cfg!(all(target_os = "macos", target_arch = "x86")) { + assert_eq!(exc.retainCount(), 3); + } else { + assert_eq!(exc.retainCount(), 1); + } exc }); diff --git a/framework-crates/objc2-core-foundation/src/number.rs b/framework-crates/objc2-core-foundation/src/number.rs index 50bd14c70..348888f48 100644 --- a/framework-crates/objc2-core-foundation/src/number.rs +++ b/framework-crates/objc2-core-foundation/src/number.rs @@ -143,7 +143,12 @@ mod tests { #[test] fn to_from_number() { let n = CFNumber::new_i32(442); - assert_eq!(n.as_i8(), Some(442i32 as i8)); + if cfg!(all(target_os = "macos", target_arch = "x86")) { + // Seems to return `None` in the old runtime. + assert_eq!(n.as_i8(), None); + } else { + assert_eq!(n.as_i8(), Some(442i32 as i8)); + } assert_eq!(n.as_i16(), Some(442)); assert_eq!(n.as_i32(), Some(442)); assert_eq!(n.as_i64(), Some(442)); diff --git a/framework-crates/objc2-foundation/src/tests/dictionary.rs b/framework-crates/objc2-foundation/src/tests/dictionary.rs index 21f8689d4..57ef1a426 100644 --- a/framework-crates/objc2-foundation/src/tests/dictionary.rs +++ b/framework-crates/objc2-foundation/src/tests/dictionary.rs @@ -159,6 +159,10 @@ fn test_from_base_class(cls: &AnyClass) { feature = "gnustep-1-7", ignore = "GNUStep stack overflows here for some reason?" )] +#[cfg_attr( + all(target_os = "macos", target_arch = "x86"), + ignore = "the old runtime seems to have been compiled with -fno-exceptions?" +)] fn no_copy() { let mut builder = base_class_builder("NoCopy").unwrap(); @@ -190,6 +194,10 @@ fn no_copy() { feature = "gnustep-1-7", ignore = "GNUStep stack overflows here for some reason?" )] +#[cfg_attr( + all(target_os = "macos", target_arch = "x86"), + ignore = "the old runtime seems to have been compiled with -fno-exceptions?" +)] fn no_is_equal_hash() { let mut builder = base_class_builder("NoIsEqualHash").unwrap(); diff --git a/framework-crates/objc2-foundation/src/tests/mutable_array.rs b/framework-crates/objc2-foundation/src/tests/mutable_array.rs index bf041f81c..504b36695 100644 --- a/framework-crates/objc2-foundation/src/tests/mutable_array.rs +++ b/framework-crates/objc2-foundation/src/tests/mutable_array.rs @@ -113,7 +113,14 @@ fn test_sort() { #[test] #[cfg(feature = "NSValue")] -#[should_panic = "`itemsPtr` was NULL, likely due to mutation during iteration"] +#[cfg_attr( + all(target_os = "macos", target_arch = "x86"), + should_panic = "mutation detected during enumeration" +)] +#[cfg_attr( + not(all(target_os = "macos", target_arch = "x86")), + should_panic = "`itemsPtr` was NULL, likely due to mutation during iteration" +)] #[cfg_attr(feature = "gnustep-1-7", ignore = "errors differently on GNUStep")] fn null_itemsptr() { use crate::NSNumber; diff --git a/framework-crates/objc2-metal/tests/null_error_handling.rs b/framework-crates/objc2-metal/tests/null_error_handling.rs index 45be1cfcc..2868637d3 100644 --- a/framework-crates/objc2-metal/tests/null_error_handling.rs +++ b/framework-crates/objc2-metal/tests/null_error_handling.rs @@ -1,5 +1,6 @@ //! Regression test for https://github.com/madsmtm/objc2/issues/653. #![cfg(all(feature = "MTLBinaryArchive", feature = "MTLDevice"))] +use objc2::available; use objc2_foundation::{ns_string, NSURL}; use objc2_metal::{ MTLBinaryArchive, MTLBinaryArchiveDescriptor, MTLCreateSystemDefaultDevice, MTLDevice, @@ -14,6 +15,10 @@ fn test() { // Ignore, this won't work in CI. return; }; + // `MTLBinaryArchiveDescriptor` is unavailable before these. + if !available!(ios = 14.0, macos = 11.0, tvos = 14.0, visionos = 1.0) { + return; + } let binary_archive = device .newBinaryArchiveWithDescriptor_error(&MTLBinaryArchiveDescriptor::new()) .unwrap();