godot-cpp icon indicating copy to clipboard operation
godot-cpp copied to clipboard

Global godot-type variable cause crash and load gdextension failed

Open pupil1337 opened this issue 1 year ago • 7 comments

Godot version

4.3.dev(11d3768)

godot-cpp version

4.3.dev(e23b117)

System information

win11

Issue description

This issue have existed for a long time, The reproduction steps are the same as this comment: https://github.com/godotengine/godot-cpp/issues/358#issuecomment-566168170.

String s; // crashes
class SomeClass {
    static Dictionary d; // crashes
    static Node *singleton; // fine, pointer doesn't call a constructor
};

This issue is quite tricky, as it is caused by godot load gdextension .dll and initializate GDExtensionInterface order. Do need to solve this problem?

Steps to reproduce

// world.h

const String level0 = "res://level/level0.tscn"; // load gdextension.dll will call String constructor, but this time gdextension function didnt initialized

class World : public Node {
	GDCLASS(World, Node)
	static void _bind_methods() {}
};

Define a global variable String, open godot.exe timeline:

  1. start open godot.exe
  2. godot.exe: Load gdextension.dll -> dll: global variable Construct -> dll: call String construct GDExtensionInterface(not initialzed and will Crash)
  3. godot.exe: initialize gdextension.dll GDExtensionInterface

Minimal reproduction project

N/A

pupil1337 avatar Apr 26 '24 04:04 pupil1337

Well, we can't make Godot types work if they are constructed before the GDExtension has been initialized.

It would be nice to not crash, although, I'm not sure how feasible that is. We could wrap everything in a check to see if it's been initialized yet, and then output a message, I guess?

dsnopek avatar Apr 26 '24 20:04 dsnopek

There maybe two types that need to be checked:

  1. function pointer type declare in gdextension_interface.h (define in godot.cpp)
  2. _method_bindings in the Variant type(for example in string_name.cpp).

Checking if they are nullptr (or assignment placeholder) requires writing a big logic and the final outcome still cannot declare global variables. I think it's better to not solved it (global variable not many usage scenarios and it's not necessary).

pupil1337 avatar Apr 27 '24 11:04 pupil1337

Doing some experimentation, one type of static storage does work: static variables inside of functions.

int Foo::get_increment()
{
  static int counter {0};
  ++counter;
  return counter;
}

OffsetMOSFET avatar Jul 29 '24 06:07 OffsetMOSFET

one type of static storage does work: static variables inside of functions

Yes, if the function is called after the GDExtension has been initialized, that should work.

dsnopek avatar Jul 29 '24 12:07 dsnopek

From my comment on #1557:

It would be nice to not crash, however, that would probably mean adding a check to see if GDExtension has been initialized yet to all constructors, and the only things we could do is (1) construct an invalid object that wouldn't actually work if you tried to use it later, or (2) crash but with a better error message :-)

I'm starting to think that maybe we should try to implement nr 2? Using unlikely() we could probably make the check speedy enough, and we'd just be adding this to constructors. (It's possible to call other functions too early as well, but I feel like that's much easier for developers to figure out, and hasn't really been a frequent support issue in the way that constructing objects too early has been.)

This limitation is also something that could use to be documented somewhere.

dsnopek avatar Sep 11 '24 14:09 dsnopek

I think we can initialize the gdextension-interface function pointer as a FuncA instead of nullptr, FuncA output an error: "Call constructor too early, earlier than GDExtendenInterface initialization, please check if global godot variables are declared" (similar error output and crash). After the initialization of the existing GDExtendeInterface, it will overwrite our FuncA, so there is no need to call the unlike() function to check.

pupil1337 avatar Sep 11 '24 15:09 pupil1337

I think we can initialize the gdextension-interface function pointer as a FuncA instead of nullptr, FuncA output an error: "Call constructor too early, earlier than GDExtendenInterface initialization, please check if global godot variables are declared" (similar error output and crash).

Ooo, that's an interesting idea! However, it would be nice to say in the error message the name of the object that's being constructed, and I think the only way to do that would be to have a FuncA for each class. I think that's doable, though.

dsnopek avatar Sep 11 '24 15:09 dsnopek