Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global godot-type variable cause crash and load gdextension failed #1449

Open
pupil1337 opened this issue Apr 26, 2024 · 7 comments
Open

Global godot-type variable cause crash and load gdextension failed #1449

pupil1337 opened this issue Apr 26, 2024 · 7 comments
Labels
bug This has been identified as a bug crash needs testing

Comments

@pupil1337
Copy link
Contributor

pupil1337 commented Apr 26, 2024

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: #358 (comment).

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 pupil1337 changed the title Global variable cause crash and load gdextension failed Global godot-type variable cause crash and load gdextension failed Apr 26, 2024
@AThousandShips AThousandShips added bug This has been identified as a bug crash needs testing labels Apr 26, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Apr 26, 2024

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?

@pupil1337
Copy link
Contributor Author

pupil1337 commented Apr 27, 2024

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).

@OffsetMOSFET
Copy link

OffsetMOSFET commented Jul 29, 2024

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;
}

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 29, 2024

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
Copy link
Collaborator

dsnopek commented Sep 11, 2024

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.

@pupil1337
Copy link
Contributor Author

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.

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug crash needs testing
Projects
None yet
Development

No branches or pull requests

4 participants