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 770608 - Wrong enum to hash conversion on 64-bit big endian
Wrong enum to hash conversion on 64-bit big endian
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2016-08-30 16:14 UTC by Aurelien Jarno
Modified: 2016-09-03 16:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to fix the issue (7.94 KB, patch)
2016-08-30 16:14 UTC, Aurelien Jarno
none Details | Review
Patch to fix the issue (11.56 KB, patch)
2016-08-31 09:28 UTC, Aurelien Jarno
none Details | Review
patch v3 (11.67 KB, patch)
2016-08-31 22:15 UTC, Aurelien Jarno
committed Details | Review

Description Aurelien Jarno 2016-08-30 16:14:33 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.
Comment 1 Christoph Reiter (lazka) 2016-08-30 17:20:40 UTC
Thanks. Would it be possible to add some tests for this?
Comment 2 Andreas Henriksson 2016-08-31 04:58:43 UTC
(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.
Comment 3 Aurelien Jarno 2016-08-31 09:28:10 UTC
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.
Comment 4 Christoph Reiter (lazka) 2016-08-31 14:19:51 UTC
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)
Comment 5 Aurelien Jarno 2016-08-31 15:34:36 UTC
(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.
Comment 6 Christoph Reiter (lazka) 2016-08-31 16:57:26 UTC
(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.
Comment 7 Aurelien Jarno 2016-08-31 22:15:58 UTC
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.
Comment 8 Christoph Reiter (lazka) 2016-09-01 16:02:04 UTC
Review of attachment 334563 [details] [review]:

Thanks, lgtm