From 4bfc0444a1b0b06df598b6a7e0489583b4a9f546 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Fri, 2 Jun 2023 11:16:49 +0200 Subject: [PATCH] Gd: replace Deref/DerefMut with base() + base_mut(); rework InstanceStorage No more implicit access to base; avoids runtime-borrowing (Godot objects don't have Rust borrow limits) and makes access more explicit. InstanceStorage is now only accessed via shared-ref, which should fix some soundness issues with unbounded &mut. All fields are immutable or have interior mutability. --- godot-core/src/obj/gd.rs | 83 +++++++++++---- godot-core/src/obj/traits.rs | 21 +++- godot-core/src/registry.rs | 17 +++- godot-core/src/storage.rs | 111 ++++++++++----------- godot-macros/src/derive_godot_class/mod.rs | 26 ----- itest/rust/src/base_test.rs | 53 +++++++--- itest/rust/src/signal_test.rs | 4 +- 7 files changed, 187 insertions(+), 128 deletions(-) diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 325c918f9..2d6ccd14e 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -160,25 +160,68 @@ where GdMut::from_cell(self.storage().get_mut()) } - /// Storage object associated with the extension instance - // FIXME proper + safe interior mutability, also that Clippy is happy - #[allow(clippy::mut_from_ref)] - pub(crate) fn storage(&self) -> &mut InstanceStorage { - let callbacks = crate::storage::nop_instance_callbacks(); + /// Access base without locking the user object. + /// + /// If you need mutations, use [`base_mut()`][Self::base_mut]. If you need a value copy, use [`base().share()`][Share::share]. + pub fn base(&self) -> &Gd { + self.storage().base().deref() + } + + /// Access base mutably without locking the user object. + /// + /// The idea is to support the `gd.base_mut().mutating_method()` pattern, which is not allowed with `gd.base()`. + /// If you need a copy of a `Gd` object, use `base().share()`. + /// + /// The return type is currently `Gd` and not `&mut Gd` because of `&mut` aliasing restrictions. This may change in the future. + pub fn base_mut(&mut self) -> Gd { + // Note: we cannot give safe mutable access to a base without RefCell, because multiple Gd pointers could call base_mut(), + // leading to aliased &mut. And we don't want RefCell, as C++ objects (nodes etc.) don't underly Rust's exclusive-ref limitations. + // The whole point of the Gd::base*() methods are to not require runtime-exclusive access to the Rust object. + // + // This is not a problem when accessing the `#[base]` field of a user struct directly, because `self` is guarded by the + // RefCell/RwLock in the InstanceStorage. + // + // Here, we instead return a copy (for now), which for the user looks mostly the same. The idea is that: + // - gd.base().mutating_method() fails + // - gd.base_mut().mutating_method() works. + // + // Downside: small ref-counting overhead for `RefCounted` types. If this is an issue in a real game (highly unlikely), + // the user could always cache Gd base pointers. + self.storage().base().share() + } + /// Storage object associated with the extension instance. + pub(crate) fn storage(&self) -> &InstanceStorage { + // SAFETY: instance pointer belongs to this instance. We only get a shared reference, no exclusive access, so even + // calling this from multiple Gd pointers is safe. + // Potential issue is a concurrent free() in multi-threaded access; but that would need to be guarded against inside free(). unsafe { - let token = sys::get_library() as *mut std::ffi::c_void; - let binding = - interface_fn!(object_get_instance_binding)(self.obj_sys(), token, &callbacks); - - debug_assert!( - !binding.is_null(), - "Class {} -- null instance; does the class have a Godot creator function?", - std::any::type_name::() - ); - crate::private::as_storage::(binding as sys::GDExtensionClassInstancePtr) + let binding = self.resolve_instance_ptr(); + crate::private::as_storage::(binding) } } + + /// Storage object associated with the extension instance. + // pub(crate) fn storage_mut(&mut self) -> &mut InstanceStorage { + // // SAFETY: + // unsafe { + // let binding = self.resolve_instance_ptr(); + // crate::private::as_storage_mut::(binding) + // } + // } + + unsafe fn resolve_instance_ptr(&self) -> sys::GDExtensionClassInstancePtr { + let callbacks = crate::storage::nop_instance_callbacks(); + let token = sys::get_library() as *mut std::ffi::c_void; + let binding = interface_fn!(object_get_instance_binding)(self.obj_sys(), token, &callbacks); + + debug_assert!( + !binding.is_null(), + "Class {} -- null instance; does the class have a Godot creator function?", + std::any::type_name::() + ); + binding as sys::GDExtensionClassInstancePtr + } } /// _The methods in this impl block are available for any `T`._

@@ -506,12 +549,13 @@ where fn deref(&self) -> &Self::Target { // SAFETY: - // This relies on Gd having the layout as Node3D (as an example), + // + // This relies on `Gd` having the layout as `Node3D` (as an example), // which also needs #[repr(transparent)]: // // struct Gd { - // opaque: OpaqueObject, <- size of GDExtensionObjectPtr - // _marker: PhantomData, <- ZST + // opaque: OpaqueObject, // <- size of GDExtensionObjectPtr + // _marker: PhantomData, // <- ZST // } // struct Node3D { // object_ptr: sys::GDExtensionObjectPtr, @@ -527,13 +571,14 @@ where fn deref_mut(&mut self) -> &mut T { // SAFETY: see also Deref // - // The resulting &mut T is transmuted from &mut OpaqueObject, i.e. a *pointer* to the `opaque` field. + // The resulting `&mut T` is transmuted from `&mut OpaqueObject`, i.e. a *pointer* to the `opaque` field. // `opaque` itself has a different *address* for each Gd instance, meaning that two simultaneous // DerefMut borrows on two Gd instances will not alias, *even if* the underlying Godot object is the // same (i.e. `opaque` has the same value, but not address). unsafe { std::mem::transmute::<&mut OpaqueObject, &mut T>(&mut self.opaque) } } } + // SAFETY: // - `move_return_ptr` // When the `call_type` is `PtrcallType::Virtual`, and the current type is known to inherit from `RefCounted` diff --git a/godot-core/src/obj/traits.rs b/godot-core/src/obj/traits.rs index cd0556066..89e28445f 100644 --- a/godot-core/src/obj/traits.rs +++ b/godot-core/src/obj/traits.rs @@ -122,17 +122,18 @@ pub trait Inherits: GodotClass {} impl Inherits for T {} -/// Trait implemented for all objects that inherit from `Resource` or `Node`. As those are the only objects -/// you can export to the editor. +/// Trait implemented for all objects that inherit from `Resource` or `Node`. +/// +/// Those are the only objects you can export to the editor. pub trait ExportableObject: GodotClass {} -/// Auto-implemented for all engine-provided classes +/// Auto-implemented for all engine-provided classes. pub trait EngineClass: GodotClass { fn as_object_ptr(&self) -> sys::GDExtensionObjectPtr; fn as_type_ptr(&self) -> sys::GDExtensionTypePtr; } -/// Auto-implemented for all engine-provided enums +/// Auto-implemented for all engine-provided enums. pub trait EngineEnum: Copy { fn try_from_ord(ord: i32) -> Option; @@ -176,6 +177,7 @@ pub trait IndexEnum: EngineEnum { /// Capability traits, providing dedicated functionalities for Godot classes pub mod cap { use super::*; + use crate::obj::Gd; /// Trait for all classes that are constructible from the Godot engine. /// @@ -192,6 +194,17 @@ pub mod cap { fn __godot_init(base: Base) -> Self; } + /// Trait that's implemented for user-defined classes that provide a `#[base]` field. + /// + /// Gives direct access to the base pointer without going through upcast FFI. + pub trait WithBaseField: GodotClass { + #[doc(hidden)] + fn __godot_base(&self) -> &Gd; + + #[doc(hidden)] + fn __godot_base_mut(&mut self) -> &mut Gd; + } + // TODO Evaluate whether we want this public or not #[doc(hidden)] pub trait GodotToString: GodotClass { diff --git a/godot-core/src/registry.rs b/godot-core/src/registry.rs index 6959773e2..fc3d8ca82 100644 --- a/godot-core/src/registry.rs +++ b/godot-core/src/registry.rs @@ -307,10 +307,14 @@ pub mod callbacks { let base_ptr = unsafe { interface_fn!(classdb_construct_object)(base_class_name.string_sys()) }; - let base = unsafe { Base::from_sys(base_ptr) }; + let base = unsafe { Base::from_sys(base_ptr) }; let user_instance = make_user_instance(base); - let instance = InstanceStorage::::construct(user_instance); + + // Create 2nd base to cache inside storage. This one will remain weak forever (no action on destruction). + let cached_base = unsafe { Base::from_sys(base_ptr) }; + + let instance = InstanceStorage::::construct(user_instance, cached_base); let instance_ptr = instance.into_raw(); let instance_ptr = instance_ptr as sys::GDExtensionClassInstancePtr; @@ -334,9 +338,12 @@ pub mod callbacks { _class_user_data: *mut std::ffi::c_void, instance: sys::GDExtensionClassInstancePtr, ) { - let storage = as_storage::(instance); - storage.mark_destroyed_by_godot(); - let _drop = Box::from_raw(storage as *mut InstanceStorage<_>); + { + let storage = as_storage::(instance); + storage.mark_destroyed_by_godot(); + } // Ref no longer valid once next statement is executed. + + crate::storage::destroy_storage::(instance); } pub unsafe extern "C" fn get_virtual( diff --git a/godot-core/src/storage.rs b/godot-core/src/storage.rs index 2eac95187..b34de73ae 100644 --- a/godot-core/src/storage.rs +++ b/godot-core/src/storage.rs @@ -28,7 +28,7 @@ mod single_thread { use std::any::type_name; use std::cell; - use crate::obj::GodotClass; + use crate::obj::{Base, GodotClass}; use crate::out; use super::Lifecycle; @@ -36,57 +36,50 @@ mod single_thread { /// Manages storage and lifecycle of user's extension class instances. pub struct InstanceStorage { user_instance: cell::RefCell, + cached_base: Base, // Declared after `user_instance`, is dropped last - pub lifecycle: Lifecycle, - godot_ref_count: u32, + pub lifecycle: cell::Cell, + godot_ref_count: cell::Cell, } /// For all Godot extension classes impl InstanceStorage { - pub fn construct(user_instance: T) -> Self { + pub fn construct(user_instance: T, cached_base: Base) -> Self { out!(" Storage::construct <{}>", type_name::()); Self { user_instance: cell::RefCell::new(user_instance), - lifecycle: Lifecycle::Alive, - godot_ref_count: 1, + cached_base, + lifecycle: cell::Cell::new(Lifecycle::Alive), + godot_ref_count: cell::Cell::new(1), } } - pub(crate) fn on_inc_ref(&mut self) { - self.godot_ref_count += 1; + pub(crate) fn on_inc_ref(&self) { + let refc = self.godot_ref_count.get() + 1; + self.godot_ref_count.set(refc); + out!( " Storage::on_inc_ref (rc={}) <{}>", // -- {:?}", - self.godot_ref_count(), + refc, type_name::(), //self.user_instance ); } - pub(crate) fn on_dec_ref(&mut self) { - self.godot_ref_count -= 1; + pub(crate) fn on_dec_ref(&self) { + let refc = self.godot_ref_count.get() - 1; + self.godot_ref_count.set(refc); + out!( " | Storage::on_dec_ref (rc={}) <{}>", // -- {:?}", - self.godot_ref_count(), + refc, type_name::(), //self.user_instance ); } - /* pub fn destroy(&mut self) { - assert!( - self.user_instance.is_some(), - "Cannot destroy user instance which is not yet initialized" - ); - assert!( - !self.destroyed, - "Cannot destroy user instance multiple times" - ); - self.user_instance = None; // drops T - // TODO drop entire Storage - }*/ - pub fn get(&self) -> cell::Ref { self.user_instance.try_borrow().unwrap_or_else(|_e| { panic!( @@ -98,7 +91,7 @@ mod single_thread { }) } - pub fn get_mut(&mut self) -> cell::RefMut { + pub fn get_mut(&self) -> cell::RefMut { self.user_instance.try_borrow_mut().unwrap_or_else(|_e| { panic!( "Gd::bind_mut() failed, already bound; T = {}.\n \ @@ -109,8 +102,12 @@ mod single_thread { }) } + pub fn base(&self) -> &Base { + &self.cached_base + } + pub(super) fn godot_ref_count(&self) -> u32 { - self.godot_ref_count + self.godot_ref_count.get() } } } @@ -118,31 +115,33 @@ mod single_thread { #[cfg(feature = "threads")] mod multi_thread { use std::any::type_name; - use std::sync; use std::sync::atomic::{AtomicU32, Ordering}; + use std::{cell, sync}; - use crate::obj::GodotClass; - use crate::out; + use crate::obj::{Base, GodotClass}; + use crate::{out, sys}; use super::Lifecycle; /// Manages storage and lifecycle of user's extension class instances. pub struct InstanceStorage { user_instance: sync::RwLock, + cached_base: Base, // Declared after `user_instance`, is dropped last - pub lifecycle: Lifecycle, + pub lifecycle: cell::Cell, godot_ref_count: AtomicU32, } /// For all Godot extension classes impl InstanceStorage { - pub fn construct(user_instance: T) -> Self { + pub fn construct(user_instance: T, cached_base: Base) -> Self { out!(" Storage::construct <{}>", type_name::()); Self { user_instance: sync::RwLock::new(user_instance), - lifecycle: Lifecycle::Alive, + cached_base, + lifecycle: cell::Cell::new(Lifecycle::Alive), godot_ref_count: AtomicU32::new(1), } } @@ -167,19 +166,6 @@ mod multi_thread { ); } - /* pub fn destroy(&mut self) { - assert!( - self.user_instance.is_some(), - "Cannot destroy user instance which is not yet initialized" - ); - assert!( - !self.destroyed, - "Cannot destroy user instance multiple times" - ); - self.user_instance = None; // drops T - // TODO drop entire Storage - }*/ - pub fn get(&self) -> sync::RwLockReadGuard { self.user_instance.read().unwrap_or_else(|_e| { panic!( @@ -191,7 +177,7 @@ mod multi_thread { }) } - pub fn get_mut(&mut self) -> sync::RwLockWriteGuard { + pub fn get_mut(&self) -> sync::RwLockWriteGuard { self.user_instance.write().unwrap_or_else(|_e| { panic!( "Gd::bind_mut() failed, already bound; T = {}.\n \ @@ -202,6 +188,10 @@ mod multi_thread { }) } + pub fn base(&self) -> &Base { + &self.cached_base + } + pub(super) fn godot_ref_count(&self) -> u32 { self.godot_ref_count.load(Ordering::Relaxed) } @@ -214,16 +204,16 @@ impl InstanceStorage { Box::into_raw(Box::new(self)) } - pub fn mark_destroyed_by_godot(&mut self) { + pub fn mark_destroyed_by_godot(&self) { out!( " Storage::mark_destroyed_by_godot", // -- {:?}", //self.user_instance ); - self.lifecycle = Lifecycle::Destroying; + self.lifecycle.set(Lifecycle::Destroying); out!( " mark; self={:?}, val={:?}", - self as *mut _, - self.lifecycle + self as *const _, + self.lifecycle.get() ); } @@ -232,9 +222,12 @@ impl InstanceStorage { out!( " is_d; self={:?}, val={:?}", self as *const _, - self.lifecycle + self.lifecycle.get() ); - matches!(self.lifecycle, Lifecycle::Destroying | Lifecycle::Dead) + matches!( + self.lifecycle.get(), + Lifecycle::Destroying | Lifecycle::Dead + ) } } @@ -261,11 +254,17 @@ impl Drop for InstanceStorage { /// /// # Safety /// `instance_ptr` is assumed to point to a valid instance. -// FIXME unbounded ref AND &mut out of thin air is a huge hazard -- consider using with_storage(ptr, closure) and drop_storage(ptr) +// Note: unbounded ref AND &mut out of thin air is not very beautiful, but it's -- consider using with_storage(ptr, closure) and drop_storage(ptr) pub unsafe fn as_storage<'u, T: GodotClass>( instance_ptr: sys::GDExtensionClassInstancePtr, -) -> &'u mut InstanceStorage { - &mut *(instance_ptr as *mut InstanceStorage) +) -> &'u InstanceStorage { + &*(instance_ptr as *mut InstanceStorage) +} + +/// # Safety +/// `instance_ptr` is assumed to point to a valid instance. This function must only be invoked once for a pointer. +pub unsafe fn destroy_storage(instance_ptr: sys::GDExtensionClassInstancePtr) { + let _drop = Box::from_raw(instance_ptr as *mut InstanceStorage); } pub fn nop_instance_callbacks() -> sys::GDExtensionInstanceBindingCallbacks { diff --git a/godot-macros/src/derive_godot_class/mod.rs b/godot-macros/src/derive_godot_class/mod.rs index 7807a4f13..78bddff6b 100644 --- a/godot-macros/src/derive_godot_class/mod.rs +++ b/godot-macros/src/derive_godot_class/mod.rs @@ -35,8 +35,6 @@ pub fn transform(decl: Declaration) -> ParseResult { let inherits_macro = format_ident!("inherits_transitive_{}", base_ty); let prv = quote! { ::godot::private }; - let deref_impl = make_deref_impl(class_name, &fields); - let godot_exports_impl = make_property_impl(class_name, &fields); let (godot_init_impl, create_fn); @@ -61,7 +59,6 @@ pub fn transform(decl: Declaration) -> ParseResult { #godot_init_impl #godot_exports_impl - #deref_impl ::godot::sys::plugin_add!(__GODOT_PLUGIN_REGISTRY in #prv; #prv::ClassPlugin { class_name: #class_name_obj, @@ -231,29 +228,6 @@ fn make_godot_init_impl(class_name: &Ident, fields: Fields) -> TokenStream { } } -fn make_deref_impl(class_name: &Ident, fields: &Fields) -> TokenStream { - let base_field = if let Some(Field { name, .. }) = &fields.base_field { - name - } else { - return TokenStream::new(); - }; - - quote! { - impl std::ops::Deref for #class_name { - type Target = ::Base; - - fn deref(&self) -> &Self::Target { - &*self.#base_field - } - } - impl std::ops::DerefMut for #class_name { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut *self.#base_field - } - } - } -} - /// Checks at compile time that a function with the given name exists on `Self`. #[must_use] fn make_existence_check(ident: &Ident) -> TokenStream { diff --git a/itest/rust/src/base_test.rs b/itest/rust/src/base_test.rs index fce5f1b79..52e644458 100644 --- a/itest/rust/src/base_test.rs +++ b/itest/rust/src/base_test.rs @@ -15,7 +15,7 @@ fn base_test_is_weak() { #[itest] fn base_instance_id() { - let obj = Gd::::new_default(); + let obj = Gd::::new_default(); let obj_id = obj.instance_id(); let base_id = obj.bind().base.instance_id(); @@ -24,15 +24,29 @@ fn base_instance_id() { } #[itest] -fn base_deref() { - let mut obj = Gd::::new_default(); +fn base_access_unbound() { + let mut obj = Gd::::new_default(); { - let mut guard = obj.bind_mut(); let pos = Vector2::new(-5.5, 7.0); - guard.set_position(pos); // GdMut as DerefMut + obj.base_mut().set_position(pos); - assert_eq!(guard.base.get_position(), pos); + assert_eq!(obj.base().get_position(), pos); + } + + obj.free(); +} + +// Tests whether access to base is possible from outside the Gd, even if there is no `#[base]` field. +#[itest] +fn base_access_unbound_no_field() { + let mut obj = Gd::::new_default(); + + { + let pos = Vector2::new(-5.5, 7.0); + obj.base_mut().set_position(pos); + + assert_eq!(obj.base().get_position(), pos); } obj.free(); @@ -40,14 +54,14 @@ fn base_deref() { #[itest] fn base_display() { - let obj = Gd::::new_default(); + let obj = Gd::::new_default(); { let guard = obj.bind(); let id = guard.base.instance_id(); - // We expect the dynamic type to be part of Godot's to_string(), so BaseHolder and not Node2D + // We expect the dynamic type to be part of Godot's to_string(), so Based and not Node2D let actual = format!(".:{}:.", guard.base); - let expected = format!(".::."); + let expected = format!(".::."); assert_eq!(actual, expected); } @@ -56,14 +70,14 @@ fn base_display() { #[itest] fn base_debug() { - let obj = Gd::::new_default(); + let obj = Gd::::new_default(); { let guard = obj.bind(); let id = guard.base.instance_id(); - // We expect the dynamic type to be part of Godot's to_string(), so BaseHolder and not Node2D + // We expect the dynamic type to be part of Godot's to_string(), so Based and not Node2D let actual = format!(".:{:?}:.", guard.base); - let expected = format!(".:Base {{ id: {id}, class: BaseHolder }}:."); + let expected = format!(".:Base {{ id: {id}, class: Based }}:."); assert_eq!(actual, expected); } @@ -72,23 +86,30 @@ fn base_debug() { #[itest] fn base_with_init() { - let obj = Gd::::with_base(|mut base| { + let obj = Gd::::with_base(|mut base| { base.set_rotation(11.0); - BaseHolder { base, i: 732 } + Based { base, i: 732 } }); { let guard = obj.bind(); assert_eq!(guard.i, 732); - assert_eq!(guard.get_rotation(), 11.0); + assert_eq!(guard.base.get_rotation(), 11.0); } obj.free(); } #[derive(GodotClass)] #[class(init, base=Node2D)] -struct BaseHolder { +struct Based { #[base] base: Base, + i: i32, } + +#[derive(GodotClass)] +#[class(init, base=Node2D)] +struct Baseless { + // No need for fields, we just test if we can access this as Gd. +} diff --git a/itest/rust/src/signal_test.rs b/itest/rust/src/signal_test.rs index 36e6c5c5d..c0f834c6d 100644 --- a/itest/rust/src/signal_test.rs +++ b/itest/rust/src/signal_test.rs @@ -82,10 +82,10 @@ fn signals() { let receiver_name = format!("receive_{i}_arg"); emitter - .bind_mut() + .base_mut() .connect(signal_name.clone().into(), receiver.callable(receiver_name)); - emitter.bind_mut().emit_signal(signal_name.into(), arg); + emitter.base_mut().emit_signal(signal_name.into(), arg); assert!(receiver.bind().used[i].get()); }