From ce637358fca7ca3b92084b4069e378124da991a9 Mon Sep 17 00:00:00 2001 From: David Snopek Date: Tue, 7 May 2024 12:54:35 -0500 Subject: [PATCH] Clean up instance bindings for engine singletons to prevent crash --- binding_generator.py | 24 +++++++++++++++++++++++- include/godot_cpp/core/class_db.hpp | 18 ++++++++++++++++++ include/godot_cpp/godot.hpp | 1 + src/core/class_db.cpp | 18 ++++++++++++++++++ src/godot.cpp | 2 ++ test/project/main.gd | 3 +++ test/src/example.cpp | 7 +++++++ test/src/example.h | 2 ++ 8 files changed, 74 insertions(+), 1 deletion(-) diff --git a/binding_generator.py b/binding_generator.py index 972cbf95b8..dfef1618c4 100644 --- a/binding_generator.py +++ b/binding_generator.py @@ -1504,6 +1504,10 @@ def generate_engine_class_header(class_api, used_classes, fully_used_classes, us result.append(f"\tGDEXTENSION_CLASS({class_name}, {inherits})") result.append("") + if is_singleton: + result.append(f"\tstatic {class_name} *singleton;") + result.append("") + result.append("public:") result.append("") @@ -1584,6 +1588,11 @@ def generate_engine_class_header(class_api, used_classes, fully_used_classes, us result.append("\t}") result.append("") + + if is_singleton: + result.append(f"\t~{class_name}();") + result.append("") + result.append("public:") # Special cases. @@ -1733,6 +1742,7 @@ def generate_engine_class_source(class_api, used_classes, fully_used_classes, us result.append(f"#include ") result.append("") + result.append("#include ") result.append("#include ") result.append("#include ") result.append("") @@ -1747,9 +1757,10 @@ def generate_engine_class_source(class_api, used_classes, fully_used_classes, us result.append("") if is_singleton: + result.append(f"{class_name} *{class_name}::singleton = nullptr;") + result.append("") result.append(f"{class_name} *{class_name}::get_singleton() {{") # We assume multi-threaded access is OK because each assignment will assign the same value every time - result.append(f"\tstatic {class_name} *singleton = nullptr;") result.append("\tif (unlikely(singleton == nullptr)) {") result.append( f"\t\tGDExtensionObjectPtr singleton_obj = internal::gdextension_interface_global_get_singleton({class_name}::get_class_static()._native_ptr());" @@ -1763,11 +1774,22 @@ def generate_engine_class_source(class_api, used_classes, fully_used_classes, us result.append("#ifdef DEBUG_ENABLED") result.append("\t\tERR_FAIL_NULL_V(singleton, nullptr);") result.append("#endif // DEBUG_ENABLED") + result.append("\t\tif (likely(singleton)) {") + result.append(f"\t\t\tClassDB::_register_engine_singleton({class_name}::get_class_static(), singleton);") + result.append("\t\t}") result.append("\t}") result.append("\treturn singleton;") result.append("}") result.append("") + result.append(f"{class_name}::~{class_name}() {{") + result.append("\tif (singleton == this) {") + result.append(f"\t\tClassDB::_unregister_engine_singleton({class_name}::get_class_static());") + result.append("\t\tsingleton = nullptr;") + result.append("\t}") + result.append("}") + result.append("") + if "methods" in class_api: for method in class_api["methods"]: if method["is_virtual"]: diff --git a/include/godot_cpp/core/class_db.hpp b/include/godot_cpp/core/class_db.hpp index 31ebddc75f..b02f227470 100644 --- a/include/godot_cpp/core/class_db.hpp +++ b/include/godot_cpp/core/class_db.hpp @@ -49,6 +49,7 @@ #include #include #include +#include // Needed to use StringName as key in `std::unordered_map` template <> @@ -104,6 +105,8 @@ class ClassDB { static std::unordered_map instance_binding_callbacks; // Used to remember the custom class registration order. static std::vector class_register_order; + static std::unordered_map engine_singletons; + static std::mutex engine_singletons_mutex; static MethodBind *bind_methodfi(uint32_t p_flags, MethodBind *p_bind, const MethodDefinition &method_name, const void **p_defs, int p_defcount); static void initialize_class(const ClassInfo &cl); @@ -153,6 +156,21 @@ class ClassDB { instance_binding_callbacks[p_name] = p_callbacks; } + static void _register_engine_singleton(const StringName &p_class_name, Object *p_singleton) { + std::lock_guard lock(engine_singletons_mutex); + std::unordered_map::const_iterator i = engine_singletons.find(p_class_name); + if (i != engine_singletons.end()) { + ERR_FAIL_COND((*i).second != p_singleton); + return; + } + engine_singletons[p_class_name] = p_singleton; + } + + static void _unregister_engine_singleton(const StringName &p_class_name) { + std::lock_guard lock(engine_singletons_mutex); + engine_singletons.erase(p_class_name); + } + template static MethodBind *bind_method(N p_method_name, M p_method, VarArgs... p_args); diff --git a/include/godot_cpp/godot.hpp b/include/godot_cpp/godot.hpp index 5a6293027e..11877458b7 100644 --- a/include/godot_cpp/godot.hpp +++ b/include/godot_cpp/godot.hpp @@ -160,6 +160,7 @@ extern "C" GDExtensionInterfaceObjectDestroy gdextension_interface_object_destro extern "C" GDExtensionInterfaceGlobalGetSingleton gdextension_interface_global_get_singleton; extern "C" GDExtensionInterfaceObjectGetInstanceBinding gdextension_interface_object_get_instance_binding; extern "C" GDExtensionInterfaceObjectSetInstanceBinding gdextension_interface_object_set_instance_binding; +extern "C" GDExtensionInterfaceObjectFreeInstanceBinding gdextension_interface_object_free_instance_binding; extern "C" GDExtensionInterfaceObjectSetInstance gdextension_interface_object_set_instance; extern "C" GDExtensionInterfaceObjectGetClassName gdextension_interface_object_get_class_name; extern "C" GDExtensionInterfaceObjectCastTo gdextension_interface_object_cast_to; diff --git a/src/core/class_db.cpp b/src/core/class_db.cpp index acead8bfa4..d9a8bf9865 100644 --- a/src/core/class_db.cpp +++ b/src/core/class_db.cpp @@ -43,6 +43,8 @@ namespace godot { std::unordered_map ClassDB::classes; std::unordered_map ClassDB::instance_binding_callbacks; std::vector ClassDB::class_register_order; +std::unordered_map ClassDB::engine_singletons; +std::mutex ClassDB::engine_singletons_mutex; GDExtensionInitializationLevel ClassDB::current_level = GDEXTENSION_INITIALIZATION_CORE; MethodDefinition D_METHOD(StringName p_name) { @@ -419,6 +421,22 @@ void ClassDB::deinitialize(GDExtensionInitializationLevel p_level) { }); class_register_order.erase(it, class_register_order.end()); } + + if (p_level == GDEXTENSION_INITIALIZATION_CORE) { + // Make a new list of the singleton objects, since freeing the instance bindings will lead to + // elements getting removed from engine_singletons. + std::vector singleton_objects; + { + std::lock_guard lock(engine_singletons_mutex); + singleton_objects.reserve(engine_singletons.size()); + for (const std::pair &pair : engine_singletons) { + singleton_objects.push_back(pair.second); + } + } + for (std::vector::iterator i = singleton_objects.begin(); i != singleton_objects.end(); i++) { + internal::gdextension_interface_object_free_instance_binding((*i)->_owner, internal::token); + } + } } } // namespace godot diff --git a/src/godot.cpp b/src/godot.cpp index 8a031be23b..68d682fbe4 100644 --- a/src/godot.cpp +++ b/src/godot.cpp @@ -166,6 +166,7 @@ GDExtensionInterfaceObjectDestroy gdextension_interface_object_destroy = nullptr GDExtensionInterfaceGlobalGetSingleton gdextension_interface_global_get_singleton = nullptr; GDExtensionInterfaceObjectGetInstanceBinding gdextension_interface_object_get_instance_binding = nullptr; GDExtensionInterfaceObjectSetInstanceBinding gdextension_interface_object_set_instance_binding = nullptr; +GDExtensionInterfaceObjectFreeInstanceBinding gdextension_interface_object_free_instance_binding = nullptr; GDExtensionInterfaceObjectSetInstance gdextension_interface_object_set_instance = nullptr; GDExtensionInterfaceObjectGetClassName gdextension_interface_object_get_class_name = nullptr; GDExtensionInterfaceObjectCastTo gdextension_interface_object_cast_to = nullptr; @@ -406,6 +407,7 @@ GDExtensionBool GDExtensionBinding::init(GDExtensionInterfaceGetProcAddress p_ge LOAD_PROC_ADDRESS(global_get_singleton, GDExtensionInterfaceGlobalGetSingleton); LOAD_PROC_ADDRESS(object_get_instance_binding, GDExtensionInterfaceObjectGetInstanceBinding); LOAD_PROC_ADDRESS(object_set_instance_binding, GDExtensionInterfaceObjectSetInstanceBinding); + LOAD_PROC_ADDRESS(object_free_instance_binding, GDExtensionInterfaceObjectFreeInstanceBinding); LOAD_PROC_ADDRESS(object_set_instance, GDExtensionInterfaceObjectSetInstance); LOAD_PROC_ADDRESS(object_get_class_name, GDExtensionInterfaceObjectGetClassName); LOAD_PROC_ADDRESS(object_cast_to, GDExtensionInterfaceObjectCastTo); diff --git a/test/project/main.gd b/test/project/main.gd index 0cfba196fa..665582fd5c 100644 --- a/test/project/main.gd +++ b/test/project/main.gd @@ -256,6 +256,9 @@ func _ready(): assert_equal(example.test_virtual_implemented_in_script("Virtual", 939), "Implemented") assert_equal(custom_signal_emitted, ["Virtual", 939]) + # Test that we can access an engine singleton. + assert_equal(example.test_use_engine_singleton(), OS.get_name()) + # Test that notifications happen on both parent and child classes. var example_child = $ExampleChild assert_equal(example_child.get_value1(), 11) diff --git a/test/src/example.cpp b/test/src/example.cpp index 3ec8bca076..78d706225f 100644 --- a/test/src/example.cpp +++ b/test/src/example.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include using namespace godot; @@ -239,6 +240,8 @@ void Example::_bind_methods() { GDVIRTUAL_BIND(_do_something_virtual, "name", "value"); ClassDB::bind_method(D_METHOD("test_virtual_implemented_in_script"), &Example::test_virtual_implemented_in_script); + ClassDB::bind_method(D_METHOD("test_use_engine_singleton"), &Example::test_use_engine_singleton); + ClassDB::bind_static_method("Example", D_METHOD("test_static", "a", "b"), &Example::test_static); ClassDB::bind_static_method("Example", D_METHOD("test_static2"), &Example::test_static2); @@ -671,6 +674,10 @@ String Example::test_virtual_implemented_in_script(const String &p_name, int p_v return "Unimplemented"; } +String Example::test_use_engine_singleton() const { + return OS::get_singleton()->get_name(); +} + void ExampleRuntime::_bind_methods() { ClassDB::bind_method(D_METHOD("set_prop_value", "value"), &ExampleRuntime::set_prop_value); ClassDB::bind_method(D_METHOD("get_prop_value"), &ExampleRuntime::get_prop_value); diff --git a/test/src/example.h b/test/src/example.h index 0ad949328a..1af4e5faaa 100644 --- a/test/src/example.h +++ b/test/src/example.h @@ -186,6 +186,8 @@ class Example : public Control { GDVIRTUAL2R(String, _do_something_virtual, String, int); String test_virtual_implemented_in_script(const String &p_name, int p_value); + + String test_use_engine_singleton() const; }; VARIANT_ENUM_CAST(Example::Constants);