godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.23k stars 21.04k forks source link

[3.x] The addition of `determinant` in Transform2D broke GDNative Rust bindings #77283

Closed necrashter closed 1 year ago

necrashter commented 1 year ago

Godot version

83e91ab818d81c6b660a46d571c9c63c201d189a and all subsequent commits in 3.x branch

System information

Ubuntu 20.04

Issue description

Commit 83e91ab818d81c6b660a46d571c9c63c201d189a #76323 exposed a determinant method in Transpose2D, and added this to GDNative API.

However, all Godot builds from 83e91ab818d81c6b660a46d571c9c63c201d189a or the subsequent commits cause the following crash when used with a GDNative library built with Rust bindings:

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v3.6.beta.custom_build (c4becb0ca44de25ee9ca9a3365245d979dad1e61)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7fbe24e42520] (??:0)
[2] /home/ilker/Programming/godot/godot/bin/godot.x11.opt.tools.64() [0x31cdf63] (/home/ilker/Programming/godot/godot/./core/cowdata.h:128)
[3] /home/ilker/Programming/godot/godot/bin/godot.x11.opt.tools.64() [0x31ce2a5] (/home/ilker/Programming/godot/godot/./core/cowdata.h:376)
[4] godot::GlobalConstants::___init_method_bindings() (??:0)
[5] godot::___init_method_bindings() (??:0)
[6] /home/ilker/Programming/godot/godot/bin/godot.x11.opt.tools.64() [0xf90da7] (/home/ilker/Programming/godot/godot/modules/gdnative/gdnative.h:88)
[7] /home/ilker/Programming/godot/godot/bin/godot.x11.opt.tools.64() [0xb60eb4] (/home/ilker/Programming/godot/godot/modules/gdnative/register_types.cpp:304)
[8] /home/ilker/Programming/godot/godot/bin/godot.x11.opt.tools.64() [0xb61e9a] (/home/ilker/Programming/godot/godot/modules/register_module_types.gen.cpp:93)
[9] /home/ilker/Programming/godot/godot/bin/godot.x11.opt.tools.64() [0xa7112a] (/home/ilker/Programming/godot/godot/main/main.cpp:1588)
[10] /lib/x86_64-linux-gnu/libc.so.6(+0x29d90) [0x7fbe24e29d90] (??:0)
[11] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) [0x7fbe24e29e40] (??:0)
[12] /home/ilker/Programming/godot/godot/bin/godot.x11.opt.tools.64() [0xa7e305] (??:?)
-- END OF BACKTRACE --
================================================================

I've updated api.json, gdnative_api.json, and used the latest headers from godot/modules/gdnative/include but to no avail.

I've opened an issue in the repository of GDNative Rust bindings. But please note that the issue persists after updating the API .json files and headers. On Discord, one of the developers @chitoyuu hypothesized the following:

I think it's because they inserted a new method in an existing extension struct, and not at the end of it either. Instead of creating a new struct linked to the end of the list. Thus there's now UB when we try to call anything after that new function.

Steps to reproduce

  1. Build Godot 3.x from a commit earlier than 83e91ab818d81c6b660a46d571c9c63c201d189a (e.g. the previous commit 571e4189fd1c9110850d49b0056fe2d4ae820af5).
  2. Create a Godot project.
  3. Create a GDNative library with Rust bindings and add it to the project.
  4. Confirm that it works.
  5. Upgrade the Godot build to the commit 83e91ab818d81c6b660a46d571c9c63c201d189a.
  6. Update the api.json and gdnative_api.json in Rust bindings. Copy the latest headers from godot/modules/gdnative/include.
  7. Create a clean build of the GDNative library (cargo clean and cargo build).
  8. Opening the project causes a crash with the given backtrace.

I don't know whether this issue occurs with GDNative C/C++ bindings.

Minimal reproduction project

Any Godot project with a GDNative library created with Rust bindings.

akien-mga commented 1 year ago

CC @aaronfranke

I suggest reverting that addition for 3.x, it wasn't particularly requested by anyone and if it's breaking GDNative, then it's a net negative.

Alternatively, it needs to be added to a new CORE_1_4 dictionary here: https://github.com/aaronfranke/godot/blob/141783d90f852cf008b0d11c907f14296633a6f0/modules/gdnative/gdnative_api.json#L26

It was an oversight from me not to flag the compat breakage by adding a new method in an already released core API.

aaronfranke commented 1 year ago

Sure, we can remove it in 3.x. I have nothing against doing that. It is a bit unfortunate though :(

EDIT: Oh, I see that you already added determinant to a new Core API 1.4, I guess that is preferred?

necrashter commented 1 year ago

I don't think we need to remove it since #77387 fixes this issue by moving it to a new API version. I think other new functions can be introduced similarly in Core API 1.4 until it's released.

akien-mga commented 1 year ago

Fixed by #77387.