Bug 842880
Summary: | ValueError: row sequence has the incorrect number of elements | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Mark Hamzy <hamzy> | ||||
Component: | pygobject3 | Assignee: | Dave Malcolm <dmalcolm> | ||||
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | urgent | Docs Contact: | |||||
Priority: | urgent | ||||||
Version: | 17 | CC: | dmalcolm, gustavold, icq, johnp, mclasen, mpitt, pknirsch | ||||
Target Milestone: | --- | ||||||
Target Release: | --- | ||||||
Hardware: | ppc64 | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2012-10-11 16:11:14 UTC | Type: | Bug | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 846990 | ||||||
Attachments: |
|
Description
Mark Hamzy
2012-07-24 19:58:42 UTC
I see this too, with: pygobject3-3.2.2-1.fc17.ppc64 gobject-introspection-1.32.1-1.fc17.ppc64 Adding: print(store.get_n_columns()) before the append call shows that the list store's number of columns is 0, rather than 2 (hence the exception), and adding a breakpoint on gtk_tree_model_get_n_columns itself, and stepping through to the underlying gtk_list_store_get_n_columns shows that priv->n_columns is indeed 0 at the C level: (gdb) p *priv $84 = {default_sort_func = 0, default_sort_destroy = 0, sort_list = 0x0, column_headers = 0x0, stamp = 1018334934, n_columns = 0, sort_column_id = -2, length = 0, order = GTK_SORT_ASCENDING, columns_dirty = 0, default_sort_data = 0x0, seq = 0x102d1d90} This suggests that the GtkListStore isn't getting the columns added correctly on construction, where: /usr/lib64/python2.7/site-packages/gi/overrides/Gtk.py:ListStore.__init__ has a call to: self.set_column_types(column_types) Putting a breakpoint in gtk_list_store_set_column_types: void gtk_list_store_set_column_types (GtkListStore *list_store, gint n_columns, GType *types) shows that n_columns is being indeed passed through as 0 for some reason. Am investigating. Upstream bug: https://bugzilla.gnome.org/show_bug.cgi?id=680693 Unfortunately neither Debian nor Ubuntu have porter boxes for powerpc64, so I cannot reproduce this. Would it be possible to get temporary access to such a machine, with the build dependencies and gdb installed? I do not immediately see the problem. It sure smells like a 64 bit int is temporarily converted to a 32 bit int somewhere, which loses the lower half on big endian platforms like ppc64). The gir and the C function both declare "gint", so these are okay. I assume "gint" is 64 bits on ppc64, unlike on x86_64? Created attachment 601336 [details] Patch against pygobject-3.2.2 tarball to fix invalid long coercion in _pygi_marshal_from_py_array Reviewing the arg within pygi-invoke.c's _invoke_callable(), (gdb) p state->in_args[1] $47 = {v_boolean = 0, v_int8 = 0 '\000', v_uint8 = 0 '\000', v_int16 = 0, v_uint16 = 0, v_int32 = 0, v_uint32 = 0, v_int64 = 2, v_uint64 = 2, v_float = 0, v_double = 9.8813129168249309e-324, v_short = 0, v_ushort = 0, v_int = 0, v_uint = 0, v_long = 2, v_ulong = 2, v_ssize = 2, v_size = 2, v_string = 0x2 <Address 0x2 out of bounds>, v_pointer = 0x2} shows that it's been incorrectly set: although the 64-bit int values in the union are 2 (which is the "correct" value), v_int is 0. The bug is in pygobject-3.2.2/gi/pygi-marshal-from-py.c:_pygi_marshal_from_py_array in the highlighted lines: 902 if (child_cache->direction == PYGI_DIRECTION_BIDIRECTIONAL) { 903 gint *len_arg = (gint *)state->in_args[child_cache->c_arg_index].v_pointer; 904 /* if we are not setup yet just set the in arg */ 905 if (len_arg == NULL) >>>906 state->in_args[child_cache->c_arg_index].v_long = length; 907 else 908 *len_arg = length; 909 } else { >>>910 state->in_args[child_cache->c_arg_index].v_long = length; 911 } where the "v_long" variant of the arguments is set to the length of the array. However the call is expecting an "int", rather than a "long", and hence on 64-bit big-endian architectures the length is truncated to 0 (for lists less than 2^32 in size). child_cache->type_tag is GI_TYPE_TAG_INT32 (for "n_columns") so we do have the size available here. I'm attaching a patch (against 3.2.2) which fixes the lines above to use child_cache to set the length, rather than treating it as a "long". This fixes the call to gtk_list_store_set_columns, but I'm then running into an issue with the call to insert_with_valuesv, where the error handler is being called for some reason. Thanks for the patch, Dave! A few comments: a) There is also a _pygi_marshal_to_py_array() function that does the exact same assumption of child args being "long" (see gi/pygi-marshal-to-py.c:287). Shouldn't your patch cover that one also? I didn't find a test case to trigger it, but I also didn't try hard :) b) The attached patch has tabs in it, the rest of the file uses only spaces. c) The rest of the patch looks fine, however it relies on "child_cache->type_tag" being initialized. It is only being initialized for child args on the corner case where the index argument comes *before* the array (it is the case for gtk_list_store_set_column_types()). To initialize "child_cache->type_tag" properly you can do the following: Index: pygobject-3.2.2/gi/pygi-cache.c =================================================================== --- pygobject-3.2.2.orig/gi/pygi-cache.c +++ pygobject-3.2.2/gi/pygi-cache.c @@ -1379,6 +1379,8 @@ _args_cache_generate (GICallableInfo *ca callable_cache->n_to_py_child_args++; } + arg_cache->type_tag = g_type_info_get_tag (type_info); + g_base_info_unref ( (GIBaseInfo *)arg_info); continue; } Just a quick drive by: Wrt. reproducing, the test suite (make check) is quite thorough and will hopefully reproduce at least some of these errors. E. g. we do test ListStore. This is the usual way to test fixes for us (and if it's not reproduced by current tests, we need to write a new one). Thanks! Will look at this a bit more closely tomorrow or Friday. (In reply to comment #6) > Thanks! Will look at this a bit more closely tomorrow or Friday. Martin: I've folded the changes in my patch into a bigger patch, which is attached to bug 841596. It's a work-in-progress: it fixes many test-suite failures, but I'm still seeing a lot, alas. (In reply to comment #7) > (In reply to comment #6) > > Thanks! Will look at this a bit more closely tomorrow or Friday. > Martin: I've folded the changes in my patch into a bigger patch, which is > attached to bug 841596. It's a work-in-progress: it fixes many test-suite > failures, but I'm still seeing a lot, alas. https://bugzilla.redhat.com/attachment.cgi?id=601820 is the current latest version. Thanks for your work so far! I managed to set up the mock instance on the porter box that Brent Baude set up, and can reproduce the gazillion test case failures. The current patch indeed helps a lot already, thanks for that! It looks good to me, although it is certainly incomplete still. Do you want me to push this already, or want to continue working on it? I'll be on holidays for the next two weeks, but you can ask in #python on GNOME IRC for reviews, etc. For the record, you can run a subset of the tests with e. g. TEST_NAMES=test_gi xvfb-run make check or for a single test TEST_NAMES=test_gi.TestGList.test_glist_int_none_in xvfb-run make check (as an example of one test which is currently failing). I fixed the marshalling from/to GList and GSList in trunk: http://git.gnome.org/browse/pygobject/commit/?id=770e6abfd5bc5dad7d5f56a18f1ef63f9754ada9 (In reply to comment #10) > I fixed the marshalling from/to GList and GSList in trunk: > > http://git.gnome.org/browse/pygobject/commit/ > ?id=770e6abfd5bc5dad7d5f56a18f1ef63f9754ada9 I've cherrypicked this patch into our Fedora packages as of 3.3.4-6; however the latest patch for bug 841596 (attachment 603367 [details]) does things in a slightly different way and moves things from that commit, it's in 3.3.4-7 onwards. See bug 841596 for more info. Just to make sure, I also added a specific reproducer for comment #0 into our packages as: http://pkgs.fedoraproject.org/cgit/pygobject3.git/plain/test-list-marshalling.patch?id=58e8a6c275c75dc06d3b2baef623932569c82240m although I believe the test suite already has coverage for this (we just hadn't been running it until 3.3.4-4). (In reply to comment #11) > http://pkgs.fedoraproject.org/cgit/pygobject3.git/plain/test-list- > marshalling.patch?id=58e8a6c275c75dc06d3b2baef623932569c82240m Sorry, that link should have been: http://pkgs.fedoraproject.org/cgit/pygobject3.git/plain/test-list-marshalling.patch?id=58e8a6c275c75dc06d3b2baef623932569c82240 (typoed a trailing "m", giving a 404) Committed to trunk: http://git.gnome.org/browse/pygobject/commit/?id=266d37719b This is already covered by test_gi.TestPythonGObject.test_interface3impl, so we do not really need another test case. This now passes with this fix. Many thanks for this, great work! FYI, I fixed the PEP-8 violations that pep8 1.3 detects; I still had version 1.2 installed on my box. It should now pass cleanly. While I'm at it, let's take a look at the remaining patches in the current fedora git: - disable-pyflakes-and-pep8-in-check.patch Is that to avoid pulling these two into the build dependencies? It generally seems harmless to me. However, I'm fine with not calling them if they are not installed. I committed http://git.gnome.org/browse/pygobject/commit/?id=1e056e4f4a which makes this patch obsolete. - endianness-fixes.patch: That's bug 841596, and went upstream now. - fix-argument-to-array.patch: That's this bug, upstream now. - fix-list-marshalling-on-big-endian-machines.patch: backported patch from trunk, fixed in 3.3.5 - ignore-more-pep8-errors.patch: ignoring E124 is in trunk now (631a9cd05cbc), the E127 ones are fixed - pygobject-3.3.4-known-failures.patch: I don't know why this is necessary. All tests succeed in all environments available to me, including ppc64 now. If you still need this, can you please file a bug with the failure output? - test-list-marshalling.patch: Should be redundant (see previous comment), but if you want something like this, it should be completed to actually verify the result, and then we can get it upstream. Thanks! Do we have a build with the fixes included already for F18? If not, could we please get one so we can include it for the F18 Beta on PowerPC? Thanks! Regards, Phil Fix has been included and been verified. We're keeping bz #841596 open for tracking any further pygobject3 issues/cleanup. Thanks & regards, Phil |