From e4b5316244a8c82fa055b4eb2d3bd0bf0a9db463 Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Fri, 31 May 2024 19:42:37 +0200 Subject: [PATCH 1/3] Cleanup bool conversions Cleanup script instance borrowing --- godot-core/src/obj/script.rs | 240 +++++++++++++++++------------------ 1 file changed, 115 insertions(+), 125 deletions(-) diff --git a/godot-core/src/obj/script.rs b/godot-core/src/obj/script.rs index 8d664bd11..b64460e91 100644 --- a/godot-core/src/obj/script.rs +++ b/godot-core/src/obj/script.rs @@ -21,10 +21,10 @@ use std::ops::{Deref, DerefMut}; use std::sync::Mutex; #[cfg(not(feature = "experimental-threads"))] -use godot_cell::panicking::{GdCell, MutGuard}; +use godot_cell::panicking::{GdCell, MutGuard, RefGuard}; #[cfg(feature = "experimental-threads")] -use godot_cell::blocking::{GdCell, MutGuard}; +use godot_cell::blocking::{GdCell, MutGuard, RefGuard}; use crate::builtin::{GString, StringName, Variant, VariantType}; use crate::classes::{Script, ScriptLanguage}; @@ -175,6 +175,39 @@ struct ScriptInstanceData { base: Base, } +impl ScriptInstanceData { + unsafe fn borrow_script_sys<'a>(p_instance: sys::GDExtensionScriptInstanceDataPtr) -> &'a Self { + &*(p_instance as *mut ScriptInstanceData) + } + + fn borrow(&self) -> RefGuard<'_, T> { + self.inner + .borrow() + .unwrap_or_else(|err| Self::borrow_panic(err)) + } + + fn borrow_mut(&self) -> MutGuard<'_, T> { + self.inner + .borrow_mut() + .unwrap_or_else(|err| Self::borrow_panic(err)) + } + + fn cell_ref(&self) -> &GdCell { + &self.inner + } + + fn borrow_panic(err: Box) -> ! { + panic!( + "\ + ScriptInstance borrow failed, already bound; T = {}.\n \ + Make sure to use `SiMut::base_mut()` when possible.\n \ + Details: {err}.\ + ", + std::any::type_name::(), + ) + } +} + impl Drop for ScriptInstanceData { fn drop(&mut self) { // SAFETY: The ownership of ScriptInstanceData is transferred to Godot after its creation. The engine then calls @@ -425,12 +458,6 @@ mod script_instance_info { use std::ffi::c_void; use std::mem::ManuallyDrop; - #[cfg(not(feature = "experimental-threads"))] - use godot_cell::panicking::{GdCell, RefGuard}; - - #[cfg(feature = "experimental-threads")] - use godot_cell::blocking::{GdCell, RefGuard}; - use crate::builtin::{GString, StringName, Variant}; use crate::classes::ScriptLanguage; use crate::obj::Gd; @@ -439,45 +466,12 @@ mod script_instance_info { use super::{ScriptInstance, ScriptInstanceData, SiMut}; - fn borrow_panic(err: Box) -> R { - panic!( - "\ - ScriptInstance borrow failed, already bound; T = {}.\n \ - Make sure to use `SiMut::base_mut()` when possible.\n \ - Details: {err}.\ - ", - type_name::(), - ); - } - - fn borrow_instance(instance: &ScriptInstanceData) -> RefGuard<'_, T> { - instance.inner.borrow().unwrap_or_else(borrow_panic::) - } - - fn borrow_instance_cell(instance: &ScriptInstanceData) -> &GdCell { - &instance.inner - } - - /// # Safety - /// - /// `p_instance` must be a valid pointer to a `ScriptInstanceData` for the duration of `'a`. - /// This pointer must have been created by [super::create_script_instance] and transfered to Godot. - unsafe fn instance_data_as_script_instance<'a, T: ScriptInstance>( - p_instance: sys::GDExtensionScriptInstanceDataPtr, - ) -> &'a ScriptInstanceData { - &*(p_instance as *mut ScriptInstanceData) - } - - fn transfer_bool_to_godot(value: bool) -> sys::GDExtensionBool { + const fn bool_into_sys(value: bool) -> sys::GDExtensionBool { value as sys::GDExtensionBool } - /// # Safety - /// - /// - We expect the engine to provide a valid variant pointer the return value can be moved into. - unsafe fn transfer_variant_to_godot(variant: Variant, return_ptr: sys::GDExtensionVariantPtr) { - variant.move_into_var_ptr(return_ptr) - } + const SYS_TRUE: sys::GDExtensionBool = bool_into_sys(true); + const SYS_FALSE: sys::GDExtensionBool = bool_into_sys(true); /// # Safety /// @@ -557,18 +551,17 @@ mod script_instance_info { let ctx = || format!("error when calling {}::set", type_name::()); let result = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - let cell = borrow_instance_cell(instance); - let mut guard = cell.borrow_mut().unwrap_or_else(borrow_panic::); + let instance = ScriptInstanceData::::borrow_script_sys(p_instance); + let mut guard = instance.borrow_mut(); - let instance_guard = SiMut::new(cell, &mut guard, &instance.base); + let instance_guard = SiMut::new(instance.cell_ref(), &mut guard, &instance.base); ScriptInstance::set_property(instance_guard, name, value) }) // Unwrapping to a default of false, to indicate that the assignment is not handled by the script. .unwrap_or_default(); - transfer_bool_to_godot(result) + bool_into_sys(result) } /// # Safety @@ -584,19 +577,18 @@ mod script_instance_info { let ctx = || format!("error when calling {}::get", type_name::()); let return_value = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - borrow_instance(instance).get_property(name.clone()) + ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .get_property(name) }); - let result = match return_value { - Ok(return_value) => return_value - .map(|variant| transfer_variant_to_godot(variant, r_ret)) - .is_some(), - Err(_) => false, - }; - - transfer_bool_to_godot(result) + match return_value { + Ok(Some(variant)) => { + variant.move_into_var_ptr(r_ret); + SYS_TRUE + } + _ => SYS_FALSE, + } } /// # Safety @@ -610,9 +602,9 @@ mod script_instance_info { let ctx = || format!("error when calling {}::get_property_list", type_name::()); let (property_list, property_sys_list) = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - let property_list = borrow_instance(instance).get_property_list(); + let property_list = ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .get_property_list(); let property_sys_list: Box<[_]> = property_list .iter() @@ -627,7 +619,7 @@ mod script_instance_info { let list_length = unsafe { &mut *r_count }; let return_pointer = transfer_ptr_list_to_godot(property_sys_list, list_length); - let instance = instance_data_as_script_instance::(p_instance); + let instance = ScriptInstanceData::::borrow_script_sys(p_instance); instance .property_list .lock() @@ -648,9 +640,9 @@ mod script_instance_info { let ctx = || format!("error when calling {}::get_method_list", type_name::()); let (method_list, method_sys_list) = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - let method_list = borrow_instance(instance).get_method_list(); + let method_list = ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .get_method_list(); let method_sys_list = method_list .iter() @@ -665,7 +657,7 @@ mod script_instance_info { let list_length = unsafe { &mut *r_count }; let return_pointer = transfer_ptr_list_to_godot(method_sys_list, list_length); - let instance = instance_data_as_script_instance::(p_instance); + let instance = ScriptInstanceData::::borrow_script_sys(p_instance); instance .method_list @@ -684,7 +676,7 @@ mod script_instance_info { p_instance: sys::GDExtensionScriptInstanceDataPtr, p_prop_info: *const sys::GDExtensionPropertyInfo, ) { - let instance = instance_data_as_script_instance::(p_instance); + let instance = ScriptInstanceData::::borrow_script_sys(p_instance); let vec = instance .property_list @@ -719,18 +711,17 @@ mod script_instance_info { let ctx = || format!("error when calling {}::call", type_name::()); let result = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_self); - let cell = borrow_instance_cell(instance); - let mut guard = cell.borrow_mut().unwrap_or_else(borrow_panic::); + let instance = ScriptInstanceData::::borrow_script_sys(p_self); + let mut guard = instance.borrow_mut(); - let instance_guard = SiMut::new(cell, &mut guard, &instance.base); + let instance_guard = SiMut::new(instance.cell_ref(), &mut guard, &instance.base); ScriptInstance::call(instance_guard, method.clone(), args) }); match result { - Ok(Ok(ret)) => { - transfer_variant_to_godot(ret, r_return); + Ok(Ok(variant)) => { + variant.move_into_var_ptr(r_return); (*r_error).error = sys::GDEXTENSION_CALL_OK; } @@ -753,9 +744,10 @@ mod script_instance_info { let ctx = || format!("error when calling {}::get_script", type_name::()); let script = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - borrow_instance(instance).get_script().to_owned() + ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .get_script() + .to_owned() }); match script { @@ -774,13 +766,13 @@ mod script_instance_info { let ctx = || format!("error when calling {}::is_placeholder", type_name::()); let is_placeholder = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - borrow_instance(instance).is_placeholder() + ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .is_placeholder() }) .unwrap_or_default(); - transfer_bool_to_godot(is_placeholder) + bool_into_sys(is_placeholder) } /// # Safety @@ -795,13 +787,13 @@ mod script_instance_info { let ctx = || format!("error when calling {}::has_method", type_name::()); let has_method = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - borrow_instance(instance).has_method(method.clone()) + ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .has_method(method) }) .unwrap_or_default(); - transfer_bool_to_godot(has_method) + bool_into_sys(has_method) } /// # Safety @@ -812,7 +804,7 @@ mod script_instance_info { p_instance: sys::GDExtensionScriptInstanceDataPtr, p_method_info: *const sys::GDExtensionMethodInfo, ) { - let instance = instance_data_as_script_instance::(p_instance); + let instance = ScriptInstanceData::::borrow_script_sys(p_instance); let vec = instance .method_list @@ -856,16 +848,16 @@ mod script_instance_info { let name = StringName::new_from_string_sys(p_name); let result = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - borrow_instance(instance).get_property_type(name.clone()) + ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .get_property_type(name.clone()) }); if let Ok(result) = result { - *r_is_valid = transfer_bool_to_godot(true); + *r_is_valid = SYS_TRUE; result.sys() } else { - *r_is_valid = transfer_bool_to_godot(false); + *r_is_valid = SYS_FALSE; 0 } } @@ -883,9 +875,9 @@ mod script_instance_info { let ctx = || format!("error when calling {}::to_string", type_name::()); let string = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - borrow_instance(instance).to_string() + ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .to_string() }) .ok(); @@ -893,7 +885,7 @@ mod script_instance_info { return; }; - *r_is_valid = transfer_bool_to_godot(true); + *r_is_valid = SYS_TRUE; transfer_string_to_godot(string, r_str); } @@ -916,9 +908,9 @@ mod script_instance_info { }; let property_states = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - borrow_instance(instance).get_property_state() + ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .get_property_state() }) .unwrap_or_default(); @@ -936,9 +928,9 @@ mod script_instance_info { let ctx = || format!("error when calling {}::get_language", type_name::()); let language = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - borrow_instance(instance).get_language() + ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .get_language() }); if let Ok(language) = language { @@ -972,13 +964,13 @@ mod script_instance_info { }; let result = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - borrow_instance(instance).on_refcount_decremented() + ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .on_refcount_decremented() }) .unwrap_or(true); - transfer_bool_to_godot(result) + bool_into_sys(result) } /// # Safety @@ -995,9 +987,9 @@ mod script_instance_info { }; handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - borrow_instance(instance).on_refcount_incremented(); + ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .on_refcount_incremented(); }) .unwrap_or_default(); } @@ -1022,19 +1014,18 @@ mod script_instance_info { }; let return_value = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - - borrow_instance(instance).property_get_fallback(name) + ScriptInstanceData::::borrow_script_sys(p_instance) + .borrow() + .property_get_fallback(name) }); - let result = match return_value { - Ok(return_value) => return_value - .map(|value| transfer_variant_to_godot(value, r_ret)) - .is_some(), - Err(_) => false, - }; - - transfer_bool_to_godot(result) + match return_value { + Ok(Some(variant)) => { + variant.move_into_var_ptr(r_ret); + SYS_TRUE + } + _ => SYS_FALSE, + } } /// # Safety @@ -1057,15 +1048,14 @@ mod script_instance_info { }; let result = handle_panic(ctx, || { - let instance = instance_data_as_script_instance::(p_instance); - let cell = borrow_instance_cell(instance); - let mut guard = cell.borrow_mut().unwrap_or_else(borrow_panic::); + let instance = ScriptInstanceData::::borrow_script_sys(p_instance); + let mut guard = instance.borrow_mut(); - let instance_guard = SiMut::new(cell, &mut guard, &instance.base); + let instance_guard = SiMut::new(instance.cell_ref(), &mut guard, &instance.base); ScriptInstance::property_set_fallback(instance_guard, name, value) }) .unwrap_or_default(); - transfer_bool_to_godot(result) + bool_into_sys(result) } } From 3e80962a0260d4315e8807467430a90c86272866 Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Sun, 2 Jun 2024 17:07:41 +0200 Subject: [PATCH 2/3] Add ptrlist container Add into/free owned_sys methods for MethodList Remove helper functions that were just used in one place and which didnt really do much to improve readability --- godot-core/src/builtin/variant/mod.rs | 28 +++ godot-core/src/meta/mod.rs | 134 ++++++++++---- godot-core/src/obj/script.rs | 251 +++++++++++--------------- 3 files changed, 235 insertions(+), 178 deletions(-) diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index bcc61a877..52ca17a3c 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -374,6 +374,34 @@ impl Variant { // SAFETY: `variant_array` isn't null so it is safe to call `from_raw_parts_mut` on the pointer cast to `*mut Variant`. unsafe { std::slice::from_raw_parts_mut(variant_array, length) } } + + /// Consumes self and turns it into a sys-ptr, should be used together with [`from_owned_var_sys`](Self::from_owned_var_sys). + /// + /// This will leak memory unless `from_owned_var_sys` is called on the returned pointer. + pub(crate) fn into_owned_var_sys(self) -> sys::GDExtensionVariantPtr { + sys::static_assert_eq_size_align!(Variant, sys::types::OpaqueVariant); + + let leaked = Box::into_raw(Box::new(self)); + leaked.cast() + } + + /// Creates a `Variant` from a sys-ptr without incrementing the refcount. + /// + /// # Safety + /// + /// * Must only be used on a pointer returned from a call to [`into_owned_var_sys`](Self::into_owned_var_sys). + /// * Must not be called more than once on the same pointer. + #[deny(unsafe_op_in_unsafe_fn)] + pub(crate) unsafe fn from_owned_var_sys(ptr: sys::GDExtensionVariantPtr) -> Self { + sys::static_assert_eq_size_align!(Variant, sys::types::OpaqueVariant); + + let ptr = ptr.cast::(); + + // SAFETY: `ptr` was returned from a call to `into_owned_var_sys`, which means it was created by a call to + // `Box::into_raw`, thus we can use `Box::from_raw` here. Additionally this is only called once on this pointer. + let boxed = unsafe { Box::from_raw(ptr) }; + *boxed + } } impl ArrayElement for Variant {} diff --git a/godot-core/src/meta/mod.rs b/godot-core/src/meta/mod.rs index 410b0ef9f..8a2c063a0 100644 --- a/godot-core/src/meta/mod.rs +++ b/godot-core/src/meta/mod.rs @@ -241,50 +241,114 @@ pub struct MethodInfo { } impl MethodInfo { - /// Converts to the FFI type. Keep this object allocated while using that! - /// - /// The struct returned by this function contains pointers into the fields of `self`. `self` should therefore not be dropped while the - /// `sys::GDExtensionMethodInfo` is still in use. - /// - /// This function also leaks memory that has to be cleaned up by the caller once it is no longer used. Specifically the `arguments` and - /// `default_arguments` vectors have to be reconstructed from the pointer and length and then dropped/freed. + /// Consumes self and turns it into a `sys::GDExtensionMethodInfo`, should be used together with + /// [`free_owned_method_sys`](Self::free_owned_method_sys). /// - /// Each vector can be reconstructed with `Vec::from_raw_parts` since the pointers were created with `Vec::into_boxed_slice`, which - /// guarantees that the vector capacity and length are equal. - pub fn method_sys(&self) -> sys::GDExtensionMethodInfo { + /// This will leak memory unless used together with `free_owned_method_sys`. + pub fn into_owned_method_sys(self) -> sys::GDExtensionMethodInfo { use crate::obj::EngineBitfield as _; - let argument_count = self.arguments.len() as u32; - let argument_vec = self - .arguments - .iter() - .map(|arg| arg.property_sys()) - .collect::>() - .into_boxed_slice(); - - // SAFETY: dereferencing the new box pointer is fine as it is guaranteed to not be null - let arguments = unsafe { (*Box::into_raw(argument_vec)).as_mut_ptr() }; - - let default_argument_count = self.default_arguments.len() as u32; - let default_argument_vec = self - .default_arguments - .iter() - .map(|arg| sys::SysPtr::force_mut(arg.var_sys())) - .collect::>() - .into_boxed_slice(); - - // SAFETY: dereferencing the new box pointer is fine as it is guaranteed to not be null - let default_arguments = unsafe { (*Box::into_raw(default_argument_vec)).as_mut_ptr() }; + // Destructure self to ensure all fields are used. + let Self { + id, + method_name, + // TODO: Do we need this? + class_name: _class_name, + return_type, + arguments, + default_arguments, + flags, + } = self; + + let argument_count: u32 = arguments + .len() + .try_into() + .expect("cannot have more than `u32::MAX` arguments"); + let arguments = arguments + .into_iter() + .map(|arg| arg.into_owned_property_sys()) + .collect::>(); + let arguments = Box::leak(arguments).as_mut_ptr(); + + let default_argument_count: u32 = default_arguments + .len() + .try_into() + .expect("cannot have more than `u32::MAX` default arguments"); + let default_argument = default_arguments + .into_iter() + .map(|arg| arg.into_owned_var_sys()) + .collect::>(); + let default_arguments = Box::leak(default_argument).as_mut_ptr(); sys::GDExtensionMethodInfo { - id: self.id, - name: sys::SysPtr::force_mut(self.method_name.string_sys()), - return_value: self.return_type.property_sys(), + id, + name: method_name.into_owned_string_sys(), + return_value: return_type.into_owned_property_sys(), argument_count, arguments, default_argument_count, default_arguments, - flags: u32::try_from(self.flags.ord()).expect("flags should be valid"), + flags: flags.ord().try_into().expect("flags should be valid"), } } + + /// Properly frees a `sys::GDExtensionMethodInfo` created by [`into_owned_method_sys`](Self::into_owned_method_sys). + /// + /// # Safety + /// + /// * Must only be used on a struct returned from a call to `into_owned_method_sys`, without modification. + /// * Must not be called more than once on a `sys::GDExtensionMethodInfo` struct. + #[deny(unsafe_op_in_unsafe_fn)] + pub unsafe fn free_owned_method_sys(info: sys::GDExtensionMethodInfo) { + // Destructure info to ensure all fields are used. + let sys::GDExtensionMethodInfo { + name, + return_value, + flags: _flags, + id: _id, + argument_count, + arguments, + default_argument_count, + default_arguments, + } = info; + + // SAFETY: `name` and `return_value` were created from the appropriate method calls, and have not been freed before this. + unsafe { + let _name = StringName::from_owned_string_sys(name); + PropertyInfo::free_owned_property_sys(return_value); + } + + // SAFETY: These pointers were both created from a call to `as_mut_ptr` on a slice. Additionally these pointer will not be accessed + // again after this function call. + let (arguments_slice, default_arguments_slice) = unsafe { + ( + std::slice::from_raw_parts_mut( + arguments, + argument_count + .try_into() + .expect("gdext only supports targets where u32 <= usize"), + ), + std::slice::from_raw_parts_mut( + default_arguments, + default_argument_count + .try_into() + .expect("gdext only supports targets where u32 <= usize"), + ), + ) + }; + + // SAFETY: We have exclusive ownership of these slices, and they were originally created from a call to `Box::leak`. + let (_arguments, default_arguments) = unsafe { + ( + Box::from_raw(arguments_slice), + Box::from_raw(default_arguments_slice), + ) + }; + + default_arguments.iter().for_each(|ptr| { + // SAFETY: These pointers were originally created from a call to `Variant::into_owner_var_sys`, and this method will not be + // called again on this pointer. + let _variant = unsafe { Variant::from_owned_var_sys(*ptr) }; + }); + } } diff --git a/godot-core/src/obj/script.rs b/godot-core/src/obj/script.rs index b64460e91..75e48dda4 100644 --- a/godot-core/src/obj/script.rs +++ b/godot-core/src/obj/script.rs @@ -15,10 +15,8 @@ // Re-export guards. pub use crate::obj::guards::{ScriptBaseMut, ScriptBaseRef}; -use std::collections::HashMap; use std::ffi::c_void; use std::ops::{Deref, DerefMut}; -use std::sync::Mutex; #[cfg(not(feature = "experimental-threads"))] use godot_cell::panicking::{GdCell, MutGuard, RefGuard}; @@ -32,6 +30,8 @@ use crate::meta::{MethodInfo, PropertyInfo}; use crate::obj::{Base, Gd, GodotClass}; use crate::sys; +use self::ptrlist_container::PtrlistContainer; + /// Implement custom scripts that can be attached to objects in Godot. /// /// To use script instances, implement this trait for your own type. @@ -170,8 +170,8 @@ type ScriptInstanceInfo = sys::GDExtensionScriptInstanceInfo2; struct ScriptInstanceData { inner: GdCell, script_instance_ptr: *mut ScriptInstanceInfo, - property_list: Mutex>>, - method_list: Mutex>>, + property_lists: PtrlistContainer, + method_lists: PtrlistContainer, base: Base, } @@ -283,8 +283,8 @@ pub unsafe fn create_script_instance( let data = ScriptInstanceData { inner: GdCell::new(rust_instance), script_instance_ptr: instance_ptr, - property_list: Default::default(), - method_list: Default::default(), + property_lists: PtrlistContainer::new(), + method_lists: PtrlistContainer::new(), // SAFETY: The script instance is always freed before the base object is destroyed. The weak reference should therefore never be // accessed after it has been freed. base: unsafe { Base::from_gd(&for_object) }, @@ -453,14 +453,71 @@ impl<'a, T: ScriptInstance> DerefMut for SiMut<'a, T> { } } +// Encapsulate PtrlistContainer to help ensure safety. +mod ptrlist_container { + use std::collections::HashMap; + use std::sync::Mutex; + + pub struct PtrlistContainer { + list_lengths: Mutex>, + } + + impl PtrlistContainer { + pub fn new() -> Self { + Self { + list_lengths: Mutex::new(HashMap::new()), + } + } + + pub fn list_into_sys(&self, list: Vec) -> (*const T, u32) { + let len: u32 = list + .len() + .try_into() + .expect("list must have length that fits in u32"); + let ptr = Box::leak(list.into_boxed_slice()).as_mut_ptr(); + + let old_value = self.list_lengths.lock().unwrap().insert(ptr, len); + assert_eq!( + old_value, None, + "attempted to insert the same list twice, this is a bug" + ); + + (ptr as *const T, len) + } + + /// # Safety + /// - `ptr` must have been returned from a call to `list_into_sys` on `self`. + /// - `ptr` must not have been used in a call to this function before. + /// - `ptr` must not have been mutated since the call to `list_into_sys`. + /// - `ptr` must not be accessed after calling this function. + #[deny(unsafe_op_in_unsafe_fn)] + pub unsafe fn list_from_sys(&self, ptr: *const T) -> Box<[T]> { + let ptr: *mut T = ptr as *mut T; + let len = self + .list_lengths + .lock() + .unwrap() + .remove(&ptr) + .expect("attempted to free list from wrong collection, this is a bug"); + let len: usize = len + .try_into() + .expect("gdext only supports targets where u32 <= usize"); + + // SAFETY: `ptr` was created in `list_into_sys` from a slice of length `len`. + // And has not been mutated since. + let slice = unsafe { std::slice::from_raw_parts_mut(ptr, len) }; + + // SAFETY: This is the first call to this function, and the list will not be accessed again after this function call. + unsafe { Box::from_raw(slice) } + } + } +} + mod script_instance_info { use std::any::type_name; use std::ffi::c_void; - use std::mem::ManuallyDrop; - use crate::builtin::{GString, StringName, Variant}; - use crate::classes::ScriptLanguage; - use crate::obj::Gd; + use crate::builtin::{StringName, Variant}; use crate::private::handle_panic; use crate::sys; @@ -473,69 +530,6 @@ mod script_instance_info { const SYS_TRUE: sys::GDExtensionBool = bool_into_sys(true); const SYS_FALSE: sys::GDExtensionBool = bool_into_sys(true); - /// # Safety - /// - /// - The returned `*const T` is guaranteed to point to a list that has an equal length and capacity. - fn transfer_ptr_list_to_godot(ptr_list: Box<[T]>, list_length: &mut u32) -> *mut T { - *list_length = ptr_list.len() as u32; - - let ptr = Box::into_raw(ptr_list); - - // SAFETY: `ptr` was just created in the line above and should be safe to dereference. - unsafe { (*ptr).as_mut_ptr() } - } - - /// The returned pointer's lifetime is equal to the lifetime of `script` - fn transfer_script_to_godot(script: &Gd) -> sys::GDExtensionObjectPtr { - script.obj_sys() - } - - /// # Safety - /// - /// - `ptr` is expected to point to a list with both a length and capacity of `list_length`. - /// - The list pointer `ptr` must have been created with [`transfer_ptr_list_to_godot`]. - unsafe fn transfer_ptr_list_from_godot(ptr: *const T, list_length: usize) -> Vec { - Vec::from_raw_parts(sys::force_mut_ptr(ptr), list_length, list_length) - } - - /// # Safety - /// - /// - The engine has to provide a valid string return pointer. - unsafe fn transfer_string_to_godot(string: GString, return_ptr: sys::GDExtensionStringPtr) { - string.move_into_string_ptr(return_ptr); - } - - /// # Safety - /// - /// - `userdata` has to be a valid pointer that upholds the invariants of `sys::GDExtensionScriptInstancePropertyStateAdd`. - unsafe fn transfer_property_state_to_godot( - propery_states: Vec<(StringName, Variant)>, - property_state_add: sys::GDExtensionScriptInstancePropertyStateAdd, - userdata: *mut c_void, - ) { - let Some(property_state_add) = property_state_add else { - return; - }; - - propery_states.into_iter().for_each(|(name, value)| { - let name = ManuallyDrop::new(name); - let value = ManuallyDrop::new(value); - - // SAFETY: Godot expects a string name and a variant pointer for each property. After receiving the pointer, the engine is responsible - // for managing the memory behind those references. - unsafe { property_state_add(name.string_sys(), value.var_sys(), userdata) }; - }); - } - - /// # Safety - /// - /// - `script_lang` must live for as long as the pointer may be dereferenced. - unsafe fn transfer_script_lang_to_godot( - script_lang: Gd, - ) -> sys::GDExtensionScriptLanguagePtr { - script_lang.obj_sys() as sys::GDExtensionScriptLanguagePtr - } - /// # Safety /// /// - `p_instance` has to be a valid pointer that can be cast to `*mut ScriptInstanceData`. @@ -601,32 +595,27 @@ mod script_instance_info { ) -> *const sys::GDExtensionPropertyInfo { let ctx = || format!("error when calling {}::get_property_list", type_name::()); - let (property_list, property_sys_list) = handle_panic(ctx, || { + let property_list = handle_panic(ctx, || { let property_list = ScriptInstanceData::::borrow_script_sys(p_instance) .borrow() .get_property_list(); - let property_sys_list: Box<[_]> = property_list - .iter() - .map(|prop| prop.property_sys()) - .collect(); - - (property_list, property_sys_list) + property_list + .into_iter() + .map(|prop| prop.into_owned_property_sys()) + .collect::>() }) .unwrap_or_default(); - // SAFETY: list_length has to be a valid pointer to a u32. - let list_length = unsafe { &mut *r_count }; - let return_pointer = transfer_ptr_list_to_godot(property_sys_list, list_length); + let (list_ptr, list_length) = ScriptInstanceData::::borrow_script_sys(p_instance) + .property_lists + .list_into_sys(property_list); - let instance = ScriptInstanceData::::borrow_script_sys(p_instance); - instance - .property_list - .lock() - .expect("Mutex should not be poisoned") - .insert(return_pointer, property_list); + unsafe { + *r_count = list_length; + } - return_pointer + list_ptr } /// # Safety @@ -639,31 +628,27 @@ mod script_instance_info { ) -> *const sys::GDExtensionMethodInfo { let ctx = || format!("error when calling {}::get_method_list", type_name::()); - let (method_list, method_sys_list) = handle_panic(ctx, || { + let method_list = handle_panic(ctx, || { let method_list = ScriptInstanceData::::borrow_script_sys(p_instance) .borrow() .get_method_list(); - let method_sys_list = method_list - .iter() - .map(|method| method.method_sys()) + let method_list = method_list + .into_iter() + .map(|method| method.into_owned_method_sys()) .collect(); - (method_list, method_sys_list) + method_list }) .unwrap_or_default(); - // SAFETY: list_length has to be a valid pointer to a u32. - let list_length = unsafe { &mut *r_count }; - let return_pointer = transfer_ptr_list_to_godot(method_sys_list, list_length); - let instance = ScriptInstanceData::::borrow_script_sys(p_instance); - instance - .method_list - .lock() - .expect("mutex should not be poisoned") - .insert(return_pointer, method_list); + let (return_pointer, list_length) = instance.method_lists.list_into_sys(method_list); + + unsafe { + *r_count = list_length; + } return_pointer } @@ -678,17 +663,7 @@ mod script_instance_info { ) { let instance = ScriptInstanceData::::borrow_script_sys(p_instance); - let vec = instance - .property_list - .lock() - .expect("mutex should not be poisoned") - .remove(&p_prop_info) - .expect("Godot is trying to free the property list, but none has been set"); - - // SAFETY: p_prop_info is expected to have been created by get_property_list_func - // and therefore should have the same length as before. get_propery_list_func - // also guarantees that both vector length and capacity are equal. - let _drop = transfer_ptr_list_from_godot(p_prop_info, vec.len()); + let _drop = instance.property_lists.list_from_sys(p_prop_info); } /// # Safety @@ -747,11 +722,11 @@ mod script_instance_info { ScriptInstanceData::::borrow_script_sys(p_instance) .borrow() .get_script() - .to_owned() + .clone() }); match script { - Ok(script) => transfer_script_to_godot(&script), + Ok(script) => script.obj_sys(), Err(_) => std::ptr::null_mut(), } } @@ -806,27 +781,7 @@ mod script_instance_info { ) { let instance = ScriptInstanceData::::borrow_script_sys(p_instance); - let vec = instance - .method_list - .lock() - .expect("method_list mutex should not be poisoned") - .remove(&p_method_info) - .expect("Godot is trying to free the method_list, but none has been set"); - - // SAFETY: p_method_info is expected to have been created by get_method_list_func and therefore should have the same length as before. - // get_method_list_func also guarantees that both vector length and capacity are equal. - let vec_sys = transfer_ptr_list_from_godot(p_method_info, vec.len()); - - vec_sys.into_iter().for_each(|method_info| { - transfer_ptr_list_from_godot( - method_info.arguments, - method_info.argument_count as usize, - ); - transfer_ptr_list_from_godot( - method_info.default_arguments, - method_info.default_argument_count as usize, - ); - }) + let _drop = instance.method_lists.list_from_sys(p_method_info); } /// # Safety @@ -886,7 +841,7 @@ mod script_instance_info { }; *r_is_valid = SYS_TRUE; - transfer_string_to_godot(string, r_str); + string.move_into_string_ptr(r_str); } /// # Safety @@ -914,7 +869,17 @@ mod script_instance_info { }) .unwrap_or_default(); - transfer_property_state_to_godot(property_states, property_state_add, userdata); + let Some(property_state_add) = property_state_add else { + return; + }; + + for (name, value) in property_states { + property_state_add( + name.into_owned_string_sys(), + value.into_owned_var_sys(), + userdata, + ); + } } /// # Safety @@ -934,9 +899,9 @@ mod script_instance_info { }); if let Ok(language) = language { - transfer_script_lang_to_godot(language) + language.obj_sys().cast() } else { - std::ptr::null::() as sys::GDExtensionScriptLanguagePtr + std::ptr::null_mut() } } @@ -947,7 +912,7 @@ mod script_instance_info { pub(super) unsafe extern "C" fn free_func( p_instance: sys::GDExtensionScriptInstanceDataPtr, ) { - drop(Box::from_raw(p_instance as *mut ScriptInstanceData)); + drop(Box::from_raw(p_instance.cast::>())); } /// # Safety From 686b59dedb23c0a5f7d766243e666922205400ce Mon Sep 17 00:00:00 2001 From: Lili Zoey Zyli Date: Sun, 2 Jun 2024 18:32:21 +0200 Subject: [PATCH 3/3] Add `sys::conv` Add `#[deny(unsafe_op_in_unsafe_fn)]` to `script_instance_info` Use `sys::conv` bools Final touches --- godot-core/src/builtin/callable.rs | 6 +- godot-core/src/builtin/string/string_name.rs | 2 +- godot-core/src/meta/mod.rs | 68 ++-- godot-core/src/obj/script.rs | 364 +++++++++++------- godot-core/src/registry/callbacks.rs | 17 +- godot-core/src/registry/class.rs | 12 +- godot-core/src/registry/constant.rs | 2 +- godot-ffi/src/conv.rs | 65 ++++ godot-ffi/src/lib.rs | 2 + itest/godot/ManualFfiTests.gd | 15 +- .../script/script_instance_tests.rs | 38 +- 11 files changed, 359 insertions(+), 232 deletions(-) create mode 100644 godot-ffi/src/conv.rs diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index ddfb5c930..277024a09 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -432,7 +432,7 @@ mod custom_callable { let a: &T = CallableUserdata::inner_from_raw(callable_userdata_a); let b: &T = CallableUserdata::inner_from_raw(callable_userdata_b); - (a == b) as sys::GDExtensionBool + sys::conv::bool_to_sys(a == b) } pub unsafe extern "C" fn rust_callable_to_string_display( @@ -444,7 +444,7 @@ mod custom_callable { let s = crate::builtin::GString::from(c.to_string()); s.move_into_string_ptr(r_out); - *r_is_valid = true as sys::GDExtensionBool; + *r_is_valid = sys::conv::SYS_TRUE; } pub unsafe extern "C" fn rust_callable_to_string_named( @@ -455,6 +455,6 @@ mod custom_callable { let w: &mut FnWrapper = CallableUserdata::inner_from_raw(callable_userdata); w.name.clone().move_into_string_ptr(r_out); - *r_is_valid = true as sys::GDExtensionBool; + *r_is_valid = sys::conv::SYS_TRUE; } } diff --git a/godot-core/src/builtin/string/string_name.rs b/godot-core/src/builtin/string/string_name.rs index d33f83e6e..f49614051 100644 --- a/godot-core/src/builtin/string/string_name.rs +++ b/godot-core/src/builtin/string/string_name.rs @@ -317,7 +317,7 @@ impl From<&'static std::ffi::CStr> for StringName { sys::interface_fn!(string_name_new_with_latin1_chars)( ptr, c_str.as_ptr(), - true as sys::GDExtensionBool, // p_is_static + sys::conv::SYS_TRUE, // p_is_static ) }) }; diff --git a/godot-core/src/meta/mod.rs b/godot-core/src/meta/mod.rs index 8a2c063a0..641688c0a 100644 --- a/godot-core/src/meta/mod.rs +++ b/godot-core/src/meta/mod.rs @@ -17,6 +17,7 @@ mod traits; pub mod error; pub use class_name::ClassName; pub use godot_convert::{FromGodot, GodotConvert, ToGodot}; +use sys::conv::u32_to_usize; pub use traits::{ArrayElement, GodotType}; pub(crate) use crate::impl_godot_as_self; @@ -312,43 +313,46 @@ impl MethodInfo { default_arguments, } = info; - // SAFETY: `name` and `return_value` were created from the appropriate method calls, and have not been freed before this. - unsafe { - let _name = StringName::from_owned_string_sys(name); - PropertyInfo::free_owned_property_sys(return_value); - } + // SAFETY: `name` is a pointer that was returned from `StringName::into_owned_string_sys`, and has not been freed before this. + let _name = unsafe { StringName::from_owned_string_sys(name) }; + + // SAFETY: `return_value` is a pointer that was returned from `PropertyInfo::into_owned_property_sys`, and has not been freed before + // this. + unsafe { PropertyInfo::free_owned_property_sys(return_value) }; + + // SAFETY: + // - `from_raw_parts_mut`: `arguments` comes from `as_mut_ptr()` on a mutable slice of length `argument_count`, and no other + // accesses to the pointer happens for the lifetime of the slice. + // - `Box::from_raw`: The slice was returned from a call to `Box::leak`, and we have ownership of the value behind this pointer. + let arguments = unsafe { + let slice = std::slice::from_raw_parts_mut(arguments, u32_to_usize(argument_count)); - // SAFETY: These pointers were both created from a call to `as_mut_ptr` on a slice. Additionally these pointer will not be accessed - // again after this function call. - let (arguments_slice, default_arguments_slice) = unsafe { - ( - std::slice::from_raw_parts_mut( - arguments, - argument_count - .try_into() - .expect("gdext only supports targets where u32 <= usize"), - ), - std::slice::from_raw_parts_mut( - default_arguments, - default_argument_count - .try_into() - .expect("gdext only supports targets where u32 <= usize"), - ), - ) + Box::from_raw(slice) }; - // SAFETY: We have exclusive ownership of these slices, and they were originally created from a call to `Box::leak`. - let (_arguments, default_arguments) = unsafe { - ( - Box::from_raw(arguments_slice), - Box::from_raw(default_arguments_slice), - ) + for info in arguments.iter() { + // SAFETY: These infos were originally created from a call to `PropertyInfo::into_owned_property_sys`, and this method + // will not be called again on this pointer. + unsafe { PropertyInfo::free_owned_property_sys(*info) } + } + + // SAFETY: + // - `from_raw_parts_mut`: `default_arguments` comes from `as_mut_ptr()` on a mutable slice of length `default_argument_count`, and no + // other accesses to the pointer happens for the lifetime of the slice. + // - `Box::from_raw`: The slice was returned from a call to `Box::leak`, and we have ownership of the value behind this pointer. + let default_arguments = unsafe { + let slice = std::slice::from_raw_parts_mut( + default_arguments, + u32_to_usize(default_argument_count), + ); + + Box::from_raw(slice) }; - default_arguments.iter().for_each(|ptr| { - // SAFETY: These pointers were originally created from a call to `Variant::into_owner_var_sys`, and this method will not be + for variant in default_arguments.iter() { + // SAFETY: These pointers were originally created from a call to `Variant::into_owned_var_sys`, and this method will not be // called again on this pointer. - let _variant = unsafe { Variant::from_owned_var_sys(*ptr) }; - }); + let _variant = unsafe { Variant::from_owned_var_sys(*variant) }; + } } } diff --git a/godot-core/src/obj/script.rs b/godot-core/src/obj/script.rs index 75e48dda4..fb54459f2 100644 --- a/godot-core/src/obj/script.rs +++ b/godot-core/src/obj/script.rs @@ -30,7 +30,7 @@ use crate::meta::{MethodInfo, PropertyInfo}; use crate::obj::{Base, Gd, GodotClass}; use crate::sys; -use self::ptrlist_container::PtrlistContainer; +use self::bounded_ptr_list::BoundedPtrList; /// Implement custom scripts that can be attached to objects in Godot. /// @@ -131,7 +131,7 @@ pub trait ScriptInstance: Sized { /// Lets the engine get a reference to the script this instance was created for. /// - /// This function has to return a reference, because Scripts are reference counted in Godot and it has to be guaranteed that the object is + /// This function has to return a reference, because Scripts are reference counted in Godot and it must be guaranteed that the object is /// not freed before the engine increased the reference count. (every time a `Gd` which contains a reference counted object is dropped the /// reference count is decremented.) fn get_script(&self) -> &Gd