From 19d0906f9b6d710f32cadea564fdc5be8139b95a Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Thu, 18 May 2023 23:11:59 +0200 Subject: [PATCH 01/13] Parse gdextension_interface.h declarations using regex --- godot-codegen/Cargo.toml | 4 + godot-codegen/src/central_generator.rs | 2 + godot-codegen/src/interface_generator.rs | 171 +++++++++++++++++++++++ godot-codegen/src/lib.rs | 11 +- godot-ffi/build.rs | 2 +- godot-ffi/src/lib.rs | 18 ++- 6 files changed, 203 insertions(+), 5 deletions(-) create mode 100644 godot-codegen/src/interface_generator.rs diff --git a/godot-codegen/Cargo.toml b/godot-codegen/Cargo.toml index 4a49edbc4..d126a003d 100644 --- a/godot-codegen/Cargo.toml +++ b/godot-codegen/Cargo.toml @@ -20,3 +20,7 @@ heck = "0.4" nanoserde = "0.1.29" proc-macro2 = "1" quote = "1" + +# Since we don't use Regex for unicode parsing, the features unicode-bool/unicode-gencat are used instead of unicode-perl. +# See also https://docs.rs/regex/latest/regex/#unicode-features. +regex = { version = "1.5.5", default-features = false, features = ["std", "unicode-bool", "unicode-gencat"] } diff --git a/godot-codegen/src/central_generator.rs b/godot-codegen/src/central_generator.rs index 9dd2e1802..4c7e27470 100644 --- a/godot-codegen/src/central_generator.rs +++ b/godot-codegen/src/central_generator.rs @@ -67,6 +67,7 @@ pub(crate) fn generate_sys_central_file( pub(crate) fn generate_sys_mod_file(core_gen_path: &Path, out_files: &mut Vec) { let code = quote! { pub mod central; + pub mod interface; pub mod gdextension_interface; }; @@ -98,6 +99,7 @@ pub(crate) fn generate_core_central_file( write_file(core_gen_path, "central.rs", core_code, out_files); } +// TODO(bromeon): move to util (postponed due to merge conflicts) pub(crate) fn write_file( gen_path: &Path, filename: &str, diff --git a/godot-codegen/src/interface_generator.rs b/godot-codegen/src/interface_generator.rs new file mode 100644 index 000000000..540d0b7db --- /dev/null +++ b/godot-codegen/src/interface_generator.rs @@ -0,0 +1,171 @@ +/* + * 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::central_generator::write_file; +use crate::util::ident; +use proc_macro2::Ident; +use quote::quote; +use regex::Regex; +use std::fs; +use std::path::{Path, PathBuf}; + +struct GodotFuncPtr { + name: Ident, + func_ptr_ty: Ident, + doc: String, +} + +pub(crate) fn generate_sys_interface_file( + h_path: &Path, + sys_gen_path: &Path, + out_files: &mut Vec, +) { + let header_code = fs::read_to_string(h_path) + .expect("failed to read gdextension_interface.h for header parsing"); + let func_ptrs = parse_function_pointers(&header_code); + + let mut fptr_decls = vec![]; + for fptr in func_ptrs { + let GodotFuncPtr { + name, + func_ptr_ty, + doc, + } = fptr; + + let decl = quote! { + #[doc = #doc] + pub #name: crate::#func_ptr_ty, + }; + + fptr_decls.push(decl); + } + + // Do not derive Copy -- even though the struct is bitwise-copyable, this is rarely needed and may point to an error. + let code = quote! { + #[derive(Clone)] + pub struct GDExtensionInterface { + #( #fptr_decls )* + } + }; + + write_file(sys_gen_path, "interface.rs", code.to_string(), out_files); +} + +fn parse_function_pointers(header_code: &str) -> Vec { + // See https://docs.rs/regex/latest/regex for docs. + let regex = Regex::new( + r#"(?xms) + # x: ignore whitespace and allow line comments (starting with `#`) + # m: multi-line mode, ^ and $ match start and end of line + # s: . matches newlines; would otherwise require (:?\n|\r\n|\r) + ^ + # Start of comment /** + /\*\* + # followed by any characters + [^*].*? + # Identifier @name variant_can_convert + @name\s(?P[a-z0-9_]+) + (?P + .+? + ) + #(?:@param\s([a-z0-9_]+))*? + #(?:\n|.)+? + # End of comment */ + \*/ + .+? + # Return type: typedef GDExtensionBool + # or pointers with space: typedef void * + #typedef\s[A-Za-z0-9_]+?\s\*? + typedef\s[^(]+? + # Function pointer: (*GDExtensionInterfaceVariantCanConvert) + \(\*(?P[A-Za-z0-9_]+?)\) + # Parameters: (GDExtensionVariantType p_from, GDExtensionVariantType p_to); + .+?; + # $ omitted, because there can be comments after `;` + "#, + ) + .unwrap(); + + let mut func_ptrs = vec![]; + for cap in regex.captures_iter(&header_code) { + let name = cap.name("name"); + let funcptr_ty = cap.name("type"); + let doc = cap.name("doc"); + + dbg!(&cap); + + let (Some(name), Some(funcptr_ty), Some(doc)) = (name, funcptr_ty, doc) else { + // Skip unparseable ones, instead of breaking build (could just be a /** */ comment around something else) + continue; + }; + + func_ptrs.push(GodotFuncPtr { + name: ident(name.as_str()), + func_ptr_ty: ident(funcptr_ty.as_str()), + doc: doc.as_str().replace("\n *", "\n").trim().to_string(), + }); + } + + func_ptrs +} + +// fn doxygen_to_rustdoc(c_doc: &str) -> String { +// // Remove leading stars +// let mut doc = c_doc .replace("\n * ", "\n"); +// +// // FIXME only compile once +// let param_regex = Regex::new(r#"@p"#) +// } + +#[test] +fn test_parse_function_pointers() { + let header_code = r#" +/* INTERFACE: ClassDB Extension */ + +/** + * @name classdb_register_extension_class + * + * Registers an extension class in the ClassDB. + * + * Provided struct can be safely freed once the function returns. + * + * @param p_library A pointer the library received by the GDExtension's entry point function. + * @param p_class_name A pointer to a StringName with the class name. + * @param p_parent_class_name A pointer to a StringName with the parent class name. + * @param p_extension_funcs A pointer to a GDExtensionClassCreationInfo struct. + */ +typedef void (*GDExtensionInterfaceClassdbRegisterExtensionClass)(GDExtensionClassLibraryPtr p_library, GDExtensionConstStringNamePtr p_class_name, GDExtensionConstStringNamePtr p_parent_class_name, const GDExtensionClassCreationInfo *p_extension_funcs); + "#; + + let func_ptrs = super::parse_function_pointers(header_code); + assert_eq!(func_ptrs.len(), 1); + + let func_ptr = &func_ptrs[0]; + assert_eq!( + func_ptr.name.to_string(), + "classdb_register_extension_class" + ); + + assert_eq!( + func_ptr.func_ptr_ty.to_string(), + "GDExtensionInterfaceClassdbRegisterExtensionClass" + ); + + assert_eq!( + func_ptr.doc, + r#" + Registers an extension class in the ClassDB. + + Provided struct can be safely freed once the function returns. + + @param p_library A pointer the library received by the GDExtension's entry point function. + @param p_class_name A pointer to a StringName with the class name. + @param p_parent_class_name A pointer to a StringName with the parent class name. + @param p_extension_funcs A pointer to a GDExtensionClassCreationInfo struct. + "# + .trim() + ); +} diff --git a/godot-codegen/src/lib.rs b/godot-codegen/src/lib.rs index 1e199b299..e46f7aba4 100644 --- a/godot-codegen/src/lib.rs +++ b/godot-codegen/src/lib.rs @@ -8,6 +8,7 @@ mod api_parser; mod central_generator; mod class_generator; mod context; +mod interface_generator; mod special_cases; mod util; mod utilities_generator; @@ -22,6 +23,7 @@ use central_generator::{ }; use class_generator::{generate_builtin_class_files, generate_class_files}; use context::Context; +use interface_generator::generate_sys_interface_file; use util::{ident, to_pascal_case, to_snake_case}; use utilities_generator::generate_utilities_file; @@ -29,7 +31,11 @@ use proc_macro2::{Ident, TokenStream}; use quote::{quote, ToTokens}; use std::path::{Path, PathBuf}; -pub fn generate_sys_files(sys_gen_path: &Path, watch: &mut godot_bindings::StopWatch) { +pub fn generate_sys_files( + sys_gen_path: &Path, + h_path: &Path, + watch: &mut godot_bindings::StopWatch, +) { let mut out_files = vec![]; generate_sys_mod_file(sys_gen_path, &mut out_files); @@ -41,6 +47,9 @@ pub fn generate_sys_files(sys_gen_path: &Path, watch: &mut godot_bindings::StopW generate_sys_central_file(&api, &mut ctx, build_config, sys_gen_path, &mut out_files); watch.record("generate_central_file"); + generate_sys_interface_file(h_path, sys_gen_path, &mut out_files); + watch.record("generate_interface_file"); + rustfmt_if_needed(out_files); watch.record("rustfmt"); } diff --git a/godot-ffi/build.rs b/godot-ffi/build.rs index ae290afee..4c6283111 100644 --- a/godot-ffi/build.rs +++ b/godot-ffi/build.rs @@ -23,7 +23,7 @@ fn main() { godot_bindings::clear_dir(gen_path, &mut watch); godot_bindings::write_gdextension_headers(&h_path, &rs_path, &mut watch); - godot_codegen::generate_sys_files(gen_path, &mut watch); + godot_codegen::generate_sys_files(gen_path, &h_path, &mut watch); watch.write_stats_to(&gen_path.join("ffi-stats.txt")); println!("cargo:rerun-if-changed=build.rs"); diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index fa94018cb..624fd8f83 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -23,6 +23,8 @@ mod godot_ffi; mod opaque; mod plugins; +use std::ffi::CStr; +use std::ptr; // 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)] @@ -31,6 +33,7 @@ pub use paste; pub use crate::godot_ffi::{GodotFfi, GodotFuncMarshal, PtrcallType}; pub use gen::central::*; pub use gen::gdextension_interface::*; +pub use gen::interface::GDExtensionInterface; // The impls only compile if those are different types -- ensures type safety through patch trait Distinct {} @@ -61,14 +64,23 @@ pub unsafe fn initialize( interface: *const GDExtensionInterface, library: GDExtensionClassLibraryPtr, ) { - let ver = std::ffi::CStr::from_ptr((*interface).version_string); + let mut version = GDExtensionGodotVersion { + major: 0, + minor: 0, + patch: 0, + string: std::ptr::null(), + }; + interface_fn!(get_godot_version)(ptr::addr_of_mut!(version)); + println!( "Initialize GDExtension API for Rust: {}", - ver.to_str().unwrap() + CStr::from_ptr(version.string) + .to_str() + .expect("unknown Godot version") ); BINDING = Some(GodotBinding { - interface: *interface, + interface: (*interface).clone(), method_table: GlobalMethodTable::new(&*interface), library, }); From 877602b961231ee7bfde61c2d8e5d7de1a368efc Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Fri, 19 May 2023 21:27:59 +0200 Subject: [PATCH 02/13] AsUninit trait to convert FFI pointers to their uninitialized versions --- godot-core/src/builtin/dictionary.rs | 48 ++++++++++++++++------------ godot-ffi/src/lib.rs | 27 ++++++++++++++++ 2 files changed, 54 insertions(+), 21 deletions(-) diff --git a/godot-core/src/builtin/dictionary.rs b/godot-core/src/builtin/dictionary.rs index eabfe2822..4760d3055 100644 --- a/godot-core/src/builtin/dictionary.rs +++ b/godot-core/src/builtin/dictionary.rs @@ -14,7 +14,7 @@ use std::fmt; use std::marker::PhantomData; use std::ptr::addr_of_mut; use sys::types::OpaqueDictionary; -use sys::{ffi_methods, interface_fn, GodotFfi}; +use sys::{ffi_methods, interface_fn, AsUninit, GodotFfi}; use super::VariantArray; @@ -399,28 +399,28 @@ impl<'a> DictionaryIter<'a> { } fn call_init(dictionary: &Dictionary) -> Option { - // SAFETY: - // `dictionary` is a valid `Dictionary` since we have a reference to it, - // so this will call the implementation for dictionaries. - // `variant` is an initialized and valid `Variant`. let variant: Variant = Variant::nil(); - unsafe { Self::ffi_iterate(interface_fn!(variant_iter_init), dictionary, variant) } + let iter_fn = |dictionary, next_value: sys::GDExtensionVariantPtr, valid| { + interface_fn!(variant_iter_init)(dictionary, next_value.as_uninit(), valid) + }; + + Self::ffi_iterate(iter_fn, dictionary, variant) } fn call_next(dictionary: &Dictionary, last_key: Variant) -> Option { - // SAFETY: - // `dictionary` is a valid `Dictionary` since we have a reference to it, - // so this will call the implementation for dictionaries. - // `last_key` is an initialized and valid `Variant`, since we own a copy of it. - unsafe { Self::ffi_iterate(interface_fn!(variant_iter_next), dictionary, last_key) } + let iter_fn = |dictionary, next_value, valid| { + interface_fn!(variant_iter_next)(dictionary, next_value, valid) + }; + + Self::ffi_iterate(iter_fn, dictionary, last_key) } /// Calls the provided Godot FFI function, in order to iterate the current state. /// /// # Safety: /// `iter_fn` must point to a valid function that interprets the parameters according to their type specification. - unsafe fn ffi_iterate( - iter_fn: unsafe extern "C" fn( + fn ffi_iterate( + iter_fn: unsafe fn( sys::GDExtensionConstVariantPtr, sys::GDExtensionVariantPtr, *mut sys::GDExtensionBool, @@ -429,14 +429,20 @@ impl<'a> DictionaryIter<'a> { next_value: Variant, ) -> Option { let dictionary = dictionary.to_variant(); - let mut valid: u8 = 0; - - let has_next = iter_fn( - dictionary.var_sys(), - next_value.var_sys(), - addr_of_mut!(valid), - ); - let valid = super::u8_to_bool(valid); + let mut valid_u8: u8 = 0; + + // SAFETY: + // `dictionary` is a valid `Dictionary` since we have a reference to it, + // so this will call the implementation for dictionaries. + // `last_key` is an initialized and valid `Variant`, since we own a copy of it. + let has_next = unsafe { + iter_fn( + dictionary.var_sys(), + next_value.var_sys(), + addr_of_mut!(valid_u8), + ) + }; + let valid = super::u8_to_bool(valid_u8); let has_next = super::u8_to_bool(has_next); if has_next { diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index 624fd8f83..40c1bf346 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -199,6 +199,33 @@ pub fn to_const_ptr(ptr: *mut T) -> *const T { ptr as *const T } +/// Convert a GDExtension pointer type to its uninitialized version. +pub trait AsUninit { + type Output; + + #[allow(clippy::wrong_self_convention)] + fn as_uninit(self) -> Self::Output; +} + +macro_rules! impl_as_uninit { + ($Ptr:ty, $Uninit:ty) => { + impl AsUninit for $Ptr { + type Output = $Uninit; + + fn as_uninit(self) -> Self::Output { + self as $Uninit + } + } + }; +} + +#[rustfmt::skip] +impl_as_uninit!(GDExtensionStringNamePtr, GDExtensionUninitializedStringNamePtr); +impl_as_uninit!(GDExtensionVariantPtr, GDExtensionUninitializedVariantPtr); +impl_as_uninit!(GDExtensionStringPtr, GDExtensionUninitializedStringPtr); +impl_as_uninit!(GDExtensionObjectPtr, GDExtensionUninitializedObjectPtr); +impl_as_uninit!(GDExtensionTypePtr, GDExtensionUninitializedTypePtr); + /// If `ptr` is not null, returns `Some(mapper(ptr))`; otherwise `None`. pub fn ptr_then(ptr: *mut T, mapper: F) -> Option where From 3ba2c641d696245c39dc49c82378a5d46f461111 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Fri, 19 May 2023 23:13:01 +0200 Subject: [PATCH 03/13] GodotFfi::from_sys_init() now uses uninitialized pointer types --- godot-ffi/src/godot_ffi.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index 90e7d815a..a0b5eacd1 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -28,7 +28,7 @@ pub unsafe trait GodotFfi { /// /// # Safety /// `init_fn` must be a function that correctly handles a (possibly-uninitialized) _type ptr_. - unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self; + unsafe fn from_sys_init(init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self; /// Like [`Self::from_sys_init`], but pre-initializes the sys pointer to a `Default::default()` instance /// before calling `init_fn`. @@ -437,7 +437,7 @@ mod scalars { // Do nothing } - unsafe fn from_sys_init(_init: impl FnOnce(sys::GDExtensionTypePtr)) -> Self { + unsafe fn from_sys_init(_init: impl FnOnce(sys::GDExtensionUninitializedTypePtr)) -> Self { // Do nothing } From 633fd5ef2a4c4dbc3649db1eb5d6b992b4f81ec0 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 20 May 2023 17:21:56 +0200 Subject: [PATCH 04/13] Introduce GDExtensionUninitialized*Ptr, without changing semantics --- godot-bindings/src/godot_exe.rs | 15 +++++++++ godot-codegen/src/central_generator.rs | 25 ++++++++++---- godot-core/src/builtin/array.rs | 6 +++- godot-core/src/builtin/callable.rs | 4 ++- godot-core/src/builtin/dictionary.rs | 4 +-- godot-core/src/builtin/macros.rs | 12 ++++--- godot-core/src/builtin/meta/signature.rs | 5 ++- godot-core/src/builtin/string/godot_string.rs | 8 +++-- godot-core/src/builtin/string/node_path.rs | 4 ++- godot-core/src/builtin/string/string_name.rs | 4 ++- godot-core/src/builtin/variant/impls.rs | 5 ++- godot-core/src/builtin/variant/mod.rs | 33 +++++++++++++++++-- godot-core/src/macros.rs | 4 ++- godot-core/src/obj/gd.rs | 20 +++++++---- godot-ffi/src/godot_ffi.rs | 16 +++++---- godot-ffi/src/lib.rs | 14 +++++--- itest/rust/src/object_test.rs | 4 ++- 17 files changed, 140 insertions(+), 43 deletions(-) diff --git a/godot-bindings/src/godot_exe.rs b/godot-bindings/src/godot_exe.rs index 916104b00..e11f72bb6 100644 --- a/godot-bindings/src/godot_exe.rs +++ b/godot-bindings/src/godot_exe.rs @@ -161,6 +161,21 @@ fn patch_c_header(inout_h_path: &Path) { let c = fs::read_to_string(inout_h_path) .unwrap_or_else(|_| panic!("failed to read C header file {}", inout_h_path.display())); + // Hardcoded fixes until upstream adopts them + let c = c + .replace( + "typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionVariantPtr, GDExtensionTypePtr);", + "typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionUninitializedVariantPtr, GDExtensionTypePtr);" + ) + .replace( + "typedef void (*GDExtensionTypeFromVariantConstructorFunc)(GDExtensionTypePtr, GDExtensionVariantPtr);", + "typedef void (*GDExtensionTypeFromVariantConstructorFunc)(GDExtensionUninitializedTypePtr, GDExtensionVariantPtr);" + ) + .replace( + "typedef void (*GDExtensionPtrConstructor)(GDExtensionTypePtr p_base, const GDExtensionConstTypePtr *p_args);", + "typedef void (*GDExtensionPtrConstructor)(GDExtensionUninitializedTypePtr p_base, const GDExtensionConstTypePtr *p_args);" + ); + // Use single regex with independent "const"/"Const", as there are definitions like this: // typedef const void *GDExtensionMethodBindPtr; let c = Regex::new(r"typedef (const )?void \*GDExtension(Const)?([a-zA-Z0-9]+?)Ptr;") // diff --git a/godot-codegen/src/central_generator.rs b/godot-codegen/src/central_generator.rs index 4c7e27470..4f693e0ad 100644 --- a/godot-codegen/src/central_generator.rs +++ b/godot-codegen/src/central_generator.rs @@ -132,7 +132,10 @@ fn make_sys_code(central_items: &CentralItems) -> String { } = central_items; let sys_tokens = quote! { - use crate::{GDExtensionVariantPtr, GDExtensionTypePtr, GDExtensionConstTypePtr, GodotFfi, ffi_methods}; + use crate::{ + ffi_methods, GDExtensionConstTypePtr, GDExtensionTypePtr, GDExtensionUninitializedTypePtr, + GDExtensionUninitializedVariantPtr, GDExtensionVariantPtr, GodotFfi, + }; pub mod types { #(#opaque_types)* @@ -501,9 +504,12 @@ fn make_variant_fns( let variant_type = quote! { crate:: #variant_type }; // Field declaration + // The target types are uninitialized-ptrs, because Godot performs placement new on those: + // https://github.com/godotengine/godot/blob/b40b35fb39f0d0768d7ec2976135adffdce1b96d/core/variant/variant_internal.h#L1535-L1535 + let decl = quote! { - pub #to_variant: unsafe extern "C" fn(GDExtensionVariantPtr, GDExtensionTypePtr), - pub #from_variant: unsafe extern "C" fn(GDExtensionTypePtr, GDExtensionVariantPtr), + pub #to_variant: unsafe extern "C" fn(GDExtensionUninitializedVariantPtr, GDExtensionTypePtr), + pub #from_variant: unsafe extern "C" fn(GDExtensionUninitializedTypePtr, GDExtensionVariantPtr), #op_eq_decls #op_lt_decls #construct_decls @@ -576,10 +582,15 @@ fn make_construct_fns( let (construct_extra_decls, construct_extra_inits) = make_extra_constructors(type_names, constructors, builtin_types); - // Generic signature: fn(base: GDExtensionTypePtr, args: *const GDExtensionTypePtr) + // Target types are uninitialized pointers, because Godot uses placement-new for raw pointer constructions. Callstack: + // https://github.com/godotengine/godot/blob/b40b35fb39f0d0768d7ec2976135adffdce1b96d/core/extension/gdextension_interface.cpp#L511 + // https://github.com/godotengine/godot/blob/b40b35fb39f0d0768d7ec2976135adffdce1b96d/core/variant/variant_construct.cpp#L299 + // https://github.com/godotengine/godot/blob/b40b35fb39f0d0768d7ec2976135adffdce1b96d/core/variant/variant_construct.cpp#L36 + // https://github.com/godotengine/godot/blob/b40b35fb39f0d0768d7ec2976135adffdce1b96d/core/variant/variant_construct.h#L267 + // https://github.com/godotengine/godot/blob/b40b35fb39f0d0768d7ec2976135adffdce1b96d/core/variant/variant_construct.h#L50 let decls = quote! { - pub #construct_default: unsafe extern "C" fn(GDExtensionTypePtr, *const GDExtensionConstTypePtr), - pub #construct_copy: unsafe extern "C" fn(GDExtensionTypePtr, *const GDExtensionConstTypePtr), + pub #construct_default: unsafe extern "C" fn(GDExtensionUninitializedTypePtr, *const GDExtensionConstTypePtr), + pub #construct_copy: unsafe extern "C" fn(GDExtensionUninitializedTypePtr, *const GDExtensionConstTypePtr), #(#construct_extra_decls)* }; @@ -628,7 +639,7 @@ fn make_extra_constructors( let err = format_load_error(&ident); extra_decls.push(quote! { - pub #ident: unsafe extern "C" fn(GDExtensionTypePtr, *const GDExtensionConstTypePtr), + pub #ident: unsafe extern "C" fn(GDExtensionUninitializedTypePtr, *const GDExtensionConstTypePtr), }); let i = i as i32; diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index 839c68a4a..a1de96be5 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -715,10 +715,14 @@ impl FromVariant for Array { if variant.get_type() != Self::variant_type() { return Err(VariantConversionError); } + + // TODO(uninit): can we use from_sys_init()? Godot uses placement-new for variant_to_type initialization. + use sys::AsUninit; + let array = unsafe { Self::from_sys_init_default(|self_ptr| { let array_from_variant = sys::builtin_fn!(array_from_variant); - array_from_variant(self_ptr, variant.var_sys()); + array_from_variant(self_ptr.as_uninit(), variant.var_sys()); }) }; diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index fcbea123d..b84f0a86d 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -10,6 +10,7 @@ use crate::builtin::{inner, ToVariant, Variant}; use crate::engine::Object; use crate::obj::mem::Memory; use crate::obj::{Gd, GodotClass, InstanceId}; +use godot_ffi::AsUninit; use std::fmt; use sys::{ffi_methods, GodotFfi}; @@ -45,10 +46,11 @@ impl Callable { // upcast not needed let method = method_name.into(); unsafe { + // TODO(uninit): can we use from_sys_init()? Godot uses placement-new for variant_to_type initialization. Self::from_sys_init_default(|self_ptr| { let ctor = sys::builtin_fn!(callable_from_object_method); let args = [object.sys_const(), method.sys_const()]; - ctor(self_ptr, args.as_ptr()); + ctor(self_ptr.as_uninit(), args.as_ptr()); }) } } diff --git a/godot-core/src/builtin/dictionary.rs b/godot-core/src/builtin/dictionary.rs index 4760d3055..1bd3cb90d 100644 --- a/godot-core/src/builtin/dictionary.rs +++ b/godot-core/src/builtin/dictionary.rs @@ -400,7 +400,7 @@ impl<'a> DictionaryIter<'a> { fn call_init(dictionary: &Dictionary) -> Option { let variant: Variant = Variant::nil(); - let iter_fn = |dictionary, next_value: sys::GDExtensionVariantPtr, valid| { + let iter_fn = |dictionary, next_value: sys::GDExtensionVariantPtr, valid| unsafe { interface_fn!(variant_iter_init)(dictionary, next_value.as_uninit(), valid) }; @@ -408,7 +408,7 @@ impl<'a> DictionaryIter<'a> { } fn call_next(dictionary: &Dictionary, last_key: Variant) -> Option { - let iter_fn = |dictionary, next_value, valid| { + let iter_fn = |dictionary, next_value, valid| unsafe { interface_fn!(variant_iter_next)(dictionary, next_value, valid) }; diff --git a/godot-core/src/builtin/macros.rs b/godot-core/src/builtin/macros.rs index 4faa2e99e..55b046e57 100644 --- a/godot-core/src/builtin/macros.rs +++ b/godot-core/src/builtin/macros.rs @@ -11,13 +11,12 @@ macro_rules! impl_builtin_traits_inner { impl Default for $Type { #[inline] fn default() -> Self { - // Note: can't use from_sys_init(), as that calls the default constructor - // (because most assignments expect initialized target type) - + // TODO(uninit): use from_sys_init() let mut uninit = std::mem::MaybeUninit::<$Type>::uninit(); + use godot_ffi::AsUninit; unsafe { - let self_ptr = (*uninit.as_mut_ptr()).sys_mut(); + let self_ptr = (*uninit.as_mut_ptr()).sys_mut().as_uninit(); sys::builtin_call! { $gd_method(self_ptr, std::ptr::null_mut()) }; @@ -32,11 +31,13 @@ macro_rules! impl_builtin_traits_inner { impl Clone for $Type { #[inline] fn clone(&self) -> Self { + // TODO(uninit) - check if we can use from_sys_init() + use godot_ffi::AsUninit; unsafe { Self::from_sys_init_default(|self_ptr| { let ctor = ::godot_ffi::builtin_fn!($gd_method); let args = [self.sys_const()]; - ctor(self_ptr, args.as_ptr()); + ctor(self_ptr.as_uninit(), args.as_ptr()); }) } } @@ -162,6 +163,7 @@ macro_rules! impl_builtin_froms { $(impl From<&$From> for $To { fn from(other: &$From) -> Self { unsafe { + // TODO should this be from_sys_init_default()? Self::from_sys_init(|ptr| { let args = [other.sys_const()]; ::godot_ffi::builtin_call! { diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs index c759ed639..f1340d769 100644 --- a/godot-core/src/builtin/meta/signature.rs +++ b/godot-core/src/builtin/meta/signature.rs @@ -17,6 +17,9 @@ pub trait SignatureTuple { fn property_info(index: i32, param_name: &str) -> PropertyInfo; fn param_metadata(index: i32) -> sys::GDExtensionClassMethodArgumentMetadata; + // TODO(uninit) - can we use this for varcall/ptrcall? + // ret: sys::GDExtensionUninitializedVariantPtr + // ret: sys::GDExtensionUninitializedTypePtr unsafe fn varcall( instance_ptr: sys::GDExtensionClassInstancePtr, args_ptr: *const sys::GDExtensionConstVariantPtr, @@ -168,7 +171,7 @@ unsafe fn varcall_arg( /// - It must be safe to write a `sys::GDExtensionCallError` once to `err`. unsafe fn varcall_return( ret_val: R, - ret: sys::GDExtensionConstVariantPtr, + ret: sys::GDExtensionVariantPtr, err: *mut sys::GDExtensionCallError, ) { let ret_variant = ret_val.to_variant(); // TODO write_sys diff --git a/godot-core/src/builtin/string/godot_string.rs b/godot-core/src/builtin/string/godot_string.rs index 8451583e9..605ec42b5 100644 --- a/godot-core/src/builtin/string/godot_string.rs +++ b/godot-core/src/builtin/string/godot_string.rs @@ -222,11 +222,13 @@ impl FromStr for GodotString { impl From<&StringName> for GodotString { fn from(string: &StringName) -> Self { + // TODO(uninit) - see if we can use from_sys_init() + use godot_ffi::AsUninit; unsafe { Self::from_sys_init_default(|self_ptr| { let ctor = sys::builtin_fn!(string_from_string_name); let args = [string.sys_const()]; - ctor(self_ptr, args.as_ptr()); + ctor(self_ptr.as_uninit(), args.as_ptr()); }) } } @@ -243,11 +245,13 @@ impl From for GodotString { impl From<&NodePath> for GodotString { fn from(path: &NodePath) -> Self { + // TODO(uninit) - see if we can use from_sys_init() + use godot_ffi::AsUninit; unsafe { Self::from_sys_init_default(|self_ptr| { let ctor = sys::builtin_fn!(string_from_node_path); let args = [path.sys_const()]; - ctor(self_ptr, args.as_ptr()); + ctor(self_ptr.as_uninit(), args.as_ptr()); }) } } diff --git a/godot-core/src/builtin/string/node_path.rs b/godot-core/src/builtin/string/node_path.rs index c80691490..e0cf4b702 100644 --- a/godot-core/src/builtin/string/node_path.rs +++ b/godot-core/src/builtin/string/node_path.rs @@ -100,11 +100,13 @@ impl_rust_string_conv!(NodePath); impl From<&GodotString> for NodePath { fn from(string: &GodotString) -> Self { + // TODO(uninit) - see if we can use from_sys_init() + use godot_ffi::AsUninit; unsafe { Self::from_sys_init_default(|self_ptr| { let ctor = sys::builtin_fn!(node_path_from_string); let args = [string.sys_const()]; - ctor(self_ptr, args.as_ptr()); + ctor(self_ptr.as_uninit(), args.as_ptr()); }) } } diff --git a/godot-core/src/builtin/string/string_name.rs b/godot-core/src/builtin/string/string_name.rs index 4a4998d32..745a9aa14 100644 --- a/godot-core/src/builtin/string/string_name.rs +++ b/godot-core/src/builtin/string/string_name.rs @@ -129,11 +129,13 @@ impl_rust_string_conv!(StringName); impl From<&GodotString> for StringName { fn from(string: &GodotString) -> Self { + // TODO(uninit) - see if we can use from_sys_init() + use godot_ffi::AsUninit; unsafe { Self::from_sys_init_default(|self_ptr| { let ctor = sys::builtin_fn!(string_name_from_string); let args = [string.sys_const()]; - ctor(self_ptr, args.as_ptr()); + ctor(self_ptr.as_uninit(), args.as_ptr()); }) } } diff --git a/godot-core/src/builtin/variant/impls.rs b/godot-core/src/builtin/variant/impls.rs index 76ba245c8..a31048e63 100644 --- a/godot-core/src/builtin/variant/impls.rs +++ b/godot-core/src/builtin/variant/impls.rs @@ -61,6 +61,9 @@ macro_rules! impl_variant_traits { return Err(VariantConversionError) } + // TODO(uninit) - see if we can use from_sys_init() + use ::godot_ffi::AsUninit; + // 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); } @@ -69,7 +72,7 @@ macro_rules! impl_variant_traits { let result = unsafe { Self::from_sys_init_default(|self_ptr| { let converter = sys::builtin_fn!($to_fn); - converter(self_ptr, variant.var_sys()); + converter(self_ptr.as_uninit(), variant.var_sys()); }) }; diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index 0ab888b58..095842e3c 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -111,17 +111,32 @@ impl Variant { #[allow(unused_mut)] let mut result = Variant::nil(); + use sys::AsUninit; unsafe { interface_fn!(variant_call)( self.var_sys(), method.string_sys(), args_sys.as_ptr(), args_sys.len() as i64, - result.var_sys(), + result.var_sys().as_uninit(), ptr::addr_of_mut!(error), ) }; + // TODO(uninit) + /*let result = unsafe { + Variant::from_var_sys_init(|variant_ptr| { + interface_fn!(variant_call)( + self.var_sys(), + method.string_sys(), + args_sys.as_ptr(), + args_sys.len() as i64, + variant_ptr, + ptr::addr_of_mut!(error), + ) + }) + };*/ + if error.error != sys::GDEXTENSION_CALL_OK { let arg_types: Vec<_> = args.iter().map(Variant::get_type).collect(); sys::panic_call_error(&error, "call", &arg_types); @@ -133,6 +148,7 @@ impl Variant { let op_sys = op.sys(); let mut is_valid = false as u8; + use sys::AsUninit; #[allow(unused_mut)] let mut result = Variant::nil(); unsafe { @@ -140,11 +156,24 @@ impl Variant { op_sys, self.var_sys(), rhs.var_sys(), - result.var_sys(), + result.var_sys().as_uninit(), ptr::addr_of_mut!(is_valid), ) }; + // TODO(uninit) + /*let mut result = unsafe { + Variant::from_var_sys_init(|variant_ptr| { + interface_fn!(variant_evaluate)( + op_sys, + self.var_sys(), + rhs.var_sys(), + variant_ptr, + ptr::addr_of_mut!(is_valid), + ) + }) + };*/ + if is_valid == 1 { Some(result) } else { diff --git a/godot-core/src/macros.rs b/godot-core/src/macros.rs index e748b95f7..eaec63000 100644 --- a/godot-core/src/macros.rs +++ b/godot-core/src/macros.rs @@ -183,7 +183,9 @@ macro_rules! gdext_register_method_inner { if success.is_none() { // Signal error and set return type to Nil (*err).error = sys::GDEXTENSION_CALL_ERROR_INVALID_METHOD; // no better fitting enum? - sys::interface_fn!(variant_new_nil)(ret); + + // TODO(uninit) + sys::interface_fn!(variant_new_nil)(sys::AsUninit::as_uninit(ret)); } } diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index f827739ac..21618144d 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -581,6 +581,11 @@ impl Gd { /// `init_fn` must be a function that correctly handles a _type pointer_ pointing to an _object pointer_. #[doc(hidden)] pub unsafe fn from_sys_init_opt(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Option { + // TODO(uninit) - should we use GDExtensionUninitializedTypePtr instead? Then update all the builtin codegen... + let init_fn = |ptr| { + init_fn(sys::AsUninit::force_init(ptr)); + }; + // Note: see _call_native_mb_ret_obj() in godot-cpp, which does things quite different (e.g. querying the instance binding). // Initialize pointer with given function, return Some(ptr) on success and None otherwise @@ -597,14 +602,14 @@ impl Gd { /// `init_fn` must be a function that correctly handles a _type pointer_ pointing to an _object pointer_. #[doc(hidden)] pub unsafe fn raw_object_init( - init_fn: impl FnOnce(sys::GDExtensionTypePtr), + init_fn: impl FnOnce(sys::GDExtensionUninitializedTypePtr), ) -> sys::GDExtensionObjectPtr { // return_ptr has type GDExtensionTypePtr = GDExtensionObjectPtr* = OpaqueObject* = Object** // (in other words, the type-ptr contains the _address_ of an object-ptr). let mut object_ptr: sys::GDExtensionObjectPtr = ptr::null_mut(); let return_ptr: *mut sys::GDExtensionObjectPtr = ptr::addr_of_mut!(object_ptr); - init_fn(return_ptr as sys::GDExtensionTypePtr); + init_fn(return_ptr as sys::GDExtensionUninitializedTypePtr); // We don't need to know if Object** is null, but if Object* is null; return_ptr has the address of a local (never null). object_ptr @@ -704,12 +709,15 @@ impl Export for Gd { impl FromVariant for Gd { fn try_from_variant(variant: &Variant) -> Result { let result_or_none = unsafe { - // TODO(#234) replace Gd:: with Self when Godot stops allowing - // illegal conversions (See - // https://github.com/godot-rust/gdext/issues/158) + // TODO(#234) replace Gd:: with Self when Godot stops allowing illegal conversions + // See https://github.com/godot-rust/gdext/issues/158 + + // TODO(uninit) - see if we can use from_sys_init() + use ::godot_ffi::AsUninit; + Gd::::from_sys_init_opt(|self_ptr| { let converter = sys::builtin_fn!(object_from_variant); - converter(self_ptr, variant.var_sys()); + converter(self_ptr.as_uninit(), variant.var_sys()); }) }; diff --git a/godot-ffi/src/godot_ffi.rs b/godot-ffi/src/godot_ffi.rs index a0b5eacd1..0230c66b3 100644 --- a/godot-ffi/src/godot_ffi.rs +++ b/godot-ffi/src/godot_ffi.rs @@ -44,11 +44,13 @@ pub unsafe trait GodotFfi { where Self: Sized, // + Default { - Self::from_sys_init(init_fn) + // SAFETY: this default implementation is potentially incorrect. + // By implementing the GodotFfi trait, you acknowledge that these may need to be overridden. + Self::from_sys_init(|ptr| init_fn(sys::AsUninit::force_init(ptr))) // TODO consider using this, if all the implementors support it // let mut result = Self::default(); - // init_fn(result.sys_mut()); + // init_fn(result.sys_mut().as_uninit()); // result } @@ -163,9 +165,9 @@ macro_rules! ffi_methods_one { }; (OpaquePtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_sys_init:ident = from_sys_init) => { $( #[$attr] )? $vis - unsafe fn $from_sys_init(init: impl FnOnce($Ptr)) -> Self { + unsafe fn $from_sys_init(init: impl FnOnce(<$Ptr as $crate::AsUninit>::Ptr)) -> Self { let mut raw = std::mem::MaybeUninit::uninit(); - init(raw.as_mut_ptr() as $Ptr); + init(raw.as_mut_ptr() as <$Ptr as $crate::AsUninit>::Ptr); Self::from_opaque(raw.assume_init()) } @@ -199,7 +201,7 @@ macro_rules! ffi_methods_one { }; (OpaqueValue $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_sys_init:ident = from_sys_init) => { $( #[$attr] )? $vis - unsafe fn $from_sys_init(init: impl FnOnce($Ptr)) -> Self { + unsafe fn $from_sys_init(init: impl FnOnce(<$Ptr as $crate::AsUninit>::Ptr)) -> Self { let mut raw = std::mem::MaybeUninit::uninit(); init(std::mem::transmute(raw.as_mut_ptr())); Self::from_opaque(raw.assume_init()) @@ -233,9 +235,9 @@ macro_rules! ffi_methods_one { }; (SelfPtr $Ptr:ty; $( #[$attr:meta] )? $vis:vis $from_sys_init:ident = from_sys_init) => { $( #[$attr] )? $vis - unsafe fn $from_sys_init(init: impl FnOnce($Ptr)) -> Self { + unsafe fn $from_sys_init(init: impl FnOnce(<$Ptr as $crate::AsUninit>::Ptr)) -> Self { let mut raw = std::mem::MaybeUninit::::uninit(); - init(raw.as_mut_ptr() as $Ptr); + init(raw.as_mut_ptr() as <$Ptr as $crate::AsUninit>::Ptr); raw.assume_init() } diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index 40c1bf346..1f284f8c3 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -201,20 +201,26 @@ pub fn to_const_ptr(ptr: *mut T) -> *const T { /// Convert a GDExtension pointer type to its uninitialized version. pub trait AsUninit { - type Output; + type Ptr; #[allow(clippy::wrong_self_convention)] - fn as_uninit(self) -> Self::Output; + fn as_uninit(self) -> Self::Ptr; + + fn force_init(uninit: Self::Ptr) -> Self; } macro_rules! impl_as_uninit { ($Ptr:ty, $Uninit:ty) => { impl AsUninit for $Ptr { - type Output = $Uninit; + type Ptr = $Uninit; - fn as_uninit(self) -> Self::Output { + fn as_uninit(self) -> $Uninit { self as $Uninit } + + fn force_init(uninit: Self::Ptr) -> Self { + uninit as Self + } } }; } diff --git a/itest/rust/src/object_test.rs b/itest/rust/src/object_test.rs index 848d169e3..9eaa3067e 100644 --- a/itest/rust/src/object_test.rs +++ b/itest/rust/src/object_test.rs @@ -95,7 +95,9 @@ fn object_user_roundtrip_write() { assert_eq!(obj.bind().value, value); let obj2 = unsafe { - Gd::::from_sys_init(|ptr| obj.move_return_ptr(ptr, sys::PtrcallType::Standard)) + Gd::::from_sys_init(|ptr| { + obj.move_return_ptr(sys::AsUninit::force_init(ptr), sys::PtrcallType::Standard) + }) }; assert_eq!(obj2.bind().value, value); } // drop From 63b9d2e06999d8ebef93f3f537b694d0fcd73415 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 20 May 2023 18:56:35 +0200 Subject: [PATCH 05/13] Adjust init code to new get_proc_address mechanism --- godot-codegen/src/interface_generator.rs | 26 +++++++++++++++++++++-- godot-core/src/init/mod.rs | 2 +- godot-core/src/log.rs | 27 +++++++++++++----------- godot-ffi/src/lib.rs | 26 ++++++++++++++++++++--- godot-macros/src/gdextension.rs | 4 ++-- 5 files changed, 65 insertions(+), 20 deletions(-) diff --git a/godot-codegen/src/interface_generator.rs b/godot-codegen/src/interface_generator.rs index 540d0b7db..e6de08d01 100644 --- a/godot-codegen/src/interface_generator.rs +++ b/godot-codegen/src/interface_generator.rs @@ -6,7 +6,7 @@ use crate::central_generator::write_file; use crate::util::ident; -use proc_macro2::Ident; +use proc_macro2::{Ident, Literal}; use quote::quote; use regex::Regex; use std::fs; @@ -28,6 +28,7 @@ pub(crate) fn generate_sys_interface_file( let func_ptrs = parse_function_pointers(&header_code); let mut fptr_decls = vec![]; + let mut fptr_inits = vec![]; for fptr in func_ptrs { let GodotFuncPtr { name, @@ -35,20 +36,41 @@ pub(crate) fn generate_sys_interface_file( doc, } = fptr; + let name_str = Literal::byte_string(format!("{}\0", name).as_bytes()); + let decl = quote! { #[doc = #doc] pub #name: crate::#func_ptr_ty, }; + // SAFETY: transmute relies on Option and Option having the same layout. + // It might be better to transmute the raw function pointers, but then we have no type names. + let init = quote! { + #name: std::mem::transmute::< + crate::GDExtensionInterfaceFunctionPtr, + crate::#func_ptr_ty + >(get_proc_address(crate::c_str(#name_str))), + }; + fptr_decls.push(decl); + fptr_inits.push(init); } // Do not derive Copy -- even though the struct is bitwise-copyable, this is rarely needed and may point to an error. let code = quote! { - #[derive(Clone)] pub struct GDExtensionInterface { #( #fptr_decls )* } + + impl GDExtensionInterface { + pub unsafe fn load(get_proc_address: crate::GDExtensionInterfaceGetProcAddress) -> Self { + let get_proc_address = get_proc_address.expect("get_proc_address null; failed to initialize gdext library"); + + Self { + #( #fptr_inits )* + } + } + } }; write_file(sys_gen_path, "interface.rs", code.to_string(), out_files); diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index 5b3765f09..8cc20bd8c 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -10,7 +10,7 @@ use std::collections::btree_map::BTreeMap; #[doc(hidden)] // TODO consider body safe despite unsafe function, and explicitly mark unsafe {} locations pub unsafe fn __gdext_load_library( - interface: *const sys::GDExtensionInterface, + interface: sys::GDExtensionInterfaceGetProcAddress, library: sys::GDExtensionClassLibraryPtr, init: *mut sys::GDExtensionInitialization, ) -> sys::GDExtensionBool { diff --git a/godot-core/src/log.rs b/godot-core/src/log.rs index e2f619180..1f151ef99 100644 --- a/godot-core/src/log.rs +++ b/godot-core/src/log.rs @@ -12,12 +12,13 @@ macro_rules! godot_warn { ($fmt:literal $(, $args:expr)* $(,)?) => { unsafe { let msg = format!("{}\0", format_args!($fmt $(, $args)*)); + assert!(msg.is_ascii(), "godot_warn: message must be ASCII"); $crate::sys::interface_fn!(print_warning)( - msg.as_bytes().as_ptr() as *const _, - "\0".as_bytes().as_ptr() as *const _, - concat!(file!(), "\0").as_ptr() as *const _, - line!() as _, + $crate::sys::c_str_from_str(&msg), + $crate::sys::c_str(b"\0"), + $crate::sys::c_str_from_str(concat!(file!(), "\0")), + line!() as i32, false as $crate::sys::GDExtensionBool, // whether to create a toast notification in editor ); } @@ -34,12 +35,13 @@ macro_rules! godot_error { //($($args:tt),* $(,)?) => { unsafe { let msg = format!("{}\0", format_args!($fmt $(, $args)*)); + assert!(msg.is_ascii(), "godot_error: message must be ASCII"); $crate::sys::interface_fn!(print_error)( - msg.as_bytes().as_ptr() as *const _, - "\0".as_bytes().as_ptr() as *const _, - concat!(file!(), "\0").as_ptr() as *const _, - line!() as _, + $crate::sys::c_str_from_str(&msg), + $crate::sys::c_str(b"\0"), + $crate::sys::c_str_from_str(concat!(file!(), "\0")), + line!() as i32, false as $crate::sys::GDExtensionBool, // whether to create a toast notification in editor ); } @@ -51,12 +53,13 @@ macro_rules! godot_script_error { ($fmt:literal $(, $args:expr)* $(,)?) => { unsafe { let msg = format!("{}\0", format_args!($fmt $(, $args)*)); + assert!(msg.is_ascii(), "godot_script_error: message must be ASCII"); $crate::sys::interface_fn!(print_script_error)( - msg.as_bytes().as_ptr() as *const _, - "\0".as_bytes().as_ptr() as *const _, - concat!(file!(), "\0").as_ptr() as *const _, - line!() as _, + $crate::sys::c_str_from_str(&msg), + $crate::sys::c_str(b"\0"), + $crate::sys::c_str_from_str(concat!(file!(), "\0")), + line!() as i32, false as $crate::sys::GDExtensionBool, // whether to create a toast notification in editor ); } diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index 1f284f8c3..3673d535b 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -61,7 +61,7 @@ static mut BINDING: Option = None; /// - This function must not be called from multiple threads. /// - This function must be called before any use of [`get_library`]. pub unsafe fn initialize( - interface: *const GDExtensionInterface, + get_proc_address: GDExtensionInterfaceGetProcAddress, library: GDExtensionClassLibraryPtr, ) { let mut version = GDExtensionGodotVersion { @@ -79,9 +79,12 @@ pub unsafe fn initialize( .expect("unknown Godot version") ); + let interface = GDExtensionInterface::load(get_proc_address); + let method_table = GlobalMethodTable::new(&interface); + BINDING = Some(GodotBinding { - interface: (*interface).clone(), - method_table: GlobalMethodTable::new(&*interface), + interface, + method_table, library, }); } @@ -233,6 +236,7 @@ impl_as_uninit!(GDExtensionObjectPtr, GDExtensionUninitializedObjectPtr); impl_as_uninit!(GDExtensionTypePtr, GDExtensionUninitializedTypePtr); /// If `ptr` is not null, returns `Some(mapper(ptr))`; otherwise `None`. +#[inline] pub fn ptr_then(ptr: *mut T, mapper: F) -> Option where F: FnOnce(*mut T) -> R, @@ -245,6 +249,22 @@ where } } +/// Returns a C `const char*` for a null-terminated byte string. +#[inline] +pub fn c_str(s: &[u8]) -> *const std::ffi::c_char { + // Ensure null-terminated + debug_assert!(!s.is_empty() && s[s.len() - 1] == 0); + + s.as_ptr() as *const std::ffi::c_char +} + +#[inline] +pub fn c_str_from_str(s: &str) -> *const std::ffi::c_char { + debug_assert!(s.is_ascii()); + + c_str(s.as_bytes()) +} + // ---------------------------------------------------------------------------------------------------------------------------------------------- #[doc(hidden)] diff --git a/godot-macros/src/gdextension.rs b/godot-macros/src/gdextension.rs index 43b993443..cdc9ead95 100644 --- a/godot-macros/src/gdextension.rs +++ b/godot-macros/src/gdextension.rs @@ -38,12 +38,12 @@ pub fn transform(decl: Declaration) -> ParseResult { #[no_mangle] unsafe extern "C" fn #entry_point( - interface: *const ::godot::sys::GDExtensionInterface, + get_proc_address: ::godot::sys::GDExtensionInterfaceGetProcAddress, library: ::godot::sys::GDExtensionClassLibraryPtr, init: *mut ::godot::sys::GDExtensionInitialization, ) -> ::godot::sys::GDExtensionBool { ::godot::init::__gdext_load_library::<#impl_ty>( - interface, + get_proc_address, library, init ) From ec863d2e527928fa341e88e2606a4f1c8b592b06 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sat, 20 May 2023 22:36:19 +0200 Subject: [PATCH 06/13] Make `trace` feature available in godot-ffi, fix interface access before initialization --- godot-codegen/src/central_generator.rs | 2 +- godot-core/Cargo.toml | 2 +- godot-core/src/lib.rs | 21 ++------------ godot-ffi/Cargo.toml | 1 + godot-ffi/src/lib.rs | 40 ++++++++++++++++++++------ 5 files changed, 37 insertions(+), 29 deletions(-) diff --git a/godot-codegen/src/central_generator.rs b/godot-codegen/src/central_generator.rs index 4f693e0ad..6814ce0c2 100644 --- a/godot-codegen/src/central_generator.rs +++ b/godot-codegen/src/central_generator.rs @@ -148,7 +148,7 @@ fn make_sys_code(central_items: &CentralItems) -> String { } impl GlobalMethodTable { - pub(crate) unsafe fn new(interface: &crate::GDExtensionInterface) -> Self { + pub(crate) unsafe fn load(interface: &crate::GDExtensionInterface) -> Self { Self { #(#variant_fn_inits)* } diff --git a/godot-core/Cargo.toml b/godot-core/Cargo.toml index a98e4982e..a606a6687 100644 --- a/godot-core/Cargo.toml +++ b/godot-core/Cargo.toml @@ -9,7 +9,7 @@ categories = ["game-engines", "graphics"] [features] default = [] -trace = [] +trace = ["godot-ffi/trace"] codegen-fmt = ["godot-ffi/codegen-fmt", "godot-codegen/codegen-fmt"] codegen-full = ["godot-codegen/codegen-full"] double-precision = ["godot-codegen/double-precision"] diff --git a/godot-core/src/lib.rs b/godot-core/src/lib.rs index 4b329fe77..c2944b9fa 100644 --- a/godot-core/src/lib.rs +++ b/godot-core/src/lib.rs @@ -16,6 +16,8 @@ pub mod macros; pub mod obj; pub use godot_ffi as sys; +#[doc(hidden)] +pub use godot_ffi::out; pub use registry::*; /// Maps the Godot class API to Rust. @@ -132,21 +134,4 @@ pub mod private { use std::io::Write; std::io::stdout().flush().expect("flush stdout"); } -} - -#[cfg(feature = "trace")] -#[macro_export] -macro_rules! out { - () => (eprintln!()); - ($fmt:literal) => (eprintln!($fmt)); - ($fmt:literal, $($arg:tt)*) => (eprintln!($fmt, $($arg)*)); -} - -#[cfg(not(feature = "trace"))] -// TODO find a better way than sink-writing to avoid warnings, #[allow(unused_variables)] doesn't work -#[macro_export] -macro_rules! out { - () => ({}); - ($fmt:literal) => ({ use std::io::{sink, Write}; let _ = write!(sink(), $fmt); }); - ($fmt:literal, $($arg:tt)*) => ({ use std::io::{sink, Write}; let _ = write!(sink(), $fmt, $($arg)*); };) -} +} \ No newline at end of file diff --git a/godot-ffi/Cargo.toml b/godot-ffi/Cargo.toml index ffd94bcd4..ccf2b6f49 100644 --- a/godot-ffi/Cargo.toml +++ b/godot-ffi/Cargo.toml @@ -10,6 +10,7 @@ categories = ["game-engines", "graphics"] [features] custom-godot = ["godot-bindings/custom-godot"] codegen-fmt = ["godot-codegen/codegen-fmt"] +trace = [] [dependencies] paste = "1" diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index 3673d535b..a5df2d135 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -43,6 +43,25 @@ impl Distinct for GDExtensionConstTypePtr {} // ---------------------------------------------------------------------------------------------------------------------------------------------- +#[cfg(feature = "trace")] +#[macro_export] +macro_rules! out { + () => (eprintln!()); + ($fmt:literal) => (eprintln!($fmt)); + ($fmt:literal, $($arg:tt)*) => (eprintln!($fmt, $($arg)*)); +} + +#[cfg(not(feature = "trace"))] +// TODO find a better way than sink-writing to avoid warnings, #[allow(unused_variables)] doesn't work +#[macro_export] +macro_rules! out { + () => ({}); + ($fmt:literal) => ({ use std::io::{sink, Write}; let _ = write!(sink(), $fmt); }); + ($fmt:literal, $($arg:tt)*) => ({ use std::io::{sink, Write}; let _ = write!(sink(), $fmt, $($arg)*); };) +} + +// ---------------------------------------------------------------------------------------------------------------------------------------------- + struct GodotBinding { interface: GDExtensionInterface, library: GDExtensionClassLibraryPtr, @@ -64,6 +83,17 @@ pub unsafe fn initialize( get_proc_address: GDExtensionInterfaceGetProcAddress, library: GDExtensionClassLibraryPtr, ) { + out!("Initialize..."); + + let interface = GDExtensionInterface::load(get_proc_address); + let method_table = GlobalMethodTable::load(&interface); + + BINDING = Some(GodotBinding { + interface, + method_table, + library, + }); + let mut version = GDExtensionGodotVersion { major: 0, minor: 0, @@ -71,6 +101,7 @@ pub unsafe fn initialize( string: std::ptr::null(), }; interface_fn!(get_godot_version)(ptr::addr_of_mut!(version)); + out!("Detected {version:?}"); println!( "Initialize GDExtension API for Rust: {}", @@ -78,15 +109,6 @@ pub unsafe fn initialize( .to_str() .expect("unknown Godot version") ); - - let interface = GDExtensionInterface::load(get_proc_address); - let method_table = GlobalMethodTable::new(&interface); - - BINDING = Some(GodotBinding { - interface, - method_table, - library, - }); } /// # Safety From d2ea9b71316969f01d3cfa7e75dbcc8b0e2e74e0 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Sun, 21 May 2023 10:40:47 +0200 Subject: [PATCH 07/13] Compatibility layer between Godot 4.0 and 4.1 (different GDExtension APIs) --- godot-bindings/src/godot_exe.rs | 98 +++++++++++++++++++----- godot-codegen/src/api_parser.rs | 13 ++++ godot-codegen/src/interface_generator.rs | 41 ++++++++-- godot-codegen/src/lib.rs | 3 +- godot-core/src/init/mod.rs | 4 +- godot-core/src/lib.rs | 2 +- godot-core/src/log.rs | 67 +++++++--------- godot-ffi/src/compat/compat_4_0.rs | 32 ++++++++ godot-ffi/src/compat/compat_4_1.rs | 64 ++++++++++++++++ godot-ffi/src/compat/mod.rs | 17 ++++ godot-ffi/src/lib.rs | 85 +++++++++++++++----- godot-macros/src/gdextension.rs | 4 +- itest/godot/itest.gdextension | 1 + 13 files changed, 343 insertions(+), 88 deletions(-) create mode 100644 godot-ffi/src/compat/compat_4_0.rs create mode 100644 godot-ffi/src/compat/compat_4_1.rs create mode 100644 godot-ffi/src/compat/mod.rs diff --git a/godot-bindings/src/godot_exe.rs b/godot-bindings/src/godot_exe.rs index e11f72bb6..17948b5d5 100644 --- a/godot-bindings/src/godot_exe.rs +++ b/godot-bindings/src/godot_exe.rs @@ -6,6 +6,7 @@ //! Commands related to Godot executable +use crate::custom::godot_version::GodotVersion; use crate::godot_version::parse_godot_version; use crate::header_gen::generate_rust_binding; use crate::watch::StopWatch; @@ -50,6 +51,9 @@ pub fn write_gdextension_headers( is_h_provided: bool, watch: &mut StopWatch, ) { + // TODO recognize 4.0 also when not using `custom-godot` + let mut is_godot_4_0 = false; + if !is_h_provided { // No external C header file: Godot binary is present, we use it to dump C header let godot_bin = locate_godot_binary(); @@ -57,7 +61,10 @@ pub fn write_gdextension_headers( watch.record("locate_godot"); // Regenerate API JSON if first time or Godot version is different - let _version = read_godot_version(&godot_bin); + let version = read_godot_version(&godot_bin); + if version.major == 4 && version.minor == 0 { + is_godot_4_0 = true; + } // if !c_header_path.exists() || has_version_changed(&version) { dump_header_file(&godot_bin, inout_h_path); @@ -67,7 +74,7 @@ pub fn write_gdextension_headers( }; rerun_on_changed(inout_h_path); - patch_c_header(inout_h_path); + patch_c_header(inout_h_path, is_godot_4_0); watch.record("patch_header_h"); generate_rust_binding(inout_h_path, out_rs_path); @@ -93,7 +100,7 @@ fn update_version_file(version: &str) { } */ -fn read_godot_version(godot_bin: &Path) -> String { +fn read_godot_version(godot_bin: &Path) -> GodotVersion { let output = Command::new(godot_bin) .arg("--version") .output() @@ -116,7 +123,7 @@ fn read_godot_version(godot_bin: &Path) -> String { output.trim() ); - parsed.full_string + parsed } Err(e) => { // Don't treat this as fatal error @@ -153,28 +160,35 @@ fn dump_header_file(godot_bin: &Path, out_file: &Path) { println!("Generated {}/gdextension_interface.h.", cwd.display()); } -fn patch_c_header(inout_h_path: &Path) { +fn patch_c_header(inout_h_path: &Path, is_godot_4_0: bool) { + println!( + "Patch C header '{}' (is_godot_4_0={is_godot_4_0})...", + inout_h_path.display() + ); + // The C header path *must* be passed in by the invoking crate, as the path cannot be relative to this crate. // Otherwise, it can be something like `/home/runner/.cargo/git/checkouts/gdext-76630c89719e160c/efd3b94/godot-bindings`. // Read the contents of the file into a string - let c = fs::read_to_string(inout_h_path) + let mut c = fs::read_to_string(inout_h_path) .unwrap_or_else(|_| panic!("failed to read C header file {}", inout_h_path.display())); - // Hardcoded fixes until upstream adopts them - let c = c - .replace( - "typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionVariantPtr, GDExtensionTypePtr);", - "typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionUninitializedVariantPtr, GDExtensionTypePtr);" - ) - .replace( - "typedef void (*GDExtensionTypeFromVariantConstructorFunc)(GDExtensionTypePtr, GDExtensionVariantPtr);", - "typedef void (*GDExtensionTypeFromVariantConstructorFunc)(GDExtensionUninitializedTypePtr, GDExtensionVariantPtr);" - ) - .replace( - "typedef void (*GDExtensionPtrConstructor)(GDExtensionTypePtr p_base, const GDExtensionConstTypePtr *p_args);", - "typedef void (*GDExtensionPtrConstructor)(GDExtensionUninitializedTypePtr p_base, const GDExtensionConstTypePtr *p_args);" - ); + if is_godot_4_0 { + polyfill_legacy_header(&mut c); + } + + c = c.replace( + "typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionVariantPtr, GDExtensionTypePtr);", + "typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionUninitializedVariantPtr, GDExtensionTypePtr);" + ) + .replace( + "typedef void (*GDExtensionTypeFromVariantConstructorFunc)(GDExtensionTypePtr, GDExtensionVariantPtr);", + "typedef void (*GDExtensionTypeFromVariantConstructorFunc)(GDExtensionUninitializedTypePtr, GDExtensionVariantPtr);" + ) + .replace( + "typedef void (*GDExtensionPtrConstructor)(GDExtensionTypePtr p_base, const GDExtensionConstTypePtr *p_args);", + "typedef void (*GDExtensionPtrConstructor)(GDExtensionUninitializedTypePtr p_base, const GDExtensionConstTypePtr *p_args);" + ); // Use single regex with independent "const"/"Const", as there are definitions like this: // typedef const void *GDExtensionMethodBindPtr; @@ -182,7 +196,7 @@ fn patch_c_header(inout_h_path: &Path) { .expect("regex for mut typedef") .replace_all(&c, "typedef ${1}struct __Gdext$3 *GDExtension${2}${3}Ptr;"); - println!("Patched contents:\n\n{}\n\n", c.as_ref()); + // println!("Patched contents:\n\n{}\n\n", c.as_ref()); // Write the modified contents back to the file fs::write(inout_h_path, c.as_ref()).unwrap_or_else(|_| { @@ -192,6 +206,48 @@ fn patch_c_header(inout_h_path: &Path) { ) }); } + +/// Backport Godot 4.1+ changes to the old GDExtension API, so gdext can use both uniformly. +fn polyfill_legacy_header(c: &mut String) { + // Newer Uninitialized* types -- use same types as initialized ones, because old functions are not written with Uninitialized* in mind + let pos = c + .find("typedef int64_t GDExtensionInt;") + .expect("Unexpected gdextension_interface.h format (int)"); + + c.insert_str( + pos, + "\ + // gdext polyfill\n\ + typedef struct __GdextVariant *GDExtensionUninitializedVariantPtr;\n\ + typedef struct __GdextStringName *GDExtensionUninitializedStringNamePtr;\n\ + typedef struct __GdextString *GDExtensionUninitializedStringPtr;\n\ + typedef struct __GdextObject *GDExtensionUninitializedObjectPtr;\n\ + typedef struct __GdextType *GDExtensionUninitializedTypePtr;\n\ + \n", + ); + + // Typedef GDExtensionInterfaceGetProcAddress (simply resolving to GDExtensionInterface, as it's the same parameter) + let pos = c + .find("/* INITIALIZATION */") + .expect("Unexpected gdextension_interface.h format (struct)"); + + c.insert_str( + pos, + "\ + // gdext polyfill\n\ + typedef struct {\n\ + uint32_t major;\n\ + uint32_t minor;\n\ + uint32_t patch;\n\ + const char *string;\n\ + } GDExtensionGodotVersion;\n\ + typedef void (*GDExtensionInterfaceFunctionPtr)();\n\ + typedef void (*GDExtensionInterfaceGetGodotVersion)(GDExtensionGodotVersion *r_godot_version);\n\ + typedef GDExtensionInterfaceFunctionPtr (*GDExtensionInterfaceGetProcAddress)(const char *p_function_name);\n\ + \n", + ); +} + fn locate_godot_binary() -> PathBuf { if let Ok(string) = std::env::var("GODOT4_BIN") { println!("Found GODOT4_BIN with path to executable: '{string}'"); diff --git a/godot-codegen/src/api_parser.rs b/godot-codegen/src/api_parser.rs index 6a67760cf..f18aaa8d7 100644 --- a/godot-codegen/src/api_parser.rs +++ b/godot-codegen/src/api_parser.rs @@ -15,6 +15,7 @@ use nanoserde::DeJson; #[derive(DeJson)] pub struct ExtensionApi { + pub header: Header, pub builtin_class_sizes: Vec, pub builtin_classes: Vec, pub classes: Vec, @@ -23,6 +24,16 @@ pub struct ExtensionApi { pub singletons: Vec, } +#[derive(DeJson, Debug)] +pub struct Header { + pub version_major: u8, + pub version_minor: u8, + pub version_patch: u8, + pub version_status: String, + pub version_build: String, + pub version_full_name: String, +} + #[derive(DeJson)] pub struct ClassSizes { pub build_configuration: String, @@ -241,5 +252,7 @@ pub fn load_extension_api(watch: &mut godot_bindings::StopWatch) -> (ExtensionAp DeJson::deserialize_json(json_str).expect("failed to deserialize JSON"); watch.record("deserialize_json"); + println!("Parsed extension_api.json for version {:?}", model.header); + (model, build_config) } diff --git a/godot-codegen/src/interface_generator.rs b/godot-codegen/src/interface_generator.rs index e6de08d01..0141a3c68 100644 --- a/godot-codegen/src/interface_generator.rs +++ b/godot-codegen/src/interface_generator.rs @@ -6,7 +6,7 @@ use crate::central_generator::write_file; use crate::util::ident; -use proc_macro2::{Ident, Literal}; +use proc_macro2::{Ident, Literal, TokenStream}; use quote::quote; use regex::Regex; use std::fs; @@ -21,8 +21,26 @@ struct GodotFuncPtr { pub(crate) fn generate_sys_interface_file( h_path: &Path, sys_gen_path: &Path, + is_godot_4_0: bool, out_files: &mut Vec, ) { + let code = if is_godot_4_0 { + // Compat for 4.0.x + // Most polyfills are in godot_exe.rs, fn polyfill_legacy_header() + quote! { + #[path = "../compat/compat_4_0.rs"] + mod compat_4_0; + + pub use compat_4_0::*; + } + } else { + generate_proc_address_funcs(h_path) + }; + + write_file(sys_gen_path, "interface.rs", code.to_string(), out_files); +} + +fn generate_proc_address_funcs(h_path: &Path) -> TokenStream { let header_code = fs::read_to_string(h_path) .expect("failed to read gdextension_interface.h for header parsing"); let func_ptrs = parse_function_pointers(&header_code); @@ -58,22 +76,35 @@ pub(crate) fn generate_sys_interface_file( // Do not derive Copy -- even though the struct is bitwise-copyable, this is rarely needed and may point to an error. let code = quote! { + #[path = "../compat/compat_4_1.rs"] + mod compat_4_1; + + pub use compat_4_1::*; + pub struct GDExtensionInterface { #( #fptr_decls )* } impl GDExtensionInterface { - pub unsafe fn load(get_proc_address: crate::GDExtensionInterfaceGetProcAddress) -> Self { - let get_proc_address = get_proc_address.expect("get_proc_address null; failed to initialize gdext library"); + pub(crate) unsafe fn load( + get_proc_address: crate::GDExtensionInterfaceGetProcAddress, + ) -> Self { + let get_proc_address = get_proc_address.expect("invalid get_proc_address function pointer"); Self { #( #fptr_inits )* } } } - }; - write_file(sys_gen_path, "interface.rs", code.to_string(), out_files); + // Exists because constructor cannot be called in legacy mode (as the struct is readily-provided by bindgen) + pub(crate) unsafe fn load_interface( + get_proc_address: crate::GDExtensionInterfaceGetProcAddress, + ) -> GDExtensionInterface { + GDExtensionInterface::load(get_proc_address) + } + }; + code } fn parse_function_pointers(header_code: &str) -> Vec { diff --git a/godot-codegen/src/lib.rs b/godot-codegen/src/lib.rs index e46f7aba4..5f76aa6ba 100644 --- a/godot-codegen/src/lib.rs +++ b/godot-codegen/src/lib.rs @@ -47,7 +47,8 @@ pub fn generate_sys_files( generate_sys_central_file(&api, &mut ctx, build_config, sys_gen_path, &mut out_files); watch.record("generate_central_file"); - generate_sys_interface_file(h_path, sys_gen_path, &mut out_files); + let is_godot_4_0 = api.header.version_major == 4 && api.header.version_minor == 0; + generate_sys_interface_file(h_path, sys_gen_path, is_godot_4_0, &mut out_files); watch.record("generate_interface_file"); rustfmt_if_needed(out_files); diff --git a/godot-core/src/init/mod.rs b/godot-core/src/init/mod.rs index 8cc20bd8c..83e30eb31 100644 --- a/godot-core/src/init/mod.rs +++ b/godot-core/src/init/mod.rs @@ -10,12 +10,12 @@ use std::collections::btree_map::BTreeMap; #[doc(hidden)] // TODO consider body safe despite unsafe function, and explicitly mark unsafe {} locations pub unsafe fn __gdext_load_library( - interface: sys::GDExtensionInterfaceGetProcAddress, + interface_or_get_proc_address: sys::InitCompat, library: sys::GDExtensionClassLibraryPtr, init: *mut sys::GDExtensionInitialization, ) -> sys::GDExtensionBool { let init_code = || { - sys::initialize(interface, library); + sys::initialize(interface_or_get_proc_address, library); let mut handle = InitHandle::new(); diff --git a/godot-core/src/lib.rs b/godot-core/src/lib.rs index c2944b9fa..eef46904d 100644 --- a/godot-core/src/lib.rs +++ b/godot-core/src/lib.rs @@ -134,4 +134,4 @@ pub mod private { use std::io::Write; std::io::stdout().flush().expect("flush stdout"); } -} \ No newline at end of file +} diff --git a/godot-core/src/log.rs b/godot-core/src/log.rs index 1f151ef99..f227f488c 100644 --- a/godot-core/src/log.rs +++ b/godot-core/src/log.rs @@ -4,24 +4,39 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ +#[macro_export] +#[doc(hidden)] +macro_rules! inner_godot_msg { + // FIXME expr needs to be parenthesised, see usages + ($godot_fn:ident; $fmt:literal $(, $args:expr)* $(,)?) => { + //($($args:tt),* $(,)?) => { + unsafe { + let msg = format!("{}\0", format_args!($fmt $(, $args)*)); + assert!(msg.is_ascii(), "godot_error: message must be ASCII"); + + // Check whether engine is loaded, otherwise fall back to stderr. + if $crate::sys::is_initialized() { + $crate::sys::interface_fn!($godot_fn)( + $crate::sys::c_str_from_str(&msg), + $crate::sys::c_str(b"\0"), + $crate::sys::c_str_from_str(concat!(file!(), "\0")), + line!() as i32, + false as $crate::sys::GDExtensionBool, // whether to create a toast notification in editor + ); + } else { + eprintln!("[godot_error unavailable] {}", &msg[..msg.len() - 1]); + } + } + }; +} + /// Pushes a warning message to Godot's built-in debugger and to the OS terminal. /// /// _Godot equivalent: @GlobalScope.push_warning()_ #[macro_export] macro_rules! godot_warn { ($fmt:literal $(, $args:expr)* $(,)?) => { - unsafe { - let msg = format!("{}\0", format_args!($fmt $(, $args)*)); - assert!(msg.is_ascii(), "godot_warn: message must be ASCII"); - - $crate::sys::interface_fn!(print_warning)( - $crate::sys::c_str_from_str(&msg), - $crate::sys::c_str(b"\0"), - $crate::sys::c_str_from_str(concat!(file!(), "\0")), - line!() as i32, - false as $crate::sys::GDExtensionBool, // whether to create a toast notification in editor - ); - } + $crate::inner_godot_msg!(print_warning; $fmt $(, $args)*); }; } @@ -30,39 +45,15 @@ macro_rules! godot_warn { /// _Godot equivalent: @GlobalScope.push_error()_ #[macro_export] macro_rules! godot_error { - // FIXME expr needs to be parenthesised, see usages ($fmt:literal $(, $args:expr)* $(,)?) => { - //($($args:tt),* $(,)?) => { - unsafe { - let msg = format!("{}\0", format_args!($fmt $(, $args)*)); - assert!(msg.is_ascii(), "godot_error: message must be ASCII"); - - $crate::sys::interface_fn!(print_error)( - $crate::sys::c_str_from_str(&msg), - $crate::sys::c_str(b"\0"), - $crate::sys::c_str_from_str(concat!(file!(), "\0")), - line!() as i32, - false as $crate::sys::GDExtensionBool, // whether to create a toast notification in editor - ); - } + $crate::inner_godot_msg!(print_error; $fmt $(, $args)*); }; } #[macro_export] macro_rules! godot_script_error { ($fmt:literal $(, $args:expr)* $(,)?) => { - unsafe { - let msg = format!("{}\0", format_args!($fmt $(, $args)*)); - assert!(msg.is_ascii(), "godot_script_error: message must be ASCII"); - - $crate::sys::interface_fn!(print_script_error)( - $crate::sys::c_str_from_str(&msg), - $crate::sys::c_str(b"\0"), - $crate::sys::c_str_from_str(concat!(file!(), "\0")), - line!() as i32, - false as $crate::sys::GDExtensionBool, // whether to create a toast notification in editor - ); - } + $crate::inner_godot_msg!(print_script_error; $fmt $(, $args)*); }; } diff --git a/godot-ffi/src/compat/compat_4_0.rs b/godot-ffi/src/compat/compat_4_0.rs new file mode 100644 index 000000000..45081ffc0 --- /dev/null +++ b/godot-ffi/src/compat/compat_4_0.rs @@ -0,0 +1,32 @@ +/* + * 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 + +use crate as sys; +use crate::compat::CompatVersion; + +pub type InitCompat = *const sys::GDExtensionInterface; + +impl CompatVersion for *const sys::GDExtensionInterface { + fn is_legacy_used_in_modern(&self) -> bool { + false + } + + fn runtime_version(&self) -> sys::GDExtensionGodotVersion { + let interface = unsafe { &**self }; + sys::GDExtensionGodotVersion { + major: interface.version_major, + minor: interface.version_minor, + patch: interface.version_patch, + string: interface.version_string, + } + } + + fn load_interface(&self) -> sys::GDExtensionInterface { + unsafe { **self } + } +} diff --git a/godot-ffi/src/compat/compat_4_1.rs b/godot-ffi/src/compat/compat_4_1.rs new file mode 100644 index 000000000..38c64fa25 --- /dev/null +++ b/godot-ffi/src/compat/compat_4_1.rs @@ -0,0 +1,64 @@ +/* + * 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/. + */ + +//! Modern 4.1+ API + +use crate as sys; + +pub type InitCompat = sys::GDExtensionInterfaceGetProcAddress; + +impl CompatVersion for sys::GDExtensionInterfaceGetProcAddress { + fn is_legacy_used_in_modern(&self) -> bool { + // 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 data_ptr = get_proc_address as *const u32; // crowbar it via `as` cast + + // Assumption is that we have at least 8 bytes of memory to safely read from (for both the data and the function case). + let major = unsafe { data_ptr.read() }; + let minor = unsafe { data_ptr.offset(1).read() }; + + return major == 4 && minor == 0; + } + + fn runtime_version(&self) -> sys::GDExtensionGodotVersion { + unsafe { + let get_proc_address = self.expect("get_proc_address unexpectedly null"); + let get_godot_version = get_proc_address(sys::c_str(b"get_godot_version\0")); //.expect("get_godot_version unexpectedly null"); + + let get_godot_version = cast_fn_ptr!(get_godot_version as sys::GDExtensionInterfaceGetGodotVersion); + + let mut version = std::mem::MaybeUninit::::zeroed(); + get_godot_version(version.as_mut_ptr()); + version.assume_init() + } + } + + fn load_interface(&self) -> sys::GDExtensionInterface { + crate::gen::interface::load_interface() + } +} diff --git a/godot-ffi/src/compat/mod.rs b/godot-ffi/src/compat/mod.rs new file mode 100644 index 000000000..c5e042d7a --- /dev/null +++ b/godot-ffi/src/compat/mod.rs @@ -0,0 +1,17 @@ +use crate as sys; + +/// Dispatch at runtime between Godot 4.0 legacy and 4.1+ APIs. +pub(crate) trait CompatVersion { + /// Return whether a gdext version compiled against 4.1+ GDExtension is invoked with an entry point using the legacy calling convention. + /// + /// This can happen in two cases: + /// * The .gdextension file's `[configuration]` section does not contain a `compatibility_minimum = 4.1` statement. + /// * gdext was compiled against a 4.1+ Godot version, but at runtime the library is loaded from a 4.0.x version. + fn is_legacy_used_in_modern(&self) -> bool; + + /// Return version dynamically passed via `gdextension_interface.h` file. + fn runtime_version(&self) -> sys::GDExtensionGodotVersion; + + /// Return the interface, either as-is from the header (legacy) or code-generated (modern API). + fn load_interface(&self) -> sys::GDExtensionInterface; +} diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index a5df2d135..696ca63d0 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -19,12 +19,14 @@ )] pub(crate) mod gen; +mod compat; mod godot_ffi; mod opaque; mod plugins; +use compat::CompatVersion; use std::ffi::CStr; -use std::ptr; + // 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)] @@ -33,7 +35,7 @@ pub use paste; pub use crate::godot_ffi::{GodotFfi, GodotFuncMarshal, PtrcallType}; pub use gen::central::*; pub use gen::gdextension_interface::*; -pub use gen::interface::GDExtensionInterface; +pub use gen::interface::*; // The impls only compile if those are different types -- ensures type safety through patch trait Distinct {} @@ -75,33 +77,38 @@ static mut BINDING: Option = None; /// # Safety /// -/// - The `interface` pointer must be a valid pointer to a [`GDExtensionInterface`] object. +/// - 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 `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( - get_proc_address: GDExtensionInterfaceGetProcAddress, - library: GDExtensionClassLibraryPtr, -) { - out!("Initialize..."); +pub unsafe fn initialize(compat: InitCompat, library: GDExtensionClassLibraryPtr) { + out!("Initialize gdext..."); + + // Before anything else: if we run into a Godot binary that's compiled differently from gdext, proceeding would be UB -> panic. + if compat.is_legacy_used_in_modern() { + panic!( + "gdext was compiled against a newer Godot version (4.1+), but initialized with a legacy (4.0.x) setup.\ + \nIn your .gdextension file, make sure to use `compatibility_minimum = 4.1` under the [configuration] section." + ); + } + + let version = compat.runtime_version(); + out!("GDExtension API version: {version:?}"); + + let interface = compat.load_interface(); + out!("Loaded interface."); - let interface = GDExtensionInterface::load(get_proc_address); let method_table = GlobalMethodTable::load(&interface); + out!("Loaded builtin table."); BINDING = Some(GodotBinding { interface, method_table, library, }); - - let mut version = GDExtensionGodotVersion { - major: 0, - minor: 0, - patch: 0, - string: std::ptr::null(), - }; - interface_fn!(get_godot_version)(ptr::addr_of_mut!(version)); - out!("Detected {version:?}"); + out!("Assigned binding."); println!( "Initialize GDExtension API for Rust: {}", @@ -111,6 +118,13 @@ pub unsafe fn initialize( ); } +/// # Safety +/// +/// Must be called from the same thread as `initialize()` previously. +pub unsafe fn is_initialized() -> bool { + BINDING.is_some() +} + /// # Safety /// /// The interface must have been initialised with [`initialize`] before calling this function. @@ -224,6 +238,8 @@ pub fn to_const_ptr(ptr: *mut T) -> *const T { ptr as *const T } +// ---------------------------------------------------------------------------------------------------------------------------------------------- + /// Convert a GDExtension pointer type to its uninitialized version. pub trait AsUninit { type Ptr; @@ -257,6 +273,39 @@ impl_as_uninit!(GDExtensionStringPtr, GDExtensionUninitializedStringPtr); impl_as_uninit!(GDExtensionObjectPtr, GDExtensionUninitializedObjectPtr); impl_as_uninit!(GDExtensionTypePtr, GDExtensionUninitializedTypePtr); +// ---------------------------------------------------------------------------------------------------------------------------------------------- + +/// Metafunction to extract inner function pointer types from all the bindgen Option type names. +pub(crate) trait Inner: Sized { + type FnPtr: Sized; + + fn extract(self, error_msg: &str) -> Self::FnPtr; +} + +impl Inner for Option { + type FnPtr = T; + + fn extract(self, error_msg: &str) -> Self::FnPtr { + self.expect(error_msg) + } +} + +/// Extract a function pointer from its `Option` and convert it to the (dereferenced) target type. +/// +/// ```ignore +/// let get_godot_version = get_proc_address(sys::c_str(b"get_godot_version\0")); +/// let get_godot_version = sys::cast_fn_ptr!(get_godot_version as sys::GDExtensionInterfaceGetGodotVersion); +/// ``` +#[allow(unused)] +macro_rules! cast_fn_ptr { + ($option:ident as $ToType:ty) => {{ + let ptr = $option.expect("null function pointer"); + std::mem::transmute::::FnPtr>(ptr) + }}; +} + +// ---------------------------------------------------------------------------------------------------------------------------------------------- + /// If `ptr` is not null, returns `Some(mapper(ptr))`; otherwise `None`. #[inline] pub fn ptr_then(ptr: *mut T, mapper: F) -> Option diff --git a/godot-macros/src/gdextension.rs b/godot-macros/src/gdextension.rs index cdc9ead95..737950c4d 100644 --- a/godot-macros/src/gdextension.rs +++ b/godot-macros/src/gdextension.rs @@ -38,12 +38,12 @@ pub fn transform(decl: Declaration) -> ParseResult { #[no_mangle] unsafe extern "C" fn #entry_point( - get_proc_address: ::godot::sys::GDExtensionInterfaceGetProcAddress, + interface_or_get_proc_address: ::godot::sys::InitCompat, library: ::godot::sys::GDExtensionClassLibraryPtr, init: *mut ::godot::sys::GDExtensionInitialization, ) -> ::godot::sys::GDExtensionBool { ::godot::init::__gdext_load_library::<#impl_ty>( - get_proc_address, + interface_or_get_proc_address, library, init ) diff --git a/itest/godot/itest.gdextension b/itest/godot/itest.gdextension index 4a9d78fe5..a48361009 100644 --- a/itest/godot/itest.gdextension +++ b/itest/godot/itest.gdextension @@ -1,5 +1,6 @@ [configuration] entry_symbol = "itest_init" +compatibility_minimum = 4.1 [libraries] linux.debug.x86_64 = "res://../../target/debug/libitest.so" From c300062c149536474624f30495c8f39b3600d312 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 22 May 2023 19:25:27 +0200 Subject: [PATCH 08/13] Add GdextBuild to access build/runtime metadata --- godot-codegen/src/api_parser.rs | 2 +- godot-codegen/src/central_generator.rs | 48 ++++++++++++++++++++++++++ godot-ffi/src/lib.rs | 25 +++++++++++++- 3 files changed, 73 insertions(+), 2 deletions(-) diff --git a/godot-codegen/src/api_parser.rs b/godot-codegen/src/api_parser.rs index f18aaa8d7..d76d19093 100644 --- a/godot-codegen/src/api_parser.rs +++ b/godot-codegen/src/api_parser.rs @@ -24,7 +24,7 @@ pub struct ExtensionApi { pub singletons: Vec, } -#[derive(DeJson, Debug)] +#[derive(DeJson, Clone, Debug)] pub struct Header { pub version_major: u8, pub version_minor: u8, diff --git a/godot-codegen/src/central_generator.rs b/godot-codegen/src/central_generator.rs index 6814ce0c2..8f13e3c64 100644 --- a/godot-codegen/src/central_generator.rs +++ b/godot-codegen/src/central_generator.rs @@ -23,6 +23,7 @@ struct CentralItems { variant_fn_decls: Vec, variant_fn_inits: Vec, global_enum_defs: Vec, + godot_version: Header, } pub(crate) struct TypeNames { @@ -128,9 +129,12 @@ fn make_sys_code(central_items: &CentralItems) -> String { variant_op_enumerators_ord, variant_fn_decls, variant_fn_inits, + godot_version, .. } = central_items; + let build_config_struct = make_build_config(godot_version); + let sys_tokens = quote! { use crate::{ ffi_methods, GDExtensionConstTypePtr, GDExtensionTypePtr, GDExtensionUninitializedTypePtr, @@ -143,6 +147,10 @@ fn make_sys_code(central_items: &CentralItems) -> String { // ---------------------------------------------------------------------------------------------------------------------------------------------- + #build_config_struct + + // ---------------------------------------------------------------------------------------------------------------------------------------------- + pub struct GlobalMethodTable { #(#variant_fn_decls)* } @@ -228,6 +236,45 @@ fn make_sys_code(central_items: &CentralItems) -> String { sys_tokens.to_string() } +fn make_build_config(header: &Header) -> TokenStream { + let version_string = header + .version_full_name + .strip_prefix("Godot Engine ") + .unwrap_or(&header.version_full_name); + let major = header.version_major; + let minor = header.version_minor; + let patch = header.version_patch; + + // Should this be mod? + quote! { + /// Provides meta-information about the library and the Godot version in use. + pub struct GdextBuild; + + impl GdextBuild { + /// Godot version against which gdext was compiled. + /// + /// Example format: `v4.0.stable.official` + pub const fn godot_static_version_string() -> &'static str { + #version_string + } + + /// Godot version against which gdext was compiled, as `(major, minor, patch)` triple. + pub const fn godot_static_version_triple() -> (u8, u8, u8) { + (#major, #minor, #patch) + } + + /// Version of the Godot engine which loaded gdext via GDExtension binding. + pub fn godot_runtime_version_string() -> String { + unsafe { + let char_ptr = crate::runtime_metadata().godot_version.string; + let c_str = std::ffi::CStr::from_ptr(char_ptr); + String::from_utf8_lossy(c_str.to_bytes()).to_string() + } + } + } + } +} + fn make_core_code(central_items: &CentralItems) -> String { let CentralItems { variant_ty_enumerators_pascal, @@ -311,6 +358,7 @@ fn make_central_items(api: &ExtensionApi, build_config: &str, ctx: &mut Context) variant_fn_decls: Vec::with_capacity(len), variant_fn_inits: Vec::with_capacity(len), global_enum_defs: Vec::new(), + godot_version: api.header.clone(), }; let mut builtin_types: Vec<_> = builtin_types_map.values().collect(); diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index 696ca63d0..c0da1379e 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -68,6 +68,11 @@ struct GodotBinding { interface: GDExtensionInterface, library: GDExtensionClassLibraryPtr, method_table: GlobalMethodTable, + runtime_metadata: GdextRuntimeMetadata, +} + +struct GdextRuntimeMetadata { + godot_version: GDExtensionGodotVersion, } /// Late-init globals @@ -86,6 +91,11 @@ static mut BINDING: Option = None; pub unsafe fn initialize(compat: InitCompat, library: GDExtensionClassLibraryPtr) { out!("Initialize gdext..."); + out!( + "Godot version against which gdext was compiled: {}", + GdextBuild::godot_static_version_string() + ); + // Before anything else: if we run into a Godot binary that's compiled differently from gdext, proceeding would be UB -> panic. if compat.is_legacy_used_in_modern() { panic!( @@ -95,7 +105,7 @@ pub unsafe fn initialize(compat: InitCompat, library: GDExtensionClassLibraryPtr } let version = compat.runtime_version(); - out!("GDExtension API version: {version:?}"); + out!("Godot version of GDExtension API at runtime: {version:?}"); let interface = compat.load_interface(); out!("Loaded interface."); @@ -103,10 +113,15 @@ pub unsafe fn initialize(compat: InitCompat, library: GDExtensionClassLibraryPtr let method_table = GlobalMethodTable::load(&interface); out!("Loaded builtin table."); + let runtime_metadata = GdextRuntimeMetadata { + godot_version: version, + }; + BINDING = Some(GodotBinding { interface, method_table, library, + runtime_metadata, }); out!("Assigned binding."); @@ -149,6 +164,14 @@ pub unsafe fn method_table() -> &'static GlobalMethodTable { &unwrap_ref_unchecked(&BINDING).method_table } +/// # Safety +/// +/// Must be accessed from the main thread. +#[inline(always)] +pub(crate) unsafe fn runtime_metadata() -> &'static GdextRuntimeMetadata { + &BINDING.as_ref().unwrap().runtime_metadata +} + /// Makes sure that Godot is running, or panics. Debug mode only! macro_rules! debug_assert_godot { ($expr:expr) => { From 83932c7d7138dcbe81927f2b37c53aaaf5e72d85 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 22 May 2023 20:06:37 +0200 Subject: [PATCH 09/13] Detect 4.0 <-> 4.1 mismatches in both directions + missing `compatibility_minimum = 4.1` --- .../godot/DodgeTheCreeps.gdextension | 1 + godot-codegen/src/interface_generator.rs | 17 ++--- godot-core/src/lib.rs | 16 ++++- godot-core/src/log.rs | 2 +- godot-ffi/src/compat/compat_4_0.rs | 30 ++++++-- godot-ffi/src/compat/compat_4_1.rs | 70 ++++++++++++++++--- godot-ffi/src/compat/mod.rs | 33 +++++++-- godot-ffi/src/lib.rs | 12 ++-- 8 files changed, 139 insertions(+), 42 deletions(-) diff --git a/examples/dodge-the-creeps/godot/DodgeTheCreeps.gdextension b/examples/dodge-the-creeps/godot/DodgeTheCreeps.gdextension index 6f5badabd..7bbaf48e8 100644 --- a/examples/dodge-the-creeps/godot/DodgeTheCreeps.gdextension +++ b/examples/dodge-the-creeps/godot/DodgeTheCreeps.gdextension @@ -1,5 +1,6 @@ [configuration] entry_symbol = "gdext_rust_init" +compatibility_minimum = 4.1 [libraries] linux.debug.x86_64 = "res://../../../target/debug/libdodge_the_creeps.so" diff --git a/godot-codegen/src/interface_generator.rs b/godot-codegen/src/interface_generator.rs index 0141a3c68..d7c2e2bb1 100644 --- a/godot-codegen/src/interface_generator.rs +++ b/godot-codegen/src/interface_generator.rs @@ -31,7 +31,7 @@ pub(crate) fn generate_sys_interface_file( #[path = "../compat/compat_4_0.rs"] mod compat_4_0; - pub use compat_4_0::*; + pub use compat_4_0::InitCompat; } } else { generate_proc_address_funcs(h_path) @@ -79,7 +79,7 @@ fn generate_proc_address_funcs(h_path: &Path) -> TokenStream { #[path = "../compat/compat_4_1.rs"] mod compat_4_1; - pub use compat_4_1::*; + pub use compat_4_1::InitCompat; pub struct GDExtensionInterface { #( #fptr_decls )* @@ -96,13 +96,6 @@ fn generate_proc_address_funcs(h_path: &Path) -> TokenStream { } } } - - // Exists because constructor cannot be called in legacy mode (as the struct is readily-provided by bindgen) - pub(crate) unsafe fn load_interface( - get_proc_address: crate::GDExtensionInterfaceGetProcAddress, - ) -> GDExtensionInterface { - GDExtensionInterface::load(get_proc_address) - } }; code } @@ -143,13 +136,11 @@ fn parse_function_pointers(header_code: &str) -> Vec { .unwrap(); let mut func_ptrs = vec![]; - for cap in regex.captures_iter(&header_code) { + for cap in regex.captures_iter(header_code) { let name = cap.name("name"); let funcptr_ty = cap.name("type"); let doc = cap.name("doc"); - dbg!(&cap); - let (Some(name), Some(funcptr_ty), Some(doc)) = (name, funcptr_ty, doc) else { // Skip unparseable ones, instead of breaking build (could just be a /** */ comment around something else) continue; @@ -193,7 +184,7 @@ fn test_parse_function_pointers() { typedef void (*GDExtensionInterfaceClassdbRegisterExtensionClass)(GDExtensionClassLibraryPtr p_library, GDExtensionConstStringNamePtr p_class_name, GDExtensionConstStringNamePtr p_parent_class_name, const GDExtensionClassCreationInfo *p_extension_funcs); "#; - let func_ptrs = super::parse_function_pointers(header_code); + let func_ptrs = parse_function_pointers(header_code); assert_eq!(func_ptrs.len(), 1); let func_ptr = &func_ptrs[0]; diff --git a/godot-core/src/lib.rs b/godot-core/src/lib.rs index eef46904d..bced3b43a 100644 --- a/godot-core/src/lib.rs +++ b/godot-core/src/lib.rs @@ -65,14 +65,26 @@ pub mod private { fn print_panic(err: Box) { if let Some(s) = err.downcast_ref::<&'static str>() { - log::godot_error!("Panic msg: {s}"); + print_panic_message(s); } else if let Some(s) = err.downcast_ref::() { - log::godot_error!("Panic msg: {s}"); + print_panic_message(s.as_str()); } else { log::godot_error!("Rust panic of type ID {:?}", err.type_id()); } } + fn print_panic_message(msg: &str) { + // If the message contains newlines, print all of the lines after a line break, and indent them. + let lbegin = "\n "; + let indented = msg.replace('\n', lbegin); + + if indented.len() != msg.len() { + log::godot_error!("Panic msg:{lbegin}{indented}"); + } else { + log::godot_error!("Panic msg: {msg}"); + } + } + struct GodotPanicInfo { line: u32, file: String, diff --git a/godot-core/src/log.rs b/godot-core/src/log.rs index f227f488c..988ea3d10 100644 --- a/godot-core/src/log.rs +++ b/godot-core/src/log.rs @@ -24,7 +24,7 @@ macro_rules! inner_godot_msg { false as $crate::sys::GDExtensionBool, // whether to create a toast notification in editor ); } else { - eprintln!("[godot_error unavailable] {}", &msg[..msg.len() - 1]); + eprintln!("[{}] {}", stringify!($godot_fn), &msg[..msg.len() - 1]); } } }; diff --git a/godot-ffi/src/compat/compat_4_0.rs b/godot-ffi/src/compat/compat_4_0.rs index 45081ffc0..709f00931 100644 --- a/godot-ffi/src/compat/compat_4_0.rs +++ b/godot-ffi/src/compat/compat_4_0.rs @@ -5,15 +5,37 @@ */ //! 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::CompatVersion; +use crate::compat::BindingCompat; pub type InitCompat = *const sys::GDExtensionInterface; -impl CompatVersion for *const sys::GDExtensionInterface { - fn is_legacy_used_in_modern(&self) -> bool { - false +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 interface = unsafe { &**self }; + let major = interface.version_major; + let minor = interface.version_minor; + + // We cannot print version (major/minor are parts of the function pointer). We _could_ theoretically interpret it as + // GetProcAddr function pointer and call get_godot_version, but that's not adding that much useful information and may + // also fail. + let static_version = crate::GdextBuild::godot_static_version_string(); + assert!(major == 4 && minor == 0, + "gdext was compiled against a legacy Godot version ({static_version}),\n\ + but initialized by a newer Godot binary (4.1+).\n\ + \n\ + You have multiple options:\n\ + 1) Recompile gdext against the newer Godot version.\n\ + 2) If you want to use a legacy extension under newer Godot, open the .gdextension file\n \ + and add `compatibility_minimum = 4.0` under the [configuration] section.\n" + ); } fn runtime_version(&self) -> sys::GDExtensionGodotVersion { diff --git a/godot-ffi/src/compat/compat_4_1.rs b/godot-ffi/src/compat/compat_4_1.rs index 38c64fa25..fba44e780 100644 --- a/godot-ffi/src/compat/compat_4_1.rs +++ b/godot-ffi/src/compat/compat_4_1.rs @@ -5,13 +5,27 @@ */ //! 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. +//! +//! Relevant upstream PR: https://github.com/godotengine/godot/pull/76406 use crate as sys; +use crate::compat::BindingCompat; pub type InitCompat = sys::GDExtensionInterfaceGetProcAddress; -impl CompatVersion for sys::GDExtensionInterfaceGetProcAddress { - fn is_legacy_used_in_modern(&self) -> bool { +#[repr(C)] +struct LegacyLayout { + version_major: u32, + version_minor: u32, + version_patch: u32, + version_string: *const std::ffi::c_char, +} + +impl BindingCompat for sys::GDExtensionInterfaceGetProcAddress { + 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. // @@ -36,13 +50,52 @@ impl CompatVersion for sys::GDExtensionInterfaceGetProcAddress { // 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 data_ptr = get_proc_address as *const u32; // crowbar it via `as` cast + let data_ptr = get_proc_address as *const LegacyLayout; // crowbar it via `as` cast // Assumption is that we have at least 8 bytes of memory to safely read from (for both the data and the function case). - let major = unsafe { data_ptr.read() }; - let minor = unsafe { data_ptr.offset(1).read() }; + let major = unsafe { data_ptr.read().version_major }; + let minor = unsafe { data_ptr.read().version_minor }; + let patch = unsafe { data_ptr.read().version_patch }; + + if major != 4 || minor != 0 { + // Technically, major should always be 4; loading Godot 3 will crash anyway. + return; + } - return major == 4 && minor == 0; + let static_version = crate::GdextBuild::godot_static_version_string(); + let runtime_version = unsafe { + let char_ptr = data_ptr.read().version_string; + let c_str = std::ffi::CStr::from_ptr(char_ptr); + + String::from_utf8_lossy(c_str.to_bytes()) + .as_ref() + .strip_prefix("Godot Engine ") + .unwrap_or(&String::from_utf8_lossy(c_str.to_bytes())) + .to_string() + }; + + // Version 4.0.999 is used to signal that we're running Godot 4.1+ but loading extensions in legacy mode. + if patch == 999 { + // Godot 4.1+ loading the extension in legacy mode. + // + // Instead of panicking, we could *theoretically* fall back to the legacy API at runtime, but then gdext would need to + // always ship two versions of gdextension_interface.h (+ generated code) and would encourage use of the legacy API. + panic!( + "gdext was compiled against a modern Godot version ({static_version}), but loaded in legacy (4.0.x) mode.\n\ + In your .gdextension file, add `compatibility_minimum = 4.1` under the [configuration] section.\n" + ) + } else { + // Truly a Godot 4.0 version. + panic!( + "gdext was compiled against a newer Godot version ({static_version}),\n\ + but loaded by a legacy Godot binary ({runtime_version}).\n\ + \n\ + You have multiple options:\n\ + 1) Run the newer Godot version.\n\ + 2) Compile gdext against the older Godot binary (see `custom-godot` feature).\n\ + \n" + ); + } } fn runtime_version(&self) -> sys::GDExtensionGodotVersion { @@ -50,7 +103,8 @@ impl CompatVersion for sys::GDExtensionInterfaceGetProcAddress { let get_proc_address = self.expect("get_proc_address unexpectedly null"); let get_godot_version = get_proc_address(sys::c_str(b"get_godot_version\0")); //.expect("get_godot_version unexpectedly null"); - let get_godot_version = cast_fn_ptr!(get_godot_version as sys::GDExtensionInterfaceGetGodotVersion); + let get_godot_version = + crate::cast_fn_ptr!(get_godot_version as sys::GDExtensionInterfaceGetGodotVersion); let mut version = std::mem::MaybeUninit::::zeroed(); get_godot_version(version.as_mut_ptr()); @@ -59,6 +113,6 @@ impl CompatVersion for sys::GDExtensionInterfaceGetProcAddress { } fn load_interface(&self) -> sys::GDExtensionInterface { - crate::gen::interface::load_interface() + unsafe { sys::GDExtensionInterface::load(*self) } } } diff --git a/godot-ffi/src/compat/mod.rs b/godot-ffi/src/compat/mod.rs index c5e042d7a..612524750 100644 --- a/godot-ffi/src/compat/mod.rs +++ b/godot-ffi/src/compat/mod.rs @@ -1,13 +1,34 @@ +/* + * 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; /// Dispatch at runtime between Godot 4.0 legacy and 4.1+ APIs. -pub(crate) trait CompatVersion { - /// Return whether a gdext version compiled against 4.1+ GDExtension is invoked with an entry point using the legacy calling convention. +/// +/// 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. +pub(crate) 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 sub-cases: + /// + /// 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 can happen in two cases: - /// * The .gdextension file's `[configuration]` section does not contain a `compatibility_minimum = 4.1` statement. - /// * gdext was compiled against a 4.1+ Godot version, but at runtime the library is loaded from a 4.0.x version. - fn is_legacy_used_in_modern(&self) -> bool; + /// 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. fn runtime_version(&self) -> sys::GDExtensionGodotVersion; diff --git a/godot-ffi/src/lib.rs b/godot-ffi/src/lib.rs index c0da1379e..9b3742b47 100644 --- a/godot-ffi/src/lib.rs +++ b/godot-ffi/src/lib.rs @@ -24,7 +24,7 @@ mod godot_ffi; mod opaque; mod plugins; -use compat::CompatVersion; +use compat::BindingCompat; use std::ffi::CStr; // See https://github.com/dtolnay/paste/issues/69#issuecomment-962418430 @@ -97,12 +97,7 @@ pub unsafe fn initialize(compat: InitCompat, library: GDExtensionClassLibraryPtr ); // Before anything else: if we run into a Godot binary that's compiled differently from gdext, proceeding would be UB -> panic. - if compat.is_legacy_used_in_modern() { - panic!( - "gdext was compiled against a newer Godot version (4.1+), but initialized with a legacy (4.0.x) setup.\ - \nIn your .gdextension file, make sure to use `compatibility_minimum = 4.1` under the [configuration] section." - ); - } + compat.ensure_static_runtime_compatibility(); let version = compat.runtime_version(); out!("Godot version of GDExtension API at runtime: {version:?}"); @@ -320,10 +315,11 @@ impl Inner for Option { /// let get_godot_version = sys::cast_fn_ptr!(get_godot_version as sys::GDExtensionInterfaceGetGodotVersion); /// ``` #[allow(unused)] +#[macro_export] macro_rules! cast_fn_ptr { ($option:ident as $ToType:ty) => {{ let ptr = $option.expect("null function pointer"); - std::mem::transmute::::FnPtr>(ptr) + std::mem::transmute::::FnPtr>(ptr) }}; } From e980f29c9818ff75bb55d5b94f05c14af5a7c05f Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 22 May 2023 23:19:24 +0200 Subject: [PATCH 10/13] Detect legacy/modern version of C header (also without `custom-godot` feature) --- godot-bindings/src/godot_exe.rs | 41 +++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/godot-bindings/src/godot_exe.rs b/godot-bindings/src/godot_exe.rs index 17948b5d5..6b2814c6d 100644 --- a/godot-bindings/src/godot_exe.rs +++ b/godot-bindings/src/godot_exe.rs @@ -51,10 +51,11 @@ pub fn write_gdextension_headers( is_h_provided: bool, watch: &mut StopWatch, ) { - // TODO recognize 4.0 also when not using `custom-godot` - let mut is_godot_4_0 = false; - - if !is_h_provided { + // None=(unknown, no engine), Some=(version of Godot). Later verified by header itself. + let is_engine_4_0; + if is_h_provided { + is_engine_4_0 = None; + } else { // No external C header file: Godot binary is present, we use it to dump C header let godot_bin = locate_godot_binary(); rerun_on_changed(&godot_bin); @@ -62,9 +63,7 @@ pub fn write_gdextension_headers( // Regenerate API JSON if first time or Godot version is different let version = read_godot_version(&godot_bin); - if version.major == 4 && version.minor == 0 { - is_godot_4_0 = true; - } + is_engine_4_0 = Some(version.major == 4 && version.minor == 0); // if !c_header_path.exists() || has_version_changed(&version) { dump_header_file(&godot_bin, inout_h_path); @@ -74,7 +73,7 @@ pub fn write_gdextension_headers( }; rerun_on_changed(inout_h_path); - patch_c_header(inout_h_path, is_godot_4_0); + patch_c_header(inout_h_path, is_engine_4_0); watch.record("patch_header_h"); generate_rust_binding(inout_h_path, out_rs_path); @@ -160,23 +159,35 @@ fn dump_header_file(godot_bin: &Path, out_file: &Path) { println!("Generated {}/gdextension_interface.h.", cwd.display()); } -fn patch_c_header(inout_h_path: &Path, is_godot_4_0: bool) { +fn patch_c_header(inout_h_path: &Path, is_engine_4_0: Option) { + // The C header path *must* be passed in by the invoking crate, as the path cannot be relative to this crate. + // Otherwise, it can be something like `/home/runner/.cargo/git/checkouts/gdext-76630c89719e160c/efd3b94/godot-bindings`. + println!( - "Patch C header '{}' (is_godot_4_0={is_godot_4_0})...", + "Patch C header '{}' (is_engine_4_0={is_engine_4_0:?})...", inout_h_path.display() ); - // The C header path *must* be passed in by the invoking crate, as the path cannot be relative to this crate. - // Otherwise, it can be something like `/home/runner/.cargo/git/checkouts/gdext-76630c89719e160c/efd3b94/godot-bindings`. - - // Read the contents of the file into a string let mut c = fs::read_to_string(inout_h_path) .unwrap_or_else(|_| panic!("failed to read C header file {}", inout_h_path.display())); - if is_godot_4_0 { + // Detect whether header is legacy (4.0) or modern (4.1+) format. + let is_header_4_0 = !c.contains("GDExtensionInterfaceGetProcAddress"); + println!("is_header_4_0={is_header_4_0}"); + + // Sanity check + if let Some(is_engine_4_0) = is_engine_4_0 { + assert_eq!( + is_header_4_0, is_engine_4_0, + "Mismatch between engine/header versions" + ); + } + + if is_header_4_0 { polyfill_legacy_header(&mut c); } + // Patch for variant converters and type constructors c = c.replace( "typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionVariantPtr, GDExtensionTypePtr);", "typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionUninitializedVariantPtr, GDExtensionTypePtr);" From 1f18a5b9a5d8718275386a7591f1cdfe6f5fed5f Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Tue, 23 May 2023 21:26:09 +0200 Subject: [PATCH 11/13] CI: add jobs that use patched 4.0.x versions --- .github/composite/godot-itest/action.yml | 51 ++++++++++++++++++- .github/workflows/full-ci.yml | 45 ++++++++++++++-- .../godot/DodgeTheCreeps.gdextension | 2 +- godot-bindings/Cargo.toml | 2 +- godot-bindings/res/tweak.patch | 41 ++++++++++++--- itest/godot/itest.gdextension | 2 +- 6 files changed, 126 insertions(+), 17 deletions(-) diff --git a/.github/composite/godot-itest/action.yml b/.github/composite/godot-itest/action.yml index fd2cd4ff4..f10db9584 100644 --- a/.github/composite/godot-itest/action.yml +++ b/.github/composite/godot-itest/action.yml @@ -24,6 +24,11 @@ inputs: default: 'false' description: "Should the job check against latest gdextension_interface.h, and warn on difference" + godot-prebuilt-patch: + required: false + default: '' + description: "If specified, sets the branch name of the godot4-prebuilt crate to this value" + rust-toolchain: required: false default: 'stable' @@ -71,6 +76,46 @@ runs: rust: ${{ inputs.rust-toolchain }} with-llvm: ${{ inputs.with-llvm }} + - name: "Patch prebuilt version ({{ inputs.godot-prebuilt-patch }})" + if: inputs.godot-prebuilt-patch != '' + env: + VERSION: ${{ inputs.godot-prebuilt-patch }} + # sed -i'' needed for macOS compatibility, see https://stackoverflow.com/q/4247068 + run: | + echo "Patch prebuilt version to $VERSION..." + + # For newer versions, update the compatibility_minimum in .gdextension files to 4.1 + # Once a 4.1.0 is released, we can invert this and set compatibility_minimum to 4.0 for older versions. + if [[ "$VERSION" == "4.1" ]]; then + echo "Update compatibility_minimum in .gdextension files..." + dirs=("itest" "examples") + for dir in "${dirs[@]}"; do + find "$dir" -type f -name "*.gdextension" -exec sed -i'.bak' 's/compatibility_minimum = 4\.0/compatibility_minimum = 4.1/' {} + + done + + # Versions 4.0.x + else + # Patch only needed if version is not already set + if grep -E 'godot4-prebuilt = { .+ branch = "$VERSION" }' godot-bindings/Cargo.toml; then + echo "Already has version $version; no need for patch." + else + cat << HEREDOC >> Cargo.toml + [patch."https://github.com/godot-rust/godot4-prebuilt"] + godot4-prebuilt = { git = "https://github.com//godot-rust/godot4-prebuilt", branch = "$VERSION" } + HEREDOC + echo "Patched Cargo.toml for version $version." + fi + fi + + shell: bash + + # else + - name: "No patch selected" + if: inputs.godot-prebuilt-patch == '' + run: | + echo "No patch selected; use default godot4-prebuilt version." + shell: bash + - name: "Build gdext (itest)" run: | cargo build -p itest ${{ inputs.rust-extra-args }} @@ -78,7 +123,9 @@ runs: env: RUSTFLAGS: ${{ inputs.rust-env-rustflags }} - # Note: no longer fails, as we expect header to be forward-compatible; instead issues a warning + # This step no longer fails if there's a diff, as we expect header to be forward-compatible; instead issues a warning + # However, still fails if patch cannot be applied (conflict). + # Step is only run in latest, not for compat 4.0.1 etc. -> no need to take into account different header versions. - name: "Copy and compare GDExtension header" if: inputs.godot-check-header == 'true' run: | @@ -96,7 +143,7 @@ runs: echo "\`\`\`diff" >> $GITHUB_STEP_SUMMARY git diff --no-index gdextension_interface_prebuilt.h gdextension_interface.h >> $GITHUB_STEP_SUMMARY || true echo "\`\`\`" >> $GITHUB_STEP_SUMMARY - echo "After manually updating file, run: \`git diff -R > tweak.patch\`." >> $GITHUB_STEP_SUMMARY + echo "After manually updating file, run: \`git diff -R > tweak2.patch && mv tweak2.patch tweak.patch\`." >> $GITHUB_STEP_SUMMARY # Undo modifications mv gdextension_interface_prebuilt.h gdextension_interface.h diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index bc0f88613..f6ac2860a 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -127,9 +127,13 @@ jobs: # Additionally, the 'linux (msrv *)' special case will then be listed next to the other 'linux' jobs. # Note: Windows uses '--target x86_64-pc-windows-msvc' by default as Cargo argument. include: - - name: macos + # macOS + + - name: macos-4.0.3 os: macos-12 + artifact-name: macos godot-binary: godot.macos.editor.dev.x86_64 + godot-prebuilt-patch: '4.0.3' - name: macos-double os: macos-12 @@ -142,10 +146,15 @@ jobs: godot-binary: godot.macos.editor.dev.x86_64 rust-extra-args: --features godot/custom-godot with-llvm: true + godot-prebuilt-patch: '4.1' - - name: windows + # Windows + + - name: windows-4.0.3 os: windows-latest + artifact-name: windows godot-binary: godot.windows.editor.dev.x86_64.exe + godot-prebuilt-patch: '4.0.3' - name: windows-double os: windows-latest @@ -157,12 +166,35 @@ jobs: artifact-name: windows godot-binary: godot.windows.editor.dev.x86_64.exe rust-extra-args: --features godot/custom-godot + godot-prebuilt-patch: '4.1' + + # Linux # Don't use latest Ubuntu (22.04) as it breaks lots of ecosystem compatibility. # If ever moving to ubuntu-latest, need to manually install libtinfo5 for LLVM. - - name: linux + - name: linux-4.0.3 os: ubuntu-20.04 + artifact-name: linux godot-binary: godot.linuxbsd.editor.dev.x86_64 + godot-check-header: false # disabled for now + + - name: linux-4.0.2 + os: ubuntu-20.04 + artifact-name: linux + godot-binary: godot.linuxbsd.editor.dev.x86_64 + godot-prebuilt-patch: '4.0.2' + + - name: linux-4.0.1 + os: ubuntu-20.04 + artifact-name: linux + godot-binary: godot.linuxbsd.editor.dev.x86_64 + godot-prebuilt-patch: '4.0.1' + + - name: linux-4.0 + os: ubuntu-20.04 + artifact-name: linux + godot-binary: godot.linuxbsd.editor.dev.x86_64 + godot-prebuilt-patch: '4.0' - name: linux-double os: ubuntu-20.04 @@ -180,6 +212,7 @@ jobs: artifact-name: linux godot-binary: godot.linuxbsd.editor.dev.x86_64 rust-extra-args: --features godot/custom-godot + godot-prebuilt-patch: '4.1' # Special Godot binaries compiled with AddressSanitizer/LeakSanitizer to detect UB/leaks. # Additionally, the Godot source is patched to make dlclose() a no-op, as unloading dynamic libraries loses stacktrace and @@ -202,6 +235,9 @@ jobs: rust-env-rustflags: -Zrandomize-layout + # TODO use godot: custom, or godot: 4.0.3, etc + + steps: - uses: actions/checkout@v3 @@ -211,11 +247,12 @@ jobs: artifact-name: godot-${{ matrix.artifact-name || matrix.name }} godot-binary: ${{ matrix.godot-binary }} godot-args: ${{ matrix.godot-args }} + godot-prebuilt-patch: ${{ matrix.godot-prebuilt-patch }} rust-extra-args: ${{ matrix.rust-extra-args }} rust-toolchain: ${{ matrix.rust-toolchain || 'stable' }} rust-env-rustflags: ${{ matrix.rust-env-rustflags }} with-llvm: ${{ matrix.with-llvm }} - godot-check-header: ${{ matrix.name == 'linux' }} + godot-check-header: ${{ matrix.godot-check-header }} license-guard: diff --git a/examples/dodge-the-creeps/godot/DodgeTheCreeps.gdextension b/examples/dodge-the-creeps/godot/DodgeTheCreeps.gdextension index 7bbaf48e8..ef1464641 100644 --- a/examples/dodge-the-creeps/godot/DodgeTheCreeps.gdextension +++ b/examples/dodge-the-creeps/godot/DodgeTheCreeps.gdextension @@ -1,6 +1,6 @@ [configuration] entry_symbol = "gdext_rust_init" -compatibility_minimum = 4.1 +compatibility_minimum = 4.0 [libraries] linux.debug.x86_64 = "res://../../../target/debug/libdodge_the_creeps.so" diff --git a/godot-bindings/Cargo.toml b/godot-bindings/Cargo.toml index 965b0e39a..4752cf75a 100644 --- a/godot-bindings/Cargo.toml +++ b/godot-bindings/Cargo.toml @@ -18,7 +18,7 @@ custom-godot = ["dep:bindgen", "dep:regex", "dep:which"] custom-godot-extheader = [] [dependencies] -godot4-prebuilt = { optional = true, git = "https://github.com/godot-rust/godot4-prebuilt", branch = "4.0.1" } +godot4-prebuilt = { optional = true, git = "https://github.com/godot-rust/godot4-prebuilt", branch = "4.0.3" } # Version >= 1.5.5 for security: https://blog.rust-lang.org/2022/03/08/cve-2022-24713.html # 'unicode-gencat' needed for \d, see: https://docs.rs/regex/1.5.5/regex/#unicode-features diff --git a/godot-bindings/res/tweak.patch b/godot-bindings/res/tweak.patch index 9aad26029..e8f46e553 100644 --- a/godot-bindings/res/tweak.patch +++ b/godot-bindings/res/tweak.patch @@ -1,32 +1,42 @@ diff --git b/godot-ffi/src/gen/gdextension_interface.h a/godot-ffi/src/gen/gdextension_interface.h -index 0b7615f..6db266e 100644 +index 4e4f300..e1cd5fb 100644 --- b/godot-ffi/src/gen/gdextension_interface.h +++ a/godot-ffi/src/gen/gdextension_interface.h -@@ -139,22 +139,22 @@ typedef enum { - - } GDExtensionVariantOperator; +@@ -155,27 +155,27 @@ typedef enum { + // - Some types have no destructor (see `extension_api.json`'s `has_destructor` field), for + // them it is always safe to skip the constructor for the return value if you are in a hurry ;-) -typedef void *GDExtensionVariantPtr; -typedef const void *GDExtensionConstVariantPtr; +-typedef void *GDExtensionUninitializedVariantPtr; -typedef void *GDExtensionStringNamePtr; -typedef const void *GDExtensionConstStringNamePtr; +-typedef void *GDExtensionUninitializedStringNamePtr; -typedef void *GDExtensionStringPtr; -typedef const void *GDExtensionConstStringPtr; +-typedef void *GDExtensionUninitializedStringPtr; -typedef void *GDExtensionObjectPtr; -typedef const void *GDExtensionConstObjectPtr; +-typedef void *GDExtensionUninitializedObjectPtr; -typedef void *GDExtensionTypePtr; -typedef const void *GDExtensionConstTypePtr; +-typedef void *GDExtensionUninitializedTypePtr; -typedef const void *GDExtensionMethodBindPtr; +typedef struct __GdextVariant *GDExtensionVariantPtr; +typedef const struct __GdextVariant *GDExtensionConstVariantPtr; ++typedef struct __GdextUninitializedVariant *GDExtensionUninitializedVariantPtr; +typedef struct __GdextStringName *GDExtensionStringNamePtr; +typedef const struct __GdextStringName *GDExtensionConstStringNamePtr; ++typedef struct __GdextUninitializedStringName *GDExtensionUninitializedStringNamePtr; +typedef struct __GdextString *GDExtensionStringPtr; +typedef const struct __GdextString *GDExtensionConstStringPtr; ++typedef struct __GdextUninitializedString *GDExtensionUninitializedStringPtr; +typedef struct __GdextObject *GDExtensionObjectPtr; +typedef const struct __GdextObject *GDExtensionConstObjectPtr; ++typedef struct __GdextUninitializedObject *GDExtensionUninitializedObjectPtr; +typedef struct __GdextType *GDExtensionTypePtr; +typedef const struct __GdextType *GDExtensionConstTypePtr; ++typedef struct __GdextUninitializedType *GDExtensionUninitializedTypePtr; +typedef const struct __GdextMethodBind *GDExtensionMethodBindPtr; typedef int64_t GDExtensionInt; typedef uint8_t GDExtensionBool; @@ -38,7 +48,22 @@ index 0b7615f..6db266e 100644 /* VARIANT DATA I/O */ -@@ -203,7 +203,7 @@ typedef struct { +@@ -195,11 +195,11 @@ typedef struct { + int32_t expected; + } GDExtensionCallError; + +-typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionVariantPtr, GDExtensionTypePtr); +-typedef void (*GDExtensionTypeFromVariantConstructorFunc)(GDExtensionTypePtr, GDExtensionVariantPtr); ++typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionUninitializedVariantPtr, GDExtensionTypePtr); ++typedef void (*GDExtensionTypeFromVariantConstructorFunc)(GDExtensionUninitializedTypePtr, GDExtensionVariantPtr); + typedef void (*GDExtensionPtrOperatorEvaluator)(GDExtensionConstTypePtr p_left, GDExtensionConstTypePtr p_right, GDExtensionTypePtr r_result); + typedef void (*GDExtensionPtrBuiltInMethod)(GDExtensionTypePtr p_base, const GDExtensionConstTypePtr *p_args, GDExtensionTypePtr r_return, int p_argument_count); +-typedef void (*GDExtensionPtrConstructor)(GDExtensionTypePtr p_base, const GDExtensionConstTypePtr *p_args); ++typedef void (*GDExtensionPtrConstructor)(GDExtensionUninitializedTypePtr p_base, const GDExtensionConstTypePtr *p_args); + typedef void (*GDExtensionPtrDestructor)(GDExtensionTypePtr p_base); + typedef void (*GDExtensionPtrSetter)(GDExtensionTypePtr p_base, GDExtensionConstTypePtr p_value); + typedef void (*GDExtensionPtrGetter)(GDExtensionConstTypePtr p_base, GDExtensionTypePtr r_value); +@@ -224,7 +224,7 @@ typedef struct { /* EXTENSION CLASSES */ @@ -47,7 +72,7 @@ index 0b7615f..6db266e 100644 typedef GDExtensionBool (*GDExtensionClassSet)(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_name, GDExtensionConstVariantPtr p_value); typedef GDExtensionBool (*GDExtensionClassGet)(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_name, GDExtensionVariantPtr r_ret); -@@ -266,7 +266,7 @@ typedef struct { +@@ -287,7 +287,7 @@ typedef struct { void *class_userdata; // Per-class user data, later accessible in instance bindings. } GDExtensionClassCreationInfo; @@ -56,7 +81,7 @@ index 0b7615f..6db266e 100644 /* Method */ -@@ -323,7 +323,7 @@ typedef struct { +@@ -345,7 +345,7 @@ typedef struct { /* SCRIPT INSTANCE EXTENSION */ @@ -65,7 +90,7 @@ index 0b7615f..6db266e 100644 typedef GDExtensionBool (*GDExtensionScriptInstanceSet)(GDExtensionScriptInstanceDataPtr p_instance, GDExtensionConstStringNamePtr p_name, GDExtensionConstVariantPtr p_value); typedef GDExtensionBool (*GDExtensionScriptInstanceGet)(GDExtensionScriptInstanceDataPtr p_instance, GDExtensionConstStringNamePtr p_name, GDExtensionVariantPtr r_ret); -@@ -353,13 +353,13 @@ typedef GDExtensionBool (*GDExtensionScriptInstanceRefCountDecremented)(GDExtens +@@ -375,13 +375,13 @@ typedef GDExtensionBool (*GDExtensionScriptInstanceRefCountDecremented)(GDExtens typedef GDExtensionObjectPtr (*GDExtensionScriptInstanceGetScript)(GDExtensionScriptInstanceDataPtr p_instance); typedef GDExtensionBool (*GDExtensionScriptInstanceIsPlaceholder)(GDExtensionScriptInstanceDataPtr p_instance); diff --git a/itest/godot/itest.gdextension b/itest/godot/itest.gdextension index a48361009..366028dda 100644 --- a/itest/godot/itest.gdextension +++ b/itest/godot/itest.gdextension @@ -1,6 +1,6 @@ [configuration] entry_symbol = "itest_init" -compatibility_minimum = 4.1 +compatibility_minimum = 4.0 [libraries] linux.debug.x86_64 = "res://../../target/debug/libitest.so" From fc4a405a2c4400a333c60d226cc5eacf1f008342 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Wed, 24 May 2023 16:00:06 +0200 Subject: [PATCH 12/13] Remove several memory leaks by constructing into uninitialized pointers --- godot-core/src/builtin/array.rs | 7 +--- godot-core/src/builtin/callable.rs | 6 +-- godot-core/src/builtin/macros.rs | 20 +++------- godot-core/src/builtin/string/godot_string.rs | 12 ++---- godot-core/src/builtin/string/node_path.rs | 6 +-- godot-core/src/builtin/string/string_name.rs | 7 +--- godot-core/src/builtin/variant/impls.rs | 7 +--- godot-core/src/builtin/variant/mod.rs | 38 ++----------------- 8 files changed, 24 insertions(+), 79 deletions(-) diff --git a/godot-core/src/builtin/array.rs b/godot-core/src/builtin/array.rs index a1de96be5..08ae18da5 100644 --- a/godot-core/src/builtin/array.rs +++ b/godot-core/src/builtin/array.rs @@ -716,13 +716,10 @@ impl FromVariant for Array { return Err(VariantConversionError); } - // TODO(uninit): can we use from_sys_init()? Godot uses placement-new for variant_to_type initialization. - use sys::AsUninit; - let array = unsafe { - Self::from_sys_init_default(|self_ptr| { + Self::from_sys_init(|self_ptr| { let array_from_variant = sys::builtin_fn!(array_from_variant); - array_from_variant(self_ptr.as_uninit(), variant.var_sys()); + array_from_variant(self_ptr, variant.var_sys()); }) }; diff --git a/godot-core/src/builtin/callable.rs b/godot-core/src/builtin/callable.rs index b84f0a86d..ed9e9e6ee 100644 --- a/godot-core/src/builtin/callable.rs +++ b/godot-core/src/builtin/callable.rs @@ -10,7 +10,6 @@ use crate::builtin::{inner, ToVariant, Variant}; use crate::engine::Object; use crate::obj::mem::Memory; use crate::obj::{Gd, GodotClass, InstanceId}; -use godot_ffi::AsUninit; use std::fmt; use sys::{ffi_methods, GodotFfi}; @@ -46,11 +45,10 @@ impl Callable { // upcast not needed let method = method_name.into(); unsafe { - // TODO(uninit): can we use from_sys_init()? Godot uses placement-new for variant_to_type initialization. - Self::from_sys_init_default(|self_ptr| { + Self::from_sys_init(|self_ptr| { let ctor = sys::builtin_fn!(callable_from_object_method); let args = [object.sys_const(), method.sys_const()]; - ctor(self_ptr.as_uninit(), args.as_ptr()); + ctor(self_ptr, args.as_ptr()); }) } } diff --git a/godot-core/src/builtin/macros.rs b/godot-core/src/builtin/macros.rs index 55b046e57..26e4214ce 100644 --- a/godot-core/src/builtin/macros.rs +++ b/godot-core/src/builtin/macros.rs @@ -11,17 +11,11 @@ macro_rules! impl_builtin_traits_inner { impl Default for $Type { #[inline] fn default() -> Self { - // TODO(uninit): use from_sys_init() - let mut uninit = std::mem::MaybeUninit::<$Type>::uninit(); - - use godot_ffi::AsUninit; unsafe { - let self_ptr = (*uninit.as_mut_ptr()).sys_mut().as_uninit(); - sys::builtin_call! { - $gd_method(self_ptr, std::ptr::null_mut()) - }; - - uninit.assume_init() + Self::from_sys_init(|self_ptr| { + let ctor = ::godot_ffi::builtin_fn!($gd_method); + ctor(self_ptr, std::ptr::null_mut()) + }) } } } @@ -31,13 +25,11 @@ macro_rules! impl_builtin_traits_inner { impl Clone for $Type { #[inline] fn clone(&self) -> Self { - // TODO(uninit) - check if we can use from_sys_init() - use godot_ffi::AsUninit; unsafe { - Self::from_sys_init_default(|self_ptr| { + Self::from_sys_init(|self_ptr| { let ctor = ::godot_ffi::builtin_fn!($gd_method); let args = [self.sys_const()]; - ctor(self_ptr.as_uninit(), args.as_ptr()); + ctor(self_ptr, args.as_ptr()); }) } } diff --git a/godot-core/src/builtin/string/godot_string.rs b/godot-core/src/builtin/string/godot_string.rs index 605ec42b5..85f8f50e6 100644 --- a/godot-core/src/builtin/string/godot_string.rs +++ b/godot-core/src/builtin/string/godot_string.rs @@ -222,13 +222,11 @@ impl FromStr for GodotString { impl From<&StringName> for GodotString { fn from(string: &StringName) -> Self { - // TODO(uninit) - see if we can use from_sys_init() - use godot_ffi::AsUninit; unsafe { - Self::from_sys_init_default(|self_ptr| { + Self::from_sys_init(|self_ptr| { let ctor = sys::builtin_fn!(string_from_string_name); let args = [string.sys_const()]; - ctor(self_ptr.as_uninit(), args.as_ptr()); + ctor(self_ptr, args.as_ptr()); }) } } @@ -245,13 +243,11 @@ impl From for GodotString { impl From<&NodePath> for GodotString { fn from(path: &NodePath) -> Self { - // TODO(uninit) - see if we can use from_sys_init() - use godot_ffi::AsUninit; unsafe { - Self::from_sys_init_default(|self_ptr| { + Self::from_sys_init(|self_ptr| { let ctor = sys::builtin_fn!(string_from_node_path); let args = [path.sys_const()]; - ctor(self_ptr.as_uninit(), args.as_ptr()); + 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 e0cf4b702..ff4c7ac88 100644 --- a/godot-core/src/builtin/string/node_path.rs +++ b/godot-core/src/builtin/string/node_path.rs @@ -100,13 +100,11 @@ impl_rust_string_conv!(NodePath); impl From<&GodotString> for NodePath { fn from(string: &GodotString) -> Self { - // TODO(uninit) - see if we can use from_sys_init() - use godot_ffi::AsUninit; unsafe { - Self::from_sys_init_default(|self_ptr| { + Self::from_sys_init(|self_ptr| { let ctor = sys::builtin_fn!(node_path_from_string); let args = [string.sys_const()]; - ctor(self_ptr.as_uninit(), args.as_ptr()); + 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 745a9aa14..b35b467a8 100644 --- a/godot-core/src/builtin/string/string_name.rs +++ b/godot-core/src/builtin/string/string_name.rs @@ -121,7 +121,6 @@ impl fmt::Debug for StringName { write!(f, "&\"{string}\"") } } - // ---------------------------------------------------------------------------------------------------------------------------------------------- // Conversion from/into other string-types @@ -129,13 +128,11 @@ impl_rust_string_conv!(StringName); impl From<&GodotString> for StringName { fn from(string: &GodotString) -> Self { - // TODO(uninit) - see if we can use from_sys_init() - use godot_ffi::AsUninit; unsafe { - Self::from_sys_init_default(|self_ptr| { + Self::from_sys_init(|self_ptr| { let ctor = sys::builtin_fn!(string_name_from_string); let args = [string.sys_const()]; - ctor(self_ptr.as_uninit(), args.as_ptr()); + 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 a31048e63..4118d2450 100644 --- a/godot-core/src/builtin/variant/impls.rs +++ b/godot-core/src/builtin/variant/impls.rs @@ -61,18 +61,15 @@ macro_rules! impl_variant_traits { return Err(VariantConversionError) } - // TODO(uninit) - see if we can use from_sys_init() - use ::godot_ffi::AsUninit; - // 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(). let result = unsafe { - Self::from_sys_init_default(|self_ptr| { + Self::from_sys_init(|self_ptr| { let converter = sys::builtin_fn!($to_fn); - converter(self_ptr.as_uninit(), variant.var_sys()); + converter(self_ptr, variant.var_sys()); }) }; diff --git a/godot-core/src/builtin/variant/mod.rs b/godot-core/src/builtin/variant/mod.rs index 095842e3c..ae57fc25d 100644 --- a/godot-core/src/builtin/variant/mod.rs +++ b/godot-core/src/builtin/variant/mod.rs @@ -108,23 +108,7 @@ impl Variant { let args_sys: Vec<_> = args.iter().map(|v| v.var_sys_const()).collect(); let mut error = sys::default_call_error(); - #[allow(unused_mut)] - let mut result = Variant::nil(); - - use sys::AsUninit; - unsafe { - interface_fn!(variant_call)( - self.var_sys(), - method.string_sys(), - args_sys.as_ptr(), - args_sys.len() as i64, - result.var_sys().as_uninit(), - ptr::addr_of_mut!(error), - ) - }; - - // TODO(uninit) - /*let result = unsafe { + let result = unsafe { Variant::from_var_sys_init(|variant_ptr| { interface_fn!(variant_call)( self.var_sys(), @@ -135,7 +119,7 @@ impl Variant { ptr::addr_of_mut!(error), ) }) - };*/ + }; if error.error != sys::GDEXTENSION_CALL_OK { let arg_types: Vec<_> = args.iter().map(Variant::get_type).collect(); @@ -148,21 +132,7 @@ impl Variant { let op_sys = op.sys(); let mut is_valid = false as u8; - use sys::AsUninit; - #[allow(unused_mut)] - let mut result = Variant::nil(); - unsafe { - interface_fn!(variant_evaluate)( - op_sys, - self.var_sys(), - rhs.var_sys(), - result.var_sys().as_uninit(), - ptr::addr_of_mut!(is_valid), - ) - }; - - // TODO(uninit) - /*let mut result = unsafe { + let result = unsafe { Variant::from_var_sys_init(|variant_ptr| { interface_fn!(variant_evaluate)( op_sys, @@ -172,7 +142,7 @@ impl Variant { ptr::addr_of_mut!(is_valid), ) }) - };*/ + }; if is_valid == 1 { Some(result) From 974085c1e4f7d8f8f413b89cbf9f78fde4953135 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Wed, 24 May 2023 19:17:59 +0200 Subject: [PATCH 13/13] CI: memcheck jobs for both 4.0.3 and nightly --- .github/workflows/full-ci.yml | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/.github/workflows/full-ci.yml b/.github/workflows/full-ci.yml index f6ac2860a..6e212c685 100644 --- a/.github/workflows/full-ci.yml +++ b/.github/workflows/full-ci.yml @@ -220,22 +220,41 @@ jobs: # The gcc version can possibly be removed later, as it is slower and needs a larger artifact than the clang one. # --disallow-focus: fail if #[itest(focus)] is encountered, to prevent running only a few tests for full CI - - name: linux-memcheck-gcc + - name: linux-memcheck-gcc-4.0.3 os: ubuntu-20.04 + artifact-name: linux-memcheck-gcc godot-binary: godot.linuxbsd.editor.dev.x86_64.san godot-args: -- --disallow-focus rust-toolchain: nightly rust-env-rustflags: -Zrandomize-layout - - name: linux-memcheck-clang + - name: linux-memcheck-clang-4.0.3 os: ubuntu-20.04 + artifact-name: linux-memcheck-clang godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san godot-args: -- --disallow-focus rust-toolchain: nightly rust-env-rustflags: -Zrandomize-layout + - name: linux-memcheck-gcc-nightly + os: ubuntu-20.04 + artifact-name: linux-memcheck-gcc + godot-binary: godot.linuxbsd.editor.dev.x86_64.san + godot-args: -- --disallow-focus + rust-toolchain: nightly + rust-env-rustflags: -Zrandomize-layout + rust-extra-args: --features godot/custom-godot + godot-prebuilt-patch: '4.1' - # TODO use godot: custom, or godot: 4.0.3, etc + - name: linux-memcheck-clang-nightly + os: ubuntu-20.04 + artifact-name: linux-memcheck-clang + godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san + godot-args: -- --disallow-focus + rust-toolchain: nightly + rust-env-rustflags: -Zrandomize-layout + rust-extra-args: --features godot/custom-godot + godot-prebuilt-patch: '4.1' steps: