GNOME Bugzilla – Bug 770608
Wrong enum to hash conversion on 64-bit big endian
Last modified: 2016-09-03 16:20:38 UTC
Created attachment 334457 [details] [review] Patch to fix the issue The libsecret testsuite hangs when running the test-py-lookup.py. It happens that the issue is actually in pygobject, when using a GHashTable object pointing to enums. A GList of enums likely have the same issue. glist and ghashtable objects both store pointers. Complex objects are stored as pointers to the objects, but simpler objects like an integer value are stored directly as a pointer, using for example the GINT_TO_POINTER and GPOINTER_TO_INT macros. This is done in pygobject with the _pygi_hash_pointer_to_arg and _pygi_arg_to_hash_pointer functions. These functions handle the various type of objects. However they consider that an enum, represented with the GI_TYPE_TAG_INTERFACE type (extended interface object), are always a pointer. This is wrong as it is often a 32-bit value. Therefore on 64-bit big endian machines, the value is handle with the 2 32-bit parts swapped. You'll find a patch to fix that as an attachement.
Thanks. Would it be possible to add some tests for this?
(In reply to Christoph Reiter (lazka) from comment #1) > Thanks. Would it be possible to add some tests for this? Fwiw, This was originally reported as libsecret fail to build from source on 64bit big endian Debian because of libsecret testsuite failure, see: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=821347 The test is thus building libsecret and doing make check there on affected architectures. ;) Maybe something could be extracted from that an put straight into a pygobject testcase if needed.
Created attachment 334512 [details] [review] Patch to fix the issue Please find a new patch attached, which includes two additional tests. I don't know the framework a lot, so don't hesitate to make some changes. I guess in the long term the two new functions added to gimarshallingtestsextra.c should be added to gimarshallingtests.c in gobject-introspection.
Review of attachment 334512 [details] [review]: ::: gi/pygi-argument.c @@ +92,3 @@ + + if (type_tag == GI_TYPE_TAG_INTERFACE) { + GIBaseInfo *interface = g_type_info_get_interface (type_info); interface is leaking ::: tests/gimarshallingtestsextra.c @@ +52,3 @@ + * gi_marshalling_tests_ghashtable_enum_none_return: + * + * Returns: (element-type gint GIMarshallingTestsExtraEnum) (transfer none): (transfer full) @@ +55,3 @@ + */ +GHashTable * +gi_marshalling_tests_ghashtable_enum_none_return (void) ..._full_return (...) ::: tests/test_gi.py @@ +1278,1 @@ Missing newline here (pep8 complains; "make check" will only invoke pep8 if it is installed)
(In reply to Christoph Reiter (lazka) from comment #4) > Review of attachment 334512 [details] [review] [review]: > > ::: gi/pygi-argument.c > @@ +92,3 @@ > + > + if (type_tag == GI_TYPE_TAG_INTERFACE) { > + GIBaseInfo *interface = g_type_info_get_interface (type_info); > > interface is leaking I'll fix that. > ::: tests/gimarshallingtestsextra.c > @@ +52,3 @@ > + * gi_marshalling_tests_ghashtable_enum_none_return: > + * > + * Returns: (element-type gint GIMarshallingTestsExtraEnum) (transfer none): > > (transfer full) > > @@ +55,3 @@ > + */ > +GHashTable * > +gi_marshalling_tests_ghashtable_enum_none_return (void) > > ..._full_return (...) This test is based on the gi_marshalling_tests_ghashtable_*_none_return tests from gobject-introspection/tests/gimarshallingtests.c (see for example gi_marshalling_tests_ghashtable_int_none_return or gi_marshalling_tests_ghashtable_utf8_none_return). They allocate the GHashTable object with g_hash_table_new and they return it. They don't have full in the name nor use transfer full. I have to say I don't fully understand what is correct there, but at least it doesn't seem consistent. > ::: tests/test_gi.py > @@ +1278,1 @@ > > > Missing newline here (pep8 complains; "make check" will only invoke pep8 if > it is installed) Ok, I'll fix that.
(In reply to Aurelien Jarno from comment #5) > This test is based on the gi_marshalling_tests_ghashtable_*_none_return > tests from gobject-introspection/tests/gimarshallingtests.c (see for example > gi_marshalling_tests_ghashtable_int_none_return or > gi_marshalling_tests_ghashtable_utf8_none_return). They allocate the > GHashTable object with g_hash_table_new and they return it. They don't have > full in the name nor use transfer full. > > I have to say I don't fully understand what is correct there, but at least > it doesn't seem consistent. Both (int/utf8) functions always return the same hash table while your test returns a new hash table on every call. In the former case the bindings need to copy the hash table and in the later the bindings can take it as is.
Created attachment 334563 [details] [review] patch v3 Please find attached a new version of the patch, which should hopefully fix all the issues reported in the review.
Review of attachment 334563 [details] [review]: Thanks, lgtm
https://git.gnome.org/browse/pygobject/commit/?id=f4d858c069f06e7060a0bb067c29f5bffb7869ee