Skip to content

Commit

Permalink
GDScript: Fix uninitialized local variables not being reset
Browse files Browse the repository at this point in the history
  • Loading branch information
@dalexeev
dalexeev committed Mar 30, 2024
1 parent 86415f0 commit 27d7760
Show file tree
Hide file tree
Showing 10 changed files with 189 additions and 72 deletions.
86 changes: 61 additions & 25 deletions modules/gdscript/gdscript_byte_codegen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -122,7 +122,7 @@ uint32_t GDScriptByteCodeGenerator::add_temporary(const GDScriptDataType &p_type

List<int> &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);
Expand All @@ -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();
}

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
}
}
}
Expand Down Expand Up @@ -1746,23 +1749,56 @@ 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));
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.
}
}
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<GDScriptCodeGenerator::Address>());
} else {
write_construct(p_address, p_address.type.builtin_type, Vector<GDScriptCodeGenerator::Address>());
}
break;
case Variant::NIL:
case Variant::OBJECT:
write_assign_null(p_address);
break;
default:
write_construct(p_address, p_address.type.builtin_type, Vector<GDScriptCodeGenerator::Address>());
break;
}
} else {
write_assign_null(p_address);
}

List<int>::Element *next = E->next();
E->erase();
E = next;
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() {
if (!ended && function != nullptr) {
memdelete(function);
Expand Down
19 changes: 13 additions & 6 deletions modules/gdscript/gdscript_byte_codegen.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,14 @@
class GDScriptByteCodeGenerator : public GDScriptCodeGenerator {
struct StackSlot {
Variant::Type type = Variant::NIL;
bool can_contain_object = true;
Vector<int> 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;
Expand Down Expand Up @@ -85,9 +84,11 @@ class GDScriptByteCodeGenerator : public GDScriptCodeGenerator {
RBMap<StringName, int> local_constants;

Vector<StackSlot> locals;
HashSet<int> dirty_locals;

Vector<StackSlot> temporaries;
List<int> used_temporaries;
List<int> temporaries_pending_clear;
HashSet<int> temporaries_pending_clear;
RBMap<Variant::Type, List<int>> temporaries_pool;

List<GDScriptFunction::StackDebug> stack_debug;
Expand Down Expand Up @@ -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<StringName, int> &E : block_identifiers) {
Expand Down Expand Up @@ -455,7 +459,9 @@ class GDScriptByteCodeGenerator : public GDScriptCodeGenerator {
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;
Expand Down Expand Up @@ -496,6 +502,7 @@ class GDScriptByteCodeGenerator : public GDScriptCodeGenerator {
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;
Expand Down
5 changes: 4 additions & 1 deletion modules/gdscript/gdscript_codegen.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ class GDScriptCodeGenerator {
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;
Expand Down Expand Up @@ -114,6 +116,7 @@ class GDScriptCodeGenerator {
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;
Expand Down
Loading

0 comments on commit 27d7760

Please sign in to comment.