GNOME Bugzilla – Bug 754882
Argument becomes zero when having GEnum/GFlags typed argument in signal on s390x/ppc64 arch
Last modified: 2015-09-16 21:11:45 UTC
Created attachment 311147 [details] test program to reproduce issue This is a copy of downstream: https://bugzilla.redhat.com/show_bug.cgi?id=1260577 When you have a glib-signal with GEnum/GFlags typed arguments, then the passed arguments are wrongly zero. - only hit it on Linux; s390x and ppc64 arch - reproducible both with GEnum/GFlags typed argument. Replacing them with G_TYPE_UINT/G_TYPE_INT works around the bug. Note that the Enum types *are* compiled as 32 bit integers: sizeof(TestFlags) == sizeof(int). - having other arguments for the signals seems relevant - only hits when having more then one handlers connected. (could obviously be a libffi bug... but it's unclear how, because glib treats GEnum/GFlags as integers before calling ffi_call(). IOW, libffi doesn't know about GEnum/GFlags. I hit this on NetworkManager, strangely there are two signals that seem candidates for this issue, but only one of the two signals showed the issue. So, there might be more involved. Good news: the attached program fails, complied with gcc -Og -g `pkg-config --cflags --libs glib-2.0 gobject-2.0` ./test-rh1062301.c Affected glib-master, but also glib2 on rhel-7.2. Tested on RHEL-7.2-alpha.
Created attachment 311469 [details] [review] signal: Simplify ffi enum handling code This causes a larger alloca than needed, but I don't think we'll blow the stack by allocating 5 unneeded ints. As a side effect, it fixes 64bit BE architectures enum handling.
The patch above fixes some weirdness in how the ffi handling code in glib uses alloca(). I have no idea if gcc trips over it. So here's the patch so you can see if it helps. Surprisingly, I have no ppc64 or s390x box here to test it myself.
Review of attachment 311469 [details] [review]: I see how it's simpler, but I don't understand how it fixes the bug. Is the bug somehow in reuse of the temporary location?
Created attachment 311473 [details] [review] ffi: Marshal flags like enums Flags are enums. Fixes broken marshalling on BE 64bit architectures.
Created attachment 311474 [details] [review] ffi: Marshal flags like enums Flags are enums. Fixes broken marshalling on BE 64bit architectures.
Created attachment 311483 [details] [review] ffi: Marshal flags like enums Flags are enums. Fixes broken marshalling on BE 64bit architectures.
Review of attachment 311483 [details] [review]: This patch makes a lot more sense to me.
Created attachment 311485 [details] [review] tests: Make testcase not pass 0 as a flags value This will not catch the case where we fail in libffi and always use 0. In fact, be a real annoying person and use (1 << 31) as a flags value to test signedness, too. Also update the testcase to actually use flags everywhere and ot uint.
(In reply to Benjamin Otte (Company) from comment #6) > Created attachment 311483 [details] [review] [review] > ffi: Marshal flags like enums > > Flags are enums. > Fixes broken marshalling on BE 64bit architectures. I tested both patches (attachment 311469 [details] [review], attachment 311483 [details] [review]), alone and in combination. The patch from attachment 311483 [details] [review] makes the test program pass and appears to fix the bug.
(In reply to Benjamin Otte (Company) from comment #8) > Created attachment 311485 [details] [review] [review] > tests: Make testcase not pass 0 as a flags value > > This will not catch the case where we fail in libffi and always use 0. > In fact, be a real annoying person and use (1 << 31) as a flags value to > test signedness, too. > > Also update the testcase to actually use flags everywhere and ot uint. Tested this patch too and the test correctly shows the failure (and the fix).
Review of attachment 311485 [details] [review]: LGTM
Committed the two fixes. Do we want the simplification, too? Attachment 311483 [details] pushed as 7b685ea - ffi: Marshal flags like enums Attachment 311485 [details] pushed as 605ff1e - tests: Make testcase not pass 0 as a flags value
(In reply to Benjamin Otte (Company) from comment #12) > Committed the two fixes. > > Do we want the simplification, too? I'd vote no - it's not a bad patch, but doesn't help anything either. But it's not a strong no.
(In reply to Thomas Haller from comment #0) > - reproducible both with GEnum/GFlags typed argument. Replacing them with hm, just retried with GEnum and failed to do reproduce the original failure. Meaning, I somehow messed up my original test (and I also know how that happened). Meaning, this bug is fixed for good. Thanks!!