Bug 842880

Summary: ValueError: row sequence has the incorrect number of elements
Product: [Fedora] Fedora Reporter: Mark Hamzy <hamzy>
Component: pygobject3Assignee: Dave Malcolm <dmalcolm>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: urgent Docs Contact:
Priority: urgent    
Version: 17CC: dmalcolm, gustavold, icq, johnp, mclasen, mpitt, pknirsch
Target Milestone: ---   
Target Release: ---   
Hardware: ppc64   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-10-11 12:11:14 EDT Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 846990    
Description Flags
Patch against pygobject-3.2.2 tarball to fix invalid long coercion in _pygi_marshal_from_py_array none

Description Mark Hamzy 2012-07-24 15:58:42 EDT
[root@sharpie ~]# cat << __EOF__ | python -
from gi.repository import Gtk
store = Gtk.ListStore(int, int)
store.append([1, 2])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python2.7/site-packages/gi/overrides/Gtk.py", line 979, in append
    return self._do_insert(-1, row)
  File "/usr/lib64/python2.7/site-packages/gi/overrides/Gtk.py", line 970, in _do_insert
    row, columns = self._convert_row(row)
  File "/usr/lib64/python2.7/site-packages/gi/overrides/Gtk.py", line 823, in _convert_row
    raise ValueError('row sequence has the incorrect number of elements')
ValueError: row sequence has the incorrect number of elements
Comment 1 Dave Malcolm 2012-07-26 16:41:27 EDT
I see this too, with:

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:
has a call to:

Putting a breakpoint in gtk_list_store_set_column_types:
  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.
Comment 2 Matthias Clasen 2012-07-27 04:22:36 EDT
Upstream bug: https://bugzilla.gnome.org/show_bug.cgi?id=680693
Comment 3 Martin Pitt 2012-07-30 02:56:14 EDT
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?
Comment 4 Dave Malcolm 2012-07-30 16:30:45 EDT
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.
Comment 5 Gustavo Luiz Duarte 2012-08-01 15:28:04 EDT
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

+            arg_cache->type_tag = g_type_info_get_tag (type_info);
             g_base_info_unref ( (GIBaseInfo *)arg_info);
Comment 6 Martin Pitt 2012-08-01 16:51:32 EDT
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.
Comment 7 Dave Malcolm 2012-08-01 17:29:46 EDT
(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.
Comment 8 Dave Malcolm 2012-08-01 17:30:54 EDT
(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.
is the current latest version.
Comment 9 Martin Pitt 2012-08-02 10:14:09 EDT
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).
Comment 10 Martin Pitt 2012-08-03 00:54:11 EDT
I fixed the marshalling from/to GList and GSList in trunk:

Comment 11 Dave Malcolm 2012-08-09 20:57:36 EDT
(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:
although I believe the test suite already has coverage for this (we just hadn't been running it until 3.3.4-4).
Comment 12 Dave Malcolm 2012-08-09 20:58:46 EDT
(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:
(typoed a trailing "m", giving a 404)
Comment 13 Martin Pitt 2012-08-20 05:31:38 EDT
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!
Comment 14 Martin Pitt 2012-08-20 05:59:02 EDT
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


  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.

Comment 15 Phil Knirsch 2012-10-10 07:17:27 EDT
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?


Regards, Phil
Comment 16 Phil Knirsch 2012-10-11 12:11:14 EDT
Fix has been included and been verified. We're keeping bz #841596 open for tracking any further pygobject3 issues/cleanup.

Thanks & regards, Phil