After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 665152 - gjs doesn't use ffi_call the way it expects to be used
gjs doesn't use ffi_call the way it expects to be used
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
ca276a8281a045949fd3c2ad338260d808559a30
: 639505 (view as bug list)
Depends on: 665150
Blocks:
 
 
Reported: 2011-11-29 19:59 UTC by Ray Strode [halfline]
Modified: 2012-02-18 16:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
need commit message (4.44 KB, patch)
2011-11-29 20:19 UTC, Ray Strode [halfline]
committed Details | Review

Description Ray Strode [halfline] 2011-11-29 19:59:22 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.
Comment 1 Ray Strode [halfline] 2011-11-29 20:00:32 UTC
This is needed for the gjs test suite to pass on ppc64.  We also need bug 665150 fixed for the test suite to pass.
Comment 2 Ray Strode [halfline] 2011-11-29 20:19:53 UTC
Created attachment 202398 [details] [review]
need commit message

Will write after review.
Comment 3 Ray Strode [halfline] 2011-11-29 20:51:51 UTC
Downstream report is here btw: https://bugzilla.redhat.com/show_bug.cgi?id=749604
Comment 4 Ray Strode [halfline] 2011-11-29 21:06:16 UTC
Note, i've only tested this on ppc64, I should probably make sure it didn't regress x86_64
Comment 5 Ray Strode [halfline] 2011-11-29 21:16:08 UTC
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.
Comment 6 Ray Strode [halfline] 2011-11-30 00:00:31 UTC
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.
Comment 7 Ray Strode [halfline] 2011-11-30 16:43:49 UTC
Just to follow up to comment 4, the test suite still passes on x86_64 with this patch and gnome-shell still runs.
Comment 8 Colin Walters 2011-12-08 20:29:17 UTC
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].
Comment 9 Ray Strode [halfline] 2011-12-08 20:50:21 UTC
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.
Comment 10 Diego Escalante Urrelo (not reading bugmail) 2011-12-29 05:56:54 UTC
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
Comment 11 Colin Walters 2012-01-04 18:53:32 UTC
I can confirm test failures in jhbuild on x86_32, we're investigating now.
Comment 12 Ray Strode [halfline] 2012-01-04 22:32:52 UTC
For easy spelunking later, the fix for this is here:

http://git.gnome.org/browse/gjs/commit/?id=dc69b12d5e44f9d3b209759082f721237a8c9a06
Comment 13 Colin Walters 2012-01-26 18:25:40 UTC
*** Bug 639505 has been marked as a duplicate of this bug. ***
Comment 14 Colin Walters 2012-02-18 16:38:58 UTC
*** Bug 670350 has been marked as a duplicate of this bug. ***