GNOME Bugzilla – Bug 665152
gjs doesn't use ffi_call the way it expects to be used
Last modified: 2012-02-18 16:38:58 UTC
The ffi_call man page says this: rvalue must point to storage that is sizeof(long) or larger. For smaller return value sizes, the ffi_arg or ffi_sarg integral type must be used to hold the return value. This, to me, means that if the return type is a char, short, or 32-bit int, it's going to be stuffed in a long, irregardless. Right now gjs stuffs a GArgument in there which is a union of various types. Some of these types are not the same size as long, and so can't be accessed following ffi_call() according to the contract mentioned above in the man page. On some platforms it doesn't matter, if the code only accesses 32bits (or whatever) of the 64bit long that ffi_call filled in, and the return type is a 32-bit return type (or whatever) then things will still work because 32-bits accessed are the "right" 32 bits. The other 32-bits would be zero anyway. On ppc64, this is not true. accessing a smaller-than-64-bt member of union will access the wrong subset of bits and give the wrong ultimate value. The problem boils down to ffi_call really wanting something different than a GArgument passed into it. The attached patch fixes the GArgument up after the call completes. So the numbers are stuffed in the right place in memory.
This is needed for the gjs test suite to pass on ppc64. We also need bug 665150 fixed for the test suite to pass.
Created attachment 202398 [details] [review] need commit message Will write after review.
Downstream report is here btw: https://bugzilla.redhat.com/show_bug.cgi?id=749604
Note, i've only tested this on ppc64, I should probably make sure it didn't regress x86_64
One thing I thought of when talking to Jasper on IRC is that this patch isn't correct in the face of 128bit types, but it's not an issue because GArgument doesn't support 128bit types anyway.
Review of attachment 202398 [details] [review]: just some drive by review on my own patch...(I'd still like review later from Colin or someone) ::: newer-but-still-old/gi/function.c.fix-ffi-on-big-endian @@ +241,3 @@ + case GI_TYPE_TAG_BOOLEAN: + case GI_TYPE_TAG_UNICHAR: + *(ffi_arg *) result = (guint32) return_value.v_uint32; all of these casts are unnecessary. @@ +255,3 @@ + case GI_INFO_TYPE_ENUM: + case GI_INFO_TYPE_FLAGS: + *(ffi_sarg *) result = (gint32) return_value.v_long; this is because a jsval for an enums is 64 bit i guess, but we prep ffi for enums to be 32 bit. it might be better to make changes in this function a separate commit since it's sort of a separate fix. @@ +757,3 @@ + break; + default: + break; spacing problems here.
Just to follow up to comment 4, the test suite still passes on x86_64 with this patch and gnome-shell still runs.
Review of attachment 202398 [details] [review]: I don't like how this fix duplicates a lot of special code between the closure case and the C function invocation case; also, gjs_invoke_c_function is already way too long. It would make more sense to me to add gjs_value_from_return () to arg.[ch].
note the code isn't really duplicated. Just similar in structure. We could factor that switch statements out, but they'd need to be two different functions.
I think this broke gnome-shell for me. I get tons of clutter errors like these: (gnome-shell:2530): Clutter-CRITICAL **: clutter_paint_volume_set_width: assertion `width >= 0.0f' failed the same for height. Practical effect is that system icons are all "crushed" next to my username. And most popups are drawn without rounded corners. Reverting this fixes the issue for me. Can try again and double-check if you have any clue about this. Not sure how to check my arch/etc but I'm using 32bits Debian on a intel core 2 duo. commit ca276a8281a045949fd3c2ad338260d808559a30 Author: Colin Walters <walters@verbum.org> Date: Wed Dec 21 16:51:30 2011 -0500 function: Correctly convert from ffi return values to GIArgument on big-endi ffi always returns a long value even if a function's return type is e.g. guint8. We can't just directly give ffi_call a GIArgument to store its return value because when the size of the return type is smaller than long, it will write into the wrong address. Instead cast the value at the C level. Based on a patch from Ray Strode <rstrode@redhat.com> https://bugzilla.gnome.org/show_bug.cgi?id=665152
I can confirm test failures in jhbuild on x86_32, we're investigating now.
For easy spelunking later, the fix for this is here: http://git.gnome.org/browse/gjs/commit/?id=dc69b12d5e44f9d3b209759082f721237a8c9a06
*** Bug 639505 has been marked as a duplicate of this bug. ***
*** Bug 670350 has been marked as a duplicate of this bug. ***