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 668903 - Broken marshalling on big endian architectures
Broken marshalling on big endian architectures
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal major
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2012-01-28 12:37 UTC by Michel Dänzer
Modified: 2012-03-27 15:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Properly convert values between glib and FFI (6.93 KB, patch)
2012-01-28 12:37 UTC, Michel Dänzer
none Details | Review
FFI marshalling fixes (5.50 KB, patch)
2012-02-10 17:46 UTC, Michel Dänzer
none Details | Review
Python marshalling fixes (5.44 KB, patch)
2012-02-10 17:47 UTC, Michel Dänzer
committed Details | Review
Use gi_cclosure_marshal_generic instead of duplicating it. (10.12 KB, patch)
2012-03-08 12:14 UTC, Michel Dänzer
none Details | Review
Use gi_cclosure_marshal_generic instead of duplicating it. (10.48 KB, patch)
2012-03-08 12:19 UTC, Michel Dänzer
committed Details | Review
make check.valgdind diff (34.31 KB, text/plain)
2012-03-08 15:25 UTC, Martin Pitt
  Details
Support marshalling of GI_TYPE_TAG_INTERFACE (1.67 KB, patch)
2012-03-20 12:55 UTC, Mardy
committed Details | Review
gobject-introspection: add regression tests for GHashTables of GValues (5.48 KB, patch)
2012-03-21 15:05 UTC, Mardy
committed Details | Review
Regression tests (1.58 KB, patch)
2012-03-21 15:19 UTC, Mardy
committed Details | Review

Description Michel Dänzer 2012-01-28 12:37:14 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.
Comment 1 Michel Dänzer 2012-02-10 08:57:12 UTC
I see pygobject 3.1.0 was released without this fix. Can it be considered for the next release?
Comment 2 Martin Pitt 2012-02-10 09:25:36 UTC
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)
Comment 3 Michel Dänzer 2012-02-10 11:47:26 UTC
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. :)
Comment 4 Michel Dänzer 2012-02-10 17:46:49 UTC
Created attachment 207282 [details] [review]
FFI marshalling fixes
Comment 5 Michel Dänzer 2012-02-10 17:47:28 UTC
Created attachment 207283 [details] [review]
Python marshalling fixes
Comment 6 Michel Dänzer 2012-02-10 17:54:48 UTC
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.
Comment 7 Paolo Borelli 2012-03-05 21:43:05 UTC
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
Comment 8 Johan (not receiving bugmail) Dahlin 2012-03-06 16:52:07 UTC
These fixes needs to be included as well; 

http://git.gnome.org/browse/gjs/commit/?id=dc69b12d5e44f9d3b209759082f721237a8c9a06
Comment 9 Michel Dänzer 2012-03-08 12:14:25 UTC
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.
Comment 10 Michel Dänzer 2012-03-08 12:19:27 UTC
Created attachment 209242 [details] [review]
Use gi_cclosure_marshal_generic instead of duplicating it.

Also remove superfluous static variable.
Comment 11 Martin Pitt 2012-03-08 14:13:08 UTC
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?
Comment 12 Michel Dänzer 2012-03-08 14:16:48 UTC
(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 .
Comment 13 Martin Pitt 2012-03-08 15:03:56 UTC
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.
Comment 14 Martin Pitt 2012-03-08 15:25:28 UTC
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.
Comment 15 Martin Pitt 2012-03-08 15:40:30 UTC
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.
Comment 16 Michel Dänzer 2012-03-08 17:25:25 UTC
(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.
Comment 17 Martin Pitt 2012-03-09 11:29:02 UTC
Michael,

I reviewed the other patch (marshalling fixes). This looks correct, I pushed it to master now. Thank you!
Comment 18 Mardy 2012-03-20 08:16:21 UTC
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?
Comment 19 Michel Dänzer 2012-03-20 08:22:39 UTC
(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.
Comment 20 Mardy 2012-03-20 08:31:56 UTC
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();
     }
 }
============
Comment 21 Michel Dänzer 2012-03-20 08:47:02 UTC
(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.
Comment 22 Paolo Borelli 2012-03-20 08:50:47 UTC
Reopening. Always make sure to either reopen the bug or file a new one otherwise things get forgotten.
Comment 23 Tomeu Vizoso 2012-03-20 09:02:44 UTC
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 .
Comment 24 Mardy 2012-03-20 12:40:34 UTC
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.
Comment 25 Mardy 2012-03-20 12:55:23 UTC
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.
Comment 26 Martin Pitt 2012-03-21 07:05:32 UTC
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?
Comment 27 Tomeu Vizoso 2012-03-21 08:00:34 UTC
(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.
Comment 28 Mardy 2012-03-21 08:47:08 UTC
Backtrace:

====================
  • #0 __GI_raise
    at ../nptl/sysdeps/unix/sysv/linux/raise.c line 64
  • #1 __GI_abort
    at abort.c line 91
  • #2 g_assertion_message
    at /build/buildd/glib2.0-2.31.22/./glib/gtestutils.c line 1860
  • #3 _pygi_hash_pointer_to_arg
    at /build/buildd/pygobject-3.1.92/gi/pygi-marshal-to-py.c line 526
  • #4 _pygi_hash_pointer_to_arg
    at /build/buildd/pygobject-3.1.92/gi/pygi-marshal-to-py.c line 520
  • #5 _pygi_marshal_to_py_ghash
    at /build/buildd/pygobject-3.1.92/gi/pygi-marshal-to-py.c line 589
  • #6 _invoke_marshal_out_args
    at /build/buildd/pygobject-3.1.92/gi/pygi-invoke.c line 521
  • #7 _wrap_g_callable_info_invoke
    at /build/buildd/pygobject-3.1.92/gi/pygi-invoke.c line 638
  • #8 PyEval_EvalFrameEx
  • #9 PyEval_EvalCodeEx

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.
Comment 29 Mardy 2012-03-21 08:51:17 UTC
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
Comment 30 Tomeu Vizoso 2012-03-21 09:55:03 UTC
(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?
Comment 31 Mardy 2012-03-21 15:05:18 UTC
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.
Comment 32 Mardy 2012-03-21 15:19:32 UTC
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 33 Martin Pitt 2012-03-21 15:46:01 UTC
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 34 Martin Pitt 2012-03-21 15:46:53 UTC
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.
Comment 35 Martin Pitt 2012-03-21 15:47:28 UTC
Keeping open as the tests still need to be committed.
Comment 36 Mardy 2012-03-21 16:14:37 UTC
My memory is amazingly leaking: I filed that GStrv issue already some time ago, see bug 666636.
Comment 37 Martin Pitt 2012-03-27 15:30:39 UTC
Comment on attachment 210250 [details] [review]
gobject-introspection: add regression tests for GHashTables of GValues

Pushed the test additions to g-i.
Comment 38 Martin Pitt 2012-03-27 15:41:15 UTC
Comment on attachment 210251 [details] [review]
Regression tests

Committed regression tests with slight PEP-8 fix. Thanks!