Merge pull request #117053 from dalexeev/gds-fix-coroutine-stack-clearing

GDScript: Fix and simplify coroutine stack clearing

modules/gdscript/gdscript_function.cpp

@@ -325,7 +325,7 @@ Variant GDScriptFunctionState::resume(const Variant &p_arg) {
 			return Variant();
 #endif
 		}
-		// Do these now to avoid locking again after the call
+		// Do these now to avoid locking again after the call.
 		scripts_list.remove_from_list();
 		instances_list.remove_from_list();
 	}
@@ -334,26 +334,9 @@ Variant GDScriptFunctionState::resume(const Variant &p_arg) {
 	Callable::CallError err;
 	Variant ret = function->call(nullptr, nullptr, 0, err, &state);
 
-	bool completed = true;
-
-	// If the return value is a GDScriptFunctionState reference,
-	// then the function did await again after resuming.
-	if (ret.is_ref_counted()) {
-		GDScriptFunctionState *gdfs = Object::cast_to<GDScriptFunctionState>(ret);
-		if (gdfs && gdfs->function == function) {
-			completed = false;
-			// Keep the first state alive via reference.
-			gdfs->first_state = first_state.is_valid() ? first_state : Ref<GDScriptFunctionState>(this);
-		}
-	}
-
-	function = nullptr; //cleaned up;
+	function = nullptr; // Cleaned up.
 	state.result = Variant();
 
-	if (completed) {
-		_clear_stack();
-	}
-
 	return ret;
 }
 

modules/gdscript/gdscript_function.h

@@ -505,7 +505,6 @@ class GDScriptFunctionState : public RefCounted {
 	GDScriptFunction *function = nullptr;
 	GDScriptFunction::CallState state;
 	Variant _signal_callback(const Variant **p_args, int p_argcount, Callable::CallError &r_error);
-	Ref<GDScriptFunctionState> first_state;
 
 	SelfList<GDScriptFunctionState> scripts_list;
 	SelfList<GDScriptFunctionState> instances_list;

modules/gdscript/gdscript_vm.cpp

@@ -543,9 +543,9 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
 	int line = _initial_line;
 
 	if (p_state) {
-		//use existing (supplied) state (awaited)
+		// Use existing (supplied) state (awaited).
 		stack = (Variant *)p_state->stack.ptr();
-		instruction_args = (Variant **)&p_state->stack.ptr()[sizeof(Variant) * p_state->stack_size]; //ptr() to avoid bounds check
+		instruction_args = (Variant **)&p_state->stack.ptr()[sizeof(Variant) * p_state->stack_size]; // `ptr()` to avoid bounds check.
 		line = p_state->line;
 		ip = p_state->ip;
 		alloca_size = p_state->stack.size();
@@ -553,6 +553,11 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
 		p_instance = p_state->instance;
 		defarg = p_state->defarg;
 
+		// Responsibility for the stack is moved from `GDScriptFunctionState` to this method. So, we reset `p_state->stack_size`
+		// to prevent `GDScriptFunctionState::_clear_stack()` from clearing the stack again.
+		// NOTE: Strictly speaking, ownership doesn't move. However, we can be sure that `p_state->stack` won't be cleared
+		// before the current call completes, and that `p_state` won't be resumed again.
+		p_state->stack_size = 0;
 	} else {
 		if (p_argcount != _argument_count) {
 			if (p_argcount > _argument_count) {
@@ -4000,15 +4005,9 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
 	// This ensures the call stack can be properly shown when using `await`, showing what resumed the function.
 	if (!p_state || awaited) {
 		GDScriptLanguage::get_singleton()->exit_function();
-
-		// Free stack, except reserved addresses.
-		for (int i = FIXED_ADDRESSES_MAX; i < _stack_size; i++) {
-			stack[i].~Variant();
-		}
 	}
 
-	// Always free reserved addresses, since they are never copied.
-	for (int i = 0; i < FIXED_ADDRESSES_MAX; i++) {
+	for (int i = 0; i < _stack_size; i++) {
 		stack[i].~Variant();
 	}
 

modules/gdscript/tests/scripts/runtime/features/coroutine_clearing.gd

@@ -1,13 +1,16 @@
-# GH-116706
-
 class Instance:
 	func _init() -> void:
 		print("Instance _init")
 
+	func _to_string() -> String:
+		return "<Instance>"
+
 	func _notification(what: int) -> void:
 		if what == NOTIFICATION_PREDELETE:
 			print("Instance predelete")
 
+# GH-116706
+
 class LocalOwner:
 	signal never_emitted()
 
@@ -24,10 +27,52 @@ class LocalOwner:
 		await never_emitted
 		print("interrupted_coroutine end")
 
-func test():
-	print("test begin")
+func subtest_order():
+	print("subtest_order begin")
 	var local_owner := LocalOwner.new()
 	@warning_ignore("missing_await")
 	local_owner.interrupted_coroutine()
 	local_owner = null
-	print("test end")
+	print("subtest_order end")
+
+# GH-117049
+
+signal tick()
+
+func await_before_and_after() -> void:
+	await tick
+	var packed_array: PackedStringArray = ["abc"]
+	var instance := Instance.new()
+	await tick
+	prints(packed_array, instance)
+
+func await_two_after() -> void:
+	var packed_array: PackedStringArray = ["abc"]
+	var instance := Instance.new()
+	await tick
+	await tick
+	prints(packed_array, instance)
+
+func subtest_resume():
+	print("subtest_resume begin")
+
+	@warning_ignore("missing_await")
+	await_before_and_after()
+	tick.emit()
+	tick.emit()
+
+	print("---")
+
+	@warning_ignore("missing_await")
+	await_two_after()
+	tick.emit()
+	tick.emit()
+
+	print("subtest_resume end")
+
+# ===
+
+func test():
+	subtest_order()
+	print("===")
+	subtest_resume()

modules/gdscript/tests/scripts/runtime/features/coroutine_clearing.out

@@ -1,8 +1,18 @@
 GDTEST_OK
-test begin
+subtest_order begin
 LocalOwner _init
 interrupted_coroutine begin
 Instance _init
 LocalOwner predelete
 Instance predelete
-test end
+subtest_order end
+===
+subtest_resume begin
+Instance _init
+["abc"] <Instance>
+Instance predelete
+---
+Instance _init
+["abc"] <Instance>
+Instance predelete
+subtest_resume end