method binding on non-MSVC compilers relies on undefined behavior

[n-morales]

Godot version

all versions since at least 3 years ago (it's as far back as I looked)

godot-cpp version

all versions since at least 3 years ago

System information

non-MSVC compilers

Issue description

Method binding in godot-cpp relies on undefined behavior if TYPED_METHOD_BIND is not defined. It seems that it's only enabled with the MSVC compiler.

Here, reinterpret_cast is used on an unrelated type to cast to MB_T:

godot-cpp/include/godot_cpp/core/method_bind.hpp

Lines 319 to 326 in 4131b7f

	virtual Variant call(GDExtensionClassInstancePtr p_instance, const GDExtensionConstVariantPtr *p_args, GDExtensionInt p_argument_count, GDExtensionCallError &r_error) const {
	#ifdef TYPED_METHOD_BIND
	call_with_variant_args_dv(static_cast<T *>(p_instance), method, p_args, (int)p_argument_count, r_error, get_default_arguments());
	#else
	call_with_variant_args_dv(reinterpret_cast<MB_T *>(p_instance), method, p_args, p_argument_count, r_error, get_default_arguments());
	#endif
	return Variant();
	}

Here, reinterpret_cast is used on unrelated member function pointers:

godot-cpp/include/godot_cpp/core/method_bind.hpp

Lines 342 to 351 in 4131b7f

	template <typename T, typename... P>
	MethodBind *create_method_bind(void (T::*p_method)(P...)) {
	#ifdef TYPED_METHOD_BIND
	MethodBind *a = memnew((MethodBindT<T, P...>)(p_method));
	#else
	MethodBind *a = memnew((MethodBindT<P...>)(reinterpret_cast<void (MB_T::*)(P...)>(p_method)));
	#endif // TYPED_METHOD_BIND
	a->set_instance_class(T::get_class_static());
	return a;
	}

reinterpret_casting here causes UB as the resulting member to function pointer cannot be used safely, as you are not casting to unsigned char * or something like that. Moreover, casting to a member function pointer of a non-base class is also UB. Member function pointers are implementation defined, so honestly, there is no way to rely on what the compiler will do here.

It seems like MSVC actually errors on the UB here which is really nice. I think that switching to TYPED_METHOD_BIND would be the right thing to do here.

If y'all are open to it, I could work on a fix. I think it would just be enabling that switch on for all builds.

[dsnopek]

Godot itself is using the same technique (this code was originally copied from Godot). So, I don't think we should address this in godot-cpp, before first addressing it in Godot. So, either this issue should be moved to Godot, or another issue opened there.

That said, I'm not personally familiar enough with how member pointers are defined in the C++ standards, in order to comment on whether or not this is something that actually needs fixing in the first place.

[n-morales]

Should I open the issue in Godot? Or can you move it?

[dsnopek]

I'd recommend opening a new issue on Godot. If it gets updated in Godot, then we can use this issue to track the follow-up changes to godot-cpp.