GNOME Bugzilla – Bug 668902
FFI calls broken on big endian architectures
Last modified: 2015-02-07 16:56:51 UTC
Created attachment 206320 [details] [review] Properly convert values between glib and FFI The attached patch is adapted from the fixes for gjs bug 665152. It makes sure values are properly converted between glib and FFI, which is critical for big endian architectures.
This patch and the patch from pygobject bug 668903 together allow running gnome-tweak-tool on PPC.
I see gobject-introspection 1.31.10 was released without these fixes. Can they be considered for the next release?
Review of attachment 206320 [details] [review]: Hi Michel, thanks a lot for doing this! ::: gobject-introspection-1.31.1.orig/girepository/ginvoke.c @@ +101,3 @@ + float rv_float; + double rv_double; +}; Can we avoid duplicating this structure at least? Either add a new internal header, or we could conceivably make it public (and actually export a shared API to extract FFI return values that gjs could use too). @@ +179,3 @@ break; + case G_TYPE_BOOLEAN: + g_value_set_boolean (gvalue, (gboolean)value->rv_ffi_arg); gboolean is a typedef for int, so this should be rv_ffi_sarg, right? In practice it shouldn't matter though. @@ +209,3 @@ break; + case G_TYPE_BOXED: + g_value_set_boxed (gvalue, (gpointer)value->rv_ffi_sarg); This should be consistent with G_TYPE_POINTER. I'd use rv_ffi_arg.
(In reply to comment #3) > ::: gobject-introspection-1.31.1.orig/girepository/ginvoke.c > @@ +101,3 @@ > + float rv_float; > + double rv_double; > +}; > > Can we avoid duplicating this structure at least? Either add a new internal > header, or we could conceivably make it public (and actually export a shared > API to extract FFI return values that gjs could use too). I made a minimal adaptation of the gjs fix. I don't have the time or particular interest in getting involved in glib development beyond that. Feel free to fix the problem any way you like. > @@ +179,3 @@ > break; > + case G_TYPE_BOOLEAN: > + g_value_set_boolean (gvalue, (gboolean)value->rv_ffi_arg); > > gboolean is a typedef for int, so this should be rv_ffi_sarg, right? You're probably right.
(In reply to comment #4) > (In reply to comment #3) > > ::: gobject-introspection-1.31.1.orig/girepository/ginvoke.c > > @@ +101,3 @@ > > + float rv_float; > > + double rv_double; > > +}; > > > > Can we avoid duplicating this structure at least? Either add a new internal > > header, or we could conceivably make it public (and actually export a shared > > API to extract FFI return values that gjs could use too). > > I made a minimal adaptation of the gjs fix. I don't have the time Fair enough. > or particular > interest But it's GNOME! It's cool! Join our club! =) > Feel free to fix the problem any way you like. It's not about "like" so much here as weighing the benefit of working on PPC64 versus long-term code maintenance etc. One thing I just realized is there is *another* copy of ffi->GValue code inside glib/gobject/gclosure.c =( So...it's actually kind of tempting to change gobject-introspection to just call g_cclosure_marshal_generic() and not have it deal with FFI at all. Then everything goes through the GValue conversion path. That would be a pretty invasive change though. One thing that occurs to me is that we could probably just reuse the "union _GIArgument" we already have instead of making up a new one.
Created attachment 207791 [details] [review] Michel's patch in git format-patch format Replacing bare patch with one with correct git attribution etc.
Created attachment 207813 [details] [review] repository: Fix conversion of FFI values on big-endian architectures Adapted from the fixes for (see bug 665152). It makes sure values are properly converted between glib and FFI, which is critical for big endian architectures. Patch adjusted to use GIArgument instead of custom union types by Colin Walters <walters@verbum.org>
Michel, can you test this patch? It's a modified version of yours which just uses GIArgument instead of the custom union. See https://bugzilla.gnome.org/show_bug.cgi?id=670249 where I make the "set_gargument_from_ffi_return_value" exactly the same between the two, so we can then later make it public in g-i and remove the duplication.
Created attachment 207816 [details] [review] repository: Add new public gi_type_info_extract_ffi_return_value() API Dealing with FFI and return values is very tricky; this API allows sharing the bits to do it between gobject-introspection and gjs (and potentially other FFI binding consumers). **NOTE** I swapped the order of the arguments, under the premise that out arguments should generally be last.
See https://bugzilla.gnome.org/show_bug.cgi?id=670249 for the gjs patches.
Ok, no review here, but I'm fairly confident these patches don't make things worse at least =)
Attachment 207813 [details] pushed as bf71aba - repository: Fix conversion of FFI values on big-endian architectures Attachment 207816 [details] pushed as c2fc7cb - repository: Add new public gi_type_info_extract_ffi_return_value() API
Created attachment 209243 [details] [review] repository: Remove extraneous leftover assignment to rvalue. Oops, looks like I made a mistake when generating the original patch. :( This followup patch removes a leftover assignment that should have been removed in the first place. Anyway, thanks for applying these!
Attachment 209243 [details] pushed as 7d70b87 - repository: Remove extraneous leftover assignment to rvalue.
(In reply to comment #13) > Created an attachment (id=209243) [details] [review] > repository: Remove extraneous leftover assignment to rvalue. > > Oops, looks like I made a mistake when generating the original patch. :( > > This followup patch removes a leftover assignment that should have been removed > in the first place. > > Anyway, thanks for applying these! No problem, hopefully it all works now =)
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]