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

[GDExtension] Casting fails when virtual functions are added #610

Closed
BastiaanOlij opened this issue Sep 14, 2021 · 4 comments
Closed

[GDExtension] Casting fails when virtual functions are added #610

BastiaanOlij opened this issue Sep 14, 2021 · 4 comments
Labels
bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation

Comments

@BastiaanOlij
Copy link
Collaborator

BastiaanOlij commented Sep 14, 2021

Further investigating why Ref<> isn't working properly I found a weird issue with casting. In C++ when inheriting it is possible that data structures shift and there is thus no guarantee that the pointer is the same. So:

BaseClass *base_class = (BaseClass *)sub_class;

Could result in the base_class pointer not being equal to the sub_class pointer.

The compiler deals with this nicely, so doing the following on our wrapper classes in GDExtensions:

Ref<XRInterfaceReference> interface;
interface.instantiate();

XRInterfaceReference *xr_interface_reference = interface.ptr();
XRInterfaceExtension *xr_interface_extension = (XRInterfaceExtension *)xr_interface_reference;
XRInterface *xr_interface = (XRInterface *)xr_interface_reference;
RefCounted *ref_counted = (RefCounted *)xr_interface_reference;
Object *object = (Object *)xr_interface_reference;

UtilityFunctions::print("XRInterfaceReference: ", Variant((uint64_t) xr_interface_reference),", owner: ", Variant((uint64_t) xr_interface_reference->_owner));
UtilityFunctions::print("XRInterfaceExtension: ", Variant((uint64_t) xr_interface_extension),", owner: ", Variant((uint64_t) xr_interface_extension->_owner));
UtilityFunctions::print("XRInterface: ", Variant((uint64_t) xr_interface),", owner: ", Variant((uint64_t) xr_interface->_owner));
UtilityFunctions::print("RefCounted: ", Variant((uint64_t) ref_counted),", owner: ", Variant((uint64_t) ref_counted->_owner));
UtilityFunctions::print("Object: ", Variant((uint64_t) object),", owner: ", Variant((uint64_t) object->_owner));

We can see this casts nicely:

XRInterfaceReference: 2226252139088, owner: 2226061244336
XRInterfaceExtension: 2226252139088, owner: 2226061244336
XRInterface: 2226252139096, owner: 2226061244336
RefCounted: 2226252139096, owner: 2226061244336
Object: 2226252139096, owner: 2226061244336

But also notice that from XRInterface our pointer is shifted 8 bytes, something is added to the XRInterfaceExtension data which the compiler puts in front of instead of after the data from the super class.

So for now not a problem, until we use Godots own casting logic:

RefCounted *ref_counted_casted = Object::cast_to<RefCounted>(xr_interface_reference);
UtilityFunctions::print("Casted: ", Variant((uint64_t) ref_counted_casted),", owner: ", Variant((uint64_t) ref_counted_casted->_owner));

Now our output is:

Casted: 2226252139088, owner: 140723262435304

We see that our original pointer is used but because this is incorrect for our RefCounted class we're reading the wrong memory for owner causing a crash if we attempt to use this object.

The reason for this is that our Object::cast_to<T> uses the owner to ask Godot to perform the cast, and then lookup the wrapper pointer belonging to that owner, which is still our subclass' pointer.

This approach to casting thus only works if there is no shift in the data.

So far my theory is that the shift is caused by the introduction of a vtable for the new virtual functions that were introduced and the pointer to this vtable precedes the subclass struct.

@BastiaanOlij BastiaanOlij added bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation labels Sep 14, 2021
@dsnopek
Copy link
Collaborator

dsnopek commented Aug 15, 2023

I strongly suspect this issue is fixed by #1050

Before that PR was merged, you could easily end up in a situation were the actual wrapper was a parent class, and godot-cpp happily (and unsafely) cast it to a child class, which would have had the wrong vptr and perhaps the fields lined up differently in memory.

@MJacred
Copy link
Contributor

MJacred commented Jan 22, 2024

@BastiaanOlij: ping! was this fixed with #1050 as mentioned above?

@BastiaanOlij
Copy link
Collaborator Author

@MJacred should have been, it's a long long time ago since we had someone report issues around this.

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 23, 2024

Let's close for now. We can always reopen :-)

@dsnopek dsnopek closed this as completed Jan 23, 2024
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 topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
Development

No branches or pull requests

3 participants