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

Segfault in Object::clear_internal_extension, reproducable on GDExtension hot reload. #92063

Closed
Splizard opened this issue May 18, 2024 · 2 comments

Comments

@Splizard
Copy link
Contributor

Splizard commented May 18, 2024

Tested versions

  • Reproducable in Godot Engine v4.2.2.stable.official

System information

Linux

Issue description

When attempting to add support for hot-reloading to the Go GDExtension bindings I have been developing, I have run into a segfault which appears to me like it could be a bug in Godot.

Object::clear_internal_extension derefences _instance_bindings[0], which my debugger tells me is a nullptr.
image

My GDExtension doesn't setup any instance bindings, it's not clear in any documentation that these are required, other parts of this file are checking whether or not _instance_bindings is a nullptr.

@dsnopek, I understand you worked on hot reloading in Godot and you are probably best to comment on this, is this a mistake, or are GDExtensions required to setup instance bindings in order to avoid unsafe behaviour?

Steps to reproduce

Create a hot-reloadable GDExtension, don't setup any instance bindings, register a class, create an instance of your class in the editor, recompile your extension (with the editor open), unregister any extension classes, segfault.

Minimal reproduction project (MRP)

Available on request.

Splizard added a commit to grow-graphics/gd that referenced this issue May 18, 2024
It works when bypassing
godotengine/godot#92063 with an if check added
to the Godot engine to prevent the segfault. No known workarounds on the
Go side at this stage.
@dsnopek
Copy link
Contributor

dsnopek commented May 18, 2024

My GDExtension doesn't setup any instance bindings, it's not clear in any documentation that these are required

It is required to set an instance binding. All instances of Godot classes created by a GDExtension binding should call object_set_instance_binding() towards the end of their creation process.

We really do need more documentation about these things! @vnen started a documentation page explaining how to use the GDExtension interface from C, and I even added a note on the PR about how object_set_instance_binding() always needs to be called. :-)

@dsnopek, I understand you worked on hot reloading in Godot and you are probably best to comment on this, is this a mistake, or are GDExtensions required to setup instance bindings in order to avoid unsafe behaviour?

The instance binding is needed for more than just hot reload. If you don't set it, then it's possible that a new wrapper class could be created in your GDExtension for the same Godot object. Ideally, you want to have a one-to-one relationship between Godot instances and their GDExtension wrappers.

@Splizard
Copy link
Contributor Author

Splizard commented May 18, 2024

Thanks, have resolved the issue by calling object_set_instance_binding with zero value for binding and an empty (zero-initialised GDExtensionInstanceBindingCallbacks. ie

GDExtensionInstanceBindingCallbacks callbacks = {};
object_set_instance_binding(extensionClass, extensionToken, nullptr, &callbacks);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants