From 7999b6620a26171a5c24d2763277571deb181cfa Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 22 Jul 2024 22:59:19 +0200 Subject: [PATCH 1/8] Optimize GString Display/Debug to not use allocations --- godot-core/src/builtin/string/gstring.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/godot-core/src/builtin/string/gstring.rs b/godot-core/src/builtin/string/gstring.rs index 426df9db2..37501355b 100644 --- a/godot-core/src/builtin/string/gstring.rs +++ b/godot-core/src/builtin/string/gstring.rs @@ -5,6 +5,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ +use std::fmt::Write; use std::{convert::Infallible, ffi::c_char, fmt, str::FromStr}; use godot_ffi as sys; @@ -230,26 +231,19 @@ impl_builtin_traits! { impl fmt::Display for GString { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let s: String; - - #[cfg(before_api = "4.1")] - { - s = self.chars_checked().iter().collect(); - } - #[cfg(since_api = "4.1")] - { - s = self.chars().iter().collect(); + for ch in self.chars() { + f.write_char(*ch)?; } - f.write_str(s.as_str()) + Ok(()) } } /// Uses literal syntax from GDScript: `"string"` impl fmt::Debug for GString { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - let s = String::from(self); - write!(f, "\"{s}\"") + // Reuse Display impl. + write!(f, "\"{self}\"") } } From 0ed8a15f1aee97956c471a79fb4b39f2e2cbac4b Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 22 Jul 2024 23:10:49 +0200 Subject: [PATCH 2/8] Remove GString::chars_checked(), chars_unchecked() + platform-specific intrinsics --- godot-core/src/builtin/string/gstring.rs | 56 +----- godot-core/src/builtin/string/mod.rs | 1 - godot-core/src/builtin/string/string_chars.rs | 174 ------------------ .../src/builtin_tests/string/gstring_test.rs | 29 +-- 4 files changed, 10 insertions(+), 250 deletions(-) delete mode 100644 godot-core/src/builtin/string/string_chars.rs diff --git a/godot-core/src/builtin/string/gstring.rs b/godot-core/src/builtin/string/gstring.rs index 37501355b..0bd76cfe3 100644 --- a/godot-core/src/builtin/string/gstring.rs +++ b/godot-core/src/builtin/string/gstring.rs @@ -12,10 +12,7 @@ use godot_ffi as sys; use sys::types::OpaqueString; use sys::{ffi_methods, interface_fn, GodotFfi}; -use crate::builtin::inner; - -use super::string_chars::validate_unicode_scalar_sequence; -use super::{NodePath, StringName}; +use crate::builtin::{inner, NodePath, StringName}; /// Godot's reference counted string type. /// @@ -85,16 +82,9 @@ impl GString { } /// Gets the internal chars slice from a [`GString`]. - /// - /// Note: This operation is *O*(*n*). Consider using [`chars_unchecked`][Self::chars_unchecked] - /// if you can make sure the string is a valid UTF-32. - #[cfg_attr( - since_api = "4.1", - deprecated = "Use `chars()` instead. \n\ - Since version 4.1, Godot ensures valid UTF-32, checked and unchecked overloads are no longer needed. \n\ - For details, see [godotengine/godot#74760](https://github.com/godotengine/godot/pull/74760)." - )] - pub fn chars_checked(&self) -> &[char] { + pub fn chars(&self) -> &[char] { + // SAFETY: Godot 4.1 ensures valid UTF-32, making interpreting as char slice safe. + // See https://github.com/godotengine/godot/pull/74760. unsafe { let s = self.string_sys(); let len = interface_fn!(string_to_utf32_chars)(s, std::ptr::null_mut(), 0); @@ -105,43 +95,7 @@ impl GString { return &[]; } - validate_unicode_scalar_sequence(std::slice::from_raw_parts(ptr, len as usize)) - .expect("GString::chars_checked: string contains invalid unicode scalar values") - } - } - - /// Gets the internal chars slice from a [`GString`]. - /// - /// # Safety - /// - /// Make sure the string only contains valid unicode scalar values, currently - /// Godot allows for unpaired surrogates and out of range code points to be appended - /// into the string. - #[cfg_attr( - since_api = "4.1", - deprecated = "Use `chars()` instead. \n\ - Since version 4.1, ensures valid UTF-32, checked and unchecked overloads are no longer needed. \n\ - For details, see [godotengine/godot#74760](https://github.com/godotengine/godot/pull/74760)." - )] - pub unsafe fn chars_unchecked(&self) -> &[char] { - let s = self.string_sys(); - let len = interface_fn!(string_to_utf32_chars)(s, std::ptr::null_mut(), 0); - let ptr = interface_fn!(string_operator_index_const)(s, 0); - - // Even when len == 0, from_raw_parts requires ptr != 0 - if ptr.is_null() { - return &[]; - } - std::slice::from_raw_parts(ptr as *const char, len as usize) - } - - /// Gets the internal chars slice from a [`GString`]. - #[cfg(since_api = "4.1")] - pub fn chars(&self) -> &[char] { - // SAFETY: Godot 4.1 ensures valid UTF-32, making this safe to call. - #[allow(deprecated)] - unsafe { - self.chars_unchecked() + std::slice::from_raw_parts(ptr as *const char, len as usize) } } diff --git a/godot-core/src/builtin/string/mod.rs b/godot-core/src/builtin/string/mod.rs index a3e5d9510..ceafd2cba 100644 --- a/godot-core/src/builtin/string/mod.rs +++ b/godot-core/src/builtin/string/mod.rs @@ -10,7 +10,6 @@ mod gstring; mod macros; mod node_path; -mod string_chars; mod string_name; use crate::meta::error::ConvertError; diff --git a/godot-core/src/builtin/string/string_chars.rs b/godot-core/src/builtin/string/string_chars.rs deleted file mode 100644 index fd4020e0f..000000000 --- a/godot-core/src/builtin/string/string_chars.rs +++ /dev/null @@ -1,174 +0,0 @@ -/* - * Copyright (c) godot-rust; Bromeon and contributors. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. - */ - -#[cfg(target_arch = "aarch64")] -use std::arch::aarch64::*; -#[cfg(target_arch = "x86")] -use std::arch::x86::*; -#[cfg(target_arch = "x86_64")] -use std::arch::x86_64::*; - -/// Validates is a [`u32`] slice contains only valid [unicode scalar values](https://www.unicode.org/glossary/#unicode_scalar_value) -pub fn validate_unicode_scalar_sequence(seq: &[u32]) -> Option<&[char]> { - unsafe { - let mut ptr = seq.as_ptr(); - let ptr_end = seq.as_ptr().add(seq.len()); - - #[cfg(any(target_arch = "x86_64", target_arch = "x86"))] - loop { - let ptr_next = ptr.add(4); - if ptr_next > ptr_end { - break; - } - - let block = _mm_loadu_si128(ptr as *const __m128i); - - // Check if there is any character greater than `char::MAX` or less than 0, (SSE2 uses signed math). - if _mm_movemask_epi8(_mm_and_si128( - _mm_cmpgt_epi32(block, _mm_set1_epi32(-1)), - _mm_cmplt_epi32(block, _mm_set1_epi32(char::MAX as i32 + 1)), - )) != 0xFFFF - { - return None; - } - - // Check if there is any high-surrogate and low-surrogate code points. - if _mm_testz_si128( - _mm_cmpgt_epi32(block, _mm_set1_epi32(0xD7FF)), - _mm_cmplt_epi32(block, _mm_set1_epi32(0xE000)), - ) == 0 - { - return None; - } - - ptr = ptr_next; - } - - #[cfg(target_arch = "aarch64")] - loop { - let ptr_next = ptr.add(4); - if ptr_next > ptr_end { - break; - } - - let block = vld1q_u32(ptr); - - // Check if there is any character bigger than `char::MAX`. - if vmaxvq_u32(block) >= char::MAX as u32 { - return None; - } - - // Check if there is any high-surrogate and low-surrogate code points. - // This is in the range `0xD800..0xE000`. - if vminvq_u32(vsubq_u32(block, vdupq_n_u32(0xD800))) < (0xE000 - 0xD800) { - return None; - } - - ptr = ptr_next; - } - - loop { - if ptr >= ptr_end { - break; - } - - char::from_u32(*ptr)?; - - ptr = ptr.add(1); - } - - Some(std::slice::from_raw_parts( - seq.as_ptr() as *const char, - seq.len(), - )) - } -} - -#[cfg(test)] -mod tests { - // Simple random pseudorandom number generator using the linear congruential method. - struct Rand { - state: u64, - } - - impl Rand { - const A: u64 = 6364136223846793005; - const C: u64 = 1442695040888963407; - - fn new(seed: u64) -> Self { - Self { state: seed } - } - - fn next(&mut self) -> u32 { - self.state = Self::A.wrapping_mul(self.state).wrapping_add(Self::C); - self.state as u32 - } - } - - #[test] - fn check_valid_unicode() { - let mut rand = Rand::new(0xA102FE1); - for _ in 0..16 { - let len = (rand.next() % 128).min(80); - let chars: Vec = (0..len) - .map(|_| rand.next() % (char::MAX as u32)) - .filter_map(char::from_u32) - .map(|x| x as u32) - .collect(); - - assert!(!chars.is_empty()); - - assert!(super::validate_unicode_scalar_sequence(chars.as_slice()).is_some()); - } - } - - #[test] - fn check_unpaired_surrogate_unicode() { - let mut rand = Rand::new(0xA102FE1); - for _ in 0..16 { - let len = (rand.next() % 128).min(80); - let mut chars: Vec = (0..len) - .map(|_| rand.next() % char::MAX as u32) - .filter_map(char::from_u32) - .map(|x| x as u32) - .collect(); - - assert!(!chars.is_empty()); - - for _ in 0..4 { - let surrogate = rand.next() % (0xE000 - 0xD800) + 0xD800; - assert!(char::from_u32(surrogate).is_none()); - chars.insert(rand.next() as usize % chars.len(), surrogate); - } - - assert!(super::validate_unicode_scalar_sequence(chars.as_slice()).is_none()); - } - } - - #[test] - fn check_out_of_range_unicode() { - let mut rand = Rand::new(0xA102FE1); - for _ in 0..16 { - let len = (rand.next() % 128).min(80); - let mut chars: Vec = (0..len) - .map(|_| rand.next() % char::MAX as u32) - .filter_map(char::from_u32) - .map(|x| x as u32) - .collect(); - - assert!(!chars.is_empty()); - - for _ in 0..4 { - let out_of_range = rand.next() % (u32::MAX - char::MAX as u32) + char::MAX as u32; - assert!(char::from_u32(out_of_range).is_none()); - chars.insert(rand.next() as usize % chars.len(), out_of_range); - } - - assert!(super::validate_unicode_scalar_sequence(chars.as_slice()).is_none()); - } - } -} diff --git a/itest/rust/src/builtin_tests/string/gstring_test.rs b/itest/rust/src/builtin_tests/string/gstring_test.rs index 7b2268db4..ec0c669f0 100644 --- a/itest/rust/src/builtin_tests/string/gstring_test.rs +++ b/itest/rust/src/builtin_tests/string/gstring_test.rs @@ -64,36 +64,17 @@ fn string_clone() { assert_eq!(first, cloned); } -#[itest] -fn empty_string_chars() { - // Tests regression from #228: Null pointer passed to slice::from_raw_parts(). - let s = GString::new(); - - #[allow(deprecated)] - { - assert_eq!(s.chars_checked(), &[]); - assert_eq!(unsafe { s.chars_unchecked() }, &[]); - } - #[cfg(since_api = "4.1")] - { - assert_eq!(s.chars(), &[]); - } -} - #[itest] fn string_chars() { + // Empty tests regression from #228: Null pointer passed to slice::from_raw_parts(). + let string = GString::new(); + assert_eq!(string.chars(), &[]); + let string = String::from("some_string"); let string_chars: Vec = string.chars().collect(); let gstring = GString::from(string); - #[allow(deprecated)] - { - assert_eq!(string_chars, gstring.chars_checked().to_vec()); - } - #[cfg(since_api = "4.1")] - { - assert_eq!(string_chars, gstring.chars().to_vec()); - } + assert_eq!(string_chars, gstring.chars().to_vec()); } #[itest] From 4a7f16c6482e9d9ad89507a7c4c3b799b77c8098 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 22 Jul 2024 23:12:09 +0200 Subject: [PATCH 3/8] Remove #[cfg]s handling Godot 4.0 code paths --- Cargo.toml | 3 -- godot-core/src/builtin/basis.rs | 16 --------- godot-core/src/builtin/transform3d.rs | 14 -------- godot-core/src/builtin/variant/mod.rs | 30 ++--------------- godot-core/src/obj/object_arg.rs | 2 +- godot-core/src/obj/raw_gd.rs | 33 +++++-------------- godot-core/src/obj/script.rs | 2 +- godot-core/src/registry/class.rs | 5 +-- godot-ffi/src/godot_ffi.rs | 19 ++--------- godot-ffi/src/string_cache.rs | 2 +- godot-macros/src/class/derive_godot_class.rs | 2 -- itest/godot/SpecialTests.gd | 7 ++-- .../geometry/transform2d_test.rs | 1 - itest/rust/src/engine_tests/codegen_test.rs | 2 +- itest/rust/src/object_tests/object_test.rs | 1 - 15 files changed, 19 insertions(+), 120 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 3f8470937..3e79a7fd2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,6 +15,3 @@ members = [ "examples/dodge-the-creeps/rust", "examples/hot-reload/rust", ] - -#[patch."https://github.com/godot-rust/godot4-prebuilt"] -#godot4-prebuilt = { git = "https://github.com//godot-rust/godot4-prebuilt", branch = "4.1" } diff --git a/godot-core/src/builtin/basis.rs b/godot-core/src/builtin/basis.rs index f1c59836b..c701d7c69 100644 --- a/godot-core/src/builtin/basis.rs +++ b/godot-core/src/builtin/basis.rs @@ -142,27 +142,11 @@ impl Basis { } } - /// Creates a `Basis` with a rotation such that the forward axis (-Z) points - /// towards the `target` position. - /// - /// The up axis (+Y) points as close to the `up` vector as possible while - /// staying perpendicular to the forward axis. The resulting Basis is - /// orthonormalized. The `target` and `up` vectors cannot be zero, and - /// cannot be parallel to each other. - /// - #[cfg(before_api = "4.1")] - /// _Godot equivalent: `Basis.looking_at()`_ - #[doc(alias = "looking_at")] - pub fn new_looking_at(target: Vector3, up: Vector3) -> Self { - super::inner::InnerBasis::looking_at(target, up) - } - /// If `use_model_front` is true, the +Z axis (asset front) is treated as forward (implies +X is left) /// and points toward the target position. By default, the -Z axis (camera forward) is treated as forward /// (implies +X is right). /// /// _Godot equivalent: `Basis.looking_at()`_ - #[cfg(since_api = "4.1")] pub fn new_looking_at(target: Vector3, up: Vector3, use_model_front: bool) -> Self { super::inner::InnerBasis::looking_at(target, up, use_model_front) } diff --git a/godot-core/src/builtin/transform3d.rs b/godot-core/src/builtin/transform3d.rs index bcf6c731f..a959b31bf 100644 --- a/godot-core/src/builtin/transform3d.rs +++ b/godot-core/src/builtin/transform3d.rs @@ -145,20 +145,6 @@ impl Transform3D { self.basis.is_finite() && self.origin.is_finite() } - /// Returns a copy of the transform rotated such that the forward axis (-Z) - /// points towards the `target` position. - /// - /// See [`Basis::new_looking_at()`] for more information. - #[cfg(before_api = "4.1")] - #[must_use] - pub fn looking_at(&self, target: Vector3, up: Vector3) -> Self { - Self { - basis: Basis::new_looking_at(target - self.origin, up), - origin: self.origin, - } - } - - #[cfg(since_api = "4.1")] #[must_use] pub fn looking_at(&self, target: Vector3, up: Vector3, use_model_front: bool) -> Self { Self { diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index 8e03bac67..5c205ec0f 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -116,7 +116,7 @@ impl Variant { let mut error = sys::default_call_error(); let result = unsafe { - Variant::new_with_var_uninit_or_init(|variant_ptr| { + Variant::new_with_var_uninit(|variant_ptr| { interface_fn!(variant_call)( sys::SysPtr::force_mut(self.var_sys()), method.string_sys(), @@ -148,7 +148,7 @@ impl Variant { let mut is_valid = false as u8; let result = unsafe { - Self::new_with_var_uninit_or_init(|variant_ptr| { + Self::new_with_var_uninit(|variant_ptr| { interface_fn!(variant_evaluate)( op_sys, self.var_sys(), @@ -238,32 +238,6 @@ impl Variant { } } - /// # Safety - /// - /// For Godot 4.0, see [`GodotFfi::new_with_init`]. - /// For all other versions, see [`GodotFfi::new_with_uninit`]. - #[cfg(before_api = "4.1")] - #[doc(hidden)] - pub unsafe fn new_with_var_uninit_or_init( - init_fn: impl FnOnce(sys::GDExtensionVariantPtr), - ) -> Self { - // SAFETY: We're in Godot 4.0, and so the caller must ensure this is safe. - unsafe { Self::new_with_var_init(init_fn) } - } - - /// # Safety - /// - /// For Godot 4.0, see [`GodotFfi::new_with_init`]. - /// For all other versions, see [`GodotFfi::new_with_uninit`]. - #[cfg(since_api = "4.1")] - #[doc(hidden)] - pub unsafe fn new_with_var_uninit_or_init( - init_fn: impl FnOnce(sys::GDExtensionUninitializedVariantPtr), - ) -> Self { - // SAFETY: We're not in Godot 4.0, and so the caller must ensure this is safe. - unsafe { Self::new_with_var_uninit(init_fn) } - } - /// Fallible construction of a `Variant` using a fallible initialization function. /// /// # Safety diff --git a/godot-core/src/obj/object_arg.rs b/godot-core/src/obj/object_arg.rs index 612422d55..8d2c62aad 100644 --- a/godot-core/src/obj/object_arg.rs +++ b/godot-core/src/obj/object_arg.rs @@ -172,7 +172,7 @@ where // https://github.com/godotengine/godot-cpp/issues/954 fn as_arg_ptr(&self) -> sys::GDExtensionConstTypePtr { - raw_gd::object_as_arg_ptr(self, &self.object_ptr) + raw_gd::object_as_arg_ptr(&self.object_ptr) } unsafe fn from_arg_ptr(_ptr: sys::GDExtensionTypePtr, _call_type: PtrcallType) -> Self { diff --git a/godot-core/src/obj/raw_gd.rs b/godot-core/src/obj/raw_gd.rs index 096aacb0d..1632df5bb 100644 --- a/godot-core/src/obj/raw_gd.rs +++ b/godot-core/src/obj/raw_gd.rs @@ -136,8 +136,8 @@ impl RawGd { where U: GodotClass, { - // Workaround for bug in Godot 4.0 that makes casts always succeed (https://github.com/godot-rust/gdext/issues/158). - // TODO once fixed in Godot, use #[cfg(before_api = "4.1")] + // Workaround for bug in Godot that makes casts always succeed (https://github.com/godot-rust/gdext/issues/158). + // TODO once fixed in Godot, remove this. if !self.is_cast_valid::() { return Err(self); } @@ -482,7 +482,7 @@ where fn as_arg_ptr(&self) -> sys::GDExtensionConstTypePtr { // Even though ObjectArg exists, this function is still relevant, e.g. in Callable. - object_as_arg_ptr(self, &self.obj) + object_as_arg_ptr(&self.obj) } unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, call_type: PtrcallType) -> Self { @@ -494,12 +494,9 @@ where // ptr is `Ref*` // See the docs for `PtrcallType::Virtual` for more info on `Ref`. interface_fn!(ref_get_object)(ptr as sys::GDExtensionRefPtr) - } else if cfg!(since_api = "4.1") || matches!(call_type, PtrcallType::Virtual) { - // ptr is `T**` - *(ptr as *mut sys::GDExtensionObjectPtr) } else { - // ptr is `T*` - ptr as sys::GDExtensionObjectPtr + // ptr is `T**` from Godot 4.1 onwards, also in virtual functions. + *(ptr as *mut sys::GDExtensionObjectPtr) }; // obj_ptr is `T*` @@ -689,10 +686,7 @@ pub(super) fn object_ffi_to_variant(self_: &T) -> Variant { } } -pub(super) fn object_as_arg_ptr( - _self: &T, - _object_ptr_field: &*mut F, -) -> sys::GDExtensionConstTypePtr { +pub(super) fn object_as_arg_ptr(_object_ptr_field: &*mut F) -> sys::GDExtensionConstTypePtr { // Be careful when refactoring this code. Address of field pointer matters, copying it into a local variable will create use-after-free. // No need to call self.check_rtti("as_arg_ptr") here, since this is already done in ToGodot impl. @@ -701,17 +695,6 @@ pub(super) fn object_as_arg_ptr( // We do not need to prematurely do so. In Rust terms, if `T` is ref-counted, then we are effectively passing a `&Arc`, and the // callee would need to invoke `.clone()` if desired. - // In 4.0, argument pointers are passed to godot as `T*`, except for in virtual method calls. We can't perform virtual method calls - // currently, so they are always `T*`. - // - // In 4.1, argument pointers were standardized to always be `T**`. - #[cfg(before_api = "4.1")] - { - _self.sys() - } - - #[cfg(since_api = "4.1")] - { - ptr::addr_of!(*_object_ptr_field) as sys::GDExtensionConstTypePtr - } + // Since 4.1, argument pointers were standardized to always be `T**`. + ptr::addr_of!(*_object_ptr_field) as sys::GDExtensionConstTypePtr } diff --git a/godot-core/src/obj/script.rs b/godot-core/src/obj/script.rs index adc4a4222..d1ec14319 100644 --- a/godot-core/src/obj/script.rs +++ b/godot-core/src/obj/script.rs @@ -252,7 +252,7 @@ pub unsafe fn create_script_instance( property_can_revert_func: None, // unimplemented until needed. property_get_revert_func: None, // unimplemented until needed. - // ScriptInstance::get_owner() is apparently not called by Godot 4.0 to 4.2 (to verify). + // ScriptInstance::get_owner() is apparently not called by Godot 4.1 to 4.2 (to verify). get_owner_func: None, get_property_state_func: Some(script_instance_info::get_property_state_func::), diff --git a/godot-core/src/registry/class.rs b/godot-core/src/registry/class.rs index 1fe8e65a5..394bbfb80 100644 --- a/godot-core/src/registry/class.rs +++ b/godot-core/src/registry/class.rs @@ -27,12 +27,11 @@ static LOADED_CLASSES: Global< // ---------------------------------------------------------------------------------------------------------------------------------------------- -/// Represents a class who is currently loaded and retained in memory. +/// Represents a class which is currently loaded and retained in memory. /// /// Besides the name, this type holds information relevant for the deregistration of the class. pub struct LoadedClass { name: ClassName, - #[cfg_attr(before_api = "4.1", allow(dead_code))] is_editor_plugin: bool, } @@ -424,7 +423,6 @@ fn register_class_raw(mut info: ClassRegistrationInfo) { (register_fn.raw)(&mut class_builder); } - #[cfg(since_api = "4.1")] if info.is_editor_plugin { unsafe { interface_fn!(editor_add_plugin)(class_name.string_sys()) }; } @@ -435,7 +433,6 @@ fn unregister_class_raw(class: LoadedClass) { out!("Unregister class: {class_name}"); // If class is an editor plugin, unregister that first. - #[cfg(since_api = "4.1")] if class.is_editor_plugin { unsafe { interface_fn!(editor_remove_plugin)(class_name.string_sys()); diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index 7d8d475a9..8603399f0 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -105,27 +105,12 @@ pub unsafe trait GodotFfi { // This method abstracts over that. Declared outside GodotFfi because it should not be overridden. /// # Safety -/// -/// - For Godot version 4.0 see [`GodotFfi::new_with_init`]. -/// - For Godot versions >= 4.1 see [`GodotFfi::new_with_uninit`]. -#[cfg(before_api = "4.1")] -pub unsafe fn new_with_uninit_or_init( - init_fn: impl FnOnce(sys::GDExtensionTypePtr), -) -> T { - // SAFETY: `before_api = "4.1"` so the user must fulfil the safety preconditions of `new_with_init`. - unsafe { T::new_with_init(init_fn) } -} - -/// # Safety -/// -/// - For Godot version 4.0 see [`GodotFfi::new_with_init`]. -/// - For Godot versions >= 4.1 see [`GodotFfi::new_with_uninit`]. +/// See [`GodotFfi::new_with_uninit`] (valid for Godot 4.1+). #[cfg(since_api = "4.1")] pub unsafe fn new_with_uninit_or_init( init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr), ) -> T { - // SAFETY: `since_api = "4.1"` so the user must fulfil the safety preconditions of `new_with_uninit`. - unsafe { T::new_with_uninit(init_fn) } + T::new_with_uninit(init_fn) } // ---------------------------------------------------------------------------------------------------------------------------------------------- diff --git a/godot-ffi/src/string_cache.rs b/godot-ffi/src/string_cache.rs index 7b741c921..44674d2cf 100644 --- a/godot-ffi/src/string_cache.rs +++ b/godot-ffi/src/string_cache.rs @@ -43,7 +43,7 @@ impl<'a> StringCache<'a> { let mut sname = MaybeUninit::::uninit(); let sname_ptr = sname.as_mut_ptr(); - // For Godot 4.0 and 4.1, construct StringName via String + conversion. + // For Godot 4.1, construct StringName via String + conversion. #[cfg(before_api = "4.2")] unsafe { let string_new_with_latin1_chars_and_len = self diff --git a/godot-macros/src/class/derive_godot_class.rs b/godot-macros/src/class/derive_godot_class.rs index f1ee01e37..2707b13cb 100644 --- a/godot-macros/src/class/derive_godot_class.rs +++ b/godot-macros/src/class/derive_godot_class.rs @@ -310,8 +310,6 @@ fn parse_struct_attributes(class: &venial::Struct) -> ParseResult Date: Sat, 27 Jul 2024 20:47:39 +0200 Subject: [PATCH 4/8] Remove new_with_uninit_or_init::() compat layer --- godot-core/src/builtin/callable.rs | 2 +- godot-core/src/builtin/collections/array.rs | 2 +- godot-core/src/builtin/signal.rs | 2 +- godot-core/src/builtin/string/gstring.rs | 4 ++-- godot-core/src/builtin/string/node_path.rs | 2 +- godot-core/src/builtin/string/string_name.rs | 2 +- godot-core/src/builtin/variant/impls.rs | 10 +--------- godot-ffi/src/godot_ffi.rs | 13 ------------- godot-ffi/src/lib.rs | 4 +--- 9 files changed, 9 insertions(+), 32 deletions(-) diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index c360bf850..331b8bc4d 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -47,7 +47,7 @@ impl Callable { // upcast not needed let method = method_name.into(); unsafe { - sys::new_with_uninit_or_init::(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = sys::builtin_fn!(callable_from_object_method); let raw = object.to_ffi(); let args = [raw.as_arg_ptr(), method.sys()]; diff --git a/godot-core/src/builtin/collections/array.rs b/godot-core/src/builtin/collections/array.rs index 08673474f..61ccb2a5b 100644 --- a/godot-core/src/builtin/collections/array.rs +++ b/godot-core/src/builtin/collections/array.rs @@ -1001,7 +1001,7 @@ impl GodotFfiVariant for Array { } let array = unsafe { - sys::new_with_uninit_or_init::(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let array_from_variant = sys::builtin_fn!(array_from_variant); array_from_variant(self_ptr, sys::SysPtr::force_mut(variant.var_sys())); }) diff --git a/godot-core/src/builtin/signal.rs b/godot-core/src/builtin/signal.rs index a7ac5ebcd..365252969 100644 --- a/godot-core/src/builtin/signal.rs +++ b/godot-core/src/builtin/signal.rs @@ -40,7 +40,7 @@ impl Signal { { let signal_name = signal_name.into(); unsafe { - sys::new_with_uninit_or_init::(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = sys::builtin_fn!(signal_from_object_signal); let raw = object.to_ffi(); let args = [raw.as_arg_ptr(), signal_name.sys()]; diff --git a/godot-core/src/builtin/string/gstring.rs b/godot-core/src/builtin/string/gstring.rs index 0bd76cfe3..f82943745 100644 --- a/godot-core/src/builtin/string/gstring.rs +++ b/godot-core/src/builtin/string/gstring.rs @@ -277,7 +277,7 @@ impl FromStr for GString { impl From<&StringName> for GString { fn from(string: &StringName) -> Self { unsafe { - sys::new_with_uninit_or_init::(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = sys::builtin_fn!(string_from_string_name); let args = [string.sys()]; ctor(self_ptr, args.as_ptr()); @@ -298,7 +298,7 @@ impl From for GString { impl From<&NodePath> for GString { fn from(path: &NodePath) -> Self { unsafe { - sys::new_with_uninit_or_init::(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = sys::builtin_fn!(string_from_node_path); let args = [path.sys()]; ctor(self_ptr, args.as_ptr()); diff --git a/godot-core/src/builtin/string/node_path.rs b/godot-core/src/builtin/string/node_path.rs index 475d85c23..b246efef6 100644 --- a/godot-core/src/builtin/string/node_path.rs +++ b/godot-core/src/builtin/string/node_path.rs @@ -118,7 +118,7 @@ impl From<&String> for NodePath { impl From<&GString> for NodePath { fn from(string: &GString) -> Self { unsafe { - sys::new_with_uninit_or_init::(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = sys::builtin_fn!(node_path_from_string); let args = [string.sys()]; ctor(self_ptr, args.as_ptr()); diff --git a/godot-core/src/builtin/string/string_name.rs b/godot-core/src/builtin/string/string_name.rs index a312510c9..9a8b85802 100644 --- a/godot-core/src/builtin/string/string_name.rs +++ b/godot-core/src/builtin/string/string_name.rs @@ -242,7 +242,7 @@ impl From<&String> for StringName { impl From<&GString> for StringName { fn from(string: &GString) -> Self { unsafe { - sys::new_with_uninit_or_init::(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let ctor = sys::builtin_fn!(string_name_from_string); let args = [string.sys()]; ctor(self_ptr, args.as_ptr()); diff --git a/godot-core/src/builtin/variant/impls.rs b/godot-core/src/builtin/variant/impls.rs index 125331b50..188c72742 100644 --- a/godot-core/src/builtin/variant/impls.rs +++ b/godot-core/src/builtin/variant/impls.rs @@ -46,16 +46,8 @@ macro_rules! impl_ffi_variant { .into_error(variant.clone())); } - // For 4.0: - // In contrast to T -> Variant, the conversion Variant -> T assumes - // that the destination is initialized (at least for some T). For example: - // void String::operator=(const String &p_str) { _cowdata._ref(p_str._cowdata); } - // does a copy-on-write and explodes if this->_cowdata is not initialized. - // We can thus NOT use Self::from_sys_init(). - // - // This was changed in 4.1. let result = unsafe { - sys::new_with_uninit_or_init(|self_ptr| { + Self::new_with_uninit(|self_ptr| { let converter = sys::builtin_fn!($to_fn); converter(self_ptr, sys::SysPtr::force_mut(variant.var_sys())); }) diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index 8603399f0..99a5477f1 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -100,19 +100,6 @@ pub unsafe trait GodotFfi { unsafe fn move_return_ptr(self, dst: sys::GDExtensionTypePtr, call_type: PtrcallType); } -// In Godot 4.0.x, a lot of that are "constructed into" require a default-initialized value. -// In Godot 4.1+, placement new is used, requiring no prior value. -// This method abstracts over that. Declared outside GodotFfi because it should not be overridden. - -/// # Safety -/// See [`GodotFfi::new_with_uninit`] (valid for Godot 4.1+). -#[cfg(since_api = "4.1")] -pub unsafe fn new_with_uninit_or_init( - init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr), -) -> T { - T::new_with_uninit(init_fn) -} - // ---------------------------------------------------------------------------------------------------------------------------------------------- /// Types that can represent null-values. diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index 2ddc161be..9ae431d94 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -53,9 +53,7 @@ pub use paste; #[cfg(target_family = "wasm")] pub use gensym::gensym; -pub use crate::godot_ffi::{ - new_with_uninit_or_init, GodotFfi, GodotNullableFfi, PrimitiveConversionError, PtrcallType, -}; +pub use crate::godot_ffi::{GodotFfi, GodotNullableFfi, PrimitiveConversionError, PtrcallType}; // Method tables pub use gen::table_builtins::*; From 7895ea15d6318d42777c5784eaeeffbcbe065a63 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 27 Jul 2024 20:56:45 +0200 Subject: [PATCH 5/8] Remove CI memcheck workflows for 4.0 --- .github/workflows/full-ci.yml | 10 ---------- .github/workflows/minimal-ci.yml | 10 ---------- 2 files changed, 20 deletions(-) diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index 16fa5767b..d0d989297 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -381,16 +381,6 @@ jobs: # Sanitizers can't build proc-macros and build scripts; with --target, cargo ignores RUSTFLAGS for those two. rust-target: x86_64-unknown-linux-gnu - - name: linux-memcheck-4.0 - os: ubuntu-20.04 - artifact-name: linux-memcheck-4.0 - godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san - godot-prebuilt-patch: '4.0.4' - rust-toolchain: nightly - rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address - # Sanitizers can't build proc-macros and build scripts; with --target, cargo ignores RUSTFLAGS for those two. - rust-target: x86_64-unknown-linux-gnu - steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/minimal-ci.yml b/.github/workflows/minimal-ci.yml index 3cff9a77c..5d7b8e458 100644 --- a/.github/workflows/minimal-ci.yml +++ b/.github/workflows/minimal-ci.yml @@ -186,16 +186,6 @@ jobs: # Sanitizers can't build proc-macros and build scripts; with --target, cargo ignores RUSTFLAGS for those two. rust-target: x86_64-unknown-linux-gnu - - name: linux-memcheck-4.0 - os: ubuntu-20.04 - artifact-name: linux-memcheck-4.0 - godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san - godot-prebuilt-patch: '4.0.4' - rust-toolchain: nightly - rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address - # Sanitizers can't build proc-macros and build scripts; with --target, cargo ignores RUSTFLAGS for those two. - rust-target: x86_64-unknown-linux-gnu - steps: - uses: actions/checkout@v4 From f67fcb8b80cdd8840313125d8a7ce58da8420380 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 27 Jul 2024 21:33:30 +0200 Subject: [PATCH 6/8] Remove `api-4-0[-x]` features and conditional compilation for 4.0 API --- godot-bindings/Cargo.toml | 5 ----- godot-bindings/build.rs | 5 ----- godot-bindings/src/import.rs | 25 ------------------------- godot-core/Cargo.toml | 5 ----- godot-ffi/Cargo.toml | 5 ----- godot/Cargo.toml | 5 ----- itest/repo-tweak/src/main.rs | 2 +- 7 files changed, 1 insertion(+), 51 deletions(-) diff --git a/godot-bindings/Cargo.toml b/godot-bindings/Cargo.toml index 74d313150..f7618de23 100644 --- a/godot-bindings/Cargo.toml +++ b/godot-bindings/Cargo.toml @@ -19,11 +19,6 @@ experimental-wasm-nothreads = [] # [version-sync] [[ # [line] api-$kebabVersion = [] -api-4-0 = [] -api-4-0-1 = [] -api-4-0-2 = [] -api-4-0-3 = [] -api-4-0-4 = [] api-4-1 = [] api-4-1-1 = [] api-4-1-2 = [] diff --git a/godot-bindings/build.rs b/godot-bindings/build.rs index 06c80ea50..b7567ccaa 100644 --- a/godot-bindings/build.rs +++ b/godot-bindings/build.rs @@ -16,11 +16,6 @@ fn main() { // [version-sync] [[ // [line] \tif cfg!(feature = "api-$kebabVersion") { count += 1; } - if cfg!(feature = "api-4-0") { count += 1; } - if cfg!(feature = "api-4-0-1") { count += 1; } - if cfg!(feature = "api-4-0-2") { count += 1; } - if cfg!(feature = "api-4-0-3") { count += 1; } - if cfg!(feature = "api-4-0-4") { count += 1; } if cfg!(feature = "api-4-1") { count += 1; } if cfg!(feature = "api-4-1-1") { count += 1; } if cfg!(feature = "api-4-1-2") { count += 1; } diff --git a/godot-bindings/src/import.rs b/godot-bindings/src/import.rs index a1995d1ac..c7a15abad 100644 --- a/godot-bindings/src/import.rs +++ b/godot-bindings/src/import.rs @@ -15,11 +15,6 @@ pub const ALL_VERSIONS: &[(u8, u8, u8)] = &[ // [version-sync] [[ // [include] past+current+future // [line] \t$triple, - (4, 0, 0), - (4, 0, 1), - (4, 0, 2), - (4, 0, 3), - (4, 0, 4), (4, 1, 0), (4, 1, 1), (4, 1, 2), @@ -34,21 +29,6 @@ pub const ALL_VERSIONS: &[(u8, u8, u8)] = &[ // [version-sync] [[ // [line] #[cfg(feature = "api-$kebabVersion")]\npub use gdextension_api::version_$snakeVersion as prebuilt;\n -#[cfg(feature = "api-4-0")] -pub use gdextension_api::version_4_0 as prebuilt; - -#[cfg(feature = "api-4-0-1")] -pub use gdextension_api::version_4_0_1 as prebuilt; - -#[cfg(feature = "api-4-0-2")] -pub use gdextension_api::version_4_0_2 as prebuilt; - -#[cfg(feature = "api-4-0-3")] -pub use gdextension_api::version_4_0_3 as prebuilt; - -#[cfg(feature = "api-4-0-4")] -pub use gdextension_api::version_4_0_4 as prebuilt; - #[cfg(feature = "api-4-1")] pub use gdextension_api::version_4_1 as prebuilt; @@ -82,11 +62,6 @@ pub use gdextension_api::version_4_2_2 as prebuilt; // [pre] #[cfg(not(any( // [post] \tfeature = "api-custom",\n)))] #[cfg(not(any( - feature = "api-4-0", - feature = "api-4-0-1", - feature = "api-4-0-2", - feature = "api-4-0-3", - feature = "api-4-0-4", feature = "api-4-1", feature = "api-4-1-1", feature = "api-4-1-2", diff --git a/godot-core/Cargo.toml b/godot-core/Cargo.toml index d5413aa30..5fb5bd2a1 100644 --- a/godot-core/Cargo.toml +++ b/godot-core/Cargo.toml @@ -29,11 +29,6 @@ trace = [] api-custom = ["godot-ffi/api-custom", "godot-codegen/api-custom"] # [version-sync] [[ # [line] api-$kebabVersion = ["godot-ffi/api-$kebabVersion"] -api-4-0 = ["godot-ffi/api-4-0"] -api-4-0-1 = ["godot-ffi/api-4-0-1"] -api-4-0-2 = ["godot-ffi/api-4-0-2"] -api-4-0-3 = ["godot-ffi/api-4-0-3"] -api-4-0-4 = ["godot-ffi/api-4-0-4"] api-4-1 = ["godot-ffi/api-4-1"] api-4-1-1 = ["godot-ffi/api-4-1-1"] api-4-1-2 = ["godot-ffi/api-4-1-2"] diff --git a/godot-ffi/Cargo.toml b/godot-ffi/Cargo.toml index 2acc3e216..e592b30b8 100644 --- a/godot-ffi/Cargo.toml +++ b/godot-ffi/Cargo.toml @@ -21,11 +21,6 @@ debug-log = [] api-custom = ["godot-bindings/api-custom"] # [version-sync] [[ # [line] api-$kebabVersion = ["godot-bindings/api-$kebabVersion"] -api-4-0 = ["godot-bindings/api-4-0"] -api-4-0-1 = ["godot-bindings/api-4-0-1"] -api-4-0-2 = ["godot-bindings/api-4-0-2"] -api-4-0-3 = ["godot-bindings/api-4-0-3"] -api-4-0-4 = ["godot-bindings/api-4-0-4"] api-4-1 = ["godot-bindings/api-4-1"] api-4-1-1 = ["godot-bindings/api-4-1-1"] api-4-1-2 = ["godot-bindings/api-4-1-2"] diff --git a/godot/Cargo.toml b/godot/Cargo.toml index 9bbd42cc3..0fa9bdad0 100644 --- a/godot/Cargo.toml +++ b/godot/Cargo.toml @@ -29,11 +29,6 @@ register-docs = ["godot-macros/docs", "godot-core/docs"] api-custom = ["godot-core/api-custom"] # [version-sync] [[ # [line] api-$kebabVersion = ["godot-core/api-$kebabVersion"] -api-4-0 = ["godot-core/api-4-0"] -api-4-0-1 = ["godot-core/api-4-0-1"] -api-4-0-2 = ["godot-core/api-4-0-2"] -api-4-0-3 = ["godot-core/api-4-0-3"] -api-4-0-4 = ["godot-core/api-4-0-4"] api-4-1 = ["godot-core/api-4-1"] api-4-1-1 = ["godot-core/api-4-1-1"] api-4-1-2 = ["godot-core/api-4-1-2"] diff --git a/itest/repo-tweak/src/main.rs b/itest/repo-tweak/src/main.rs index cab234f4a..f9dceebdf 100644 --- a/itest/repo-tweak/src/main.rs +++ b/itest/repo-tweak/src/main.rs @@ -17,7 +17,7 @@ use std::path::Path; // Manual input. #[rustfmt::skip] pub const GODOT_LATEST_PATCH_VERSIONS: &[&str] = &[ - "4.0.4", + // 4.0.x no longer supported. "4.1.4", "4.2.2", "4.3.0", From 4bfba34e1d86942eeabc311858f5bd1c58eb9a90 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 27 Jul 2024 22:04:29 +0200 Subject: [PATCH 7/8] Remove compat bridge between Godot 4.0 and 4.1+ --- .../src/generator/extension_interface.rs | 3 - godot-core/src/init/mod.rs | 4 +- godot-ffi/src/compat/compat_4_0.rs | 78 ------ godot-ffi/src/compat/compat_4_1plus.rs | 223 +++++++++--------- godot-ffi/src/compat/mod.rs | 62 ----- godot-ffi/src/lib.rs | 41 +--- godot-ffi/src/toolbox.rs | 6 +- godot-macros/src/gdextension.rs | 4 +- 8 files changed, 135 insertions(+), 286 deletions(-) delete mode 100644 godot-ffi/src/compat/compat_4_0.rs delete mode 100644 godot-ffi/src/compat/mod.rs diff --git a/godot-codegen/src/generator/extension_interface.rs b/godot-codegen/src/generator/extension_interface.rs index 7c334197a..fabb91156 100644 --- a/godot-codegen/src/generator/extension_interface.rs +++ b/godot-codegen/src/generator/extension_interface.rs @@ -76,9 +76,6 @@ fn generate_proc_address_funcs(h_path: &Path) -> TokenStream { // Do not derive Copy -- even though the struct is bitwise-copyable, this is rarely needed and may point to an error. let code = quote! { - pub use crate::compat::InitCompat; - // pub use crate::compat::compat_4_1plus::InitCompat; - pub struct GDExtensionInterface { #( #fptr_decls )* } diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index d7048978f..ba54a3b27 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -19,7 +19,7 @@ pub use sys::GdextBuild; #[doc(hidden)] // TODO consider body safe despite unsafe function, and explicitly mark unsafe {} locations pub unsafe fn __gdext_load_library( - interface_or_get_proc_address: sys::InitCompat, + get_proc_address: sys::GDExtensionInterfaceGetProcAddress, library: sys::GDExtensionClassLibraryPtr, init: *mut sys::GDExtensionInitialization, ) -> sys::GDExtensionBool { @@ -41,7 +41,7 @@ pub unsafe fn __gdext_load_library( let config = sys::GdextConfig::new(tool_only_in_editor); - sys::initialize(interface_or_get_proc_address, library, config); + sys::initialize(get_proc_address, library, config); // Currently no way to express failure; could be exposed to E if necessary. // No early exit, unclear if Godot still requires output parameters to be set. diff --git a/godot-ffi/src/compat/compat_4_0.rs b/godot-ffi/src/compat/compat_4_0.rs deleted file mode 100644 index d238820b7..000000000 --- a/godot-ffi/src/compat/compat_4_0.rs +++ /dev/null @@ -1,78 +0,0 @@ -/* - * Copyright (c) godot-rust; Bromeon and contributors. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. - */ - -//! Legacy 4.0 API -//! -//! The old API uses a struct `GDExtensionInterface`, which is passed to the extension entry point (via pointer). -//! This struct contains function pointers to all FFI functions defined in the `gdextension_interface.h` header. - -use crate as sys; -use crate::compat::BindingCompat; - -pub type InitCompat = *const sys::GDExtensionInterface; - -// SAFETY: If `ensure_static_runtime_compatibility` succeeds then the other two functions should be safe to call. -unsafe impl BindingCompat for *const sys::GDExtensionInterface { - fn ensure_static_runtime_compatibility(&self) { - // We try to read the first fields of the GDExtensionInterface struct, which are version numbers. - // If those are unrealistic numbers, chances are high that `self` is in fact a function pointer (used for Godot 4.1.x). - let data_ptr = *self; - - // We cannot print runtime version. We _could_ theoretically fetch the `get_godot_version` function pointer through `get_proc_address`, - // but that's not adding that much information. The Godot engine already prints its version on startup. - let static_version = crate::GdextBuild::godot_static_version_string(); - assert!( - // SAFETY: None. Reading a function pointer as data is UB. - // However, the alternative is to run into even harder UB because we happily interpret the pointer as *const GDExtensionInterface. - // So, this is a best-effort and "works in practice" heuristic to warn the user when running a 4.0.x extension under 4.1+. - // If comparing the first field already fails, we don't even need to read the 2nd field. - unsafe { data_ptr.read().version_major } == 4 - && unsafe { data_ptr.read().version_minor } == 0, - - "gdext was compiled against a legacy Godot version ({static_version}),\n\ - but initialized by a newer Godot binary (4.1+).\n\ - \n\ - This setup is not supported. Please recompile the Rust extension with a newer Godot version\n\ - (or run it with an older Godot version).\n" - ); - } - - unsafe fn runtime_version(&self) -> sys::GDExtensionGodotVersion { - // SAFETY: this method is only invoked after the static compatibility check has passed. - // We thus know that Godot 4.0.x runs, and *self is a GDExtensionInterface pointer. - let interface = unsafe { &**self }; - sys::GDExtensionGodotVersion { - major: interface.version_major, - minor: interface.version_minor, - patch: interface.version_patch, - string: interface.version_string, - } - } - - unsafe fn load_interface(&self) -> sys::GDExtensionInterface { - unsafe { **self } - } -} - -// ---------------------------------------------------------------------------------------------------------------------------------------------- -// Polyfill for types referenced in function pointer tables - -pub(crate) type GDExtensionInterfaceVariantGetPtrBuiltinMethod = Option< - unsafe extern "C" fn( - p_type: crate::GDExtensionVariantType, - p_method: crate::GDExtensionConstStringNamePtr, - p_hash: crate::GDExtensionInt, - ) -> crate::GDExtensionPtrBuiltInMethod, ->; - -pub(crate) type GDExtensionInterfaceClassdbGetMethodBind = Option< - unsafe extern "C" fn( - p_classname: crate::GDExtensionConstStringNamePtr, - p_methodname: crate::GDExtensionConstStringNamePtr, - p_hash: crate::GDExtensionInt, - ) -> crate::GDExtensionMethodBindPtr, ->; diff --git a/godot-ffi/src/compat/compat_4_1plus.rs b/godot-ffi/src/compat/compat_4_1plus.rs index c2ff11f80..e2c39160c 100644 --- a/godot-ffi/src/compat/compat_4_1plus.rs +++ b/godot-ffi/src/compat/compat_4_1plus.rs @@ -5,135 +5,146 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -//! Modern 4.1+ API +//! # Modern 4.1+ API //! //! The extension entry point is passed `get_proc_address` function pointer, which can be used to load all other //! GDExtension FFI functions dynamically. This is a departure from the previous struct-based approach. //! +//! No longer supports Godot 4.0.x. +//! //! Relevant upstream PR: . use crate as sys; -use crate::compat::BindingCompat; - -pub type InitCompat = sys::GDExtensionInterfaceGetProcAddress; - -// SAFETY: If `ensure_static_runtime_compatibility` succeeds then the other two functions should be safe to call, except on wasm where -// `ensure_static_runtime_compatibility` is a no-op. -// TODO: fix `ensure_static_runtime_compatibility` on wasm. -unsafe impl BindingCompat for sys::GDExtensionInterfaceGetProcAddress { - // In WebAssembly, function references and data pointers live in different memory spaces, so trying to read the "memory" - // at a function pointer (an index into a table) to heuristically determine which API we have (as is done below) won't work. - #[cfg(target_family = "wasm")] - fn ensure_static_runtime_compatibility(&self) {} - - #[cfg(not(target_family = "wasm"))] - fn ensure_static_runtime_compatibility(&self) { - // In Godot 4.0.x, before the new GetProcAddress mechanism, the init function looked as follows. - // In place of the `get_proc_address` function pointer, the `p_interface` data pointer was passed. - // - // typedef GDExtensionBool (*GDExtensionInitializationFunction)( - // const GDExtensionInterface *p_interface, - // GDExtensionClassLibraryPtr p_library, - // GDExtensionInitialization *r_initialization - // ); - // - // Also, the GDExtensionInterface struct was beginning with these fields: - // - // typedef struct { - // uint32_t version_major; - // uint32_t version_minor; - // uint32_t version_patch; - // const char *version_string; - // ... - // } GDExtensionInterface; - // - // As a result, we can try to interpret the function pointer as a legacy GDExtensionInterface data pointer and check if the - // first fields have values version_major=4 and version_minor=0. This might be deep in UB territory, but the alternative is - // to not be able to detect Godot 4.0.x at all, and run into UB anyway. - let get_proc_address = self.expect("get_proc_address unexpectedly null"); - - let static_version_str = crate::GdextBuild::godot_static_version_string(); - - // Strictly speaking, this is NOT the type GDExtensionGodotVersion but a 4.0 legacy version of it. They have the exact same - // layout, and due to GDExtension's compatibility promise, the 4.1+ struct won't change; so we can reuse the type. - // We thus read u32 pointers (field by field). - let data_ptr = get_proc_address as *const u32; // crowbar it via `as` cast - - // SAFETY: borderline UB, but on Desktop systems, we should be able to reinterpret function pointers as data. - // On 64-bit systems, a function pointer is typically 8 bytes long, meaning we can interpret 8 bytes of it. - // On 32-bit systems, we can only read the first 4 bytes safely. If that happens to have value 4 (exceedingly unlikely for - // a function pointer), it's likely that it's the actual version and we run 4.0.x. In that case, read 4 more bytes. - let major = unsafe { data_ptr.read() }; - if major == 4 { - // SAFETY: see above. - let minor = unsafe { data_ptr.offset(1).read() }; - if minor == 0 { - // SAFETY: at this point it's reasonably safe to say that we are indeed dealing with that version struct; read the whole. - let data_ptr = get_proc_address as *const sys::GDExtensionGodotVersion; - let runtime_version_str = unsafe { read_version_string(&data_ptr.read()) }; - - panic!( - "gdext was compiled against a newer Godot version: {static_version_str}\n\ - but loaded by legacy Godot binary, with version: {runtime_version_str}\n\ - \n\ - Update your Godot engine version, or read https://godot-rust.github.io/book/toolchain/compatibility.html.\n\ - \n" - ); - } - } - - // From here we can assume Godot 4.1+. We need to make sure that the runtime version is >= static version. - // Lexicographical tuple comparison does that. - let static_version = crate::GdextBuild::godot_static_version_triple(); - - // SAFETY: We are now reasonably sure the runtime version is 4.1. - let runtime_version_raw = unsafe { self.runtime_version() }; - // SAFETY: Godot provides this version struct. - let runtime_version = ( - runtime_version_raw.major as u8, - runtime_version_raw.minor as u8, - runtime_version_raw.patch as u8, - ); +// In WebAssembly, function references and data pointers live in different memory spaces, so trying to read the "memory" +// at a function pointer (an index into a table) to heuristically determine which API we have (as is done below) won't work. +#[cfg(target_family = "wasm")] +pub fn ensure_static_runtime_compatibility( + _get_proc_address: sys::GDExtensionInterfaceGetProcAddress, +) { +} - if runtime_version < static_version { - let runtime_version_str = read_version_string(&runtime_version_raw); +#[cfg(not(target_family = "wasm"))] +pub fn ensure_static_runtime_compatibility( + get_proc_address: sys::GDExtensionInterfaceGetProcAddress, +) { + let static_version_str = crate::GdextBuild::godot_static_version_string(); + + // In Godot 4.0.x, before the new GetProcAddress mechanism, the init function looked as follows. + // In place of the `get_proc_address` function pointer, the `p_interface` data pointer was passed. + // + // typedef GDExtensionBool (*GDExtensionInitializationFunction)( + // const GDExtensionInterface *p_interface, + // GDExtensionClassLibraryPtr p_library, + // GDExtensionInitialization *r_initialization + // ); + // + // Also, the GDExtensionInterface struct was beginning with these fields: + // + // typedef struct { + // uint32_t version_major; + // uint32_t version_minor; + // uint32_t version_patch; + // const char *version_string; + // ... + // } GDExtensionInterface; + // + // As a result, we can try to interpret the function pointer as a legacy GDExtensionInterface data pointer and check if the + // first fields have values version_major=4 and version_minor=0. This might be deep in UB territory, but the alternative is + // to not be able to detect Godot 4.0.x at all, and run into UB anyway. + let get_proc_address = get_proc_address.expect("get_proc_address unexpectedly null"); + + // Strictly speaking, this is NOT the type GDExtensionGodotVersion but a 4.0 legacy version of it. They have the exact same + // layout, and due to GDExtension's compatibility promise, the 4.1+ struct won't change; so we can reuse the type. + // We thus read u32 pointers (field by field). + let data_ptr = get_proc_address as *const u32; // crowbar it via `as` cast + + // SAFETY: borderline UB, but on Desktop systems, we should be able to reinterpret function pointers as data. + // On 64-bit systems, a function pointer is typically 8 bytes long, meaning we can interpret 8 bytes of it. + // On 32-bit systems, we can only read the first 4 bytes safely. If that happens to have value 4 (exceedingly unlikely for + // a function pointer), it's likely that it's the actual version and we run 4.0.x. In that case, read 4 more bytes. + let major = unsafe { data_ptr.read() }; + if major == 4 { + // SAFETY: see above. + let minor = unsafe { data_ptr.offset(1).read() }; + if minor == 0 { + // SAFETY: at this point it's reasonably safe to say that we are indeed dealing with that version struct; read the whole. + let data_ptr = get_proc_address as *const sys::GDExtensionGodotVersion; + let runtime_version_str = unsafe { read_version_string(&data_ptr.read()) }; panic!( - "gdext was compiled against newer Godot version: {static_version_str}\n\ - but loaded by older Godot binary, with version: {runtime_version_str}\n\ + "gdext was compiled against a newer Godot version: {static_version_str}\n\ + but loaded by legacy Godot binary, with version: {runtime_version_str}\n\ \n\ - Update your Godot engine version, or compile gdext against an older version.\n\ - For more information, read https://godot-rust.github.io/book/toolchain/compatibility.html.\n\ + Update your Godot engine version, or read https://godot-rust.github.io/book/toolchain/compatibility.html.\n\ \n" ); } } - unsafe fn runtime_version(&self) -> sys::GDExtensionGodotVersion { - let get_proc_address = self.expect("get_proc_address unexpectedly null"); + // From here we can assume Godot 4.1+. We need to make sure that the runtime version is >= static version. + // Lexicographical tuple comparison does that. + let static_version = crate::GdextBuild::godot_static_version_triple(); + + // SAFETY: We are now reasonably sure the runtime version is 4.1. + let runtime_version_raw = unsafe { runtime_version_inner(get_proc_address) }; + + // SAFETY: Godot provides this version struct. + let runtime_version = ( + runtime_version_raw.major as u8, + runtime_version_raw.minor as u8, + runtime_version_raw.patch as u8, + ); + + if runtime_version < static_version { + let runtime_version_str = read_version_string(&runtime_version_raw); + + panic!( + "gdext was compiled against newer Godot version: {static_version_str}\n\ + but loaded by older Godot binary, with version: {runtime_version_str}\n\ + \n\ + Update your Godot engine version, or compile gdext against an older version.\n\ + For more information, read https://godot-rust.github.io/book/toolchain/compatibility.html.\n\ + \n" + ); + } +} - // SAFETY: `self.0` is a valid `get_proc_address` pointer. - let get_godot_version = unsafe { get_proc_address(sys::c_str(b"get_godot_version\0")) }; //.expect("get_godot_version unexpectedly null"); +pub unsafe fn runtime_version( + get_proc_address: sys::GDExtensionInterfaceGetProcAddress, +) -> sys::GDExtensionGodotVersion { + let get_proc_address = get_proc_address.expect("get_proc_address unexpectedly null"); - // SAFETY: `sys::GDExtensionInterfaceGetGodotVersion` is an `Option` of an `unsafe extern "C"` function pointer. - let get_godot_version = crate::cast_fn_ptr!(unsafe { - get_godot_version as sys::GDExtensionInterfaceGetGodotVersion - }); + runtime_version_inner(get_proc_address) +} - let mut version = std::mem::MaybeUninit::::zeroed(); +#[deny(unsafe_op_in_unsafe_fn)] +unsafe fn runtime_version_inner( + get_proc_address: unsafe extern "C" fn( + *const std::ffi::c_char, + ) -> sys::GDExtensionInterfaceFunctionPtr, +) -> sys::GDExtensionGodotVersion { + // SAFETY: `self.0` is a valid `get_proc_address` pointer. + let get_godot_version = unsafe { get_proc_address(sys::c_str(b"get_godot_version\0")) }; //.expect("get_godot_version unexpectedly null"); - // SAFETY: `get_proc_address` with "get_godot_version" does return a valid `sys::GDExtensionInterfaceGetGodotVersion` pointer, and since we have a valid - // `get_proc_address` pointer then it must be callable. - unsafe { get_godot_version(version.as_mut_ptr()) }; + // SAFETY: `sys::GDExtensionInterfaceGetGodotVersion` is an `Option` of an `unsafe extern "C"` function pointer. + let get_godot_version = + crate::unsafe_cast_fn_ptr!(get_godot_version as sys::GDExtensionInterfaceGetGodotVersion); - // SAFETY: `get_godot_version` initializes `version`. - unsafe { version.assume_init() } - } + let mut version = std::mem::MaybeUninit::::zeroed(); - unsafe fn load_interface(&self) -> sys::GDExtensionInterface { - unsafe { sys::GDExtensionInterface::load(*self) } - } + // SAFETY: `get_proc_address` with "get_godot_version" does return a valid `sys::GDExtensionInterfaceGetGodotVersion` pointer, and since we have a valid + // `get_proc_address` pointer then it must be callable. + unsafe { get_godot_version(version.as_mut_ptr()) }; + + // SAFETY: `get_godot_version` initializes `version`. + unsafe { version.assume_init() } +} + +pub unsafe fn load_interface( + get_proc_address: sys::GDExtensionInterfaceGetProcAddress, +) -> sys::GDExtensionInterface { + sys::GDExtensionInterface::load(get_proc_address) } fn read_version_string(version_ptr: &sys::GDExtensionGodotVersion) -> String { diff --git a/godot-ffi/src/compat/mod.rs b/godot-ffi/src/compat/mod.rs deleted file mode 100644 index 4293327a0..000000000 --- a/godot-ffi/src/compat/mod.rs +++ /dev/null @@ -1,62 +0,0 @@ -/* - * Copyright (c) godot-rust; Bromeon and contributors. - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. - */ - -use crate as sys; - -#[cfg(since_api = "4.1")] -mod compat_4_1plus; -#[cfg(since_api = "4.1")] -pub use compat_4_1plus::*; - -#[cfg(before_api = "4.1")] -mod compat_4_0; -#[cfg(before_api = "4.1")] -pub use compat_4_0::*; - -/// Dispatch at runtime between Godot 4.0 legacy and 4.1+ APIs. -/// -/// Provides a compatibility layer to be able to use 4.0.x extensions under Godot versions >= 4.1. -/// Also performs deterministic checks and expressive errors for cases where compatibility cannot be provided. -/// -/// # Safety -/// -/// [`ensure_static_runtime_compatibility`](BindingCompat::ensure_static_runtime_compatibility) succeeding should be sufficient to ensure that -/// both [`runtime_version`](BindingCompat::runtime_version) and [`load_interface`](BindingCompat::load_interface) can be called safely. -pub(crate) unsafe trait BindingCompat { - // Implementation note: these methods could be unsafe, but that would remove any `unsafe` statements _inside_ - // the function bodies, making reasoning about them harder. Also, the call site is already an unsafe function, - // so it would not add safety there, either. - // Either case, given the spec of the GDExtension C API in 4.0 and 4.1, the operations should be safe. - - /// Panics on mismatch between compiled and runtime Godot version. - /// - /// This can happen in the following cases, with their respective subcases: - /// - /// 1) When a gdext version compiled against 4.1+ GDExtension API is invoked with an entry point using the legacy calling convention. - /// a) The .gdextension file's `[configuration]` section does not contain a `compatibility_minimum = 4.1` statement. - /// b) gdext was compiled against a 4.1+ Godot version, but at runtime the library is loaded from a 4.0.x version. - /// - /// 2) When a gdext version compiled against 4.0.x GDExtension API is invoked using the modern way. - /// - /// This is no guarantee, but rather a best-effort heuristic to attempt aborting rather than causing UB/crashes. - /// Changes in the way how Godot loads GDExtension can invalidate assumptions made here. - fn ensure_static_runtime_compatibility(&self); - - /// Return version dynamically passed via `gdextension_interface.h` file. - /// - /// # Safety - /// - /// `self` must be a valid interface or get proc address pointer. - unsafe fn runtime_version(&self) -> sys::GDExtensionGodotVersion; - - /// Return the interface, either as-is from the header (legacy) or code-generated (modern API). - /// - /// # Safety - /// - /// `self` must be a valid interface or get proc address pointer. - unsafe fn load_interface(&self) -> sys::GDExtensionInterface; -} diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index 9ae431d94..4959eee3b 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -31,10 +31,15 @@ pub(crate) mod gen { pub mod conv; -mod compat; +mod compat { + pub(super) mod compat_4_1plus; +} mod extras; mod global; mod godot_ffi; +mod interface_init { + pub use super::compat::compat_4_1plus::*; +} #[cfg(target_os = "linux")] pub mod linux_reload_workaround; mod opaque; @@ -42,8 +47,6 @@ mod plugins; mod string_cache; mod toolbox; -use compat::BindingCompat; - // See https://github.com/dtolnay/paste/issues/69#issuecomment-962418430 // and https://users.rust-lang.org/t/proc-macros-using-third-party-crate/42465/4 #[doc(hidden)] @@ -72,26 +75,6 @@ pub use global::*; pub use string_cache::StringCache; pub use toolbox::*; -#[cfg(before_api = "4.1")] -mod godot_4_0_imported { - // SAFETY: In Godot 4.0.4, the extension interface stores a c_char pointer. This is safe to access from different threads, as no - // mutation happens after initialization. This was changed in 4.1, so we don't need to manually implement `Sync` or `Send` after 4.0. - // Instead, we rely on Rust to infer that it is `Sync` and `Send`. - unsafe impl Sync for super::GDExtensionInterface {} - - // SAFETY: See `Sync` impl. - unsafe impl Send for super::GDExtensionInterface {} - - // Re-import polyfills so that code can use the symbols as if 4.0 would natively define them. - pub use super::compat::InitCompat; - pub(crate) use super::compat::{ - GDExtensionInterfaceClassdbGetMethodBind, GDExtensionInterfaceVariantGetPtrBuiltinMethod, - }; -} - -#[cfg(before_api = "4.1")] -pub use godot_4_0_imported::*; - // ---------------------------------------------------------------------------------------------------------------------------------------------- // API to access Godot via FFI @@ -171,14 +154,12 @@ unsafe impl Send for GdextRuntimeMetadata {} /// /// # Safety /// -/// - The `interface` pointer must be either: -/// - a data pointer to a [`GDExtensionInterface`] object (for Godot 4.0.x) -/// - a function pointer of type [`GDExtensionInterfaceGetProcAddress`] (for Godot 4.1+) +/// - The `get_proc_address` pointer must be a function pointer of type [`GDExtensionInterfaceGetProcAddress`] (valid for Godot 4.1+). /// - The `library` pointer must be the pointer given by Godot at initialisation. /// - This function must not be called from multiple threads. /// - This function must be called before any use of [`get_library`]. pub unsafe fn initialize( - compat: InitCompat, + get_proc_address: GDExtensionInterfaceGetProcAddress, library: GDExtensionClassLibraryPtr, config: GdextConfig, ) { @@ -190,14 +171,14 @@ pub unsafe fn initialize( ); // Before anything else: if we run into a Godot binary that's compiled differently from gdext, proceeding would be UB -> panic. - compat.ensure_static_runtime_compatibility(); + interface_init::ensure_static_runtime_compatibility(get_proc_address); // SAFETY: `ensure_static_runtime_compatibility` succeeded. - let version = unsafe { compat.runtime_version() }; + let version = unsafe { interface_init::runtime_version(get_proc_address) }; out!("Godot version of GDExtension API at runtime: {version:?}"); // SAFETY: `ensure_static_runtime_compatibility` succeeded. - let interface = unsafe { compat.load_interface() }; + let interface = unsafe { interface_init::load_interface(get_proc_address) }; out!("Loaded interface."); // SAFETY: The interface was successfully loaded from Godot, so we should be able to load the builtin lifecycle table. diff --git a/godot-ffi/src/toolbox.rs b/godot-ffi/src/toolbox.rs index 87528cc50..615a6bf6a 100644 --- a/godot-ffi/src/toolbox.rs +++ b/godot-ffi/src/toolbox.rs @@ -77,8 +77,8 @@ macro_rules! out { /// `$ToType` must be an option of an `unsafe extern "C"` function pointer. #[allow(unused)] #[macro_export] -macro_rules! cast_fn_ptr { - (unsafe { $option:ident as $ToType:ty }) => {{ +macro_rules! unsafe_cast_fn_ptr { + ($option:ident as $ToType:ty) => {{ // SAFETY: `$ToType` is an `unsafe extern "C"` function pointer and is thus compatible with `unsafe extern "C" fn()`. // And `Option` is compatible with `Option` when both `T` and `U` are compatible function pointers. #[allow(unused_unsafe)] @@ -191,7 +191,7 @@ pub fn unqualified_type_name() -> &'static str { // Private helpers /// Metafunction to extract inner function pointer types from all the bindgen Option type names. -/// Needed for `cast_fn_ptr` macro. +/// Needed for `unsafe_cast_fn_ptr` macro. pub(crate) trait Inner: Sized { type FnPtr: Sized; } diff --git a/godot-macros/src/gdextension.rs b/godot-macros/src/gdextension.rs index 9fadc80f5..987896d90 100644 --- a/godot-macros/src/gdextension.rs +++ b/godot-macros/src/gdextension.rs @@ -86,7 +86,7 @@ pub fn attribute_gdextension(item: venial::Item) -> ParseResult { #[no_mangle] unsafe extern "C" fn #entry_point( - interface_or_get_proc_address: ::godot::sys::InitCompat, + get_proc_address: ::godot::sys::GDExtensionInterfaceGetProcAddress, library: ::godot::sys::GDExtensionClassLibraryPtr, init: *mut ::godot::sys::GDExtensionInitialization, ) -> ::godot::sys::GDExtensionBool { @@ -95,7 +95,7 @@ pub fn attribute_gdextension(item: venial::Item) -> ParseResult { emscripten_preregistration(); ::godot::init::__gdext_load_library::<#impl_ty>( - interface_or_get_proc_address, + get_proc_address, library, init ) From c548898194aff1c85b187108c209af0610a76bca Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 27 Jul 2024 22:05:31 +0200 Subject: [PATCH 8/8] Git rename to preserve compat_4_1plus.rs history --- .../src/{compat/compat_4_1plus.rs => interface_init.rs} | 0 godot-ffi/src/lib.rs | 7 +------ 2 files changed, 1 insertion(+), 6 deletions(-) rename godot-ffi/src/{compat/compat_4_1plus.rs => interface_init.rs} (100%) diff --git a/godot-ffi/src/compat/compat_4_1plus.rs b/godot-ffi/src/interface_init.rs similarity index 100% rename from godot-ffi/src/compat/compat_4_1plus.rs rename to godot-ffi/src/interface_init.rs diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index 4959eee3b..c29396a27 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -31,15 +31,10 @@ pub(crate) mod gen { pub mod conv; -mod compat { - pub(super) mod compat_4_1plus; -} mod extras; mod global; mod godot_ffi; -mod interface_init { - pub use super::compat::compat_4_1plus::*; -} +mod interface_init; #[cfg(target_os = "linux")] pub mod linux_reload_workaround; mod opaque;