From 102c312497ace1e81bb54b6c3ed3665d16ee1fef Mon Sep 17 00:00:00 2001 From: George Marques Date: Fri, 13 May 2022 14:03:48 -0300 Subject: [PATCH] GDScript: Fix stack manipulation for `await` The stack now contains three special addresses that should no be copied to the state, since it contains references that creates cycles. They can be recreated when the function is resumed. This commit also removes the clearing of stack from the GDScriptFunctionState destructor, since it should be cleared when the function exits. The state stack should only be cleared manually if the instance is freed before the state resumes (which is already being done). Otherwise this would destruct the stack twice, causing crashes. --- modules/gdscript/gdscript_function.cpp | 5 +-- modules/gdscript/gdscript_vm.cpp | 59 ++++++++++++-------------- 2 files changed, 30 insertions(+), 34 deletions(-) diff --git a/modules/gdscript/gdscript_function.cpp b/modules/gdscript/gdscript_function.cpp index 3d708955ed4..a40fc6ca6df 100644 --- a/modules/gdscript/gdscript_function.cpp +++ b/modules/gdscript/gdscript_function.cpp @@ -279,7 +279,8 @@ Variant GDScriptFunctionState::resume(const Variant &p_arg) { void GDScriptFunctionState::_clear_stack() { if (state.stack_size) { Variant *stack = (Variant *)state.stack.ptr(); - for (int i = 0; i < state.stack_size; i++) { + // The first 3 are special addresses and not copied to the state, so we skip them here. + for (int i = 3; i < state.stack_size; i++) { stack[i].~Variant(); } state.stack_size = 0; @@ -300,8 +301,6 @@ GDScriptFunctionState::GDScriptFunctionState() : } GDScriptFunctionState::~GDScriptFunctionState() { - _clear_stack(); - { MutexLock lock(GDScriptLanguage::singleton->lock); scripts_list.remove_from_list(); diff --git a/modules/gdscript/gdscript_vm.cpp b/modules/gdscript/gdscript_vm.cpp index e28dd26c286..4d8655764f3 100644 --- a/modules/gdscript/gdscript_vm.cpp +++ b/modules/gdscript/gdscript_vm.cpp @@ -546,33 +546,32 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a memnew_placement(&stack[i], Variant); } - memnew_placement(&stack[ADDR_STACK_NIL], Variant); - if (_instruction_args_size) { instruction_args = (Variant **)&aptr[sizeof(Variant) * _stack_size]; } else { instruction_args = nullptr; } - if (p_instance) { - memnew_placement(&stack[ADDR_STACK_SELF], Variant(p_instance->owner)); - script = p_instance->script.ptr(); - } else { - memnew_placement(&stack[ADDR_STACK_SELF], Variant); - script = _script; + for (const KeyValue &E : temporary_slots) { + type_init_function_table[E.value](&stack[E.key]); } } + if (_ptrcall_args_size) { call_args_ptr = (const void **)alloca(_ptrcall_args_size * sizeof(void *)); } else { call_args_ptr = nullptr; } - memnew_placement(&stack[ADDR_STACK_CLASS], Variant(script)); - - for (const KeyValue &E : temporary_slots) { - type_init_function_table[E.value](&stack[E.key]); + if (p_instance) { + memnew_placement(&stack[ADDR_STACK_SELF], Variant(p_instance->owner)); + script = p_instance->script.ptr(); + } else { + memnew_placement(&stack[ADDR_STACK_SELF], Variant); + script = _script; } + memnew_placement(&stack[ADDR_STACK_CLASS], Variant(script)); + memnew_placement(&stack[ADDR_STACK_NIL], Variant); String err_text; @@ -2171,8 +2170,7 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a // Is this even possible to be null at this point? if (obj) { if (obj->is_class_ptr(GDScriptFunctionState::get_class_ptr_static())) { - static StringName completed = _scs_create("completed"); - result = Signal(obj, completed); + result = Signal(obj, "completed"); } } } @@ -2193,8 +2191,9 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a gdfs->function = this; gdfs->state.stack.resize(alloca_size); - //copy variant stack - for (int i = 0; i < _stack_size; i++) { + + // First 3 stack addresses are special, so we just skip them here. + for (int i = 3; i < _stack_size; i++) { memnew_placement(&gdfs->state.stack.write[sizeof(Variant) * i], Variant(stack[i])); } gdfs->state.stack_size = _stack_size; @@ -3451,26 +3450,24 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a GDScriptLanguage::get_singleton()->script_frame_time += time_taken - function_call_time; } - // Check if this is the last time the function is resuming from await - // Will be true if never awaited as well - // When it's the last resume it will postpone the exit from stack, - // so the debugger knows which function triggered the resume of the next function (if any) - if (!p_state || awaited) { + // Check if this function has been interrupted by `await`. + // If that is the case we want to keep it in the debugger until it actually exits. + // This ensures the call stack can be properly shown when using `await`, showing what resumed the function. + if (!awaited) { if (EngineDebugger::is_active()) { GDScriptLanguage::get_singleton()->exit_function(); } -#endif - - if (_stack_size) { - //free stack - for (int i = 0; i < _stack_size; i++) { - stack[i].~Variant(); - } - } - -#ifdef DEBUG_ENABLED } #endif + // Clear the stack even if there was an `await`. + // The stack saved in the state is a copy, so this needs to be destructed to avoid leaks. + if (_stack_size) { + // Free stack. + for (int i = 0; i < _stack_size; i++) { + stack[i].~Variant(); + } + } + return retvalue; }