Skip to content

Commit

Permalink
Merge #284
Browse files Browse the repository at this point in the history
284: Compatibility with 4.1 GDExtension API r=Bromeon a=Bromeon

This pull request migrates gdext to the **major GDExtension API change**, as introduced in godotengine/godot#76406.
It also adopts to the new "uninitialized pointer" types, which were added in godotengine/godot#35813.

Doing so renders the CI green again, unblocking other PRs that have been broken by the upstream changes.

---

# Major GDExtension API update

TLDR of Godot's changes: instead of one giant `GDExtensionInterface` struct with all the function pointers as data members, the C API now offers only a single function pointer `get_proc_address`. This one is passed to the entry point, in place of a pointer to the interface. With `get_proc_address`, it is possible to retrieve concrete API functions. In C++, this is done by looking up via name and casting the result to the correct function pointer type:
```cpp
auto variant_call = (GDExtensionInterfaceVariantCall) get_proc_address("variant_call");
variant_call(...);
```

On the Rust side, I incorporated these changes by only modifying the FFI layer (`godot-ffi` crate), so the rest of the project is mostly unaffected. This is done by generating a `GDExtensionInterface` struct very similarly to how it existed before, in `godot-codegen`. As input, we parse the `gdextension_interface.h` header's documentation metadata. This works well so far, but we'll need to make sure with Godot devs that the doc metadata doesn't suddenly change format.

---

# Uninitialized pointer types

Certain FFI functions construct objects "into an uninitialized pointer" (placement new in C++). There used to be quite some confusion regarding _which_ functions do that. As a result, the C API introduced new types such as `GDExtensionUninitializedStringPtr`, which represent the uninitialized counterpart to `GDExtensionStringPtr`.

These typedefs are declared as `void*` in the C API, but we turn them into strongly typed opaque pointers by post-processing the C header. As such, it is impossible to accidentally mix initialized and uninitialized pointer types. I also added a trait `AsUninit` which allows explicit conversion between the two. This may not be necessary in the future, but is helpful until we are sure about correct usage everywhere. I marked some places that may need another look as `// TODO(uninit)`.


---

# Compatibility between Godot 4.0.x and 4.1+

Due to the API changes in GDExtension, extensions compiled under Godot 4.0.x are by default **not compatible** with Godot 4.1+ (which includes current `master` versions of Godot, so also our nightly CI builds).

 From now on, the `.gdextension` interface must contain a new property `compatibility_minimum`:
```toml
[configuration]
entry_symbol = "gdext_rust_init"
compatibility_minimum = 4.0

[libraries]
linux.debug.x86_64 = "res://../../../target/debug/libdodge_the_creeps.so"
...
```
This value must be set to `4.0` if you compile against the old GDExtension API (current gdext uses 4.0.3 by default).
Set it to `4.1` if you use a Godot development version (Cargo feature `custom-godot`).
If you do this wrong, gdext will abort initialization and print a descriptive error message.

## Compat bridge

Since breaking changes can be very disruptive, I built a bridging layer that allows to use existing compiled extensions under Godot v4.1 (or current development versions). Because the Rust code relies on certain features only available in the newer C header (e.g. uninitialized pointers), such changes were backported via dynamic post-processing of `gdextension_interface.h`.

Therefore, you don't need to mess around with `custom-godot`, just keep using gdext as-is and set `compatibility_minimum = 4.0` to run it under newer engine versions. Developing against a specific GDExtension API is still possible via patch, the prebuilt artifacts have been updated for all 4.0.x versions. For example, to use version `4.0`:
```toml
[patch."https://github.com/godot-rust/godot4-prebuilt"]
godot4-prebuilt = { git = "https://github.com//godot-rust/godot4-prebuilt", branch = "4.0"}
```

Once Godot 4.1.0 is released, we will likely make that the default version used by gdext, but I plan to maintain the compat layer as long as this is possible with reasonable effort.

A new CI setup verifies that the integration tests run against 4.0, 4.0.1, 4.0.2, 4.0.3 and nightly Godot versions, at least for Linux.
This will become a challenge as soon as there are breaking API changes (and there is already one upcoming in `Basis::looking_at()`).

---

# Other changes

There is now an experimental `godot::sys::GdextBuild` struct, which provides informations about static (compiled-against) and runtime (loaded-by) Godot versions. This may be extended by other build/runtime metadata and will likely be promoted to an official API in the `godot::init` module.

Several memory leaks that came up during the change to uninitialized pointers were addressed. In many places, we now use `from_sys_init()` instead of `from_sys_init_default()`, avoiding the need to construct throwaway default instances.

In addition to more CI jobs for integration tests, there are now also 2 more linux-memcheck ones, to cover both 4.0.3 and nightly. This is mostly to have extra confidence during the migration phase and may be removed later.

Godot 4.0.3 is now the default version used by gdext.

Co-authored-by: Jan Haller <bromeon@gmail.com>
  • Loading branch information
bors[bot] and Bromeon authored May 24, 2023
2 parents 8990464 + 974085c commit 81f81c6
Show file tree
Hide file tree
Showing 37 changed files with 1,075 additions and 187 deletions.
51 changes: 49 additions & 2 deletions .github/composite/godot-itest/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ inputs:
default: 'false'
description: "Should the job check against latest gdextension_interface.h, and warn on difference"

godot-prebuilt-patch:
required: false
default: ''
description: "If specified, sets the branch name of the godot4-prebuilt crate to this value"

rust-toolchain:
required: false
default: 'stable'
Expand Down Expand Up @@ -71,14 +76,56 @@ runs:
rust: ${{ inputs.rust-toolchain }}
with-llvm: ${{ inputs.with-llvm }}

- name: "Patch prebuilt version ({{ inputs.godot-prebuilt-patch }})"
if: inputs.godot-prebuilt-patch != ''
env:
VERSION: ${{ inputs.godot-prebuilt-patch }}
# sed -i'' needed for macOS compatibility, see https://stackoverflow.com/q/4247068
run: |
echo "Patch prebuilt version to $VERSION..."
# For newer versions, update the compatibility_minimum in .gdextension files to 4.1
# Once a 4.1.0 is released, we can invert this and set compatibility_minimum to 4.0 for older versions.
if [[ "$VERSION" == "4.1" ]]; then
echo "Update compatibility_minimum in .gdextension files..."
dirs=("itest" "examples")
for dir in "${dirs[@]}"; do
find "$dir" -type f -name "*.gdextension" -exec sed -i'.bak' 's/compatibility_minimum = 4\.0/compatibility_minimum = 4.1/' {} +
done
# Versions 4.0.x
else
# Patch only needed if version is not already set
if grep -E 'godot4-prebuilt = { .+ branch = "$VERSION" }' godot-bindings/Cargo.toml; then
echo "Already has version $version; no need for patch."
else
cat << HEREDOC >> Cargo.toml
[patch."https://github.com/godot-rust/godot4-prebuilt"]
godot4-prebuilt = { git = "https://github.com//godot-rust/godot4-prebuilt", branch = "$VERSION" }
HEREDOC
echo "Patched Cargo.toml for version $version."
fi
fi
shell: bash

# else
- name: "No patch selected"
if: inputs.godot-prebuilt-patch == ''
run: |
echo "No patch selected; use default godot4-prebuilt version."
shell: bash

- name: "Build gdext (itest)"
run: |
cargo build -p itest ${{ inputs.rust-extra-args }}
shell: bash
env:
RUSTFLAGS: ${{ inputs.rust-env-rustflags }}

# Note: no longer fails, as we expect header to be forward-compatible; instead issues a warning
# This step no longer fails if there's a diff, as we expect header to be forward-compatible; instead issues a warning
# However, still fails if patch cannot be applied (conflict).
# Step is only run in latest, not for compat 4.0.1 etc. -> no need to take into account different header versions.
- name: "Copy and compare GDExtension header"
if: inputs.godot-check-header == 'true'
run: |
Expand All @@ -96,7 +143,7 @@ runs:
echo "\`\`\`diff" >> $GITHUB_STEP_SUMMARY
git diff --no-index gdextension_interface_prebuilt.h gdextension_interface.h >> $GITHUB_STEP_SUMMARY || true
echo "\`\`\`" >> $GITHUB_STEP_SUMMARY
echo "After manually updating file, run: \`git diff -R > tweak.patch\`." >> $GITHUB_STEP_SUMMARY
echo "After manually updating file, run: \`git diff -R > tweak2.patch && mv tweak2.patch tweak.patch\`." >> $GITHUB_STEP_SUMMARY
# Undo modifications
mv gdextension_interface_prebuilt.h gdextension_interface.h
Expand Down
68 changes: 62 additions & 6 deletions .github/workflows/full-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,13 @@ jobs:
# Additionally, the 'linux (msrv *)' special case will then be listed next to the other 'linux' jobs.
# Note: Windows uses '--target x86_64-pc-windows-msvc' by default as Cargo argument.
include:
- name: macos
# macOS

- name: macos-4.0.3
os: macos-12
artifact-name: macos
godot-binary: godot.macos.editor.dev.x86_64
godot-prebuilt-patch: '4.0.3'

- name: macos-double
os: macos-12
Expand All @@ -142,10 +146,15 @@ jobs:
godot-binary: godot.macos.editor.dev.x86_64
rust-extra-args: --features godot/custom-godot
with-llvm: true
godot-prebuilt-patch: '4.1'

- name: windows
# Windows

- name: windows-4.0.3
os: windows-latest
artifact-name: windows
godot-binary: godot.windows.editor.dev.x86_64.exe
godot-prebuilt-patch: '4.0.3'

- name: windows-double
os: windows-latest
Expand All @@ -157,12 +166,35 @@ jobs:
artifact-name: windows
godot-binary: godot.windows.editor.dev.x86_64.exe
rust-extra-args: --features godot/custom-godot
godot-prebuilt-patch: '4.1'

# Linux

# Don't use latest Ubuntu (22.04) as it breaks lots of ecosystem compatibility.
# If ever moving to ubuntu-latest, need to manually install libtinfo5 for LLVM.
- name: linux
- name: linux-4.0.3
os: ubuntu-20.04
artifact-name: linux
godot-binary: godot.linuxbsd.editor.dev.x86_64
godot-check-header: false # disabled for now

- name: linux-4.0.2
os: ubuntu-20.04
artifact-name: linux
godot-binary: godot.linuxbsd.editor.dev.x86_64
godot-prebuilt-patch: '4.0.2'

- name: linux-4.0.1
os: ubuntu-20.04
artifact-name: linux
godot-binary: godot.linuxbsd.editor.dev.x86_64
godot-prebuilt-patch: '4.0.1'

- name: linux-4.0
os: ubuntu-20.04
artifact-name: linux
godot-binary: godot.linuxbsd.editor.dev.x86_64
godot-prebuilt-patch: '4.0'

- name: linux-double
os: ubuntu-20.04
Expand All @@ -180,27 +212,50 @@ jobs:
artifact-name: linux
godot-binary: godot.linuxbsd.editor.dev.x86_64
rust-extra-args: --features godot/custom-godot
godot-prebuilt-patch: '4.1'

# Special Godot binaries compiled with AddressSanitizer/LeakSanitizer to detect UB/leaks.
# Additionally, the Godot source is patched to make dlclose() a no-op, as unloading dynamic libraries loses stacktrace and
# cause false positives like println!. See https://github.com/google/sanitizers/issues/89.
# The gcc version can possibly be removed later, as it is slower and needs a larger artifact than the clang one.

# --disallow-focus: fail if #[itest(focus)] is encountered, to prevent running only a few tests for full CI
- name: linux-memcheck-gcc
- name: linux-memcheck-gcc-4.0.3
os: ubuntu-20.04
artifact-name: linux-memcheck-gcc
godot-binary: godot.linuxbsd.editor.dev.x86_64.san
godot-args: -- --disallow-focus
rust-toolchain: nightly
rust-env-rustflags: -Zrandomize-layout

- name: linux-memcheck-clang
- name: linux-memcheck-clang-4.0.3
os: ubuntu-20.04
artifact-name: linux-memcheck-clang
godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san
godot-args: -- --disallow-focus
rust-toolchain: nightly
rust-env-rustflags: -Zrandomize-layout

- name: linux-memcheck-gcc-nightly
os: ubuntu-20.04
artifact-name: linux-memcheck-gcc
godot-binary: godot.linuxbsd.editor.dev.x86_64.san
godot-args: -- --disallow-focus
rust-toolchain: nightly
rust-env-rustflags: -Zrandomize-layout
rust-extra-args: --features godot/custom-godot
godot-prebuilt-patch: '4.1'

- name: linux-memcheck-clang-nightly
os: ubuntu-20.04
artifact-name: linux-memcheck-clang
godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san
godot-args: -- --disallow-focus
rust-toolchain: nightly
rust-env-rustflags: -Zrandomize-layout
rust-extra-args: --features godot/custom-godot
godot-prebuilt-patch: '4.1'


steps:
- uses: actions/checkout@v3
Expand All @@ -211,11 +266,12 @@ jobs:
artifact-name: godot-${{ matrix.artifact-name || matrix.name }}
godot-binary: ${{ matrix.godot-binary }}
godot-args: ${{ matrix.godot-args }}
godot-prebuilt-patch: ${{ matrix.godot-prebuilt-patch }}
rust-extra-args: ${{ matrix.rust-extra-args }}
rust-toolchain: ${{ matrix.rust-toolchain || 'stable' }}
rust-env-rustflags: ${{ matrix.rust-env-rustflags }}
with-llvm: ${{ matrix.with-llvm }}
godot-check-header: ${{ matrix.name == 'linux' }}
godot-check-header: ${{ matrix.godot-check-header }}


license-guard:
Expand Down
1 change: 1 addition & 0 deletions examples/dodge-the-creeps/godot/DodgeTheCreeps.gdextension
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
[configuration]
entry_symbol = "gdext_rust_init"
compatibility_minimum = 4.0

[libraries]
linux.debug.x86_64 = "res://../../../target/debug/libdodge_the_creeps.so"
Expand Down
2 changes: 1 addition & 1 deletion godot-bindings/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ custom-godot = ["dep:bindgen", "dep:regex", "dep:which"]
custom-godot-extheader = []

[dependencies]
godot4-prebuilt = { optional = true, git = "https://github.com/godot-rust/godot4-prebuilt", branch = "4.0.1" }
godot4-prebuilt = { optional = true, git = "https://github.com/godot-rust/godot4-prebuilt", branch = "4.0.3" }

# Version >= 1.5.5 for security: https://blog.rust-lang.org/2022/03/08/cve-2022-24713.html
# 'unicode-gencat' needed for \d, see: https://docs.rs/regex/1.5.5/regex/#unicode-features
Expand Down
41 changes: 33 additions & 8 deletions godot-bindings/res/tweak.patch
Original file line number Diff line number Diff line change
@@ -1,32 +1,42 @@
diff --git b/godot-ffi/src/gen/gdextension_interface.h a/godot-ffi/src/gen/gdextension_interface.h
index 0b7615f..6db266e 100644
index 4e4f300..e1cd5fb 100644
--- b/godot-ffi/src/gen/gdextension_interface.h
+++ a/godot-ffi/src/gen/gdextension_interface.h
@@ -139,22 +139,22 @@ typedef enum {

} GDExtensionVariantOperator;
@@ -155,27 +155,27 @@ typedef enum {
// - Some types have no destructor (see `extension_api.json`'s `has_destructor` field), for
// them it is always safe to skip the constructor for the return value if you are in a hurry ;-)

-typedef void *GDExtensionVariantPtr;
-typedef const void *GDExtensionConstVariantPtr;
-typedef void *GDExtensionUninitializedVariantPtr;
-typedef void *GDExtensionStringNamePtr;
-typedef const void *GDExtensionConstStringNamePtr;
-typedef void *GDExtensionUninitializedStringNamePtr;
-typedef void *GDExtensionStringPtr;
-typedef const void *GDExtensionConstStringPtr;
-typedef void *GDExtensionUninitializedStringPtr;
-typedef void *GDExtensionObjectPtr;
-typedef const void *GDExtensionConstObjectPtr;
-typedef void *GDExtensionUninitializedObjectPtr;
-typedef void *GDExtensionTypePtr;
-typedef const void *GDExtensionConstTypePtr;
-typedef void *GDExtensionUninitializedTypePtr;
-typedef const void *GDExtensionMethodBindPtr;
+typedef struct __GdextVariant *GDExtensionVariantPtr;
+typedef const struct __GdextVariant *GDExtensionConstVariantPtr;
+typedef struct __GdextUninitializedVariant *GDExtensionUninitializedVariantPtr;
+typedef struct __GdextStringName *GDExtensionStringNamePtr;
+typedef const struct __GdextStringName *GDExtensionConstStringNamePtr;
+typedef struct __GdextUninitializedStringName *GDExtensionUninitializedStringNamePtr;
+typedef struct __GdextString *GDExtensionStringPtr;
+typedef const struct __GdextString *GDExtensionConstStringPtr;
+typedef struct __GdextUninitializedString *GDExtensionUninitializedStringPtr;
+typedef struct __GdextObject *GDExtensionObjectPtr;
+typedef const struct __GdextObject *GDExtensionConstObjectPtr;
+typedef struct __GdextUninitializedObject *GDExtensionUninitializedObjectPtr;
+typedef struct __GdextType *GDExtensionTypePtr;
+typedef const struct __GdextType *GDExtensionConstTypePtr;
+typedef struct __GdextUninitializedType *GDExtensionUninitializedTypePtr;
+typedef const struct __GdextMethodBind *GDExtensionMethodBindPtr;
typedef int64_t GDExtensionInt;
typedef uint8_t GDExtensionBool;
Expand All @@ -38,7 +48,22 @@ index 0b7615f..6db266e 100644

/* VARIANT DATA I/O */

@@ -203,7 +203,7 @@ typedef struct {
@@ -195,11 +195,11 @@ typedef struct {
int32_t expected;
} GDExtensionCallError;

-typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionVariantPtr, GDExtensionTypePtr);
-typedef void (*GDExtensionTypeFromVariantConstructorFunc)(GDExtensionTypePtr, GDExtensionVariantPtr);
+typedef void (*GDExtensionVariantFromTypeConstructorFunc)(GDExtensionUninitializedVariantPtr, GDExtensionTypePtr);
+typedef void (*GDExtensionTypeFromVariantConstructorFunc)(GDExtensionUninitializedTypePtr, GDExtensionVariantPtr);
typedef void (*GDExtensionPtrOperatorEvaluator)(GDExtensionConstTypePtr p_left, GDExtensionConstTypePtr p_right, GDExtensionTypePtr r_result);
typedef void (*GDExtensionPtrBuiltInMethod)(GDExtensionTypePtr p_base, const GDExtensionConstTypePtr *p_args, GDExtensionTypePtr r_return, int p_argument_count);
-typedef void (*GDExtensionPtrConstructor)(GDExtensionTypePtr p_base, const GDExtensionConstTypePtr *p_args);
+typedef void (*GDExtensionPtrConstructor)(GDExtensionUninitializedTypePtr p_base, const GDExtensionConstTypePtr *p_args);
typedef void (*GDExtensionPtrDestructor)(GDExtensionTypePtr p_base);
typedef void (*GDExtensionPtrSetter)(GDExtensionTypePtr p_base, GDExtensionConstTypePtr p_value);
typedef void (*GDExtensionPtrGetter)(GDExtensionConstTypePtr p_base, GDExtensionTypePtr r_value);
@@ -224,7 +224,7 @@ typedef struct {

/* EXTENSION CLASSES */

Expand All @@ -47,7 +72,7 @@ index 0b7615f..6db266e 100644

typedef GDExtensionBool (*GDExtensionClassSet)(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_name, GDExtensionConstVariantPtr p_value);
typedef GDExtensionBool (*GDExtensionClassGet)(GDExtensionClassInstancePtr p_instance, GDExtensionConstStringNamePtr p_name, GDExtensionVariantPtr r_ret);
@@ -266,7 +266,7 @@ typedef struct {
@@ -287,7 +287,7 @@ typedef struct {
void *class_userdata; // Per-class user data, later accessible in instance bindings.
} GDExtensionClassCreationInfo;

Expand All @@ -56,7 +81,7 @@ index 0b7615f..6db266e 100644

/* Method */

@@ -323,7 +323,7 @@ typedef struct {
@@ -345,7 +345,7 @@ typedef struct {

/* SCRIPT INSTANCE EXTENSION */

Expand All @@ -65,7 +90,7 @@ index 0b7615f..6db266e 100644

typedef GDExtensionBool (*GDExtensionScriptInstanceSet)(GDExtensionScriptInstanceDataPtr p_instance, GDExtensionConstStringNamePtr p_name, GDExtensionConstVariantPtr p_value);
typedef GDExtensionBool (*GDExtensionScriptInstanceGet)(GDExtensionScriptInstanceDataPtr p_instance, GDExtensionConstStringNamePtr p_name, GDExtensionVariantPtr r_ret);
@@ -353,13 +353,13 @@ typedef GDExtensionBool (*GDExtensionScriptInstanceRefCountDecremented)(GDExtens
@@ -375,13 +375,13 @@ typedef GDExtensionBool (*GDExtensionScriptInstanceRefCountDecremented)(GDExtens
typedef GDExtensionObjectPtr (*GDExtensionScriptInstanceGetScript)(GDExtensionScriptInstanceDataPtr p_instance);
typedef GDExtensionBool (*GDExtensionScriptInstanceIsPlaceholder)(GDExtensionScriptInstanceDataPtr p_instance);

Expand Down
Loading

0 comments on commit 81f81c6

Please sign in to comment.