Bug 842880 - ValueError: row sequence has the incorrect number of elements
Summary: ValueError: row sequence has the incorrect number of elements
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: pygobject3
Version: 17
Hardware: ppc64
OS: Linux
urgent
urgent
Target Milestone: ---
Assignee: Dave Malcolm
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: F18Betappc
TreeView+ depends on / blocked
 
Reported: 2012-07-24 19:58 UTC by Mark Hamzy
Modified: 2012-10-11 16:11 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-10-11 16:11:14 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Patch against pygobject-3.2.2 tarball to fix invalid long coercion in _pygi_marshal_from_py_array (3.42 KB, patch)
2012-07-30 20:30 UTC, Dave Malcolm
no flags Details | Diff

Description Mark Hamzy 2012-07-24 19:58:42 UTC
[root@sharpie ~]# cat << __EOF__ | python -
from gi.repository import Gtk
store = Gtk.ListStore(int, int)
store.append([1, 2])
__EOF__
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 20:41:27 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.

Comment 2 Matthias Clasen 2012-07-27 08:22:36 UTC
Upstream bug: https://bugzilla.gnome.org/show_bug.cgi?id=680693

Comment 3 Martin Pitt 2012-07-30 06:56:14 UTC
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 20:30:45 UTC
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 19:28:04 UTC
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;
         }

Comment 6 Martin Pitt 2012-08-01 20:51:32 UTC
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 21:29:46 UTC
(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 21:30:54 UTC
(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.

Comment 9 Martin Pitt 2012-08-02 14:14:09 UTC
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 04:54:11 UTC
I fixed the marshalling from/to GList and GSList in trunk:

http://git.gnome.org/browse/pygobject/commit/?id=770e6abfd5bc5dad7d5f56a18f1ef63f9754ada9

Comment 11 Dave Malcolm 2012-08-10 00:57:36 UTC
(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).

Comment 12 Dave Malcolm 2012-08-10 00:58:46 UTC
(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)

Comment 13 Martin Pitt 2012-08-20 09:31:38 UTC
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 09:59:02 UTC
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!

Comment 15 Phil Knirsch 2012-10-10 11:17:27 UTC
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

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

Thanks & regards, Phil


Note You need to log in before you can comment on or make changes to this bug.