[gjs/wip/smcv/issue319: 2/2] function: Don't assume FFI argument always matches GIArgument

[Simon McVittie <smcv src gnome org>]

commit 75b9f37136f12ead90d3373ac03fa42e1e394a17
Author: Simon McVittie <smcv debian org>
Date:   Sun Aug 23 18:13:17 2020 +0100

    function: Don't assume FFI argument always matches GIArgument
    
    Everywhere else in gjs_callback_closure() that writes into result
    does so by first converting the JavaScript object into a GIArgument
    according to GIArgument conventions, and then copying the GIArgument
    into result according to FFI conventions. In particular, for small
    integers like enums and flags, the value copied into the GIArgument
    is 32-bit, but the value pointed to by result is typically
    pointer-sized, which is larger on 64-bit platforms.
    
    However, the code path for vfuncs that fail at the JavaScript level, but
    have to return *something* at the C level, was instead treating result as
    being a pointer to a GIArgument. Writing a small integer to a GIArgument
    only sets the first few bits (the first 32 for enums and flags),
    leaving the next 32 bits of a pointer-sized quantity on a 64-bit
    platform uninitialized. On little-endian CPUs, if the next 32 bits
    happen to already be all-zeroes, the right thing would happen anyway,
    but we can't count on that.
    
    In particular, this resulted in the test-case
    "Wrong virtual functions marshals an enum return value" in GIMarshalling
    failing on big-endian LP64 architectures like s390x, after I fixed the
    other code paths involving GArgument and enums/flags for s390x in
    commit 1ba19d63 "function: Use GIArgument.v_int for enum and flags types".
    
    This change requires initialization of ret_type_is_void to be moved
    further up, so that it occurs before the first "goto out".
    
    Resolves: https://gitlab.gnome.org/GNOME/gjs/-/issues/319
    Signed-off-by: Simon McVittie <smcv debian org>

 gi/function.cpp | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
---
diff --git a/gi/function.cpp b/gi/function.cpp
index b6f6fe11..3973c6da 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -241,7 +241,6 @@ static void gjs_callback_closure(ffi_cif* cif [[maybe_unused]], void* result,
     int i, n_args, n_jsargs, n_outargs, c_args_offset = 0;
     GITypeInfo ret_type;
     bool success = false;
-    bool ret_type_is_void;
     auto args = reinterpret_cast<GIArgument **>(ffi_args);
 
     trampoline = (GjsCallbackTrampoline *) data;
@@ -307,6 +306,9 @@ static void gjs_callback_closure(ffi_cif* cif [[maybe_unused]], void* result,
 
     JS::RootedValue rval(context);
 
+    g_callable_info_load_return_type(trampoline->info, &ret_type);
+    bool ret_type_is_void = g_type_info_get_tag (&ret_type) == GI_TYPE_TAG_VOID;
+
     for (i = 0, n_jsargs = 0; i < n_args; i++) {
         GIArgInfo arg_info;
         GITypeInfo type_info;
@@ -382,9 +384,6 @@ static void gjs_callback_closure(ffi_cif* cif [[maybe_unused]], void* result,
                             true))
         goto out;
 
-    g_callable_info_load_return_type(trampoline->info, &ret_type);
-    ret_type_is_void = g_type_info_get_tag (&ret_type) == GI_TYPE_TAG_VOID;
-
     if (n_outargs == 0 && ret_type_is_void) {
         /* void return value, no out args, nothing to do */
     } else if (n_outargs == 0) {
@@ -502,9 +501,12 @@ out:
         }
 
         /* Fill in the result with some hopefully neutral value */
-        g_callable_info_load_return_type(trampoline->info, &ret_type);
-        gjs_gi_argument_init_default(&ret_type,
-                                     static_cast<GIArgument*>(result));
+        if (!ret_type_is_void) {
+            GIArgument argument = {};
+            g_callable_info_load_return_type(trampoline->info, &ret_type);
+            gjs_gi_argument_init_default(&ret_type, &argument);
+            set_return_ffi_arg_from_giargument(&ret_type, result, &argument);
+        }
 
         /* If the callback has a GError** argument and invoking the closure
          * returned an error, try to make a GError from it */