Fix various issues with pointer calls #204

lilizoey · 2023-03-26T18:10:34Z

Combines #165 and #200 with more complete fixes.

Overview

Properly clones strings and shares arrays/dictionaries in pointer calls to ensure they're not prematurely freed.
Frees the value currently in the ret pointer in pointer calls, to avoid memory leak.
Moves ret_val into ret, to avoid premature destruction.
Changes integer conversions in pointer calls to use as, since that seems like the right way of converting the values we get from godot. try_from lead to wrong conversions sometimes.
Objects inheriting from RefCounted now use ref_get_object and ref_set_object in virtual method calls.

Details

Add from_arg_ptr and move_return_ptr to GodotFfi that will be used when converting argument pointers in pointer calls, and moving things into a pointer.
Add CallType so from_arg_ptr and move_return_ptr can have different behavior depending on whether it's a virtual method call or not.
Remove write_sys in GodotFfi.
Override from_arg_ptr in GodotString, NodePath, StringName, Array, Dictionary, Gd.
Override move_return_ptr in Gd.
Rename try_write_sys to try_return, and have it use move_return_ptr instead of write_sys.
Rename try_from_sys to try_from_arg, and have it use from_arg_ptr instead of from_sys.
Add u8, u16, u32 to the ptrcall tests.
Add a couple of test of virtual methods.
Fix a test for virtual method with return values to actually call the virtual method as a virtual method.

closes #195
closes #189

Bromeon · 2023-03-31T13:38:35Z

Thanks a lot!

I'm not sure if unsafe impl is the correct way to express the safety around that trait. From StackOverflow:

A function is marked unsafe to indicate that it is possible to violate memory safety by calling it. A trait is marked unsafe to indicate that it is possible to violate memory safety by implementing it at all. This is commonly because the trait has invariants that other unsafe code relies on being upheld, and that these invariants cannot be expressed any other way.

In the case of Searcher, the methods themselves should be safe to call. That is, users should not have to worry about whether or not they're using a Searcher correctly; the interface contract says all calls are safe. There's nothing you can do that will cause the methods to violate memory safety.

The trait itself is not unsafe to implement -- some of its methods are unsafe to call.

I know it's a border case because implementing GodotFfi essentially means enabling it for potentially unsafe implementations, but this is rather a problem with the macro. If you expand the macro, you have actual unsafe on each method that is unsafe to call. So maybe the macro should be prefixed with unsafe_.

lilizoey · 2023-03-31T13:54:17Z

The trait itself is not unsafe to implement -- some of its methods are unsafe to call.

I know it's a border case because implementing GodotFfi essentially means enabling it for potentially unsafe implementations, but this is rather a problem with the macro. If you expand the macro, you have actual unsafe on each method that is unsafe to call. So maybe the macro should be prefixed with unsafe_.

If the safety invariants i've listed for GodotFfi are violated, that is a buggy implementation. I don't think we should require the caller of from_arg_ptr and move_return_ptr to ensure the implementation is correct. And there is no way that we can have rust check that the implementation is correct for us.

Not having the unsafe on the trait would mean that if someone re-implemented GodotString without properly incrementing the refcount in from_arg_ptr, that would be a correct implementation. However it would break every single one of our macros to generate pointer calls.

So we'd then have a weird situation where every single use of #[unsafe_godot_api] is also implicitly a statement of "i have checked that everyone who implements GodotFfi for a type correctly initialized and cleans up values in from_arg_ptr and move_return_ptr".

So in my opinion, we should allow people who call from_arg_ptr and move_return_ptr to assume that they will properly handle initialization/cleanup of the values they're called with/they return.

That is what unsafe on traits is for, establishing safety invariants that users of the trait are allowed to rely on for safety. As mentioned in your quote:

This is commonly because the trait has invariants that other unsafe code relies on being upheld, and that these invariants cannot be expressed any other way.

unsafe on a function call can only express invariants related to the call of the function. You could not express the invariant of "from_arg_ptr was implemented to take a pointer to an argument in a pointer-call, and returns a properly initialized value of this type". Since that is not within the scope of what the caller should check, a violation of that is just a bug.

It's not my fault for instance if i call a function add(a: i32, b: i32) -> i32 function with add(1,2) and it returns 25 when the documentation said it would add the two values. That is a buggy implementation.

There's also just the fact that we have unsafe code relying on this invariant. Like our SignatureTuple impls.

Bromeon · 2023-03-31T14:10:29Z

unsafe on a function call can only express invariants related to the call of the function. You could not express the invariant of "from_arg_ptr was implemented to ensure that it takes a pointer to an argument in a pointer-call, and returns a valid value of this type". Since that is not within the scope of what the caller should check, a violation of that is just a bug.

Good point, I think you're right here. It's confusing to me that both trait and its functions are unsafe, but now that I think about it, it makes sense:

The trait is unsafe because a wrong implementation of it causes UB
The functions are unsafe because calling them with wrong argument also causes UB

It's just that I don't recall seeing such APIs in Rust; it's usually one or the other.

Not having the unsafe on the trait would mean that if someone re-implemented GodotString without properly incrementing the refcount in from_arg_ptr, that would be a correct implementation. However it would break every single one of our macros to generate pointer calls.

GodotFfi should not be implemented by users. We can seal or hide it from the API.

Extensibility is possibly via From/ToVariant.

lilizoey · 2023-03-31T14:12:37Z

It's just that I don't recall seeing such APIs in Rust; it's usually one or the other.

Ideally we should be able to split it in two, one trait that does the ffi-calls, and one that does the initialization/cleanup. I tried it but it became a really big and complicated refactor.

Bromeon · 2023-04-01T09:17:44Z

Yeah, I'd focus on getting a working version first that passes CI. Cleanups can follow later 🙂

Bromeon · 2023-04-02T07:38:30Z

Trying with latest CI changes...

bors try

bors · 2023-04-02T07:46:00Z

try

Build succeeded:

full-ci

… GDScript and Rust)

(cherry picked from commit 4906bea)

Make `Gd` in virtual method-calls use `ref_get_object`/`ref_set_object` if it inherits from `RefCounted` Add some tests of objects in pointer calls Add tests for Array passing through pointer calls Add more tests for refcounted Add a test for _input Rename some stuff in TestContext Add unsafe to rect and such GodotFfi impl along with `SAFETY` comments

Bromeon

Fantastic work! Thanks a lot for combining the multiple PRs and coming up with a model that streamlines GodotFfi, considers the Ref-call for virtuals and fixes various bugs along the way 🥇 also good job at finding example classes that exhibit behavior useful in virtual method tests!

I added some comments, many of it just minor documentation questions/suggestions 🙂

One thing, you have a lot of

// SAFETY:
// This type is transparently represented as `Self` in Godot, so `*mut Self` is sound.

What does the "transparently" mean here? Can it be omitted?

Bromeon · 2023-04-11T18:21:45Z

godot-core/src/builtin/array.rs

+        // SAFETY:
+        // Nothing special needs to be done beyond a `std::mem::swap` when returning an Array.


Is this meant to be before move_return_ptr? There is no unsafe in that context.

there are safety invariants that need to be upheld when implementing move_return_ptr in GodotFfi. Im just documenting here that the default impl provided by ffi_methods is a sound impl for move_return_ptr.

The // SAFETY: block should then be above unsafe impl and not fn.

This is a technicality, but may become relevant if we activate clippy::undocumented_unsafe_blocks in the future.

Bromeon · 2023-04-11T18:27:35Z

godot-core/src/builtin/array.rs

+    // SAFETY:
+    // Arrays are properly initialized through a `from_sys` call, but the ref-count should be
+    // incremented as that is the callee's responsibility.
+    //
+    // Using `std::mem::forget(array.share())` increments the ref count.


This should probably be moved inside the function, as it documents what's happening there and not how the function should be called (in the latter case, the convention is to use a # Safety block).

this is me documenting that the implementation of from_arg_ptr upholds the safety invariants that GodotFfi requires from an implementation of from_arg_ptr. I could move it inside if you want.

In that case, it should probably be part of the unsafe impl block docs, see comment above.

It's a bit non-local, but you could make two paragraphs in the impl docs, one for each function. The code is still relatively close, so that one sees the impl docs when looking at the function.

Bromeon · 2023-04-11T18:40:25Z

godot-core/src/obj/traits.rs

+        /// Returns `true` if argument and return pointers are passed as `Ref<T>` pointers given this
+        /// [`CallType`].
+        fn pass_as_ref(_call_type: CallType) -> bool {
+            false
+        }


Re comment: it's not clear what Ref<T> refers to. I assume you mean the Godot C++ code? How does this impact gdext, or what are the semantics?

These things would be nice to quickly mention 🙂

true, i think i document it better in CallType so i can probably just explicitly refer the reader to that.

Bromeon · 2023-04-11T18:42:15Z

godot-core/src/builtin/string.rs

-        fn write_string_sys = write_sys;
+    }
+
+    /// Move `self` into a system pointer.


Maybe additionally mention that ownership is transferred to Godot and the destructor of self is not called.

Bromeon · 2023-04-11T18:44:25Z

godot-ffi/src/godot_ffi.rs

+    /// `ptr` must be a valid _type ptr_: it must follow Godot's convention to encode `Self`,
    /// which is different depending on the type.
-    unsafe fn write_sys(&self, dst: sys::GDExtensionTypePtr);
+    /// `ptr` must encode `Self` according to the given `call_type`'s encoding of argument values.


You could use bullet points here:

/// * `ptr` must be ... /// which is ... /// * `ptr` must ...

(also some other places)

Bromeon · 2023-04-11T18:55:56Z

godot-ffi/src/godot_ffi.rs

+                unsafe fn try_from_arg(
+                    ptr: sys::GDExtensionTypePtr,
+                    call_type: CallType,
+                ) -> Result<Self, $Via> {
+                    let via = <$Via as GodotFfi>::from_arg_ptr(ptr, call_type);
+                    // Godot does not always give us a valid `i64`/`f64`, nor `Self` when it does a pointer
+                    // call, so we cannot use `try_from` here.
                    Ok(via as Self)
                }


The point of $Via was that you would use an intermediate type across the FFI boundary (usually i64), and that other types such as u32 would be converted to that.

Now you unconditionally convert via into Self, even though this value may not be representable -- I don't think you should abandon the try_from here.

The ; lossy would allow as-casting, because for floats, we take precision loss into account.

Maybe I'm not understanding the "Godot does not always give us a valid ..." comment.

ill look into it again i just remember that we'd get weird inconsistent values when going through the Via type because we'd be interpreting garbage bits as actual bits in the integer or something.

But then that would be another bug. In any case, "garbage values" wouldn't just be fixed by truncating integers. At best, you hide some problems.

In general, we must make sure we use the same type on Rust side as we do on C (Godot API) side. Not every integer type can be passed to Godot, they typically use i64.

It's probably best if we limit #[func] interfaces to i64 as well. That doesn't mean the user cannot declare an i16 parameter, but behind the scenes, we'd pass an i64 from GDScript. Otherwise it's impossible for Rust to recognize truncations.

Bromeon · 2023-04-11T19:04:11Z

itest/godot/ManualFfiTests.gd

@@ -72,22 +72,116 @@ func test_export():
 	obj.free()
 	node.free()

-func test_untyped_array_pass_to_user_func():
+func test_untyped_array_pass_to_user_func_varcall():


Question, are all these tests still needed with the new ones in GenFfi?
The latter now tests both ptrcall and varcall, as well as many new types.

ill check again

the array/dictionary ones are definitely not necessary, however the object and refcounted one doesn't seem to have anything similar in GenFfi so those are still gonna be useful.

Also the InheritTest still uses ArrayTest. It's likely not strictly necessary as the issue there was caused by pointer calls. but it's still useful to have as we dont test inheriting much elsewhere.

Bromeon · 2023-04-11T19:18:00Z

itest/rust/src/runner.rs

+        let ctx = TestContext {
+            _scene_tree: scene_tree,
+            root_node,
+            test_node,
+        };


I don't think the new fields in TestContext are necessary:

_scene_tree is unused

the user can always call Node::get_tree() if absolutely necessary

root_node is only used for push_input()

Must this happen on the root node? Can it not be the node itself or its parent? I don't want to generally encourage people to touch the scene tree too much, as we have zero test isolation and any side effects are persisted. Of course we cannot prevent it, but we should also not serve things on a silver platter 😉

If it's needed, we can also consider get_tree() + get_root(), even if it's a bit ugly with intermediate unwraps.

Also, please don't rename scene_tree -> test_node while there are so many PRs open, this creates extra conflicts for little benefit. I can take care of this later if necessary. The main idea was that you would get access to the scene tree through that field, even if it's technically not a SceneTree but a Node. Maybe test_node is clearer though.

I don't think the new fields in TestContext are necessary:

* `_scene_tree` is unused

fair, i kept it cause it was the old name of the field, which was confusing cause it wasn't actually the scene tree.

* `root_node` is only used for `push_input()` * Must this happen on the _root_ node? Can it not be the node itself or its parent? I don't want to generally encourage people to touch the scene tree too much, as we have zero test isolation and any side effects are persisted. Of course we cannot _prevent_ it, but we should also not serve things on a silver platter 😉 * If it's needed, we can also consider `get_tree()` + `get_root()`, even if it's a bit ugly with intermediate unwraps.

push_input must be done on a viewport. i guess it might be possible to make a viewport and add the node as a child and call push_input? ill check if that's possible.

Also, please don't rename scene_tree -> test_node while there are so many PRs open, this creates extra conflicts for little benefit. I can take care of this later if necessary. The main idea was that you would get access to the scene tree through that field, even if it's technically not a SceneTree but a Node. Maybe test_node is clearer though.

fair, it was very confusing to me because i thought it was the scene tree at first but it actually isnt...

Bromeon · 2023-04-11T19:19:15Z

itest/rust/src/virtual_methods_test.rs

+use godot::prelude::{
+    is_equal_approx, varray, Color, PackedByteArray, PackedColorArray, PackedFloat32Array,
+    PackedInt32Array, PackedStringArray, PackedVector2Array, PackedVector3Array, ToVariant,
+    Variant, VariantArray, Vector2, Vector3,
+};


You could use godot::builtin here, like already for GodotString above.

In test code, I would generally avoid the prelude (we can use that in examples though). The idea is that tests are mainly relevant for developers, so knowing where exactly symbols are located can help the understanding. Examples on the other hand are read by end users.

Bromeon · 2023-04-11T19:20:21Z

itest/rust/src/virtual_methods_test.rs

+        [GodotString::from("extension")].into_iter().collect()
+    }
+
+    fn handles_type(&self, type_: godot::prelude::StringName) -> bool {


Any particular reason why StringName is not imported alongside the other builtins?

not really, i just had rust analyzer add all default impls and it apparently added it as a fully qualified name and i didnt notice.

ill change it.

Bromeon · 2023-04-11T19:26:06Z

bors try

bors · 2023-04-11T19:37:38Z

try

Build failed:

full-ci

Bromeon · 2023-04-11T19:41:16Z

The double builds currently fail with this error:

error[E0308]: arguments to this function are incorrect
   --> itest/rust/src/virtual_methods_test.rs:334:9
    |
331 | /     assert_eq_approx!(
332 | |         arr.get(2).to::<PackedFloat32Array>().get(3),
333 | |         arr_rust.get(2).to::<PackedFloat32Array>().get(3),
334 | |         is_equal_approx
    | |         ^^^^^^^^^^^^^^^
335 | |     );
    | |     -
    | |_____|
    | |_____expected `f64`, found `f32`
    |       expected `f64`, found `f32`
    |
note: function defined here
   --> /Users/runner/work/gdext/gdext/godot-core/src/builtin/math.rs:18:8
    |
18  | pub fn is_equal_approx(a: real, b: real) -> bool {
    |        ^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0308`.

Looks like is_equal_approx accepts real when it should be called with f32.

lilizoey · 2023-04-11T22:04:25Z

What does the "transparently" mean here? Can it be omitted?

Well every type that implements GodotFfi is represented as Self in godot in some way. However some types are represented with the exact same layout as the rust type layout. I chose to describe that as "transparently" to mirror how #[repr(transparent)] means that the type has the same layout as the inner type, here meaning the godot type has the exact same layout as the rust type.

Bromeon

Some of the // SAFETY comments should be moved directly above the unsafe blocks; see comments for details.

I think the "transparently" doesn't add much or can even be confusing; I'd remove it from the wording.

Bromeon · 2023-04-12T10:41:36Z

godot-core/src/builtin/array.rs

+        // SAFETY:
+        // Nothing special needs to be done beyond a `std::mem::swap` when returning an Array.


The // SAFETY: block should then be above unsafe impl and not fn.

This is a technicality, but may become relevant if we activate clippy::undocumented_unsafe_blocks in the future.

Bromeon · 2023-04-12T10:43:10Z

godot-core/src/builtin/array.rs

+    // SAFETY:
+    // Arrays are properly initialized through a `from_sys` call, but the ref-count should be
+    // incremented as that is the callee's responsibility.
+    //
+    // Using `std::mem::forget(array.share())` increments the ref count.


In that case, it should probably be part of the unsafe impl block docs, see comment above.

It's a bit non-local, but you could make two paragraphs in the impl docs, one for each function. The code is still relatively close, so that one sees the impl docs when looking at the function.

Bromeon · 2023-04-12T10:50:11Z

godot-ffi/src/godot_ffi.rs

+                unsafe fn try_from_arg(
+                    ptr: sys::GDExtensionTypePtr,
+                    call_type: CallType,
+                ) -> Result<Self, $Via> {
+                    let via = <$Via as GodotFfi>::from_arg_ptr(ptr, call_type);
+                    // Godot does not always give us a valid `i64`/`f64`, nor `Self` when it does a pointer
+                    // call, so we cannot use `try_from` here.
                    Ok(via as Self)
                }


But then that would be another bug. In any case, "garbage values" wouldn't just be fixed by truncating integers. At best, you hide some problems.

In general, we must make sure we use the same type on Rust side as we do on C (Godot API) side. Not every integer type can be passed to Godot, they typically use i64.

It's probably best if we limit #[func] interfaces to i64 as well. That doesn't mean the user cannot declare an i16 parameter, but behind the scenes, we'd pass an i64 from GDScript. Otherwise it's impossible for Rust to recognize truncations.

Add another virtual method return test Make test that causes memory leak use `#[itest(skip)]` Move the logic for determining whether to use `Ref` or not entirely into `Mem` Remove some unnecessary manual ffi tests Rename CallType

Bromeon · 2023-04-12T16:31:20Z

Thanks a lot! 🚀

bors r+

bors · 2023-04-12T16:40:43Z

Build succeeded:

full-ci

Bromeon added bug c: ffi Low-level components and interaction with GDExtension API labels Mar 26, 2023

lilizoey mentioned this pull request Mar 28, 2023

Add return values when generating virtual methods #207

Merged

lilizoey force-pushed the fix/ptrcall-ub branch from 0428cb4 to 604481f Compare March 29, 2023 20:07

ttencate mentioned this pull request Mar 31, 2023

Static methods and custom constructors on custom types #175

Closed

lilizoey mentioned this pull request Mar 31, 2023

Add abstraction for ScriptInstanceExtension #196

Closed

bors bot added a commit that referenced this pull request Apr 2, 2023

Try #204:

8822ea3

lilizoey force-pushed the fix/ptrcall-ub branch 2 times, most recently from 931a28a to a732ab3 Compare April 11, 2023 16:54

Bromeon and others added 4 commits April 11, 2023 19:02

Test runner: print test name before execution

93bc73d

More generated #[func] tests (argument/return value passing between…

d6d73e4

… GDScript and Rust)

Projection tests: fix is_equal_approx bug, reduce number of iterations

3073f3a

-Fixes godot-rust#165.

3d0e3d2

(cherry picked from commit 4906bea)

lilizoey force-pushed the fix/ptrcall-ub branch from a732ab3 to d6623a1 Compare April 11, 2023 17:10

lilizoey force-pushed the fix/ptrcall-ub branch 2 times, most recently from c3273b6 to 535abd9 Compare April 11, 2023 17:29

lilizoey marked this pull request as ready for review April 11, 2023 17:33

Bromeon reviewed Apr 11, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Apr 11, 2023

Try #204:

5bec65c

Bromeon reviewed Apr 12, 2023

View reviewed changes

lilizoey requested a review from Bromeon April 12, 2023 15:53

Fix return virtual method type

8206697

Add another virtual method return test Make test that causes memory leak use `#[itest(skip)]` Move the logic for determining whether to use `Ref` or not entirely into `Mem` Remove some unnecessary manual ffi tests Rename CallType

lilizoey force-pushed the fix/ptrcall-ub branch from 496f022 to 8206697 Compare April 12, 2023 16:29

bors bot merged commit afbd2cd into godot-rust:master Apr 12, 2023

This was referenced Apr 12, 2023

Extensive #[func] testing (GDScript -> Rust calls) #200

Closed

Fix early destruction of objects returned from Rust (#138) #165

Closed

Impl Display for built-ins #224

Merged

lilizoey mentioned this pull request Apr 13, 2023

ERROR: Condition "!success" is true when returning an Array from gdextension #138

Closed

lilizoey deleted the fix/ptrcall-ub branch April 16, 2023 16:24

LordCocoNut mentioned this pull request May 3, 2023

Using input method causes game to crash #257

Closed

Bromeon added the ub Undefined behavior label Jul 18, 2023

		// SAFETY:
		// Nothing special needs to be done beyond a `std::mem::swap` when returning an Array.

Fix various issues with pointer calls #204

Fix various issues with pointer calls #204

Conversation

lilizoey commented Mar 26, 2023 • edited Loading

Overview

Details

Bromeon commented Mar 31, 2023

lilizoey commented Mar 31, 2023 • edited Loading

Bromeon commented Mar 31, 2023 • edited Loading

lilizoey commented Mar 31, 2023 • edited Loading

Bromeon commented Apr 1, 2023

Bromeon commented Apr 2, 2023

bors bot commented Apr 2, 2023

try

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Bromeon commented Apr 11, 2023

bors bot commented Apr 11, 2023

try

Bromeon commented Apr 11, 2023

lilizoey commented Apr 11, 2023 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Bromeon commented Apr 12, 2023

bors bot commented Apr 12, 2023

lilizoey commented Mar 26, 2023 •

edited

Loading

lilizoey commented Mar 31, 2023 •

edited

Loading

Bromeon commented Mar 31, 2023 •

edited

Loading

lilizoey commented Mar 31, 2023 •

edited

Loading

Bromeon Apr 12, 2023 •

edited

Loading

lilizoey Apr 12, 2023 •

edited

Loading

Bromeon Apr 11, 2023 •

edited

Loading

lilizoey Apr 11, 2023 •

edited

Loading

lilizoey commented Apr 11, 2023 •

edited

Loading

Bromeon Apr 12, 2023 •

edited

Loading