Virtual functions invoked through Callable aren't correctly dispatched to GDExtension

[Bromeon]

Tested versions

• Reproducible in 4.3-stable.

Issue description

Some virtual methods are not invoked via GDVIRTUAL_CALL, but using a Callable:

godot/editor/plugins/lightmap_gi_editor_plugin.cpp

Line 204 in 0f20e67

bake->connect(SceneStringName(pressed), Callable(this, "_bake"));

godot/editor/plugins/occluder_instance_3d_editor_plugin.cpp

Line 111 in 0f20e67

bake->connect(SceneStringName(pressed), Callable(this, "_bake"));

godot/modules/gdscript/gdscript_vm.cpp

Line 2570 in 0f20e67

Error err = sig.connect(Callable(gdfs.ptr(), "_signal_callback").bind(retvalue), Object::CONNECT_ONE_SHOT);

godot/scene/resources/compositor.cpp

Line 108 in 0f20e67

rs->compositor_effect_set_callback(rid, RenderingServer::CompositorEffectCallbackType(effect_callback_type), Callable(this, "_render_callback"));

godot/scene/resources/compositor.cpp

Line 185 in 0f20e67

rs->compositor_effect_set_callback(rid, RenderingServer::CompositorEffectCallbackType(effect_callback_type), Callable(this, "_render_callback"));

(just briefly searched with regex Callable\(.+, "_, there might be more)

---

Impact

This seems OK for GDScript, but for GDExtension this means that the call is not going through this API:

GDExtensionClassGetVirtual get_virtual_func;

So a GDExtension binding that exposes a virtual method like CompositorEffect._render_callback will never have this method called. However, if the method is declared as a regular one, it works -- but this is quite unintuitive.

---

Solution

I'm not sure what the best solution would be -- maybe an indirection?
I.e. could the Callable point to a function that invokes GDVIRTUAL_CALL?

[dsnopek]

My initial feeling is that using a Callable() to call a virtual method doesn't seem like the right way to do it. It's similar to how we still sometimes find code in Godot that does object->get_script_instance()->call(...) rather than GDVIRTUAL_CALL(), and need to make a bug fix to switch to GDVIRTUAL_CALL().

Also, looking at some of the examples in the OP, not all of them appear to deal with virtual methods. For example, the _bake() methods on lightmap_gi_editor_plugin.cpp and occluder_instance_3d_editor_plugin.cpp aren't virtual functions, those look like a case where callable_mp() should be used just to call a concrete C++ method.

I think only _render_callback() on CompositorEffect is a virtual method. And in that case, it seems like a bug. I think we should add a CompositorEffect::_call_render_callback() method which uses GDVIRTUAL_CALL(), and then use callable_mp(this, &CompositorEffect::_call_render_callback) to make the Callable.

Are there any other examples of this that you are aware of?

[Bromeon]

Are there any other examples of this that you are aware of?

No, in fact CompositorEffect::_render_callback() is exactly the method that a godot-rust user reported.

And as you see from my false-positive-riddled regex, there might be better approaches to finding others 😉 not sure how to do that systematically. Check for every method declared as virtual in extension_api.json, whether there's at least one GDVIRTUAL_CALL statement in the codebase?

[dsnopek]

I threw together draft PR #99981 that does what I described earlier to address CompositorEffect specifically

Check for every method declared as virtual in extension_api.json, whether there's at least one GDVIRTUAL_CALL statement in the codebase?

That sounds like it'd be a decent way to check! Of course, it only means that the right thing is being done at least once, not that there is no code doing the wrong thing, but it would have detected this case.

[Bromeon]

That was super fast, thanks a lot! ❤️

---

That sounds like it'd be a decent way to check!

So I extracted all virtual functions inside classes declared in extension_api.json (just quick&dirty inside godot-rust parsing code).
Let's call this file api-declared-virtuals.txt. Each line is a method name, the file is sorted and unique.

Then, I extracted all GDVIRTUAL_CALL statements in the Godot codebase, with their method name, as follows:

cd path/to/godot

# Find all virtual calls in Godot code base
rg --no-filename --only-matching "GDVIRTUAL_CALL\([a-zA-Z0-9_]+" | sed 's/GDVIRTUAL_CALL(//' \
   | sort | uniq > found-virtuals.txt

Now I have two sorted unique files, I can get the difference.

comm -23 api-declared-virtuals.txt found-virtuals.txt > never_called.txt
comm -13 api-declared-virtuals.txt found-virtuals.txt > called_but_not_exposed.txt

There's a huge list for never_called, which surprises me.

$ wc -l *.txt

 1030 api-declared-virtuals.txt
    4 called_but_not_exposed.txt
  507 found-virtuals.txt
  527 never_called.txt

I checked a few and many have GDVIRTUAL_BIND but no call, which only registers them, no? Can't be that half the virtual methods is never called, what am I missing?

Here are the files, ran on commit 47bc374.

• api-declared-virtuals.txt
• found-virtuals.txt
• never_called.txt
• called_but_not_exposed.txt

[dsnopek]

@Bromeon: Thanks for doing this investigation!

I checked a few and many have GDVIRTUAL_BIND but no call, which only registers them, no? Can't be that half the virtual methods is never called, what am I missing?

Ah, I think you're missing the EXBIND*() macros. Those can be used to generate the functions that do GDVIRTUAL_CALL(). For example, take a look at PhysicsServer2DExtension - it uses the EXBIND*() macros quite extensively.

[dsnopek]

I just posted PR #100126 to address the ones in your called_but_not_exposed.txt.

Two of them appear to be false positives (m_name and _create_editor), but I added the missing GDVIRTUAL_BIND() for the other two.

(FYI, the GDVIRTUAL_BIND() registers them so they get listed in extension_api.json and the documentation, but that isn't necessary for them to work with GDScript, which is probably why no one noticed these were missing.)

[Bromeon]

Cool, thank you!

I tried to look for EXBIND* with the following, then prefixed with _:

rg --no-filename --only-matching "EXBIND[A-Za-z0-9_]+\([a-zA-Z0-9_]+, [a-zA-Z0-9_]+" | sd ".+, " "" > found-exbinds.txt
rg --no-filename --only-matching "EXBIND[A-Za-z0-9_]+\([a-zA-Z0-9_]+" | sd ".+, " "" > found-exbinds-2.txt

With that (+previous list), I could limit the never_called_v2.txt list to 11 entries:

_body_get_direct_state
_create_data_channel
_get_base_script
_get_contact_collider_object
_get_language
_get_space_state
_make_template
_put_partial_data
_render_callback
_space_get_contacts
_space_get_direct_state

Some of them seem to be false positives, e.g. my regex didn't catch the star here:

EXBIND0R(PhysicsDirectSpaceState3D *, get_space_state)

Could you maybe verify if those virtual methods are truly called or not?

[Bromeon]

Could anyone verify if the remaining methods in my comment are registered correctly?

[dsnopek]

Could anyone verify if the remaining methods in my comment are registered correctly?

Thanks for the reminder!

I just went through all the items on your list, and most were false positives from EXBIND*() with return values with pointers or Ref<T> or similar. But there is one true positive!

StreamPeerExtension::put_partial_data() is calling the wrong virtual method:

Error StreamPeerExtension::put_partial_data(const uint8_t *p_data, int p_bytes, int &r_sent) {
	Error err;
	if (GDVIRTUAL_CALL(_put_data, p_data, p_bytes, &r_sent, err)) {
		return err;
	}
	WARN_PRINT_ONCE("StreamPeerExtension::_put_partial_data is unimplemented!");
	return FAILED;
}

I'll make a PR to fix it in a moment!

[dsnopek]

Here's PR #100318 to fix that last issue

[Bromeon]

Very good, thanks a lot for your help with hunting all those down! 💂‍♂