GDScript: Fix some bugs with static variables and functions #77129

dalexeev · 2023-05-16T10:08:27Z

Closes #41919.
Closes #77098.
Closes #77331.

dalexeev · 2023-05-23T07:24:26Z

I think it makes sense to separate point 5 of #77098 (property list and documentation) into a new PR.

EDIT: Done (branch).

vnen

Overall it looks good, just a couple of things can be improved.

vnen · 2023-05-23T14:04:09Z

modules/gdscript/gdscript_compiler.cpp

+												member_property_setter_function = minfo.setter;
+												member_property_has_setter = member_property_setter_function != StringName();
+												member_property_is_in_setter = member_property_has_setter && member_property_setter_function == codegen.function_name;
+												static_var_class = codegen.add_constant(scr);


This might be problematic if scr refers to the script being compiled, because it will make a reference to itself so it will never be freed. In such case it should use a CLASS address instead to use the current script.

There are other instances of this pattern in the compiler, all of them have the same issue.

The problem is that the method can be inherited (see #77331) in which case CLASS will point to the current class, not the one the static variable is located. This will result in an error (the child class variable with the same index will be used or the GD_ERR_BREAK index check will be triggered).

So the CLASS address can only be used in the static initializer, since in this case the current class always matches.

Also, if these constants are counted, then we can still have problems in case of cyclic references. But I think they shouldn't count because it's GDScript* (not Ref<GDScript>)?

I tested it, it does add an extra reference and doesn't depend on GDScript* vs Ref<GDScript> (some Variant/Object magic?). But we could store ObjectID instead of GDScript* (this is how WeakRef is implemented).

I also found that we already have an extra reference to each script somewhere, the bug is reproduced in master and in 4.0.3. Later I will check/open an issue.

I tested it, it does add an extra reference and doesn't depend on GDScript* vs Ref<GDScript> (some Variant/Object magic?)

All constants are Variants, if you put a RefCounted object it will increment the count (it does not matter if the Ref<> wrapper is used, the Variant itself act as the wrapper).

I also found that we already have an extra reference to each script somewhere, the bug is reproduced in master and in 4.0.3. Later I will check/open an issue.

I mentioned this in the static variables PR. But I don't think there's an open issue yet. There's a bit of weird stuff happening in the GDScriptCache so the issue is a bit complicated.

The problem is that the method can be inherited (see #77331) in which case CLASS will point to the current class, not the one the static variable is located

What I meant is to use CLASS only in the case where it is in current class. If it is from a super class then it's fine because there's already a reference to it anyway.

I mentioned this in the static variables PR. But I don't think there's an open issue yet.

See GDScript resources are not unloaded due to an extra reference #77513.

modules/gdscript/gdscript_codegen.h

modules/gdscript/gdscript_vm.cpp

modules/gdscript/gdscript.cpp

dalexeev · 2023-05-25T20:12:34Z

modules/gdscript/gdscript_compiler.cpp

+						gen->write_assign(temp, to_assign);
+					}
+					gen->write_set_static_variable(temp, static_var_class, static_var_index);
+					gen->pop_temporary();


This makes me worry about performance (the need for intermediate temporaries) and maintainability (with this approach, we have to duplicate a lot of tests for static variables that we already have for non-static variables). In this sense, addressing modes is better than separate opcodes.

Since we can't get a contiguous array, maybe we could have a dynamically sized array (or a two-dimensional array if we assume that in the future we might add something else after static variables arrays)? The address already consists of two parts: the index of the nested array in variant_addresses and the index of the element in the nested array.

The compiler could take into account the depth of class inheritance, instead of storing pointers to scripts. A script that doesn't have a base script has a depth of 0, then 1, 2, 3, and so on.

maybe we could have a dynamically sized array (...) The compiler could take into account the depth of class inheritance, instead of storing pointers to scripts. A script that doesn't have a base script has a depth of 0, then 1, 2, 3, and so on.

Done: dalexeev@992a8b3

I'm not sure about that. Sure it's slower having a two-step approach, but generating this address array dynamically every time seems to be worse for me. Regarding tests, we will need tests for static variables a anyway, so I don't think it makes much of a difference (IMO testing for generating a particular addressing mode is as important as testing for generating opcodes).

dalexeev · 2023-06-09T06:24:39Z

I reset the branch to the new version as there are more fixes. You can see the old version with separate opcodes here: dalexeev@fee4ea4.

dalexeev · 2023-06-09T06:28:22Z

modules/gdscript/gdscript_vm.cpp

 #endif

-	Variant *variant_addresses[ADDR_TYPE_MAX] = { stack, _constants_ptr, p_instance ? p_instance->members.ptrw() : nullptr, script->static_variables.ptrw() };
+	Variant **variant_addresses = memnew_arr(Variant *, ADDR_TYPE_STATIC_VAR + script->inheritance_level + 1);


I'm not sure about the preferred way to allocate memory, maybe new[] or alloca() is better here?

I think this is the preferred way in the Godot codebase:

godot/scene/animation/tween.cpp

Line 748 in 49243a9

const Variant **argptr = (const Variant **)alloca(sizeof(Variant *));

adamscott · 2023-06-13T16:22:04Z

@dalexeev Can you isolate point 5 of #77098 in its own issue? We could close then #77098 when merging this PR.

Edit: just tested every other issues, it fixes those successfully.

dalexeev · 2023-06-13T17:46:59Z

@dalexeev Can you isolate point 5 of #77098 in its own issue?

Done: Static variables missing from property list and indistinguishable in user documentation #78195.

akien-mga

@vnen last reviewed with "Overall it looks good", and the suggestions for improvements have been made, so this should be good to merge and test in the next beta.

Suggested changes done.

dalexeev · 2023-06-15T13:56:25Z

I think it's better to wait for George's comment. While this successfully closes linked issues, I'm not sure which approach is better: dedicated opcodes (this option is approved) or addressing modes (I've updated the PR for the second option). And also there is Adam's comment about alloca() (stack/heap). And I have a doubt about how inheritance_level will work with hot reloading.

vnen · 2023-06-16T12:56:05Z

modules/gdscript/gdscript_vm.cpp

+		while (scr) {
+#ifdef DEBUG_ENABLED
+			if (scr->inheritance_level != prev_inheritance_level - 1 || scr->inheritance_level < 0) {
+				// TODO: error print.
+				r_err.error = Callable::CallError::CALL_ERROR_INVALID_METHOD;
+				return _get_default_variant_for_data_type(return_type);
+			}
+			prev_inheritance_level--;
+			variant_address_limits[ADDR_TYPE_STATIC_VAR + scr->inheritance_level] = scr->static_variables.size();
+#endif
+			variant_addresses[ADDR_TYPE_STATIC_VAR + scr->inheritance_level] = scr->static_variables.ptrw();
+			scr = scr->_base;
+		}
+	}


Like I said in the other comment, I do feel like doing this every call a bit too much. Usually scripts are not deeply inherited, but if they are this starts to add up, even if they don't use static variables.

vnen

IMO the previous approach with opcodes was better. The changes in addressing modes is more complex and seems to create extra overhead in all function calls to populate the array.

dalexeev · 2023-06-16T13:34:52Z

Okay, I can go back to the opcode version and add some later changes to it. Not sure how to deal with RefCounted, replace with ObjectID as I suggested above? Or we can keep pointers to scripts in an array instead of the constants array (since it's Variant). In this case, the reference count shouldn't increase, but couldn't the script be freed prematurely?

dalexeev · 2023-06-16T16:00:10Z

Done. Only the issue with RefCounted remained. But at the moment the scripts are never unloaded due to the GDScriptCache issue anyway.

vnen

Looks OK to me. The issue with references can be sorted out later since it's shadowed by a bigger issue for now anyway.

akien-mga · 2023-06-19T19:22:04Z

Thanks!

capnm · 2023-06-20T09:40:47Z

Looks like this is recent enough to cause the 73ac333 linux/clang 16.0.5 compiler error:

[ 22%] clang++ -o modules/gdscript/editor/scu/scu_modules_gdscript_editor.gen.linuxbsd.editor.dev.x86_64.llvm.o -c -std=gnu++17 -fpic -pipe -gdwarf-4 -g3 -Og -Wall -Wshadow-field-in-constructor -Wshadow-uncaptured-local -Wno-ordered-compare-function-pointers -isystem thirdparty/glad -DTOOLS_ENABLED -DDEBUG_ENABLED -DDEV_ENABLED -DNO_EDITOR_SPLASH -DSOWRAP_ENABLED -DTOUCH_ENABLED -DFONTCONFIG_ENABLED -DALSA_ENABLED -DALSAMIDI_ENABLED -DPULSEAUDIO_ENABLED -D_REENTRANT -DDBUS_ENABLED -DSPEECHD_ENABLED -DXKB_ENABLED -DJOYDEV_ENABLED -DUDEV_ENABLED -DLINUXBSD_ENABLED -DUNIX_ENABLED -D_FILE_OFFSET_BITS=64 -DX11_ENABLED -DVULKAN_ENABLED -DGLES3_ENABLED -DMINIZIP_ENABLED -DBROTLI_ENABLED -DZSTD_STATIC_LINKING_ONLY -DUSE_VOLK -DVK_USE_PLATFORM_XLIB_KHR -DGLAD_ENABLED -DGLES_OVER_GL -Ithirdparty/libpng -Ithirdparty/volk -Ithirdparty/vulkan -Ithirdparty/vulkan/include -Ithirdparty/zstd -Ithirdparty/zlib -Ithirdparty/brotli/include -Ithirdparty/linuxbsd_headers -Iplatform/linuxbsd -I. modules/gdscript/editor/scu/scu_modules_gdscript_editor.gen.cpp
In file included from modules/gdscript/scu/scu_modules_gdscript.gen.cpp:14:
./modules/gdscript/gdscript_vm.cpp:52:10: error: call to '_get_script_name' is ambiguous
                return _get_script_name(script_type);
                       ^~~~~~~~~~~~~~~~
./modules/gdscript/gdscript_disassembler.cpp:38:15: note: candidate function
static String _get_script_name(const Ref<Script> &p_script) {
              ^
./modules/gdscript/gdscript_vm.cpp:39:15: note: candidate function
static String _get_script_name(const Ref<Script> p_script) {
              ^
./modules/gdscript/gdscript_vm.cpp:78:24: error: call to '_get_script_name' is ambiguous
                                        basestr += " (" + _get_script_name(bobj->get_script_instance()->get_script()) + ")";
                                                          ^~~~~~~~~~~~~~~~
./modules/gdscript/gdscript_disassembler.cpp:38:15: note: candidate function
static String _get_script_name(const Ref<Script> &p_script) {
              ^
./modules/gdscript/gdscript_vm.cpp:39:15: note: candidate function
static String _get_script_name(const Ref<Script> p_script) {
              ^
./modules/gdscript/gdscript_vm.cpp:2687:47: error: call to '_get_script_name' is ambiguous
                                                        Variant::get_type_name(r->get_type()), _get_script_name(Ref<Script>(base_type)));
                                                                                               ^~~~~~~~~~~~~~~~
./modules/gdscript/gdscript_disassembler.cpp:38:15: note: candidate function
static String _get_script_name(const Ref<Script> &p_script) {
              ^
./modules/gdscript/gdscript_vm.cpp:39:15: note: candidate function
static String _get_script_name(const Ref<Script> p_script) {
              ^
./modules/gdscript/gdscript_vm.cpp:2709:36: error: call to '_get_script_name' is ambiguous
                                                                ret_obj->get_class_name(), _get_script_name(Ref<GDScript>(base_type)));
                                                                                           ^~~~~~~~~~~~~~~~
./modules/gdscript/gdscript_disassembler.cpp:38:15: note: candidate function
static String _get_script_name(const Ref<Script> &p_script) {
              ^
./modules/gdscript/gdscript_vm.cpp:39:15: note: candidate function
static String _get_script_name(const Ref<Script> p_script) {
              ^
./modules/gdscript/gdscript_vm.cpp:2728:9: error: call to '_get_script_name' is ambiguous
                                                                _get_script_name(ret_obj->get_script_instance()->get_script()), _get_script_name(Ref<GDScript>(base_type)));
                                                                ^~~~~~~~~~~~~~~~
./modules/gdscript/gdscript_disassembler.cpp:38:15: note: candidate function
static String _get_script_name(const Ref<Script> &p_script) {
              ^
./modules/gdscript/gdscript_vm.cpp:39:15: note: candidate function
static String _get_script_name(const Ref<Script> p_script) {
              ^
./modules/gdscript/gdscript_vm.cpp:2728:73: error: call to '_get_script_name' is ambiguous
                                                                _get_script_name(ret_obj->get_script_instance()->get_script()), _get_script_name(Ref<GDScript>(base_type)));
                                                                                                                                ^~~~~~~~~~~~~~~~
./modules/gdscript/gdscript_disassembler.cpp:38:15: note: candidate function
static String _get_script_name(const Ref<Script> &p_script) {
              ^
./modules/gdscript/gdscript_vm.cpp:39:15: note: candidate function
static String _get_script_name(const Ref<Script> p_script) {
              ^
6 errors generated.
scons: *** [modules/gdscript/scu/scu_modules_gdscript.gen.linuxbsd.editor.dev.x86_64.llvm.o] Error 1
scons: building terminated because of errors.
[Time elapsed: 00:00:15.977]

akien-mga · 2023-06-20T09:52:19Z

@capnm See #78465.

capnm · 2023-06-20T10:10:39Z

Thanks after #78465 .patch it compiles again.

dalexeev requested a review from a team as a code owner May 16, 2023 10:08

dalexeev force-pushed the gds-fix-static-var-bugs-part-1 branch 2 times, most recently from 5a0d17c to 6923c0d Compare May 16, 2023 15:20

YuriSizov added this to the 4.1 milestone May 17, 2023

YuriSizov added bug topic:gdscript labels May 17, 2023

dalexeev force-pushed the gds-fix-static-var-bugs-part-1 branch from 6923c0d to c9bd216 Compare May 23, 2023 06:56

dalexeev requested review from a team as code owners May 23, 2023 06:56

dalexeev force-pushed the gds-fix-static-var-bugs-part-1 branch 3 times, most recently from ed25748 to c7f311d Compare May 23, 2023 07:06

dalexeev force-pushed the gds-fix-static-var-bugs-part-1 branch from c7f311d to 36c1ec1 Compare May 23, 2023 09:04

vnen previously requested changes May 23, 2023

View reviewed changes

dalexeev force-pushed the gds-fix-static-var-bugs-part-1 branch 4 times, most recently from c16bfeb to fc30e2f Compare May 25, 2023 14:00

dalexeev commented May 25, 2023

View reviewed changes

This was referenced May 26, 2023

Static method override fails when call site is base script #72973

Open

GDScript resources are not unloaded due to an extra reference #77513

Open

dalexeev requested a review from vnen June 5, 2023 07:59

dalexeev force-pushed the gds-fix-static-var-bugs-part-1 branch 2 times, most recently from fee4ea4 to e14f4ae Compare June 9, 2023 06:17

dalexeev changed the title ~~GDScript: Fix some bugs with static variables~~ GDScript: Fix some bugs with static variables and functions Jun 9, 2023

dalexeev commented Jun 9, 2023

View reviewed changes

dalexeev force-pushed the gds-fix-static-var-bugs-part-1 branch from e14f4ae to d860533 Compare June 13, 2023 12:11

dalexeev mentioned this pull request Jun 13, 2023

Static variables missing from property list and indistinguishable in user documentation #78195

Open

dalexeev mentioned this pull request Jun 15, 2023

GDScript: Document static variables godotengine/godot-docs#7524

Merged

akien-mga approved these changes Jun 15, 2023

View reviewed changes

vnen reviewed Jun 16, 2023

View reviewed changes

vnen requested changes Jun 16, 2023

View reviewed changes

dalexeev force-pushed the gds-fix-static-var-bugs-part-1 branch 2 times, most recently from 7c2ea61 to c4e836b Compare June 16, 2023 15:53

GDScript: Fix some bugs with static variables and functions

aebbbda

dalexeev force-pushed the gds-fix-static-var-bugs-part-1 branch from c4e836b to aebbbda Compare June 16, 2023 19:52

dalexeev mentioned this pull request Jun 17, 2023

GDScript: Fix crash in disassembler #77528

Merged

vnen approved these changes Jun 19, 2023

View reviewed changes

akien-mga merged commit ae00187 into godotengine:master Jun 19, 2023

dalexeev deleted the gds-fix-static-var-bugs-part-1 branch June 19, 2023 19:30

dalexeev mentioned this pull request Jul 2, 2023

Allowed setters and getters to take additional arguments (superseded by #78991) #76665

Closed

2 tasks

dalexeev mentioned this pull request Jul 18, 2023

[GDScript] Perform update-and-assign operations in place when possible. #76496

Open

dalexeev mentioned this pull request Oct 23, 2023

Bring autoload, project and static variable access speeds inline with script/function godotengine/godot-proposals#8234

Open

gshadows mentioned this pull request Feb 21, 2024

Static callable not fully fixed in #41919 #88622

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GDScript: Fix some bugs with static variables and functions #77129

GDScript: Fix some bugs with static variables and functions #77129

dalexeev commented May 16, 2023 •

edited

Loading

dalexeev commented May 23, 2023 •

edited

Loading

vnen left a comment

vnen May 23, 2023

dalexeev May 25, 2023 •

edited

Loading

dalexeev May 25, 2023

vnen Jun 16, 2023

vnen Jun 16, 2023

dalexeev Jun 16, 2023

dalexeev May 25, 2023 •

edited

Loading

dalexeev May 26, 2023 •

edited

Loading

vnen Jun 16, 2023

dalexeev commented Jun 9, 2023

dalexeev Jun 9, 2023

adamscott Jun 13, 2023

adamscott commented Jun 13, 2023 •

edited

Loading

dalexeev commented Jun 13, 2023

akien-mga left a comment

dalexeev commented Jun 15, 2023 •

edited

Loading

vnen Jun 16, 2023

vnen left a comment

dalexeev commented Jun 16, 2023 •

edited

Loading

dalexeev commented Jun 16, 2023

vnen left a comment

akien-mga commented Jun 19, 2023

capnm commented Jun 20, 2023

akien-mga commented Jun 20, 2023

capnm commented Jun 20, 2023

GDScript: Fix some bugs with static variables and functions #77129

GDScript: Fix some bugs with static variables and functions #77129

Conversation

dalexeev commented May 16, 2023 • edited Loading

dalexeev commented May 23, 2023 • edited Loading

vnen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalexeev May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalexeev May 25, 2023 • edited Loading

Choose a reason for hiding this comment

dalexeev May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalexeev commented Jun 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamscott commented Jun 13, 2023 • edited Loading

dalexeev commented Jun 13, 2023

akien-mga left a comment

Choose a reason for hiding this comment

dalexeev commented Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

vnen left a comment

Choose a reason for hiding this comment

dalexeev commented Jun 16, 2023 • edited Loading

dalexeev commented Jun 16, 2023

vnen left a comment

Choose a reason for hiding this comment

akien-mga commented Jun 19, 2023

capnm commented Jun 20, 2023

akien-mga commented Jun 20, 2023

capnm commented Jun 20, 2023

dalexeev commented May 16, 2023 •

edited

Loading

dalexeev commented May 23, 2023 •

edited

Loading

dalexeev May 25, 2023 •

edited

Loading

dalexeev May 25, 2023 •

edited

Loading

dalexeev May 26, 2023 •

edited

Loading

adamscott commented Jun 13, 2023 •

edited

Loading

dalexeev commented Jun 15, 2023 •

edited

Loading

dalexeev commented Jun 16, 2023 •

edited

Loading