diff --git a/modules/gdscript/gdscript_byte_codegen.cpp b/modules/gdscript/gdscript_byte_codegen.cpp index 36b157521d5..bfe090edb03 100644 --- a/modules/gdscript/gdscript_byte_codegen.cpp +++ b/modules/gdscript/gdscript_byte_codegen.cpp @@ -45,8 +45,8 @@ uint32_t GDScriptByteCodeGenerator::add_parameter(const StringName &p_name, bool } uint32_t GDScriptByteCodeGenerator::add_local(const StringName &p_name, const GDScriptDataType &p_type) { - int stack_pos = locals.size() + RESERVED_STACK; - locals.push_back(StackSlot(p_type.builtin_type)); + int stack_pos = locals.size() + GDScriptFunction::FIXED_ADDRESSES_MAX; + locals.push_back(StackSlot(p_type.builtin_type, p_type.can_contain_object())); add_stack_identifier(p_name, stack_pos); return stack_pos; } @@ -122,7 +122,7 @@ uint32_t GDScriptByteCodeGenerator::add_temporary(const GDScriptDataType &p_type List &pool = temporaries_pool[temp_type]; if (pool.is_empty()) { - StackSlot new_temp(temp_type); + StackSlot new_temp(temp_type, p_type.can_contain_object()); int idx = temporaries.size(); pool.push_back(idx); temporaries.push_back(new_temp); @@ -136,15 +136,14 @@ uint32_t GDScriptByteCodeGenerator::add_temporary(const GDScriptDataType &p_type 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::NIL) { + if (temporaries[slot_idx].can_contain_object) { // Avoid keeping in the stack long-lived references to objects, - // which may prevent RefCounted objects from being freed. + // which may prevent `RefCounted` objects from being freed. // 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_pending_clear.insert(slot_idx); } - temporaries_pool[slot.type].push_back(slot_idx); + temporaries_pool[temporaries[slot_idx].type].push_back(slot_idx); used_temporaries.pop_back(); } @@ -187,7 +186,7 @@ GDScriptFunction *GDScriptByteCodeGenerator::write_end() { append_opcode(GDScriptFunction::OPCODE_END); for (int i = 0; i < temporaries.size(); i++) { - int stack_index = i + max_locals + RESERVED_STACK; + int stack_index = i + max_locals + GDScriptFunction::FIXED_ADDRESSES_MAX; for (int j = 0; j < temporaries[i].bytecode_indices.size(); j++) { opcodes.write[temporaries[i].bytecode_indices[j]] = stack_index | (GDScriptFunction::ADDR_TYPE_STACK << GDScriptFunction::ADDR_BITS); } @@ -398,7 +397,7 @@ GDScriptFunction *GDScriptByteCodeGenerator::write_end() { if (debug_stack) { function->stack_debug = stack_debug; } - function->_stack_size = RESERVED_STACK + max_locals + temporaries.size(); + function->_stack_size = GDScriptFunction::FIXED_ADDRESSES_MAX + max_locals + temporaries.size(); function->_instruction_args_size = instr_args_max; #ifdef DEBUG_ENABLED @@ -945,6 +944,11 @@ void GDScriptByteCodeGenerator::write_assign(const Address &p_target, const Addr } } +void GDScriptByteCodeGenerator::write_assign_null(const Address &p_target) { + append_opcode(GDScriptFunction::OPCODE_ASSIGN_NULL); + append(p_target); +} + void GDScriptByteCodeGenerator::write_assign_true(const Address &p_target) { append_opcode(GDScriptFunction::OPCODE_ASSIGN_TRUE); append(p_target); @@ -1579,9 +1583,8 @@ void GDScriptByteCodeGenerator::write_for(const Address &p_variable, bool p_use_ if (p_use_conversion) { write_assign_with_conversion(p_variable, temp); - const GDScriptDataType &type = p_variable.type; - if (type.kind != GDScriptDataType::BUILTIN || type.builtin_type == Variant::ARRAY || type.builtin_type == Variant::DICTIONARY) { - write_assign_false(temp); // Can contain RefCounted, so clear it. + if (p_variable.type.can_contain_object()) { + clear_address(temp); // Can contain `RefCounted`, so clear it. } } } @@ -1746,21 +1749,54 @@ void GDScriptByteCodeGenerator::end_block() { pop_stack_identifiers(); } -void GDScriptByteCodeGenerator::clean_temporaries() { - List::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)); +void GDScriptByteCodeGenerator::clear_temporaries() { + for (int slot_idx : temporaries_pending_clear) { + // The temporary may have been re-used as something else since it was added to the list. + // In that case, there's **no** need to clear it. + if (temporaries[slot_idx].can_contain_object) { + clear_address(Address(Address::TEMPORARY, slot_idx)); // Can contain `RefCounted`, so clear it. } - - List::Element *next = E->next(); - E->erase(); - E = next; } + temporaries_pending_clear.clear(); +} + +void GDScriptByteCodeGenerator::clear_address(const Address &p_address) { + // Do not check `is_local_dirty()` here! Always clear the address since the codegen doesn't track the compiler. + // Also, this method is used to initialize local variables of built-in types, since they cannot be `null`. + + if (p_address.type.has_type && p_address.type.kind == GDScriptDataType::BUILTIN) { + switch (p_address.type.builtin_type) { + case Variant::BOOL: + write_assign_false(p_address); + break; + case Variant::ARRAY: + if (p_address.type.has_container_element_type(0)) { + write_construct_typed_array(p_address, p_address.type.get_container_element_type(0), Vector()); + } else { + write_construct(p_address, p_address.type.builtin_type, Vector()); + } + break; + case Variant::NIL: + case Variant::OBJECT: + write_assign_null(p_address); + break; + default: + write_construct(p_address, p_address.type.builtin_type, Vector()); + break; + } + } else { + write_assign_null(p_address); + } + + if (p_address.mode == Address::LOCAL_VARIABLE) { + dirty_locals.erase(p_address.address); + } +} + +// Returns `true` if the local has been re-used and not cleaned up with `clear_address()`. +bool GDScriptByteCodeGenerator::is_local_dirty(const Address &p_address) const { + ERR_FAIL_COND_V(p_address.mode != Address::LOCAL_VARIABLE, false); + return dirty_locals.has(p_address.address); } GDScriptByteCodeGenerator::~GDScriptByteCodeGenerator() { diff --git a/modules/gdscript/gdscript_byte_codegen.h b/modules/gdscript/gdscript_byte_codegen.h index f902cb10cc4..5a736b2554a 100644 --- a/modules/gdscript/gdscript_byte_codegen.h +++ b/modules/gdscript/gdscript_byte_codegen.h @@ -38,15 +38,14 @@ class GDScriptByteCodeGenerator : public GDScriptCodeGenerator { struct StackSlot { Variant::Type type = Variant::NIL; + bool can_contain_object = true; Vector bytecode_indices; StackSlot() = default; - StackSlot(Variant::Type p_type) : - type(p_type) {} + StackSlot(Variant::Type p_type, bool p_can_contain_object) : + type(p_type), can_contain_object(p_can_contain_object) {} }; - const static int RESERVED_STACK = 3; // For self, class, and nil. - struct CallTarget { Address target; bool is_new_temporary = false; @@ -85,9 +84,11 @@ class GDScriptByteCodeGenerator : public GDScriptCodeGenerator { RBMap local_constants; Vector locals; + HashSet dirty_locals; + Vector temporaries; List used_temporaries; - List temporaries_pending_clear; + HashSet temporaries_pending_clear; RBMap> temporaries_pool; List stack_debug; @@ -193,6 +194,9 @@ class GDScriptByteCodeGenerator : public GDScriptCodeGenerator { ERR_PRINT("Leaving block with non-zero temporary variables: " + itos(used_temporaries.size())); } #endif + for (int i = current_locals; i < locals.size(); i++) { + dirty_locals.insert(i + GDScriptFunction::FIXED_ADDRESSES_MAX); + } locals.resize(current_locals); if (debug_stack) { for (const KeyValue &E : block_identifiers) { @@ -455,7 +459,9 @@ 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 clear_temporaries() override; + virtual void clear_address(const Address &p_address) override; + virtual bool is_local_dirty(const Address &p_address) const override; virtual void start_parameters() override; virtual void end_parameters() override; @@ -496,6 +502,7 @@ public: virtual void write_get_static_variable(const Address &p_target, const Address &p_class, int p_index) override; virtual void write_assign(const Address &p_target, const Address &p_source) override; virtual void write_assign_with_conversion(const Address &p_target, const Address &p_source) override; + virtual void write_assign_null(const Address &p_target) override; virtual void write_assign_true(const Address &p_target) override; virtual void write_assign_false(const Address &p_target) override; virtual void write_assign_default_parameter(const Address &p_dst, const Address &p_src, bool p_use_conversion) override; diff --git a/modules/gdscript/gdscript_codegen.h b/modules/gdscript/gdscript_codegen.h index 7ad8f841aac..4c33ed499a6 100644 --- a/modules/gdscript/gdscript_codegen.h +++ b/modules/gdscript/gdscript_codegen.h @@ -73,7 +73,9 @@ 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 clear_temporaries() = 0; + virtual void clear_address(const Address &p_address) = 0; + virtual bool is_local_dirty(const Address &p_address) const = 0; virtual void start_parameters() = 0; virtual void end_parameters() = 0; @@ -114,6 +116,7 @@ public: virtual void write_get_static_variable(const Address &p_target, const Address &p_class, int p_index) = 0; virtual void write_assign(const Address &p_target, const Address &p_source) = 0; virtual void write_assign_with_conversion(const Address &p_target, const Address &p_source) = 0; + virtual void write_assign_null(const Address &p_target) = 0; virtual void write_assign_true(const Address &p_target) = 0; virtual void write_assign_false(const Address &p_target) = 0; virtual void write_assign_default_parameter(const Address &dst, const Address &src, bool p_use_conversion) = 0; diff --git a/modules/gdscript/gdscript_compiler.cpp b/modules/gdscript/gdscript_compiler.cpp index c526d9c0a48..734e37bc091 100644 --- a/modules/gdscript/gdscript_compiler.cpp +++ b/modules/gdscript/gdscript_compiler.cpp @@ -1826,7 +1826,7 @@ GDScriptCodeGenerator::Address GDScriptCompiler::_parse_match_pattern(CodeGen &c ERR_FAIL_V_MSG(p_previous_test, "Reaching the end of pattern compilation without matching a pattern."); } -List GDScriptCompiler::_add_locals_in_block(CodeGen &codegen, const GDScriptParser::SuiteNode *p_block) { +List GDScriptCompiler::_add_block_locals(CodeGen &codegen, const GDScriptParser::SuiteNode *p_block) { List addresses; for (int i = 0; i < p_block->locals.size(); i++) { if (p_block->locals[i].type == GDScriptParser::SuiteNode::Local::PARAMETER || p_block->locals[i].type == GDScriptParser::SuiteNode::Local::FOR_VARIABLE) { @@ -1838,27 +1838,25 @@ List GDScriptCompiler::_add_locals_in_block(Code return addresses; } -// Avoid keeping in the stack long-lived references to objects, which may prevent RefCounted objects from being freed. -void GDScriptCompiler::_clear_addresses(CodeGen &codegen, const List &p_addresses) { - for (const List::Element *E = p_addresses.front(); E; E = E->next()) { - GDScriptDataType type = E->get().type; - // If not an object and cannot contain an object, no need to clear. - if (type.kind != GDScriptDataType::BUILTIN || type.builtin_type == Variant::ARRAY || type.builtin_type == Variant::DICTIONARY) { - codegen.generator->write_assign_false(E->get()); +// Avoid keeping in the stack long-lived references to objects, which may prevent `RefCounted` objects from being freed. +void GDScriptCompiler::_clear_block_locals(CodeGen &codegen, const List &p_locals) { + for (const GDScriptCodeGenerator::Address &local : p_locals) { + if (local.type.can_contain_object()) { + codegen.generator->clear_address(local); } } } -Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::SuiteNode *p_block, bool p_add_locals, bool p_reset_locals) { +Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::SuiteNode *p_block, bool p_add_locals, bool p_clear_locals) { Error err = OK; GDScriptCodeGenerator *gen = codegen.generator; List block_locals; - gen->clean_temporaries(); + gen->clear_temporaries(); codegen.start_block(); if (p_add_locals) { - block_locals = _add_locals_in_block(codegen, p_block); + block_locals = _add_block_locals(codegen, p_block); } for (int i = 0; i < p_block->statements.size(); i++) { @@ -1873,7 +1871,7 @@ Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::Sui case GDScriptParser::Node::MATCH: { const GDScriptParser::MatchNode *match = static_cast(s); - codegen.start_block(); + codegen.start_block(); // Add an extra block, since the binding pattern and @special variables belong to the branch scope. // Evaluate the match expression. GDScriptCodeGenerator::Address value = codegen.add_local("@match_value", _gdtype_from_datatype(match->test->get_datatype(), codegen.script)); @@ -1914,7 +1912,7 @@ Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::Sui codegen.start_block(); // Create an extra block around for binds. // Add locals in block before patterns, so temporaries don't use the stack address for binds. - List branch_locals = _add_locals_in_block(codegen, branch->block); + List branch_locals = _add_block_locals(codegen, branch->block); #ifdef DEBUG_ENABLED // Add a newline before each branch, since the debugger needs those. @@ -1961,7 +1959,7 @@ Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::Sui return err; } - _clear_addresses(codegen, branch_locals); + _clear_block_locals(codegen, branch_locals); codegen.end_block(); // Get out of extra block. } @@ -2003,7 +2001,8 @@ Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::Sui case GDScriptParser::Node::FOR: { const GDScriptParser::ForNode *for_n = static_cast(s); - codegen.start_block(); + codegen.start_block(); // Add an extra block, since the iterator and @special variables belong to the loop scope. + GDScriptCodeGenerator::Address iterator = codegen.add_local(for_n->variable->name, _gdtype_from_datatype(for_n->variable->get_datatype(), codegen.script)); gen->start_for(iterator.type, _gdtype_from_datatype(for_n->list->get_datatype(), codegen.script)); @@ -2021,14 +2020,21 @@ Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::Sui gen->write_for(iterator, for_n->use_conversion_assign); - err = _parse_block(codegen, for_n->loop); + // Loop variables must be cleared even when `break`/`continue` is used. + List loop_locals = _add_block_locals(codegen, for_n->loop); + + //_clear_block_locals(codegen, loop_locals); // Inside loop, before block - for `continue`. // TODO + + err = _parse_block(codegen, for_n->loop, false); // Don't add locals again. if (err) { return err; } gen->write_endfor(); - codegen.end_block(); + _clear_block_locals(codegen, loop_locals); // Outside loop, after block - for `break` and normal exit. + + codegen.end_block(); // Get out of extra block. } break; case GDScriptParser::Node::WHILE: { const GDScriptParser::WhileNode *while_n = static_cast(s); @@ -2046,12 +2052,19 @@ Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::Sui codegen.generator->pop_temporary(); } - err = _parse_block(codegen, while_n->loop); + // Loop variables must be cleared even when `break`/`continue` is used. + List loop_locals = _add_block_locals(codegen, while_n->loop); + + //_clear_block_locals(codegen, loop_locals); // Inside loop, before block - for `continue`. // TODO + + err = _parse_block(codegen, while_n->loop, false); // Don't add locals again. if (err) { return err; } gen->write_endwhile(); + + _clear_block_locals(codegen, loop_locals); // Outside loop, after block - for `break` and normal exit. } break; case GDScriptParser::Node::BREAK: { gen->write_break(); @@ -2134,21 +2147,16 @@ Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::Sui codegen.generator->pop_temporary(); } initialized = true; - } else if (local_type.has_type) { - // Initialize with default for type. - if (local_type.has_container_element_type(0)) { - codegen.generator->write_construct_typed_array(local, local_type.get_container_element_type(0), Vector()); - initialized = true; - } else if (local_type.kind == GDScriptDataType::BUILTIN) { - codegen.generator->write_construct(local, local_type.builtin_type, Vector()); - initialized = true; - } - // The `else` branch is for objects, in such case we leave it as `null`. + } else if ((local_type.has_type && local_type.kind == GDScriptDataType::BUILTIN) || codegen.generator->is_local_dirty(local)) { + // Initialize with default for the type. Built-in types must always be cleared (they cannot be `null`). + // Objects and untyped variables are assigned to `null` only if the stack address has been re-used and not cleared. + codegen.generator->clear_address(local); + initialized = true; } - // Assigns a null for the unassigned variables in loops. + // Don't check `is_local_dirty()` since the variable must be assigned to `null` **on each iteration**. if (!initialized && p_block->is_in_loop) { - codegen.generator->write_construct(local, Variant::NIL, Vector()); + codegen.generator->clear_address(local); } } break; case GDScriptParser::Node::CONSTANT: { @@ -2180,11 +2188,11 @@ Error GDScriptCompiler::_parse_block(CodeGen &codegen, const GDScriptParser::Sui } break; } - gen->clean_temporaries(); + gen->clear_temporaries(); } - if (p_add_locals && p_reset_locals) { - _clear_addresses(codegen, block_locals); + if (p_add_locals && p_clear_locals) { + _clear_block_locals(codegen, block_locals); } codegen.end_block(); diff --git a/modules/gdscript/gdscript_compiler.h b/modules/gdscript/gdscript_compiler.h index 637d61ca3be..45f0f9e19b9 100644 --- a/modules/gdscript/gdscript_compiler.h +++ b/modules/gdscript/gdscript_compiler.h @@ -153,9 +153,9 @@ class GDScriptCompiler { GDScriptCodeGenerator::Address _parse_expression(CodeGen &codegen, Error &r_error, const GDScriptParser::ExpressionNode *p_expression, bool p_root = false, bool p_initializer = false); GDScriptCodeGenerator::Address _parse_match_pattern(CodeGen &codegen, Error &r_error, const GDScriptParser::PatternNode *p_pattern, const GDScriptCodeGenerator::Address &p_value_addr, const GDScriptCodeGenerator::Address &p_type_addr, const GDScriptCodeGenerator::Address &p_previous_test, bool p_is_first, bool p_is_nested); - List _add_locals_in_block(CodeGen &codegen, const GDScriptParser::SuiteNode *p_block); - void _clear_addresses(CodeGen &codegen, const List &p_addresses); - Error _parse_block(CodeGen &codegen, const GDScriptParser::SuiteNode *p_block, bool p_add_locals = true, bool p_reset_locals = true); + List _add_block_locals(CodeGen &codegen, const GDScriptParser::SuiteNode *p_block); + void _clear_block_locals(CodeGen &codegen, const List &p_locals); + Error _parse_block(CodeGen &codegen, const GDScriptParser::SuiteNode *p_block, bool p_add_locals = true, bool p_clear_locals = true); GDScriptFunction *_parse_function(Error &r_error, GDScript *p_script, const GDScriptParser::ClassNode *p_class, const GDScriptParser::FunctionNode *p_func, bool p_for_ready = false, bool p_for_lambda = false); GDScriptFunction *_make_static_initializer(Error &r_error, GDScript *p_script, const GDScriptParser::ClassNode *p_class); Error _parse_setter_getter(GDScript *p_script, const GDScriptParser::ClassNode *p_class, const GDScriptParser::VariableNode *p_variable, bool p_is_setter); diff --git a/modules/gdscript/gdscript_disassembler.cpp b/modules/gdscript/gdscript_disassembler.cpp index 26f7cb75376..c7873dcd528 100644 --- a/modules/gdscript/gdscript_disassembler.cpp +++ b/modules/gdscript/gdscript_disassembler.cpp @@ -360,6 +360,13 @@ void GDScriptFunction::disassemble(const Vector &p_code_lines) const { incr += 3; } break; + case OPCODE_ASSIGN_NULL: { + text += "assign "; + text += DADDR(1); + text += " = null"; + + incr += 2; + } break; case OPCODE_ASSIGN_TRUE: { text += "assign "; text += DADDR(1); diff --git a/modules/gdscript/gdscript_function.h b/modules/gdscript/gdscript_function.h index 002fc159fa1..184d256bcd8 100644 --- a/modules/gdscript/gdscript_function.h +++ b/modules/gdscript/gdscript_function.h @@ -78,17 +78,17 @@ public: if (valid && builtin_type == Variant::ARRAY && has_container_element_type(0)) { Array array = p_variant; if (array.is_typed()) { - GDScriptDataType array_container_type = get_container_element_type(0); + const GDScriptDataType &elem_type = container_element_types[0]; Variant::Type array_builtin_type = (Variant::Type)array.get_typed_builtin(); StringName array_native_type = array.get_typed_class_name(); Ref