GNOME Bugzilla – Bug 668903
Broken marshalling on big endian architectures
Last modified: 2012-03-27 15:41:28 UTC
Created attachment 206321 [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. Note that some unit tests still fail, so there's probably more issues left, but this patch and the patch from gobject-introspection bug 668902 together allow running gnome-tweak-tool on PPC.
I see pygobject 3.1.0 was released without this fix. Can it be considered for the next release?
When I apply this, I get a test failure on x86_64: test_ghashtable_int_none_in (test_gi.TestGHashTable) ... ** ERROR:/home/martin-scratch/gnome/share/gobject-introspection-1.0/tests/gimarshallingtests.c:2739:gi_marshalling_tests_ghashtable_int_none_in: assertion failed: (GPOINTER_TO_INT(g_hash_table_lookup(hash_table, GINT_TO_POINTER(-1))) == 1)
AFAICT that's another similar bug in _pygi_marshal_from_py_ghash: g_hash_table_insert (hash_, key.v_pointer, value.v_pointer); key and value contain 32 bit signed integers at this point (possibly other types for other hash tables?), so their v_pointer members can't be used directly. I'll look into a possible solution for this after lunch, or feel free to beat me to it. :)
Created attachment 207282 [details] [review] FFI marshalling fixes
Created attachment 207283 [details] [review] Python marshalling fixes
I found and fixed more Python marshalling issues and split up the patch between the FFI and Python specific parts. No test regressions on amd64 anymore, and the test results on powerpc are almost on par now: test_python_calls_sync (test_gdbus.TestGDBusClient) fails, but I'm leaving that one for now.
In bug https://bugzilla.gnome.org/show_bug.cgi?id=668902 walters introduced gi_type_info_extract_ffi_return_value() so I guess these patches should use it as it was done in https://bugzilla.gnome.org/show_bug.cgi?id=670249 for gjs
These fixes needs to be included as well; http://git.gnome.org/browse/gjs/commit/?id=dc69b12d5e44f9d3b209759082f721237a8c9a06
Created attachment 209241 [details] [review] Use gi_cclosure_marshal_generic instead of duplicating it. Actually, it occurred to me that this code is duplicating gi_cclosure_marshal_generic. This patch uses that instead.
Created attachment 209242 [details] [review] Use gi_cclosure_marshal_generic instead of duplicating it. Also remove superfluous static variable.
That's a nice cleanup indeed! However, with that I get some test case failures on x86_64 and a crash: testTest3 (test_signal.TestCMarshaller) ... FAIL testTest4 (test_signal.TestCMarshaller) ... ok testTestReturnDouble (test_signal.TestCMarshaller) ... FAIL testTestReturnFloat (test_signal.TestCMarshaller) ... FAIL testTestReturnObject (test_signal.TestCMarshaller) ... make[2]: *** [check-local] Segmentation fault (core dumped) Do you get these as well?
(In reply to comment #11) > Do you get these as well? I did, fixed by https://bugzilla.gnome.org/show_bug.cgi?id=668902#c13 .
Ah, splendid. Indeed these were commited just now, so I didn't have them in jhbuild yet. I'll bump introspection_required_version to 1.31.21 for this (i. e. newer than the current release 1.31.20), to make sure that these two work well together.
Created attachment 209255 [details] make check.valgdind diff As Tomeu asked,this is the diff between make check.valgrind without and with the patch. I edited the PIDs and addresses to make the logs diffable.
Running check.valgrind twice without the patch gives me an even bigger difference, so this is well within the noise limit. Thanks for your work! I pushed this now. Unfortunately g-i did not bump its version after the 1.31.20 release yet, so I left the dependency at that version.
(In reply to comment #15) > I pushed this now. Thanks! However, I'm reopening this report, as the Python marshalling bugs fixed by the other attached patch remain unfixed. Could that one be applied as well? Without it, a lot of make check tests and gnome-tweak-tool still fail on PPC. > Unfortunately g-i did not bump its version after the 1.31.20 release yet, so I > left the dependency at that version. That's fine; g-i 1.31.20 should work as well as pygobject's previous code.
Michael, I reviewed the other patch (marshalling fixes). This looks correct, I pushed it to master now. Thank you!
Hi, commit 7746d2188ac4933c2c9011d84525d1e62fc18953 broke my python application: ======== ** ERROR:pygi-marshal-to-py.c:526:_pygi_hash_pointer_to_arg: code should not be reached Annullato (core dump creato) ======== I'll try to understand why, and maybe file a bug (or reopen this one). Meanwhile, does anyone have any clue on what could be going wrong?
(In reply to comment #18) > ERROR:pygi-marshal-to-py.c:526:_pygi_hash_pointer_to_arg: code should not be > reached This means _pygi_hash_pointer_to_arg is called for a type it doesn't know how to handle yet. Please run your app in a debugger, set a breakpoint on g_assert_not_reached and attach the backtrace from when it's hit. Please make sure the pygobject binaries have debugging symbols for this.
Hi Michel, the following patch fixes the problem for me: ============ diff --git a/gi/pygi-marshal-from-py.c b/gi/pygi-marshal-from-py.c index 1926b35..70a2b27 100644 --- a/gi/pygi-marshal-from-py.c +++ b/gi/pygi-marshal-from-py.c @@ -1050,8 +1050,10 @@ _pygi_arg_to_hash_pointer (const GIArgument *arg, return GINT_TO_POINTER(arg->v_int32); case GI_TYPE_TAG_UTF8: case GI_TYPE_TAG_FILENAME: + case GI_TYPE_TAG_INTERFACE: return arg->v_pointer; default: + g_warning("Unsupported type %d", type_tag); g_assert_not_reached(); return arg->v_pointer; } diff --git a/gi/pygi-marshal-to-py.c b/gi/pygi-marshal-to-py.c index ce93257..c4ef6f6 100644 --- a/gi/pygi-marshal-to-py.c +++ b/gi/pygi-marshal-to-py.c @@ -521,8 +521,10 @@ _pygi_hash_pointer_to_arg (GIArgument *arg, break; case GI_TYPE_TAG_UTF8: case GI_TYPE_TAG_FILENAME: + case GI_TYPE_TAG_INTERFACE: break; default: + g_warning("Unsupported type %d", type_tag); g_assert_not_reached(); } } ============
(In reply to comment #20) > the following patch fixes the problem for me: Great, thanks, I hope someone can apply it. Maybe the pygobject developers can see more types that need to be added, I just took a guess, not being familiar with the code.
Reopening. Always make sure to either reopen the bug or file a new one otherwise things get forgotten.
Patch looks good to me, logging the unsupported type tag is a good idea, but would be better to use g_type_tag_to_string(). Instead of g_warning we may want to use g_critical and remove the g_assert_not_reached. I recommend to only land this with tests, otherwise it is bound to happen again. I would also suggest following https://live.gnome.org/GnomeLove/SubmittingPatches .
I've now been spending some time trying to write a regression test, but with no success: the tests always work, even when they shouldn't. I've tried to create a function (in GI) which takes a RegressTestInterface * as a parameter, then I've built an object which implements that interface (I've called it TestImplObj) which I instantiate from the python code, and pass as parameter to the function. I would expect to run into the assertion, but it just works. If someone has any clue on how to run into that assertion, please advise and I'll write a test for that.
Created attachment 210175 [details] [review] Support marshalling of GI_TYPE_TAG_INTERFACE Here is the revised patch, as suggested by Tomeu. As I wrote on comment 24, I couldn't figure out how to write a regression test for this.
How did you run into the regression then? I guess you have some code which triggers it, so it might be easier to strip this down?
(In reply to comment #26) > How did you run into the regression then? I guess you have some code which > triggers it, so it might be easier to strip this down? Yeah, or even a backtrace would be enough to figure out what exact conditions trigger this bug.
Backtrace: ====================
+ Trace 229923
The failing code is at: http://bazaar.launchpad.net/~mardy/unity-lens-gdocs/opensesame/view/head:/unity-lens-gdocs#L182 The crash happens when executing this line: ====== session_data = auth_data.get_parameters() ====== AuthData is this boxed type: http://code.google.com/p/accounts-sso/source/browse/libaccounts-glib/ag-auth-data.c?repo=libaccounts-glib I can swear that the GValues in the GHashTable it returns do not contain any GObject, but just strings, integers, booleans, and list of strings.
Actually, this is what the session_data dictionary will contain, when running the program with the patched python-gobject: ====== (Pdb) p session_data {'ResponseType': 'token', 'ClientId': '1041829795610-htf69c529db58qcq8jvf58bijn1ie3oi.apps.googleusercontent.com', 'Host': 'accounts.google.com', 'Scope': ['https://docs.google.com/feeds/'], 'RedirectUri': 'http://www.mardy.it/oauth2callback', 'AuthPath': 'o/oauth2/auth'} ====== Could the GStrv be interpreted as a GInterface? :-O
(In reply to comment #28) > Backtrace: > > #3 0x00007ffff5f518d1 in _pygi_hash_pointer_to_arg (type_tag=<optimized out>, arg=<optimized out>) Any chance you could find out what are these arguments?
Created attachment 210250 [details] [review] gobject-introspection: add regression tests for GHashTables of GValues Here's the patch to add a couple of tests to gobject-introspection, which help in exposing this bug.
Created attachment 210251 [details] [review] Regression tests Regression tests (to be applied in python-gobject). Martin Pitt pointed out that the failure is not related to the marshalling of the GStrv, but rather of any GValue used as a GHashTable value. The second test is known to fail, because it GStrv cannot be marshalled in a GValue by python-gobject (should I file a separate bug for that?), but I think it's a good idea to keep it anyway. :-)
Comment on attachment 210250 [details] [review] gobject-introspection: add regression tests for GHashTables of GValues I checked with Colin, and g-i is currently in hard freeze like the rest of GNOME, so we cannot currently commit the test cases. I'm happy to commit them after the freeze, though. The patches look fine to me, thank you!
Comment on attachment 210251 [details] [review] Regression tests The skipped GStrv one is a good reminder, so I'm fine with keeping this. Logging a separate bug for this would be nice, though.
Keeping open as the tests still need to be committed.
My memory is amazingly leaking: I filed that GStrv issue already some time ago, see bug 666636.
Comment on attachment 210250 [details] [review] gobject-introspection: add regression tests for GHashTables of GValues Pushed the test additions to g-i.
Comment on attachment 210251 [details] [review] Regression tests Committed regression tests with slight PEP-8 fix. Thanks!