Possible regression with validated calls? #83016

Bromeon · 2023-10-08T22:00:29Z

Godot version

4.2-dev (2 versions, see below)

System information

Windows 10

Issue description

Our godot-rust CI has started failing with recent Godot master versions; there were two events. I did a bisect-style mass test run to find where exactly the behavior changed.

GDScript: Replace ptrcalls on MethodBind to validated calls #79893, commit 4a7d49a, CI run
This one made a particular test fail, but seems relatively deterministic. The PR looks GDScript-focused at first, but changes the way how calls work.
GDExtension: Convert validated_call() to ptrcall() (rather than call()) #82794, commit a6a2d0d, CI run
This one fails even before actual tests are run.

It's of course possible that we are doing something wrong, i.e. godot-rust contained a bug that was already present and only manifested now. If that's the case, any ideas in which direction to look? Have others encountered similar issues?

For 2., we also have a stacktrace in the CI job. Click to expand...

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.2.dev.custom_build (a6a2d0d1599835364cdf5f912f98d87c5317dd05)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x43090) [0x7f4f798db090] (??:0)
[2] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0x4d30ce) [0x7f4f760c30ce] (??:0)
[3] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0x4c16ec) [0x7f4f760b16ec] (??:0)
[4] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0x52be3) [0x7f4f75c42be3] (??:0)
[5] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0x1913c4) [0x7f4f75d813c4] (??:0)
[6] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0x525c1) [0x7f4f75c425c1] (??:0)
[7] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0xca9c5) [0x7f4f75cba9c5] (??:0)
[8] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0x41702e) [0x7f4f7600702e] (??:0)
[9] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0x42c56b) [0x7f4f7601c56b] (??:0)
[10] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0x402f8e) [0x7f4f75ff2f8e] (??:0)
[11] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0x6d996) [0x7f4f75c5d996] (??:0)
[12] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0x1c0d88) [0x7f4f75db0d88] (??:0)
[13] /home/runner/work/godot4-mass-tester/godot4-mass-tester/gdext/itest/godot/../../target/debug/libitest.so(+0x80930) [0x7f4f75c70930] (??:0)
[14] GDExtensionMethodBind::ptrcall(Object*, void const**, void*) const (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/extension/gdextension.cpp:255)
[15] GDExtensionMethodBind::validated_call(Object*, Variant const**, Variant*) const (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/extension/gdextension.cpp:242)
[16] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/modules/gdscript/gdscript_vm.cpp:1956)
[17] GDScriptInstance::callp(StringName const&, Variant const**, int, Callable::CallError&) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/modules/gdscript/gdscript.cpp:1849)
[18] Object::callp(StringName const&, Variant const**, int, Callable::CallError&) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/object/object.cpp:752)
[19] Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/variant/callable.cpp:62)
[20] _VariantCall::func_Callable_call(Variant*, Variant const**, int, Variant&, Callable::CallError&) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/variant/variant_call.cpp:1025)
[21] /home/runner/work/_temp/godot_bin/godot.linuxbsd.editor.dev.x86_64(+0x6c64dbc) [0x556be5b52dbc] (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/variant/variant_call.cpp:2038)
[22] Variant::callp(StringName const&, Variant const**, int, Variant&, Callable::CallError&) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/variant/variant_call.cpp:1182)
[23] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/modules/gdscript/gdscript_vm.cpp:1668)
[24] GDScriptInstance::callp(StringName const&, Variant const**, int, Callable::CallError&) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/modules/gdscript/gdscript.cpp:1849)
[25] Object::callp(StringName const&, Variant const**, int, Callable::CallError&) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/object/object.cpp:752)
[26] Variant::callp(StringName const&, Variant const**, int, Variant&, Callable::CallError&) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/variant/variant_call.cpp:1168)
[27] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/modules/gdscript/gdscript_vm.cpp:1668)
[28] GDScriptFunctionState::resume(Variant const&) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/modules/gdscript/gdscript_function.cpp:216)
[29] GDScriptFunctionState::_signal_callback(Variant const**, int, Callable::CallError&) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/modules/gdscript/gdscript_function.cpp:157)
[30] MethodBindVarArgTR<GDScriptFunctionState, Variant>::call(Object*, Variant const**, int, Callable::CallError&) const (/home/runner/work/godot4-mass-tester/godot4-mass-tester/./core/object/method_bind.h:265 (discriminator 4))
[31] Object::callp(StringName const&, Variant const**, int, Callable::CallError&) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/object/object.cpp:774)
[32] Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/variant/callable.cpp:62)
[33] CallableCustomBind::call(Variant const**, int, Variant&, Callable::CallError&) const (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/variant/callable_bind.cpp:145)
[34] Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/variant/callable.cpp:50)
[35] Object::emit_signalp(StringName const&, Variant const**, int) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/core/object/object.cpp:1128)
[36] Error Object::emit_signal<>(StringName const&) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/./core/object/object.h:920)
[37] SceneTree::physics_process(double) (/home/runner/work/godot4-mass-tester/godot4-mass-tester/scene/main/scene_tree.cpp:466)
[38] Main::iteration() (/home/runner/work/godot4-mass-tester/godot4-mass-tester/main/main.cpp:3550)
[39] OS_LinuxBSD::run() (/home/runner/work/godot4-mass-tester/godot4-mass-tester/platform/linuxbsd/os_linuxbsd.cpp:933)
[40] /home/runner/work/_temp/godot_bin/godot.linuxbsd.editor.dev.x86_64(main+0x193) [0x556be190a78c] (/home/runner/work/godot4-mass-tester/godot4-mass-tester/platform/linuxbsd/godot_linuxbsd.cpp:76)
[41] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7f4f798bc083] (??:0)
[42] /home/runner/work/_temp/godot_bin/godot.linuxbsd.editor.dev.x86_64(_start+0x2e) [0x556be190a53e] (??:?)
-- END OF BACKTRACE --
================================================================
/home/runner/work/_temp/ab405092-db9f-4f07-939c-f211055b54cf.sh: line 8:  3196 Aborted                 (core dumped) $GODOT4_BIN --headless 2>&1

Steps to reproduce

Build Godot 4a7d49a89a381f78f19d0b989c5cb5b500f098c9
Build godot-rust/gdext master (a92164bb3a98108b95506f80769408583421e6dc)
Run integration test suite

Minimal reproduction project

N/A

The text was updated successfully, but these errors were encountered:

lilizoey · 2023-10-09T12:01:03Z

I added some asserts to check for null pointers (https://github.com/lilizoey/gdextension/tree/feature/ptrcall-errors), and it appears that we're getting null pointers as return pointers in some cases

~/D/r/g/i/godot (feature/ptrcall-errors) [SIGABRT]> RUST_BACKTRACE=1 ~/Programs/Godot/godot.linuxbsd.editor.dev.x86_64 --headless
Initialize GDExtension API for Rust: Godot Engine v4.2.dev.custom_build
Godot Engine v4.2.dev.custom_build.691634969 - https://godotengine.org

Executing special case tests...
ERROR: Rust function panicked in file /home/<username>/Development/rust/gdextension/godot-core/src/builtin/meta/signature.rs at line 443. Context: get_viewport
   at: <function unset> (godot-core/src/lib.rs:154)
ERROR: Panic msg:  get_viewport: ret pointer is null
   at: <function unset> (godot-core/src/lib.rs:102)
WARNING: The "push_unhandled_input()" method is deprecated, use "push_input()" instead.
     at: push_unhandled_input (scene/main/viewport.cpp:3201)
thread '<unnamed>' panicked at /home/<username>/Development/rust/gdextension/godot-core/src/builtin/meta/signature.rs:443:5:
input_event: ret pointer is null
stack backtrace:
   0: rust_begin_unwind
             at /rustc/2e5a9dd6c9eaa42f0684b4b760bd68fc27cbe51b/library/std/src/panicking.rs:597:5
   1: core::panicking::panic_fmt
             at /rustc/2e5a9dd6c9eaa42f0684b4b760bd68fc27cbe51b/library/core/src/panicking.rs:72:14
   2: godot_core::builtin::meta::signature::ptrcall_return
             at /home/<username>/Development/rust/gdextension/godot-core/src/builtin/meta/signature.rs:443:5
   3: <(R,P0,P1,P2) as godot_core::builtin::meta::signature::PtrcallSignatureTuple>::in_ptrcall
             at /home/<username>/Development/rust/gdextension/godot-core/src/builtin/meta/signature.rs:274:17
   4: <itest::object_tests::virtual_methods_test::CollisionObject2DTest as godot_core::obj::traits::cap::ImplementsGodotVirtual>::__virtual_call::function
             at /home/<username>/Development/rust/gdextension/itest/rust/src/object_tests/virtual_methods_test.rs:468:1
   5: _ZN17CollisionObject2D28_gdvirtual__input_event_callILb0EEEbP8Viewport3RefI10InputEventEi
             at /home/runner/work/godot4-nightly/godot4-nightly/scene/2d/collision_object_2d.h:112:2
   6: _ZN17CollisionObject2D17_input_event_callEP8ViewportRK3RefI10InputEventEi
             at /home/runner/work/godot4-nightly/godot4-nightly/scene/2d/collision_object_2d.cpp:516:2
   7: _ZN8Viewport16_process_pickingEv
             at /home/runner/work/godot4-nightly/godot4-nightly/scene/main/viewport.cpp:851:30
   8: _Z29call_with_variant_args_helperI17__UnexistingClassJEJEEvPT_MS1_FvDpT0_EPPK7VariantRN8Callable9CallErrorE13IndexSequenceIJXspT1_EEE
             at /home/runner/work/godot4-nightly/godot4-nightly/./core/variant/binder_common.h:303:25
   9: _Z25call_with_variant_args_dvI17__UnexistingClassJEEvPT_MS1_FvDpT0_EPPK7VariantiRN8Callable9CallErrorERK6VectorIS7_E
             at /home/runner/work/godot4-nightly/godot4-nightly/./core/variant/binder_common.h:450:31
  10: _ZNK11MethodBindTIJEE4callEP6ObjectPPK7VariantiRN8Callable9CallErrorE
             at /home/runner/work/godot4-nightly/godot4-nightly/./core/object/method_bind.h:335:28
  11: _ZN6Object5callpERK10StringNamePPK7VariantiRN8Callable9CallErrorE
             at /home/runner/work/godot4-nightly/godot4-nightly/core/object/object.cpp:774:21
  12: _ZN9SceneTree17call_group_flagspEjRK10StringNameS2_PPK7Varianti
             at /home/runner/work/godot4-nightly/godot4-nightly/scene/main/scene_tree.cpp:298:23
  13: _ZN9SceneTree10call_groupIJEEEvRK10StringNameS3_DpT_
             at /home/runner/work/godot4-nightly/godot4-nightly/scene/main/scene_tree.h:299:20
  14: _ZN9SceneTree15physics_processEd
             at /home/runner/work/godot4-nightly/godot4-nightly/scene/main/scene_tree.cpp:466:12
  15: _ZN4Main9iterationEv
             at /home/runner/work/godot4-nightly/godot4-nightly/main/main.cpp:3550:60
  16: _ZN11OS_LinuxBSD3runEv
             at /home/runner/work/godot4-nightly/godot4-nightly/platform/linuxbsd/os_linuxbsd.cpp:933:22
  17: main
             at /home/runner/work/godot4-nightly/godot4-nightly/platform/linuxbsd/godot_linuxbsd.cpp:74:9
  18: <unknown>
  19: __libc_start_main
  20: _start
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
fatal runtime error: failed to initiate panic, error 5
fish: Job 1, 'RUST_BACKTRACE=1 ~/Programs/God…' terminated by signal SIGABRT (Abort)

In fact i think methods that return Variant gets a null pointer as the return pointer. Im not entirely sure how the godot code works here, but i think the return pointer might not get initialized in validated_call when doing a pointer call and the return type is Variant.

dsnopek · 2023-10-09T15:37:51Z

In fact i think methods that return Variant gets a null pointer as the return pointer. Im not entirely sure how the godot code works here, but i think the return pointer might not get initialized in validated_call when doing a pointer call and the return type is Variant.

Hm. Looking at the code, I think GDExtensionMethodBind::validated_call() will only pass a nullptr as the return value into the ptrcall if it was passed a nullptr as the return value. Assuming I'm understanding this correctly, we could create a temporary value in GDExtensionMethodBind::validated_call() so that we are always passing something into the ptrcall, but all we can do is throw that away afterwards, because we can't pass it up to the caller since they didn't give us a pointer to do so.

If the return value isn't going to be used at all by the caller, it seems like it's better to pass in nullptr, just in case the language binding has to do some extra work to construct the return value, this would allow the binding to skip this work. But, if we were always passing a value for the return value previously, this would be a compatibility break, so we'd need to decide if this breakage would be worth it. (My general inclination is towards not breaking compatibility, but we should discuss in any case.)

However, I'm not 100% sure I'm understanding this all correctly :-)

dsnopek · 2023-10-09T15:51:35Z

Er, actually, I just noticed this:

	_FORCE_INLINE_ static void *get_opaque_pointer(Variant *v) {
		switch (v->type) {
			case Variant::NIL:
				return nullptr;

So, perhaps it is giving us a Variant to store the return value (but it's type NIL)? I guess we need to figure out which branch is leading to it being nullptr

dsnopek · 2023-10-09T16:03:44Z

@Bromeon @lilizoey Can you see if PR #83054 fixes the problem for you? I'm not sure it's the right way to fix it (even if it fixes the problem), but it'll show us which branch the problem is happening on

kisg · 2023-10-09T19:15:55Z

We had a similar issue today when trying to return a Variant from a singleton (implemented with godot-cpp). We will check out the proposed PR tomorrow.

dsnopek · 2023-10-09T19:19:02Z

I'd love it if someone could share a simple MRP?

We had a similar issue today when trying to return a Variant from a singleton (implemented with godot-cpp).

Personally, I haven't managed to reproduce this with godot-cpp. One of the automated tests returns a Variant and it doesn't seem to have any issues.

kisg · 2023-10-09T19:35:23Z

I'd love it if someone could share a simple MRP?

We had a similar issue today when trying to return a Variant from a singleton (implemented with godot-cpp).

Personally, I haven't managed to reproduce this with godot-cpp. One of the automated tests returns a Variant and it doesn't seem to have any issues.

What we have is that in MethodBind::bind_ptrcall() r_return is null for some reason, and the Variant assignment crashes because of that. The code was not pushed yet, so we will have to wait until tomorrow.

Bromeon · 2023-10-09T21:54:50Z

@Bromeon @lilizoey Can you see if PR #83054 fixes the problem for you? I'm not sure it's the right way to fix it (even if it fixes the problem), but it'll show us which branch the problem is happening on

Tried your commit 188650d in this CI run, but I still get a crash 🤔

dsnopek · 2023-10-09T21:58:47Z

Tried your commit 188650d in this CI run, but I still get a crash 🤔

Ok, thanks, then it must be the other branch! That means we need make a temporary variant so we always have something to pass to the ptrcall (but throw it away, because we can't pass back to the caller). I'll update the PR, so you can try that variation.

dsnopek · 2023-10-09T22:09:44Z

@Bromeon I just updated PR #83054 to make sure we don't pass nullptr on the other branch. Can you see if this fixes it for you?

Bromeon · 2023-10-10T08:47:40Z

Unfortunately still fails, with commit 74eccb1.

Maybe kisg could come up with a minimal godot-cpp example, that might be easier to investigate?
It's of course also possible that we do something wrong on Rust side.

kisg · 2023-10-10T09:45:41Z

@Bromeon my team member @konczg is preparing the reproduction sample.

konczg · 2023-10-10T09:52:21Z

@Bromeon @kisg I've created an issue for the potentially related crash with singletons with a minimal example: #83097

kisg · 2023-10-10T10:30:57Z

@Bromeon Can you check with this Godot commit: 3cf1bc0

We cannot reproduce the issue there (see #83097).

dsnopek · 2023-10-10T13:37:09Z

@kisg Thanks for the MRP!

@Bromeon @lilizoey Can you test with the latest version of PR #83054? It fixes the crash for me with the new MRP

Bromeon · 2023-10-10T19:01:22Z

Looks like commit 88b6fee works (CI run)!

@kisg for the record, 3cf1bc0 did not work (CI run)

Bromeon added the topic:gdextension label Oct 8, 2023

akien-mga added bug regression labels Oct 9, 2023

akien-mga added this to the 4.2 milestone Oct 9, 2023

akien-mga assigned dsnopek and vnen Oct 9, 2023

akien-mga added the discussion label Oct 9, 2023

dsnopek mentioned this issue Oct 9, 2023

GDExtension: Use correct return pointer for validated calls that return Variant #83054

Merged

akien-mga closed this as completed in #83054 Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Possible regression with validated calls? #83016

Possible regression with validated calls? #83016

Bromeon commented Oct 8, 2023

lilizoey commented Oct 9, 2023

dsnopek commented Oct 9, 2023

dsnopek commented Oct 9, 2023

dsnopek commented Oct 9, 2023

kisg commented Oct 9, 2023

dsnopek commented Oct 9, 2023

kisg commented Oct 9, 2023

Bromeon commented Oct 9, 2023

dsnopek commented Oct 9, 2023

dsnopek commented Oct 9, 2023

Bromeon commented Oct 10, 2023 •

edited

Loading

kisg commented Oct 10, 2023

konczg commented Oct 10, 2023

kisg commented Oct 10, 2023

dsnopek commented Oct 10, 2023

Bromeon commented Oct 10, 2023

Possible regression with validated calls? #83016

Possible regression with validated calls? #83016

Comments

Bromeon commented Oct 8, 2023

Godot version

System information

Issue description

Steps to reproduce

Minimal reproduction project

lilizoey commented Oct 9, 2023

dsnopek commented Oct 9, 2023

dsnopek commented Oct 9, 2023

dsnopek commented Oct 9, 2023

kisg commented Oct 9, 2023

dsnopek commented Oct 9, 2023

kisg commented Oct 9, 2023

Bromeon commented Oct 9, 2023

dsnopek commented Oct 9, 2023

dsnopek commented Oct 9, 2023

Bromeon commented Oct 10, 2023 • edited Loading

kisg commented Oct 10, 2023

konczg commented Oct 10, 2023

kisg commented Oct 10, 2023

dsnopek commented Oct 10, 2023

Bromeon commented Oct 10, 2023

Bromeon commented Oct 10, 2023 •

edited

Loading