From ac8b4708a371d9dfdf5dedcf2120433d6f03cce7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20J=2E=20Est=C3=A9banez?= Date: Mon, 27 Apr 2020 13:07:52 +0200 Subject: [PATCH 1/2] Fix GDNative compat breakage due to dangling Variants fix This moves the instance id member from Variant to the ObjectRC so that Variant is still the same size as before the fix (and also regardless if debug or release build). --- core/object_rc.h | 8 +++++++- core/variant.cpp | 7 ++----- core/variant.h | 6 ------ core/variant_call.cpp | 4 ++-- core/variant_op.cpp | 18 +++++++++--------- 5 files changed, 20 insertions(+), 23 deletions(-) diff --git a/core/object_rc.h b/core/object_rc.h index 4ac576f08cc..1a713d768ef 100644 --- a/core/object_rc.h +++ b/core/object_rc.h @@ -46,6 +46,11 @@ class ObjectRC { std::atomic _users; public: + // This is for allowing debug builds to check for instance ID validity, + // so warnings are shown in debug builds when a stray Variant (one pointing + // to a released Object) would have happened. + const ObjectID instance_id; + _FORCE_INLINE_ void increment() { _users.fetch_add(1, std::memory_order_relaxed); } @@ -63,7 +68,8 @@ public: return _ptr.load(std::memory_order_acquire); } - _FORCE_INLINE_ ObjectRC(Object *p_object) { + _FORCE_INLINE_ ObjectRC(Object *p_object) : + instance_id(p_object->get_instance_id()) { // 1 (the Object) + 1 (the first user) _users.store(2, std::memory_order_relaxed); _ptr.store(p_object, std::memory_order_release); diff --git a/core/variant.cpp b/core/variant.cpp index 875b9029359..592296d91b8 100644 --- a/core/variant.cpp +++ b/core/variant.cpp @@ -1125,7 +1125,6 @@ void Variant::clear() { if (_get_obj().rc->decrement()) { memfree(_get_obj().rc); } - _get_obj().instance_id = 0; } else { _get_obj().ref.unref(); } @@ -1602,7 +1601,7 @@ String Variant::stringify(List &stack) const { return obj->to_string(); } else { #ifdef DEBUG_ENABLED - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { return "[Deleted Object]"; } #endif @@ -1765,7 +1764,7 @@ Variant::operator RID() const { #ifdef DEBUG_ENABLED Object *obj = _get_obj().rc->get_ptr(); if (unlikely(!obj)) { - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted get RID on a deleted object."); } } @@ -2318,7 +2317,6 @@ Variant::Variant(const RefPtr &p_resource) { memnew_placement(_data._mem, ObjData); #ifdef DEBUG_ENABLED _get_obj().rc = NULL; - _get_obj().instance_id = 0; #else REF *ref = reinterpret_cast(p_resource.get_data()); _get_obj().obj = ref->ptr(); @@ -2339,7 +2337,6 @@ Variant::Variant(const Object *p_object) { memnew_placement(_data._mem, ObjData); #ifdef DEBUG_ENABLED _get_obj().rc = p_object ? const_cast(p_object)->_use_rc() : NULL; - _get_obj().instance_id = p_object ? p_object->get_instance_id() : 0; #else _get_obj().obj = const_cast(p_object); #endif diff --git a/core/variant.h b/core/variant.h index c430797dbe7..42d67e86723 100644 --- a/core/variant.h +++ b/core/variant.h @@ -139,12 +139,6 @@ private: // Will be null for every type deriving from Reference as they have their // own reference count mechanism ObjectRC *rc; - // This is for allowing debug build to check for instance ID validity, - // so warnings are shown in debug builds when a stray Variant (one pointing - // to a released Object) would have happened. - // If it's zero, that means the Variant is has a legit null object value, - // thus not needing instance id validation. - ObjectID instance_id; #else Object *obj; #endif diff --git a/core/variant_call.cpp b/core/variant_call.cpp index 9f1e4bc4dc2..074b93d9102 100644 --- a/core/variant_call.cpp +++ b/core/variant_call.cpp @@ -1098,7 +1098,7 @@ void Variant::call_ptr(const StringName &p_method, const Variant **p_args, int p Object *obj = _OBJ_PTR(*this); if (!obj) { #ifdef DEBUG_ENABLED - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted call on stray pointer object."); } #endif @@ -1275,7 +1275,7 @@ bool Variant::has_method(const StringName &p_method) const { Object *obj = _OBJ_PTR(*this); if (!obj) { #ifdef DEBUG_ENABLED - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted method check on stray pointer object."); } #endif diff --git a/core/variant_op.cpp b/core/variant_op.cpp index d665c73eb05..b051b0a6c25 100644 --- a/core/variant_op.cpp +++ b/core/variant_op.cpp @@ -1516,7 +1516,7 @@ void Variant::set_named(const StringName &p_index, const Variant &p_value, bool Object *obj = _OBJ_PTR(*this); #ifdef DEBUG_ENABLED if (unlikely(!obj)) { - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted set on a deleted object."); } break; @@ -1684,7 +1684,7 @@ Variant Variant::get_named(const StringName &p_index, bool *r_valid) const { if (unlikely(!obj)) { if (r_valid) *r_valid = false; - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted get on a deleted object."); } return Variant(); @@ -2169,7 +2169,7 @@ void Variant::set(const Variant &p_index, const Variant &p_value, bool *r_valid) if (unlikely(!obj)) { #ifdef DEBUG_ENABLED valid = false; - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted set on a deleted object."); } #endif @@ -2539,7 +2539,7 @@ Variant Variant::get(const Variant &p_index, bool *r_valid) const { if (unlikely(!obj)) { #ifdef DEBUG_ENABLED valid = false; - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted get on a deleted object."); } #endif @@ -2602,7 +2602,7 @@ bool Variant::in(const Variant &p_index, bool *r_valid) const { if (r_valid) { *r_valid = false; } - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted 'in' on a deleted object."); } #endif @@ -2865,7 +2865,7 @@ void Variant::get_property_list(List *p_list) const { Object *obj = _OBJ_PTR(*this); if (unlikely(!obj)) { #ifdef DEBUG_ENABLED - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted get property list on a deleted object."); } #endif @@ -2943,7 +2943,7 @@ bool Variant::iter_init(Variant &r_iter, bool &valid) const { #ifdef DEBUG_ENABLED if (unlikely(!obj)) { valid = false; - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted iteration start on a deleted object."); } return false; @@ -3110,7 +3110,7 @@ bool Variant::iter_next(Variant &r_iter, bool &valid) const { #ifdef DEBUG_ENABLED if (unlikely(!obj)) { valid = false; - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted iteration check next on a deleted object."); } return false; @@ -3268,7 +3268,7 @@ Variant Variant::iter_get(const Variant &r_iter, bool &r_valid) const { #ifdef DEBUG_ENABLED if (unlikely(!obj)) { r_valid = false; - if (ScriptDebugger::get_singleton() && _get_obj().instance_id != 0 && !ObjectDB::get_instance(_get_obj().instance_id)) { + if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { WARN_PRINT("Attempted iteration get next on a deleted object."); } return Variant(); From 3bdc0913a524f257f7fe9081b926b5920f29d42c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20J=2E=20Est=C3=A9banez?= Date: Mon, 27 Apr 2020 13:15:52 +0200 Subject: [PATCH 2/2] Make wording of all Variant warnings consistent --- core/variant_call.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/variant_call.cpp b/core/variant_call.cpp index 074b93d9102..9b5a638a326 100644 --- a/core/variant_call.cpp +++ b/core/variant_call.cpp @@ -1099,7 +1099,7 @@ void Variant::call_ptr(const StringName &p_method, const Variant **p_args, int p if (!obj) { #ifdef DEBUG_ENABLED if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { - WARN_PRINT("Attempted call on stray pointer object."); + WARN_PRINT("Attempted call on a deleted object."); } #endif r_error.error = CallError::CALL_ERROR_INSTANCE_IS_NULL; @@ -1276,7 +1276,7 @@ bool Variant::has_method(const StringName &p_method) const { if (!obj) { #ifdef DEBUG_ENABLED if (ScriptDebugger::get_singleton() && _get_obj().rc && !ObjectDB::get_instance(_get_obj().rc->instance_id)) { - WARN_PRINT("Attempted method check on stray pointer object."); + WARN_PRINT("Attempted method check on a deleted object."); } #endif return false;