Virtual functions called on objects produced by GDExtension doesn't work on languages other than C++

[VladoCC]

Godot version

4.0.alpha12

System information

Windows 10

Issue description

When you register a new class from GDExtension, class_get_default_property_value instantiates it through calling create_instance_func passed into GDNativeExtensionClassCreationInfo.
Then it gets casted to Object:

godot/core/object/class_db.cpp

Line 1429 in a9e4eac

c = ClassDB::instantiate(p_class);

After which 'get_property_list' gets called on this Object. And inside of it there are at least 2 calles to virtual functions. Like this one:

godot/core/object/object.cpp

Line 505 in a9e4eac

if (!is_class("Script")) { // can still be set, but this is for user-friendliness

This calles would only makes sense if the object has __vfptr table, which is C++ specific.
Pointof GDExtensions is to allow for other languages to interact with Godot, which this issue limits significantly.

Steps to reproduce

You can return C struct from create_instance_func in your GDExtension and see how Godot crashes because of access violation.
I'm not sure how to describe steps to reproduce otherwise.

Minimal reproduction project

No response

[Splizard]

My understanding is that this is what the GDNativeExtensionClassCreationInfo get_virtual_func is for, it takes a name and is supposed to return a pointer to the class' virtual function implementation so that Godot can call it.

[VladoCC]

Yes, this is how I would expect it to work.
But I don't think that just replacing _vfptr call with this function is going to solve the issue

Yes, it gives extension developer a way to supply function pointers to godot, but they still doesn't know what functions they are expected to supply

[Splizard]

but they still doesn't know what functions they are expected to supply

Well I've encountered this as a challenge whilst writing a Go extension interface, it would be nice if GDNativeExtensionClassCallVirtual passed some sort of userdata argument. My workaround was to generate C functions callMethod1, callMethod2 up to callMethod255 since C doesn't have closures, this allows me to identify which method is being called (limited to having 255 methods attached to an exported type at the moment).

Ie. like this:

extern uint8_t goClassGetVirtual(void *classID, const char *p_name);
extern void goMethodCallDirect(uintptr_t methodIndex, uintptr_t instance, const GDNativeTypePtr *p_args, GDNativeTypePtr r_ret);

void goClassCallVirtual1(GDExtensionClassInstancePtr p_instance, const GDNativeTypePtr *p_args, GDNativeTypePtr r_ret) {goMethodCallDirect((uintptr_t)1, (uintptr_t)p_instance,  p_args, r_ret);}
void goClassCallVirtual2(GDExtensionClassInstancePtr p_instance, const GDNativeTypePtr *p_args, GDNativeTypePtr r_ret) {goMethodCallDirect((uintptr_t)2, (uintptr_t)p_instance,  p_args, r_ret);}
void goClassCallVirtual255(GDExtensionClassInstancePtr p_instance, const GDNativeTypePtr *p_args, GDNativeTypePtr r_ret) {goMethodCallDirect((uintptr_t)255, (uintptr_t)p_instance,  p_args, r_ret);}

GDNativeExtensionClassCallVirtual get_virtual_func(void *p_userdata, const char *p_name) {
  switch (goClassGetVirtual(p_userdata, p_name)) {
	  case 1: return goClassCallVirtual1;
	  case 2: return goClassCallVirtual2;
	  ...
	case 255: return goClassCallVirtual255;
	default: return NULL;
  }
}

Which language are you working with?

[pcting]

but they still doesn't know what functions they are expected to supply

Well I've encountered this as a challenge whilst writing a Go extension interface, it would be nice if GDNativeExtensionClassCallVirtual passed some sort of userdata argument. My workaround was to generate C functions callMethod1, callMethod2 up to callMethod255 since C doesn't have closures, this allows me to identify which method is being called (limited to having 255 methods attached to an exported type at the moment).

Ie. like this:

extern uint8_t goClassGetVirtual(void *classID, const char *p_name);
extern void goMethodCallDirect(uintptr_t methodIndex, uintptr_t instance, const GDNativeTypePtr *p_args, GDNativeTypePtr r_ret);

void goClassCallVirtual1(GDExtensionClassInstancePtr p_instance, const GDNativeTypePtr *p_args, GDNativeTypePtr r_ret) {goMethodCallDirect((uintptr_t)1, (uintptr_t)p_instance,  p_args, r_ret);}
void goClassCallVirtual2(GDExtensionClassInstancePtr p_instance, const GDNativeTypePtr *p_args, GDNativeTypePtr r_ret) {goMethodCallDirect((uintptr_t)2, (uintptr_t)p_instance,  p_args, r_ret);}
void goClassCallVirtual255(GDExtensionClassInstancePtr p_instance, const GDNativeTypePtr *p_args, GDNativeTypePtr r_ret) {goMethodCallDirect((uintptr_t)255, (uintptr_t)p_instance,  p_args, r_ret);}

GDNativeExtensionClassCallVirtual get_virtual_func(void *p_userdata, const char *p_name) {
  switch (goClassGetVirtual(p_userdata, p_name)) {
	  case 1: return goClassCallVirtual1;
	  case 2: return goClassCallVirtual2;
	  ...
	case 255: return goClassCallVirtual255;
	default: return NULL;
  }
}

Which language are you working with?

I've also run into the exact same issue in chat a couple weeks back

In summary, you cannot call a go method directly from C as only functions can be exported to C. So, you have to workaround this by setting up virtual function callback in a non-idiomatic go way.

I was able to implement an example _ready() function shown here in my godot-go library:

//export Example_Ready
func Example_Ready(inst unsafe.Pointer) {
	log.Info("Example_Ready called")

	...
}

as opposed to a method "attached" to a go struct.... something like this:

func (e *Example) Ready() {
	log.Info("Example_Ready called")

	...
}

this effectively requires users of the library to be exposed to cgo as part of the library DSL, which is not ideal for the average go developer. the average go developer won't know the intricacies of working in cgo.

To get this working, you also have to wrap the callback in C code:

void cgo_callback_example_ready(GDExtensionClassInstancePtr p_instance, const GDExtensionTypePtr *p_args, GDExtensionTypePtr r_ret) {
	Example_Ready((void*)(p_instance));
}

this functions can now be registered in go code when initializing the custom GDClass:

gdextension.ClassDBBindMethodVirtual(t, "_ready", (gdextensionffi.GDExtensionClassCallVirtual)(C.cgo_callback_example_ready))

if there was a way to pass a user_data as a parameter, we can hide all this behind the scenes and abstracted away from users of the godot-go library. 💯

maybe a "virtual function call mode" can be provided when registering the GDExtension with Godot... either to keep the current behavior or add in a user_data parameter for GDExtensions that can benefit from it?

[reduz]

So, besides the function pointer, you would like to be requested an userdata (void*) too that can be called together with the function called? If this is the case and it makes everything simpler for you, let me know.

[pcting]

@reduz for my use case, a userdata param would provide the flexibility for me to store a pointer to the go function as the userdata so that i can have a single go function exported to C as a gateway to call into go

[fuzzybinary]

This is also an issue for Dart (where I've used a template metaprogramming aproach to get around it. A similar problem is going to exist for me for GDExtensionInstanceBindingCallbacks as well, as I need to be able to wrap the binding funcitons to jump to the main Dart thread. A single piece of user data would help immensely.

Not sure if I should create a separate issue for the binding callbacks?

[fuzzybinary]

I'm thinking about helping with this, but I don't see any way to add it without breaking compatibility, as it's a change to the GDExtensionClassCreationInfo struct. Are there thoughts on how this can / should be done?

One possibility would be to add a GDExtensionClassCreationInfoEx, then add classdb_register_extension_class_ex. The Ex structure could contain some metadata at the front saying what version of Godot it's from, and future fields can be added to the end. The classdb_register_extension_class_ex function would then only read the structure as far as it knows for the version its looking at.

Thoughts?

[VladoCC]

My opinion is that compatability doesn't matter in this case.

1. This feature was supposed to be finished before beta. At that stage there wasn't any comparability concerns.
2. Coming straight from the 1. It's been a year since creation of this issue. Any solution would be better than none.
3. Compatability with what would be broken? As far as I know there's not much of extensions finished right now, which would be affected.
4. Even if compatability us a concern, this can be a feature for version 4.x. Major versions are expected to not be fully compatable with previous ones as far as I know.

[dsnopek]

I'm thinking about helping with this, but I don't see any way to add it without breaking compatibility, as it's a change to the GDExtensionClassCreationInfo struct. Are there thoughts on how this can / should be done?

One possibility would be to add a GDExtensionClassCreationInfoEx, then add classdb_register_extension_class_ex.

This is actually exactly the approach that we've discussed for doing this at previous GDExtension meetings. We still need to hash out the naming convention, but I'd personally be for something like GDExtensionClassCreationInfo2 and classdb_register_extension_class2 because we may need to extend it multiple times. Anyway, once the new struct and function are there, the old function can be updated to just produce the new struct and then call the new function.

I'm actually working on another PR (#80284) that also needs to add a field to the GDExtensionClassCreationInfo struct, so if both changes get merged in 4.2, they could share the same new extended struct :-)

That said, I haven't yet had a chance to actually look at this issue and evaluate it, I'm just responding to the question of how to make this sort of change to gdextension_interface.h in a compatible way.

[fuzzybinary]

I can work on a PR based on #80284 if you'd like, once we have an approach.

The initial request was to allow a piece of user data to be returned in addition to the function pointer from get_virtual_func, but that would require saving a piece for every virtual function call, which isn't ideal.

I have two potential thoughts. First would be to change GDExtensionClassCallVirtual's signature to also pass the method name:

typedef void (*GDExtensionClassCallVirtual)(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_method, const GDExtensionConstTypePtr *p_args, GDExtensionTypePtr r_ret);

I feel like this would deal with my use case fairly easily. I'd always return the same function for get_virtual_func, then use the name to get the correct function for what I'm calling. Languages or extensions that can supply C++ / vtable like function pointers can safely ignore the function name. Go and others can likely switch on the name.

The second would be to instead add a function to GDExtensionClassCreationInfo called call_virtual_func with a signature similar to above. Users that need the extra info would set this instead of get_virtual_func, and it would be called on virtual functions if get_virtual_func is not bound.

Personally, I like the first solution better.

[dsnopek]

I can work on a PR based on #80284 if you'd like, once we have an approach.

It may take a while for PR #80284 to be finished, so I wouldn't base your work on it. However, PR #78634 is currently in progress of adding the necessary compatibility changes. It's not quite there yet, but once all the review is addressed it should be a good example, and hopefully it won't take too long to get merged.

The initial request was to allow a piece of user data to be returned in addition to the function pointer from get_virtual_func, but that would require saving a piece for every virtual function call, which isn't ideal.

I have two potential thoughts. First would be to change GDExtensionClassCallVirtual's signature to also pass the method name:

typedef void (*GDExtensionClassCallVirtual)(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_method, const GDExtensionConstTypePtr *p_args, GDExtensionTypePtr r_ret);

Hm, I'm not sure about this. In your case the method name might be enough, but what if the particular language binding needs a pointer to some object in the VM (for a VM-based language)? I don't entirely understand the Go case described above (because I'm not very familiar with Go), but it sounds like they may need more than the name too?

We don't have a C language binding, but imagining in my head how I'd want a nice C binding to work, I think I'd probably want to use a pointer to the user's C function (with nice argument types) in the user data, and give Godot a pointer to a different C function (defined in the language bindings, not the user code) that can decode those arguments into the right types and call the user's C function.

The second would be to instead add a function to GDExtensionClassCreationInfo called call_virtual_func with a signature similar to above. Users that need the extra info would set this instead of get_virtual_func, and it would be called on virtual functions if get_virtual_func is not bound.

So, you're imagining that we have a get_virtual_call_data callback that returns the user data, and which would then be passed to call_virtual_func? Assuming I'm understanding this correctly, I think I like this approach better, because it's more flexible. And it would work for my imaginary C binding :-)

[fuzzybinary]

In reality both ideas were just about sending in the method name with the call, not any user data. I've had a chance to look over how virtual methods in Godot work, and I'm starting to think that letting extension authors keep track of user data for virtual methods is going to be better, even though it (might) be more work on their part, it would end up saving Godot a lot of memory it might not need to allocate. At least not without a complete re-think on how Godot handles virtual methods. Here's why:

Every virtual methods in Godot generate 3 variables in the final class (from core/object/make_virtuals.py):

StringName _gdvirtual_##m_name##_sn = #m_name;\\
mutable bool _gdvirtual_##m_name##_initialized = false;\\
mutable GDExtensionClassCallVirtual _gdvirtual_##m_name = nullptr;\\

When a virtual method is called, it checks if the virtual is initialized. If it's not, it calls the get_virtual_func for the extension, and caches that for future use on this object. To allow user data, we'd have to add another variable:

mutable void* _gdvirtual_##m_name##_user_data = nullptr;

And change the signature of get_virtual_func to set that variable. The extra pointer incurs a 4-8 byte memory hit per virtual function. I'm already surprised virtual functions incur a 8-16 byte memory hit per function depending on memory alignment because of the padding on the bools (but that's a discussion for another time).

My guess is most extension developers can get everything they need just from the GDExtensionClassInstancePtr and the virtual function name if they were provided during the actual function call, even if they have to save something out during the get_virtual_func call. I'd be interested to hear from @pcting or @Splizard if this would be sufficient for Go.

TL;DR - Adding user data for virtual methods incurs a 4-8 byte penalty per virtual method per object regardless of whether that object is ever overridden in any extension. That seems excessive to me, but maybe that's a trade-off we're willing to make?

[dsnopek]

And change the signature of get_virtual_func to set that variable. The extra pointer incurs a 4-8 byte memory hit per virtual function. I'm already surprised virtual functions incur a 8-16 byte memory hit per function depending on memory alignment because of the padding on the bools (but that's a discussion for another time).

If we went with the approach that I thought you were suggesting above:

So, you're imagining that we have a get_virtual_call_data callback that returns the user data, and which would then be passed to call_virtual_func?

... then we'd only need to keep the one pointer for the user data, since we'd always be calling the call_virtual_func function pointer.

I think we could even have that one pointer do double duty for the compatibility mode: if the GDExtension provides the older get_virtual_func then it'd store that function pointer in there, but if it provides get_virtual_call_data and call_virtual_func it could store the user data pointer instead.

Anyway, this is probably something that should be discussed at a GDExtension meeting! And, ideally, we should get input from as many language binding authors as we can.

[fuzzybinary]

So after the GDExtension meeting I think I get the need for User Data over just passing the name, but let me see if I have your logic right in pseudo code format:

if virtual_call_is_initialized == false {
  if get_virtual_call_data != null {
     _gdvirtual_##m_name = get_virtual_call_data(class_userdata, virtual_name)
  } else {
     // Call get_virtual_func as normal
     _gdvirtual_##m_name = get_virtual_func(class_userdata, virtual_name)
  }
} else {
   if get_virtual_call_data != null {
     // Here _gdvirtual##m_name is the user data and not the function
     call_virtual_func(extension_instance, _gdvirtual_##m_name, ptr_args, ptr_ret)
   } else {
      _gdvirtual_##m_name(extension_instance, ptr_args, ptr_ret)
   }
}

I worry a little bit about doubling up the use of that pointer, but it does avoid the extra memory allocation. We may also want to drop a warning or something in somewhere that checks that if you bind get_virtual_call_data, you must bind call_virtual_func, and you should NOT bind get_virtual_func, as it will never be called.

Does that all look correct?

[dsnopek]

Yep, that's basically what I was describing above!

We may also want to drop a warning or something in somewhere that checks that if you bind get_virtual_call_data, you must bind call_virtual_func, and you should NOT bind get_virtual_func, as it will never be called.

This should definitely all be documented in comments in gdextension_interface.h. A warning may make sense, but I'm not totally sure: the warnings are generally aimed at the end-developer (so, the person using the language binding), whereas this warning would be for the developer creating the binding. In GDExtension currently we mostly don't validate what the binding is doing.

[fuzzybinary]

So... I was implementing this and realized that it really should have an extra method: free_virtual_call_data. That means hooking into object destruction -- I'll have to look how hard that's going to be.

Alternately, we assume / document that pointers returned from get_virtual_call_data are safe to be deleted (and should be deleted) when free_instance_func is called.

I was able to implement this for godot_dart without the free (as the only data I need fits in a pointer) and it works pretty well.

[fuzzybinary]

Opened a draft PR for this anyway, assuming we don't want to add complexity to Object destruction.

[dsnopek]

Thanks for starting the PR!

So... I was implementing this and realized that it really should have an extra method: free_virtual_call_data. That means hooking into object destruction -- I'll have to look how hard that's going to be.

Ah, yeah, if the user data is constructed specifically for the given Object instance then freeing it with the instance would be very tricky. We'd need to do something similar to the tracking being added in PR #80284 for hot-reload, but that's purposefully only enabled when necessary (ie. not in release builds) to avoid the performance hit of the extra accounting.

Perhaps we should just say as part of the API that user data shouldn't be constructed for a given Object, but for the class as a whole, and should be freed (if necessary) by the GDExtension when it's being deinitialized? That way this problem isn't handled at all on the Godot side, it's all up to the GDExtension to manage. (That would also fit in pretty nicely with how the hot-reload PR works so far.)

This would also mean we don't need to add anything additional to your PR to handle this. :-)

Alternately, we assume / document that pointers returned from get_virtual_call_data are safe to be deleted (and should be deleted) when free_instance_func is called.

I'm not sure I understand what you mean by this.

If you mean that the GDExtension should free the user data when the free_instance_func is called for a given Object, I don't think the GDExtension would even know which user data was constructed for that Object, since we aren't passing the Object instance to get_virtual_call_data_func.

Or, if you mean that the Godot side will attempt to free the user data when it's calling free_instance_func, I don't think that's safe to do, since Godot won't know how the memory was allocated.

[fuzzybinary]

@pcting @VladoCC The PR attached to this should be ready for testing. Would you be able to take some time to see if it works for your use cases? There are still open questions about how these fields get added in 4.2 without breaking compatibility with 4.1 extensions.

[pcting]

@pcting @VladoCC The PR attached to this should be ready for testing. Would you be able to take some time to see if it works for your use cases? There are still open questions about how these fields get added in 4.2 without breaking compatibility with 4.1 extensions.

I've got a PR build against your godot changes. I've take the godot-cpp test harness as a template for this repo. I've been able to get _ready implemented. , but i'm having trouble implementing functions with gdextension objects arguments such as _input used in the godot-cpp unit test because of Ref<InputKeyEvent> as i'm unsure of how to marshal between C/C++ memory layout and go at this point in time.

Edit: virtual functions are working as expected