From 90ed6a7fe2055d711d49b84d0ff9a9c2d622aa91 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Tue, 21 Dec 2021 22:32:53 +0100 Subject: [PATCH 1/3] Mark most objc2 types Send and Sync again The change to OpaqueData is to make types !UnwindSafe by default --- block-sys/CHANGELOG.md | 2 ++ block-sys/src/lib.rs | 2 +- objc-sys/CHANGELOG.md | 3 ++ objc-sys/src/lib.rs | 12 ++++--- objc2/CHANGELOG.md | 7 +++- objc2/src/declare.rs | 18 ++++++++++ objc2/src/rc/autorelease.rs | 11 ++++-- objc2/src/runtime.rs | 71 +++++++++++++++++++++++++++++++++++-- 8 files changed, 114 insertions(+), 12 deletions(-) diff --git a/block-sys/CHANGELOG.md b/block-sys/CHANGELOG.md index 4ba02b6bc..76f1fb7a1 100644 --- a/block-sys/CHANGELOG.md +++ b/block-sys/CHANGELOG.md @@ -6,6 +6,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## Unreleased - YYYY-MM-DD +### Fixed +* **BREAKING**: `Class` is now `!UnwindSafe`. ## 0.0.1 - 2021-11-22 diff --git a/block-sys/src/lib.rs b/block-sys/src/lib.rs index 8afb76540..ed4825691 100644 --- a/block-sys/src/lib.rs +++ b/block-sys/src/lib.rs @@ -34,7 +34,7 @@ pub struct Class { _priv: [u8; 0], /// See objc_sys::OpaqueData - _opaque: PhantomData<(UnsafeCell<*const ()>, PhantomPinned)>, + _opaque: PhantomData<(UnsafeCell<()>, *const UnsafeCell<()>, PhantomPinned)>, } /// Block descriptor flags. diff --git a/objc-sys/CHANGELOG.md b/objc-sys/CHANGELOG.md index 7adfbb8aa..5a1565883 100644 --- a/objc-sys/CHANGELOG.md +++ b/objc-sys/CHANGELOG.md @@ -19,6 +19,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). present on other platforms anyhow, so users will just get an error at link-time. +### Fixed +* **BREAKING**: Opaque types are now also `!UnwindSafe`. + ## 0.1.0 - 2021-11-22 diff --git a/objc-sys/src/lib.rs b/objc-sys/src/lib.rs index 4017806a8..95b8f035b 100644 --- a/objc-sys/src/lib.rs +++ b/objc-sys/src/lib.rs @@ -56,9 +56,11 @@ pub use types::*; pub use various::*; /// We don't know much about the actual structs, so better mark them `!Send`, -/// `!Sync`, `!Unpin` and as mutable behind shared references. Downstream -/// libraries can always manually opt in to these types afterwards. (It's -/// also less of a breaking change on our part if we re-add these later). +/// `!Sync`, `!UnwindSafe`, `!RefUnwindSafe`, `!Unpin` and as mutable behind +/// shared references. /// -/// TODO: Replace this with `extern type` to also mark it as unsized. -type OpaqueData = PhantomData<(UnsafeCell<*const ()>, PhantomPinned)>; +/// Downstream libraries can always manually opt in to these types afterwards. +/// (It's also less of a breaking change on our part if we re-add these). +/// +/// TODO: Replace this with `extern type` to also mark it as `!Sized`. +type OpaqueData = PhantomData<(UnsafeCell<()>, *const UnsafeCell<()>, PhantomPinned)>; diff --git a/objc2/CHANGELOG.md b/objc2/CHANGELOG.md index 6ef525980..02e703e55 100644 --- a/objc2/CHANGELOG.md +++ b/objc2/CHANGELOG.md @@ -21,6 +21,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Fixed * An issue with inlining various `rc` methods. +* Most types (e.g. `Class` and `Method`) are now `Send`, `Sync`, `UnwindSafe` + and `RefUnwindSafe` again. + Notable exception is `Object`, because that depends on the specific + subclass. ## 0.3.0-alpha.4 - 2021-11-22 @@ -73,7 +77,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). `runtime` module. * **BREAKING**: Most types are now `!UnwindSafe`, to discourage trying to use them after an unwind. This restriction may be lifted in the future. -* **BREAKING**: Most types are now `!Send` and `!Sync`. **TODO**: Reevaluate this! +* **BREAKING**: Most types are now `!Send` and `!Sync`. This was an oversight + that is fixed in a later version. * A lot of smaller things. * **BREAKING**: Dynamic message sending with `Message::send_message` is moved to `MessageReceiver`. diff --git a/objc2/src/declare.rs b/objc2/src/declare.rs index 99992b586..691d5b197 100644 --- a/objc2/src/declare.rs +++ b/objc2/src/declare.rs @@ -120,6 +120,20 @@ pub struct ClassDecl { cls: *mut Class, } +// SAFETY: The stuff that touch global state does so using locks internally. +// +// Modifying the class itself can only be done through `&mut`, so Sync is +// safe (e.g. we can't accidentally call `add_ivar` at the same time from two +// different threads). +// +// (Though actually, that would be safe since the entire runtime is locked +// when doing so...). +// +// Finally, there are no requirements that the class must be registered on the +// same thread that allocated it. +unsafe impl Send for ClassDecl {} +unsafe impl Sync for ClassDecl {} + impl ClassDecl { fn with_superclass(name: &str, superclass: Option<&Class>) -> Option { let name = CString::new(name).unwrap(); @@ -296,6 +310,10 @@ pub struct ProtocolDecl { proto: *mut Protocol, } +// SAFETY: Similar to ClassDecl +unsafe impl Send for ProtocolDecl {} +unsafe impl Sync for ProtocolDecl {} + impl ProtocolDecl { /// Constructs a [`ProtocolDecl`] with the given name. /// diff --git a/objc2/src/rc/autorelease.rs b/objc2/src/rc/autorelease.rs index c27f47fb2..4cff998eb 100644 --- a/objc2/src/rc/autorelease.rs +++ b/objc2/src/rc/autorelease.rs @@ -21,12 +21,17 @@ pub struct AutoreleasePool { p: PhantomData<*mut UnsafeCell>, } -/// ```rust,compile_fail +/// ``` /// use objc2::rc::AutoreleasePool; -/// fn needs_sync() {} +/// fn needs_nothing() {} +/// needs_nothing::(); +/// ``` +/// ```compile_fail +/// use objc2::rc::AutoreleasePool; +/// fn needs_sync() {} /// needs_sync::(); /// ``` -/// ```rust,compile_fail +/// ```compile_fail /// use objc2::rc::AutoreleasePool; /// fn needs_send() {} /// needs_send::(); diff --git a/objc2/src/runtime.rs b/objc2/src/runtime.rs index b3bf21683..da48b84cf 100644 --- a/objc2/src/runtime.rs +++ b/objc2/src/runtime.rs @@ -5,6 +5,7 @@ use core::ffi::c_void; use core::fmt; +use core::panic::{RefUnwindSafe, UnwindSafe}; use core::ptr; use core::str; use malloc_buf::Malloc; @@ -50,6 +51,13 @@ pub struct Class(ffi::objc_class); pub struct Protocol(ffi::Protocol); /// A type that represents an instance of a class. +/// +/// Note: This is intentionally neither [`Sync`], [`Send`], [`UnwindSafe`], +/// [`RefUnwindSafe`] nor [`Unpin`], since it is something that changes +/// depending on the specific subclass. +/// +/// Examples: `NSAutoreleasePool` is not `Send`, it has to be deallocated +/// on the same thread that it was created. `NSLock` is not `Send` either. #[repr(C)] pub struct Object(ffi::objc_object); @@ -105,9 +113,12 @@ impl PartialEq for Sel { impl Eq for Sel {} -// Sel is safe to share across threads because it is immutable +// SAFETY: Sel is immutable (and can be retrieved from any thread using the +// `sel!` macro). unsafe impl Sync for Sel {} unsafe impl Send for Sel {} +impl UnwindSafe for Sel {} +impl RefUnwindSafe for Sel {} impl Copy for Sel {} @@ -147,6 +158,12 @@ impl Ivar { } } +// SAFETY: Ivar is immutable (and can be retrieved from Class anyhow). +unsafe impl Sync for Ivar {} +unsafe impl Send for Ivar {} +impl UnwindSafe for Ivar {} +impl RefUnwindSafe for Ivar {} + impl Method { pub(crate) fn as_ptr(&self) -> *const ffi::objc_method { self as *const Self as *const _ @@ -197,6 +214,12 @@ impl Method { // unsafe fn exchange_implementation(&mut self, other: &mut Method); } +// SAFETY: Method is immutable (and can be retrieved from Class anyhow). +unsafe impl Sync for Method {} +unsafe impl Send for Method {} +impl UnwindSafe for Method {} +impl RefUnwindSafe for Method {} + impl Class { pub(crate) fn as_ptr(&self) -> *const ffi::objc_class { self as *const Self as *const _ @@ -362,6 +385,14 @@ impl Class { // unsafe fn set_version(&mut self, version: u32); } +// SAFETY: Class is immutable (and can be retrieved from any thread using the +// `class!` macro). +unsafe impl Sync for Class {} +unsafe impl Send for Class {} +impl UnwindSafe for Class {} +impl RefUnwindSafe for Class {} +// Note that Unpin is not applicable. + unsafe impl RefEncode for Class { const ENCODING_REF: Encoding<'static> = Encoding::Class; } @@ -449,6 +480,13 @@ impl fmt::Debug for Protocol { } } +// SAFETY: Protocol is immutable (and can be retrieved from Class anyhow). +unsafe impl Sync for Protocol {} +unsafe impl Send for Protocol {} +impl UnwindSafe for Protocol {} +impl RefUnwindSafe for Protocol {} +// Note that Unpin is not applicable. + fn get_ivar_offset(cls: &Class, name: &str) -> isize { match cls.instance_variable(name) { Some(ivar) => { @@ -525,6 +563,24 @@ impl Object { // objc_removeAssociatedObjects } +/// ``` +/// use objc2::runtime::Object; +/// fn needs_nothing() {} +/// needs_nothing::(); +/// ``` +/// ```compile_fail +/// use objc2::runtime::Object; +/// fn needs_sync() {} +/// needs_sync::(); +/// ``` +/// ```compile_fail +/// use objc2::runtime::Object; +/// fn needs_send() {} +/// needs_send::(); +/// ``` +#[cfg(doctest)] +pub struct ObjectNotSendNorSync; + unsafe impl RefEncode for Object { const ENCODING_REF: Encoding<'static> = Encoding::Object; } @@ -539,7 +595,7 @@ impl fmt::Debug for Object { mod tests { use alloc::string::ToString; - use super::{Class, Object, Protocol, Sel}; + use super::{Bool, Class, Ivar, Method, Object, Protocol, Sel}; use crate::test_utils; use crate::Encode; @@ -646,4 +702,15 @@ mod tests { assert!(<&Class>::ENCODING.to_string() == "#"); assert!(Sel::ENCODING.to_string() == ":"); } + + #[test] + fn test_send_sync() { + fn assert_send_sync() {} + assert_send_sync::(); + assert_send_sync::(); + assert_send_sync::(); + assert_send_sync::(); + assert_send_sync::(); + assert_send_sync::(); + } } From e245f34bad38ee18400d7475f460bcfba8bba900 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Tue, 21 Dec 2021 21:14:36 +0100 Subject: [PATCH 2/3] Explicitly mark objc2-foundation Send and Sync-ness --- objc2-foundation/src/array.rs | 28 ++++++++++++++++++++++++++++ objc2-foundation/src/data.rs | 8 ++++++++ objc2-foundation/src/dictionary.rs | 4 ++++ objc2-foundation/src/string.rs | 4 ++++ objc2-foundation/src/value.rs | 4 ++++ 5 files changed, 48 insertions(+) diff --git a/objc2-foundation/src/array.rs b/objc2-foundation/src/array.rs index cfb6637b4..06eea453c 100644 --- a/objc2-foundation/src/array.rs +++ b/objc2-foundation/src/array.rs @@ -174,6 +174,16 @@ object!( } ); +// SAFETY: Same as Id (which is what NSArray effectively stores). +// +// The `PhantomData` can't get these impls to display in the docs. +// +// TODO: Properly verify this +unsafe impl Sync for NSArray {} +unsafe impl Send for NSArray {} +unsafe impl Sync for NSArray {} +unsafe impl Send for NSArray {} + unsafe impl INSArray for NSArray { /// The `NSArray` itself (length and number of items) is always immutable, /// but we would like to know when we're the only owner of the array, to @@ -319,6 +329,14 @@ object!( } ); +// SAFETY: Same as NSArray. +// +// TODO: Properly verify this +unsafe impl Sync for NSMutableArray {} +unsafe impl Send for NSMutableArray {} +unsafe impl Sync for NSMutableArray {} +unsafe impl Send for NSMutableArray {} + unsafe impl INSArray for NSMutableArray { type Ownership = Owned; type Item = T; @@ -540,4 +558,14 @@ mod tests { assert_eq!(strings[1].as_str(pool), "hello"); }); } + + #[test] + fn test_send_sync() { + fn assert_send_sync() {} + + assert_send_sync::>(); + assert_send_sync::>(); + assert_send_sync::, Shared>>(); + assert_send_sync::, Owned>>(); + } } diff --git a/objc2-foundation/src/data.rs b/objc2-foundation/src/data.rs index eb95b3a23..aed26318b 100644 --- a/objc2-foundation/src/data.rs +++ b/objc2-foundation/src/data.rs @@ -94,6 +94,10 @@ pub unsafe trait INSData: INSObject { object!(unsafe pub struct NSData); +// TODO: SAFETY +unsafe impl Sync for NSData {} +unsafe impl Send for NSData {} + unsafe impl INSData for NSData { type Ownership = Shared; } @@ -157,6 +161,10 @@ pub unsafe trait INSMutableData: INSData { object!(unsafe pub struct NSMutableData); +// TODO: SAFETY +unsafe impl Sync for NSMutableData {} +unsafe impl Send for NSMutableData {} + unsafe impl INSData for NSMutableData { type Ownership = Owned; } diff --git a/objc2-foundation/src/dictionary.rs b/objc2-foundation/src/dictionary.rs index d6bf59750..a68d63cb4 100644 --- a/objc2-foundation/src/dictionary.rs +++ b/objc2-foundation/src/dictionary.rs @@ -145,6 +145,10 @@ object!( } ); +// TODO: SAFETY +unsafe impl Sync for NSDictionary {} +unsafe impl Send for NSDictionary {} + impl NSDictionary { unsafe_def_fn!(pub fn new -> Shared); } diff --git a/objc2-foundation/src/string.rs b/objc2-foundation/src/string.rs index 06b4b21b7..b15730c17 100644 --- a/objc2-foundation/src/string.rs +++ b/objc2-foundation/src/string.rs @@ -102,6 +102,10 @@ pub unsafe trait INSString: INSObject { object!(unsafe pub struct NSString); +// TODO: SAFETY +unsafe impl Sync for NSString {} +unsafe impl Send for NSString {} + impl NSString { unsafe_def_fn!(pub fn new -> Shared); } diff --git a/objc2-foundation/src/value.rs b/objc2-foundation/src/value.rs index c23fe4e45..1df5194eb 100644 --- a/objc2-foundation/src/value.rs +++ b/objc2-foundation/src/value.rs @@ -75,6 +75,10 @@ object!( } ); +// TODO: SAFETY +unsafe impl Sync for NSValue {} +unsafe impl Send for NSValue {} + unsafe impl INSValue for NSValue { type Value = T; } From e478974cadb47157bcb75c112b7aca513d5d86bb Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Tue, 21 Dec 2021 21:15:04 +0100 Subject: [PATCH 3/3] Make NSObject !Send and !Sync Similar to objc2::runtime::Object --- objc2-foundation/CHANGELOG.md | 2 ++ objc2-foundation/src/array.rs | 10 ++++++++++ objc2-foundation/src/object.rs | 20 ++++++++++++++++++-- 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/objc2-foundation/CHANGELOG.md b/objc2-foundation/CHANGELOG.md index 1be45e9d6..241496e00 100644 --- a/objc2-foundation/CHANGELOG.md +++ b/objc2-foundation/CHANGELOG.md @@ -25,6 +25,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Fixed * Soundness issue with `NSValue`, `NSDictionary`, `NSArray` and `NSMutableArray` not being `#[repr(C)]`. +* **BREAKING**: `NSObject` is no longer `Send` and `Sync` (because its + subclasses may not be). ## 0.2.0-alpha.2 - 2021-11-22 diff --git a/objc2-foundation/src/array.rs b/objc2-foundation/src/array.rs index 06eea453c..bfac18047 100644 --- a/objc2-foundation/src/array.rs +++ b/objc2-foundation/src/array.rs @@ -184,6 +184,16 @@ unsafe impl Send for NSArray {} unsafe impl Sync for NSArray {} unsafe impl Send for NSArray {} +/// ```compile_fail +/// use objc2::rc::Shared; +/// use objc2::runtime::Object; +/// use objc2_foundation::NSArray; +/// fn needs_send_sync() {} +/// needs_send_sync::>(); +/// ``` +#[cfg(doctest)] +pub struct NSArrayWithObjectNotSendSync; + unsafe impl INSArray for NSArray { /// The `NSArray` itself (length and number of items) is always immutable, /// but we would like to know when we're the only owner of the array, to diff --git a/objc2-foundation/src/object.rs b/objc2-foundation/src/object.rs index 33d17c2b8..11cb2b3e7 100644 --- a/objc2-foundation/src/object.rs +++ b/objc2-foundation/src/object.rs @@ -1,8 +1,9 @@ +use core::marker::PhantomData; use core::ptr::NonNull; use objc2::msg_send; use objc2::rc::{Id, Owned, Shared}; -use objc2::runtime::{Bool, Class}; +use objc2::runtime::{Bool, Class, Object}; use objc2::Message; use super::NSString; @@ -37,7 +38,22 @@ pub unsafe trait INSObject: Sized + Message { } } -object!(unsafe pub struct NSObject); +object!(unsafe pub struct NSObject<> { + p: PhantomData, // Temporary +}); + +/// ```compile_fail +/// use objc2_foundation::NSObject; +/// fn needs_sync() {} +/// needs_sync::(); +/// ``` +/// ```compile_fail +/// use objc2_foundation::NSObject; +/// fn needs_send() {} +/// needs_send::(); +/// ``` +#[cfg(doctest)] +pub struct NSObjectNotSendNorSync; impl NSObject { unsafe_def_fn!(pub fn new -> Owned);