Merge pull request #74101 from RandomShaper/fix_gds_obj_temps

Fix edge cases of object lifetime when signals involved

4 files changed, 73 insertions, 52 deletions

diff --git a/modules/gdscript/gdscript_byte_codegen.cpp b/modules/gdscript/gdscript_byte_codegen.cpp
index d6f21d297a..1414075ba8 100644
--- a/modules/gdscript/gdscript_byte_codegen.cpp
+++ b/modules/gdscript/gdscript_byte_codegen.cpp
@@ -69,56 +69,52 @@ uint32_t GDScriptByteCodeGenerator::add_or_get_name(const StringName &p_name) {
 
 uint32_t GDScriptByteCodeGenerator::add_temporary(const GDScriptDataType &p_type) {
 	Variant::Type temp_type = Variant::NIL;
-	if (p_type.has_type) {
-		if (p_type.kind == GDScriptDataType::BUILTIN) {
-			switch (p_type.builtin_type) {
-				case Variant::NIL:
-				case Variant::BOOL:
-				case Variant::INT:
-				case Variant::FLOAT:
-				case Variant::STRING:
-				case Variant::VECTOR2:
-				case Variant::VECTOR2I:
-				case Variant::RECT2:
-				case Variant::RECT2I:
-				case Variant::VECTOR3:
-				case Variant::VECTOR3I:
-				case Variant::TRANSFORM2D:
-				case Variant::VECTOR4:
-				case Variant::VECTOR4I:
-				case Variant::PLANE:
-				case Variant::QUATERNION:
-				case Variant::AABB:
-				case Variant::BASIS:
-				case Variant::TRANSFORM3D:
-				case Variant::PROJECTION:
-				case Variant::COLOR:
-				case Variant::STRING_NAME:
-				case Variant::NODE_PATH:
-				case Variant::RID:
-				case Variant::OBJECT:
-				case Variant::CALLABLE:
-				case Variant::SIGNAL:
-				case Variant::DICTIONARY:
-				case Variant::ARRAY:
-					temp_type = p_type.builtin_type;
-					break;
-				case Variant::PACKED_BYTE_ARRAY:
-				case Variant::PACKED_INT32_ARRAY:
-				case Variant::PACKED_INT64_ARRAY:
-				case Variant::PACKED_FLOAT32_ARRAY:
-				case Variant::PACKED_FLOAT64_ARRAY:
-				case Variant::PACKED_STRING_ARRAY:
-				case Variant::PACKED_VECTOR2_ARRAY:
-				case Variant::PACKED_VECTOR3_ARRAY:
-				case Variant::PACKED_COLOR_ARRAY:
-				case Variant::VARIANT_MAX:
-					// Packed arrays are reference counted, so we don't use the pool for them.
-					temp_type = Variant::NIL;
-					break;
-			}
-		} else {
-			temp_type = Variant::OBJECT;
+	if (p_type.has_type && p_type.kind == GDScriptDataType::BUILTIN) {
+		switch (p_type.builtin_type) {
+			case Variant::NIL:
+			case Variant::BOOL:
+			case Variant::INT:
+			case Variant::FLOAT:
+			case Variant::STRING:
+			case Variant::VECTOR2:
+			case Variant::VECTOR2I:
+			case Variant::RECT2:
+			case Variant::RECT2I:
+			case Variant::VECTOR3:
+			case Variant::VECTOR3I:
+			case Variant::TRANSFORM2D:
+			case Variant::VECTOR4:
+			case Variant::VECTOR4I:
+			case Variant::PLANE:
+			case Variant::QUATERNION:
+			case Variant::AABB:
+			case Variant::BASIS:
+			case Variant::TRANSFORM3D:
+			case Variant::PROJECTION:
+			case Variant::COLOR:
+			case Variant::STRING_NAME:
+			case Variant::NODE_PATH:
+			case Variant::RID:
+			case Variant::CALLABLE:
+			case Variant::SIGNAL:
+				temp_type = p_type.builtin_type;
+				break;
+			case Variant::OBJECT:
+			case Variant::DICTIONARY:
+			case Variant::ARRAY:
+			case Variant::PACKED_BYTE_ARRAY:
+			case Variant::PACKED_INT32_ARRAY:
+			case Variant::PACKED_INT64_ARRAY:
+			case Variant::PACKED_FLOAT32_ARRAY:
+			case Variant::PACKED_FLOAT64_ARRAY:
+			case Variant::PACKED_STRING_ARRAY:
+			case Variant::PACKED_VECTOR2_ARRAY:
+			case Variant::PACKED_VECTOR3_ARRAY:
+			case Variant::PACKED_COLOR_ARRAY:
+			case Variant::VARIANT_MAX:
+				// Arrays, dictionaries, and objects are reference counted, so we don't use the pool for them.
+				temp_type = Variant::NIL;
+				break;
 		}
 	}
 
@@ -143,10 +139,12 @@ void GDScriptByteCodeGenerator::pop_temporary() {
 	ERR_FAIL_COND(used_temporaries.is_empty());
 	int slot_idx = used_temporaries.back()->get();
 	const StackSlot &slot = temporaries[slot_idx];
-	if (slot.type == Variant::OBJECT) {
+	if (slot.type == Variant::NIL) {
 		// Avoid keeping in the stack long-lived references to objects,
 		// which may prevent RefCounted objects from being freed.
-		write_assign_false(Address(Address::TEMPORARY, slot_idx));
+		// However, the cleanup will be performed an the end of the
+		// statement, to allow object references to survive chaining.
+		temporaries_pending_clear.push_back(slot_idx);
 	}
 	temporaries_pool[slot.type].push_back(slot_idx);
 	used_temporaries.pop_back();
@@ -1756,6 +1754,23 @@ void GDScriptByteCodeGenerator::end_block() {
 	pop_stack_identifiers();
 }
 
+void GDScriptByteCodeGenerator::clean_temporaries() {
+	List<int>::Element *E = temporaries_pending_clear.front();
+	while (E) {
+		// The temporary may have been re-used as something else than an object
+		// since it was added to the list. In that case, there's no need to clear it.
+		int slot_idx = E->get();
+		const StackSlot &slot = temporaries[slot_idx];
+		if (slot.type == Variant::NIL) {
+			write_assign_false(Address(Address::TEMPORARY, slot_idx));
+		}
+
+		List<int>::Element *next = E->next();
+		E->erase();
+		E = next;
+	}
+}
+
 GDScriptByteCodeGenerator::~GDScriptByteCodeGenerator() {
 	if (!ended && function != nullptr) {
 		memdelete(function);
diff --git a/modules/gdscript/gdscript_byte_codegen.h b/modules/gdscript/gdscript_byte_codegen.h
index dc05de9fc6..42c6f80455 100644
--- a/modules/gdscript/gdscript_byte_codegen.h
+++ b/modules/gdscript/gdscript_byte_codegen.h
@@ -88,6 +88,7 @@ class GDScriptByteCodeGenerator : public GDScriptCodeGenerator {
 	Vector<StackSlot> locals;
 	Vector<StackSlot> temporaries;
 	List<int> used_temporaries;
+	List<int> temporaries_pending_clear;
 	RBMap<Variant::Type, List<int>> temporaries_pool;
 
 	List<GDScriptFunction::StackDebug> stack_debug;
@@ -463,6 +464,7 @@ public:
 	virtual uint32_t add_or_get_name(const StringName &p_name) override;
 	virtual uint32_t add_temporary(const GDScriptDataType &p_type) override;
 	virtual void pop_temporary() override;
+	virtual void clean_temporaries() override;
 
 	virtual void start_parameters() override;
 	virtual void end_parameters() override;
diff --git a/modules/gdscript/gdscript_codegen.h b/modules/gdscript/gdscript_codegen.h
index 7847ab28c7..e82b4b08ab 100644
--- a/modules/gdscript/gdscript_codegen.h
+++ b/modules/gdscript/gdscript_codegen.h
@@ -72,6 +72,7 @@ public:
 	virtual uint32_t add_or_get_name(const StringName &p_name) = 0;
 	virtual uint32_t add_temporary(const GDScriptDataType &p_type) = 0;
 	virtual void pop_temporary() = 0;
+	virtual void clean_temporaries() = 0;
 
 	virtual void start_parameters() = 0;
 	virtual void end_parameters() = 0;
diff --git a/modules/gdscript/gdscript_compiler.cpp b/modules/gdscript/gdscript_compiler.cpp
index 5413eadf60..9e5c83d08c 100644
--- a/modules/gdscript/gdscript_compiler.cpp
+++ b/modules/gdscript/gdscript_compiler.cpp
@@ -1665,6 +1665,7 @@ Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::Sui
 	Error err = OK;
 	GDScriptCodeGenerator *gen = codegen.generator;
 
+	gen->clean_temporaries();
 	codegen.start_block();
 
 	if (p_add_locals) {
@@ -1967,6 +1968,8 @@ Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::Sui
 				}
 			} break;
 		}
+
+		gen->clean_temporaries();
 	}
 
 	codegen.end_block();