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 672065 - string GValues in flat arrays are always empty
string GValues in flat arrays are always empty
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal normal
: ---
Assigned To: martin.pitt
Python bindings maintainers
Depends on:
Blocks: 671610
Reported: 2012-03-14 14:58 UTC by Martin Pitt
Modified: 2012-03-16 11:50 UTC
See Also:
GNOME target: ---
GNOME version: ---

test case (660 bytes, patch)
2012-03-14 14:58 UTC, Martin Pitt
none Details | Review
Fix Python to C marshalling of GValue arrays (3.97 KB, patch)
2012-03-14 21:59 UTC, Martin Pitt
none Details | Review
Fix Python to C marshalling of GValue arrays (4.51 KB, patch)
2012-03-15 07:27 UTC, Martin Pitt
committed Details | Review

Description Martin Pitt 2012-03-14 14:58:41 UTC
Created attachment 209720 [details] [review]
test case

While working on bug 671610 I noticed that string values in a GValue* array are always empty in the called function. It does work fine when marshalling back from C to Python.

We already have a GIMarshallingTest function for both directions, test cases attached. test_gvalue_flat_array_out() succeeds, test_gvalue_flat_array_in() demonstrates this bug.
Comment 1 Martin Pitt 2012-03-14 16:03:10 UTC
The problem happens in gi/pygi-marshal-from-py.c _pygi_marshal_from_py_array(), here:

                    } else if (!is_boxed || is_gvalue) {
                        memcpy (array_->data + (i * item_size), item.v_pointer, item_size);
                        if (from_py_cleanup)
                            from_py_cleanup (state, item_arg_cache, item.v_pointer, TRUE);

In the test case, the second argument is "42", and after the memcpy the value is correct:

(gdb) p (char*) ((GValue*) array_->data)[1]->data->v_pointer
$6 = 0xc07c50 "42"

but the from_py_cleanup kills it with g_value_unset ((GValue *) data), so after that call we have

(gdb) p (char*) ((GValue*) array_->data)[1]->data->v_pointer
$8 = 0xc07c50 ""

So the bug is that the memcpy() copies v_pointer by reference instead of by value, or we should not actually remove the GValue.
Comment 2 Martin Pitt 2012-03-14 21:13:52 UTC
I have a patch for properly copying the GValues, and that gets me a little further. Now it crashes in:

  • #0 g_type_check_value
    at gtype.c line 4140
  • #1 g_value_unset
    at gvalue.c line 271
  • #2 _pygi_marshal_cleanup_from_py_interface_struct_gvalue
    at pygi-marshal-cleanup.c line 246
  • #3 _pygi_marshal_cleanup_from_py_array
    at pygi-marshal-cleanup.c line 340

This code in _pygi_marshal_cleanup_from_py_array() is broken:

                cleanup_func (state,
                              (array_ != NULL) ? g_array_index (array_, gpointer, i) : g_ptr_array_index (ptr_array_, i),

The array_ is an array of structs (GValue), not an array of struct pointers (GValue*), so it tries to interpret the first couple of bytes of the struct (the GType) as a pointer, which results in data=0x18 in pygi_marshal_cleanup_from_py_interface_struct_gvalue(). I need to check whether this is a special case for GValues, but judging how they are created in _pygi_marshal_from_py_array() I think not. I rather think that all the other cases we had so far were arrays of simple types (int etc.) which do not have a cleanup function, or arrays of actual pointers, which go through the g_ptr_array_index code path. I'll verify this.
Comment 3 Martin Pitt 2012-03-14 21:59:10 UTC
Created attachment 209788 [details] [review]
Fix Python to C marshalling of GValue arrays

This fixes both problems. The two new test cases, as well as all existing ones run fine now. I believe after staring at and debugging this code for some two hours I now understand what it does, so please don't hesitate to ask me questions or challenge assumptions during patch review. I'm still by far not an expert in the intricacies of all that marshalling.

Thanks in advance for a review!
Comment 4 Martin Pitt 2012-03-14 22:09:18 UTC
Note, the real hard-core test for this is in bug 671610 when using Gtk.ListStore.insert_with_valuesv() instead of the per-item set_value() methods in the overrides. With this crash, my preliminary patch for that bug already works a whole lot better, but it still causes a crash. This might or might not be a problem with the patch here, but it's getting too late now, I'll continue this tomorrow.

So the patch is still fine for review, and I believe it's reasonably correct, but I'd hold it back for a while until I get bug 671610 working as well.
Comment 5 Paolo Borelli 2012-03-14 22:17:37 UTC
Review of attachment 209788 [details] [review]:

Purely stylistic review, I need more attention for a real review...

::: gi/pygi-marshal-cleanup.c
@@ -337,3 +337,3 @@
             for(i = 0; i < len; i++) {

fix space in "for (" whoile touching this code

@@ -339,1 +339,5 @@
             for(i = 0; i < len; i++) {
+                gpointer item;
+                if (array_ != NULL)
+                    item = sequence_cache->item_cache->is_pointer 

this looks unreadable enough to warrant a real "if...else"

@@ -340,1 +349,1 @@
                 cleanup_func (state,

now that you shortened it, this can go all on a single line

::: gi/pygi-marshal-from-py.c
@@ +840,3 @@
                         g_assert (item_size == sizeof (item.v_pointer));
                         g_array_insert_val (array_, i, item.v_pointer);
                     } else if (!is_boxed || is_gvalue) {

this nested if looks not very readable: if these two cases share so little we may be better off with

else if (is_gvalue)
else if (!is_boxed)
Comment 6 Martin Pitt 2012-03-15 07:27:46 UTC
Created attachment 209803 [details] [review]
Fix Python to C marshalling of GValue arrays

Thanks Paolo. This updated patch addresses your review comments and also fixes the crash I mentioned before -- it happened when having None entries in the passed array, which is now checked explicitly. This is not explicitly covered by the test case in this attached patch, but well covered in the test_{list,tree}_store test cases (see bug 671610). If you prefer testing this case explicitly here, we need to add a new method to the GIMarshallingTests in gobject-introspection.
Comment 7 Tomeu Vizoso 2012-03-15 13:24:43 UTC
Review of attachment 209803 [details] [review]:

Looks great to me.
Comment 8 Martin Pitt 2012-03-15 13:30:08 UTC
Thanks for the review! Pushed.
Comment 9 Martin Pitt 2012-03-16 11:50:06 UTC
For the record, this uncovered another bug which results in random crashes. Reported as bug 672224