-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
[DRAFT] GDExtension: Fix crash by copying GDExtensionScriptInstanceInfo2
on the Godot side
#81205
[DRAFT] GDExtension: Fix crash by copying GDExtensionScriptInstanceInfo2
on the Godot side
#81205
Conversation
@maiself brought up over on #78634 (comment) that this is copying this struct once per script instance which means each instance will take up 23 pointers more memory (92 or 184 bytes depending if it's a 32-bit or 64-bit architecture). That's not ideal... So, perhaps this will remain a draft until we can figure out if there's a better solution. |
So, originally, I didn't like the idea of having the compatibility function allocate a new Maybe that is a better way afterall? |
Tested this, fixes all crashing I was seeing. |
@maiself Thanks for testing! |
PR #81206 has an alternative approach in line with my previous comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to find #81206 a better approach than this.
Superseded by #81206 |
Fixes a crash that was introduced in PR #78634
Previously, Godot was keeping a pointer to the
GDExtensionScriptInstanceInfo
orGDExtensionScriptInstanceInfo2
created on the GDExtension side. The crash is caused because we provided compatibility by copying the data from provided aGDExtensionScriptInstanceInfo
into a newGDExtensionScriptInstanceInfo2
on the stack, which is, of course, temporary. We could certainly allocate a newGDExtensionScriptInstanceInfo2
on the heap, but then we need to keep track of whether it was created on the GDExtension side or the Godot side so we know if we should free it or not.This PR side steps that whole problem by instead making a copy of the struct on the Godot side, which we should probably do anyway, so that the GDExtension can't accidentally modify or free the struct on its side.
This is currently marked as a draft because I haven't had a chance to test it yet!