Merge godot-rust#326

326: Fix UB related to engine enum layout r=Bromeon a=lilizoey

`EngineEnum`s should be represented as 32-bit values, but also should be passed as i64 to/from godot in function parameters/return values.

This fixes that by having engine enums not implement `GodotFfi` but instead only implements `GodotFuncMarshal`.

To support this change, `GodotFuncMarshal` has been refactored:
- `Via` now represents only the intermediate type, and must implement `GodotFfi`
- Add error-types for when conversion fails
- Add explicit conversion methods to/from `Via`, we can't just use `TryFrom` here since not all types we wanna use have the proper `TryFrom` conversions. like f32 and f64

With this change, a minor change was made to the class generator, such that all arguments are first converted to their `Via` type, before a pointer to them is made. And return pointers are now dereferenced to the `Via` type before they're converted to the proper type. 

This means that `GodotFuncMarshal` is now the type that represents whether something can be passed as an argument/return value to a ptrcall.

`AsArg` was also removed and its method moved into `GodotFfi` as that simplifies codegen. (we can just always call `GodotFfi::as_arg_ptr` to create the pointer, rather than treating classes and other types differently). In the future we can try splitting up `GodotFfi` more, but i do think that `AsArg` belonged more in `godot_ffi` anyway.

Co-authored-by: Lili Zoey <[email protected]>

Author: bors[bot]

godot-codegen/src/class_generator.rs

@@ -6,14 +6,14 @@
 
 //! Generates a file for each Godot engine + builtin class
 
-use proc_macro2::{Ident, TokenStream};
+use proc_macro2::{Ident, Literal, TokenStream};
 use quote::{format_ident, quote, ToTokens};
 use std::path::Path;
 
 use crate::central_generator::{collect_builtin_types, BuiltinTypeInfo};
 use crate::util::{
     ident, option_as_slice, parse_native_structures_format, safe_ident, to_pascal_case,
-    to_rust_expr, to_rust_type, to_rust_type_abi, to_snake_case, unmap_meta, NativeStructuresField,
+    to_rust_expr, to_rust_type, to_rust_type_abi, to_snake_case, NativeStructuresField,
 };
 use crate::{api_parser::*, SubmitFn};
 use crate::{
@@ -541,7 +541,7 @@ fn make_class(class: &Class, class_name: &TyName, ctx: &mut Context) -> Generate
         use crate::engine::notify::*;
         use crate::builtin::*;
         use crate::engine::native::*;
-        use crate::obj::{AsArg, Gd};
+        use crate::obj::Gd;
         use sys::GodotFfi as _;
         use std::ffi::c_void;
 
@@ -787,7 +787,7 @@ fn make_builtin_class(
         use godot_ffi as sys;
         use crate::builtin::*;
         use crate::engine::native::*;
-        use crate::obj::{AsArg, Gd};
+        use crate::obj::Gd;
         use crate::sys::GodotFfi as _;
         use crate::engine::Object;
 
@@ -829,7 +829,7 @@ fn make_native_structure(
         use godot_ffi as sys;
         use crate::builtin::*;
         use crate::engine::native::*;
-        use crate::obj::{AsArg, Gd};
+        use crate::obj::Gd;
         use crate::sys::GodotFfi as _;
         use crate::engine::Object;
 
@@ -1334,7 +1334,7 @@ fn make_function_definition(sig: &FnSignature, code: &FnCode) -> FnDefinition {
     };
 
     let is_varcall = code.variant_ffi.is_some();
-    let [params, variant_types, arg_exprs, arg_names, meta_arg_decls] =
+    let [params, variant_types, arg_exprs, arg_names] =
         make_params_and_impl(&sig.params, is_varcall, false);
 
     let primary_fn_name = if has_default_params {
@@ -1349,6 +1349,8 @@ fn make_function_definition(sig: &FnSignature, code: &FnCode) -> FnDefinition {
         (TokenStream::new(), TokenStream::new())
     };
 
+    let args_indices = (0..arg_exprs.len()).map(Literal::usize_unsuffixed);
+
     let (prepare_arg_types, error_fn_context);
     if code.variant_ffi.is_some() {
         // varcall (using varargs)
@@ -1437,9 +1439,13 @@ fn make_function_definition(sig: &FnSignature, code: &FnCode) -> FnDefinition {
                 unsafe {
                     #init_code
 
-                    #( #meta_arg_decls )*
+                    #[allow(clippy::let_unit_value)]
+                    let __args = (
+                        #( #arg_exprs, )*
+                    );
+
                     let __args = [
-                        #( #arg_exprs ),*
+                        #( sys::GodotFfi::as_arg_ptr(&__args.#args_indices) ),*
                     ];
 
                     let __args_ptr = __args.as_ptr();
@@ -1740,12 +1746,11 @@ fn make_params_and_impl(
     method_args: &[FnParam],
     is_varcall: bool,
     skip_defaults: bool,
-) -> [Vec<TokenStream>; 5] {
+) -> [Vec<TokenStream>; 4] {
     let mut params = vec![];
     let mut variant_types = vec![];
     let mut arg_exprs = vec![];
     let mut arg_names = vec![];
-    let mut meta_arg_decls = vec![];
 
     for param in method_args.iter() {
         if skip_defaults && param.default_value.is_some() {
@@ -1754,36 +1759,22 @@ fn make_params_and_impl(
 
         let param_name = &param.name;
         let param_ty = &param.type_;
-        let canonical_ty = unmap_meta(param_ty);
 
         let arg_expr = if is_varcall {
             quote! { <#param_ty as ToVariant>::to_variant(&#param_name) }
         } else if let RustTy::EngineClass { tokens: path, .. } = &param_ty {
-            quote! { <#path as AsArg>::as_arg_ptr(&#param_name) }
-        } else {
-            let param_ty = if let Some(canonical_ty) = canonical_ty.as_ref() {
-                canonical_ty.to_token_stream()
-            } else {
-                param_ty.to_token_stream()
-            };
-            quote! { <#param_ty as sys::GodotFfi>::sys_const(&#param_name) }
-        };
-
-        // Note: could maybe reuse GodotFuncMarshal in the future
-        let meta_arg_decl = if let Some(canonical) = canonical_ty {
-            quote! { let #param_name = #param_name as #canonical; }
+            quote! { <#path as sys::GodotFuncMarshal>::try_into_via(#param_name).unwrap() }
         } else {
-            quote! {}
+            quote! { <#param_ty as sys::GodotFuncMarshal>::try_into_via(#param_name).unwrap() }
         };
 
         params.push(quote! { #param_name: #param_ty });
         variant_types.push(quote! { <#param_ty as VariantMetadata>::variant_type() });
         arg_exprs.push(arg_expr);
         arg_names.push(quote! { #param_name });
-        meta_arg_decls.push(meta_arg_decl);
     }
 
-    [params, variant_types, arg_exprs, arg_names, meta_arg_decls]
+    [params, variant_types, arg_exprs, arg_names]
 }
 
 fn make_params_and_args(method_args: &[&FnParam]) -> (Vec<TokenStream>, Vec<TokenStream>) {
@@ -1867,19 +1858,11 @@ fn make_return_and_impl(
             }
         }
         (false, None, Some(return_ty)) => {
-            let (ffi_ty, conversion);
-            if let Some(canonical_ty) = unmap_meta(return_ty) {
-                ffi_ty = canonical_ty.to_token_stream();
-                conversion = quote! { as #return_ty };
-            } else {
-                ffi_ty = return_ty.to_token_stream();
-                conversion = quote! {};
-            };
-
             quote! {
-                <#ffi_ty as sys::GodotFfi>::from_sys_init_default(|return_ptr| {
+                let via = <<#return_ty as sys::GodotFuncMarshal>::Via as sys::GodotFfi>::from_sys_init_default(|return_ptr| {
                     #ptrcall_invocation
-                }) #conversion
+                });
+                <#return_ty as sys::GodotFuncMarshal>::try_from_via(via).unwrap()
             }
         }
         (false, None, None) => {

godot-codegen/src/util.rs

@@ -110,10 +110,21 @@ pub fn make_enum_definition(enum_: &Enum) -> TokenStream {
                 self.ord
             }
         }
-        // SAFETY:
-        // The enums are transparently represented as an `i32`, so `*mut Self` is sound.
-        unsafe impl sys::GodotFfi for #enum_name {
-            sys::ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. }
+
+        impl sys::GodotFuncMarshal for #enum_name {
+            type Via = i64;
+            type FromViaError = sys::PrimitiveConversionError<i64, i32>;
+            type IntoViaError = std::convert::Infallible;
+
+            fn try_from_via(via: Self::Via) -> std::result::Result<Self, Self::FromViaError> {
+                let err = sys::PrimitiveConversionError::new(via);
+                let ord = i32::try_from(via).map_err(|_| err)?;
+                <Self as crate::obj::EngineEnum>::try_from_ord(ord).ok_or(err)
+            }
+
+            fn try_into_via(self) -> std::result::Result<Self::Via, Self::IntoViaError> {
+                Ok(<Self as crate::obj::EngineEnum>::ord(self).into())
+            }
         }
         #bitfield_ops
     }

godot-core/src/builtin/callable.rs

@@ -9,7 +9,7 @@ use godot_ffi as sys;
 use crate::builtin::{inner, ToVariant, Variant};
 use crate::engine::Object;
 use crate::obj::mem::Memory;
-use crate::obj::{AsArg, Gd, GodotClass, InstanceId};
+use crate::obj::{Gd, GodotClass, InstanceId};
 use std::fmt;
 use sys::{ffi_methods, GodotFfi};
 

godot-core/src/obj/as_arg.rs

@@ -577,6 +577,24 @@ where
         // We've passed ownership to caller.
         std::mem::forget(self);
     }
+
+    fn as_arg_ptr(&self) -> sys::GDExtensionConstTypePtr {
+        // We're passing a reference to the object to the callee. If the reference count needs to be
+        // incremented then the callee will do so. We do not need to prematurely do so.
+        //
+        // In Rust terms, if `T` is refcounted then we are effectively passing a `&Arc<T>`, and the callee
+        // would need to call `.clone()` if desired.
+
+        // In 4.0, argument pointers are passed to godot as `T*`, except for in virtual method calls. We
+        // can't perform virtual method calls currently, so they are always `T*`.
+        //
+        // In 4.1 argument pointers were standardized to always be `T**`.
+        if cfg!(gdextension_api = "4.0") {
+            self.sys_const()
+        } else {
+            std::ptr::addr_of!(self.opaque) as sys::GDExtensionConstTypePtr
+        }
+    }
 }
 
 // SAFETY:

godot-core/src/obj/gd.rs

@@ -10,14 +10,12 @@
 //! * [`GodotClass`], which is implemented for every class that Godot can work with (either engine- or user-provided).
 //! * [`Gd`], a smart pointer that manages instances of Godot classes.
 
-mod as_arg;
 mod base;
 mod gd;
 mod guards;
 mod instance_id;
 mod traits;
 
-pub use as_arg::*;
 pub use base::*;
 pub use gd::*;
 pub use guards::*;