Created attachment 232366 [details] [review]
Fix out marshalling of C arrays

This is needed to make it possible to implement GApplication::local_command_line vfunc. See https://bugzilla.gnome.org/show_bug.cgi?id=690670 for a patch adding the annotations to glib

Review of attachment 232366 [details] [review]:

I think a corresponding check for GI_ARRAY_TYPE_C in _pygi_argument_release might also be needed to avoid an invalid g_array_free? A regression test for this and related situations would be nice if possible to make sure memory and references are handled correctly.

This patch changes the type of the returned array (correctly, apparently). Although we have plenty of existing regression tests for all kinds of arrays already, we apparently don't have one for a C array in signal handlers (which would be what this code path touches apparently?); so I do agree that we should add a test for this.

Thanks!

Comment on attachment 232366 [details] [review]
Fix out marshalling of C arrays

Setting to "needs work" as this needs the corresponding check in _release(), what Simon said.

Created attachment 233179 [details] [review]
g-i patch

Patch to g-i to create a testcase

Created attachment 233184 [details] [review]
unit test

Since Martin poked me today about this I quickly tried to create a testcase and it looks my patch is not enough to fix things: without my patch this test crashes, with my patch it does not crashes but it still fails... I am either doing something stupid in the test or more work is missing to marshal this array properly.

Help is very appreciated, I do not have much time to look into this and doing it in bits and pieces after dinner does not help :)

Comment on attachment 233179 [details] [review]
g-i patch

Thanks for the test case, that's very helpful! The g-i patch looks good, please commit with a bug reference in the commit log, and maybe with a more textual summary ("gimarshallingtests: Add vfunc with an out array parameter")

Comment on attachment 233179 [details] [review]
g-i patch

pushed to g-i

Keeping debug notes:

With Paolo's patch, the test case delivers [42.41999816894531, 0.0]. If I add a third array element, it comes out as [42.41999816894531, 0.0, 3.140000104904175], so somewhere it mishandles the size of the array elements.

Debugging _pygi_argument_from_object() for processing the returned python array from the vfunc shows that it's building a GArray of GIArguments that are all valid gfloats. Now we hit the place of Paolo's (slightly simplified) patch:

            if (g_type_info_get_array_type (type_info) == GI_ARRAY_TYPE_C)
                arg.v_pointer = g_array_free (array, FALSE);
            else
                arg.v_pointer = array;

In current pygobject, it would assign arg.v_pointer to a GArray of GIArgument, now it turns that into a GIArgument* C array.

Now, I'm not sure what the expected format of a returned array GIArgument is -- should it be an array of GIArgument (which seems to work for at least the GARRAY type), or a plain C array with the data? The test case seems to indicate the latter.

To illustrate, that's the returned value in _pygi_closure_assign_pyobj_to_out_argument():

gdb) p ((GIArgument*) arg.v_pointer)[0]
$46 = {[...], v_float = 42.4199982, [...]}
(gdb) p ((GIArgument*) arg.v_pointer)[1]
$47 = {[...], v_float = 3.1400001, [...]}

While apparently it's expecting this format:

(gdb) p ((gfloat*) arg.v_pointer)[0]
$49 = 42.4199982
(gdb) p ((gfloat*) arg.v_pointer)[1]
$50 = 0
(gdb) p ((gfloat*) arg.v_pointer)[2]
$51 = 3.1400001

Created attachment 233217 [details] [review]
(broken attempt) produce proper C arrays in _pygi_argument_from_object()

I just realized that this approach isn't going to work. If _pygi_argument_from_object() already produces plain C arrays, then we lose all length information for the caller. This would work for pointer arrays which can be NULL terminated, but it wouldn't work for this example of a gfloat[].

So I think we need to keep at least the GArray return here; I'm not decided yet whether or not the items should be GIArguments or direct values, but as for the "GArray" case they are GIArguments, they should probably stay that way for the "C array" case as well, and the distinction needs to happen in _pygi_closure_assign_pyobj_to_out_argument().

Attaching the broken patch for reference.

Comment on attachment 233184 [details] [review]
unit test

This unit test needs some cleanup, but is by and large good. It is included in the next patch attachment which also contains an attempt to fix this.

For cleaning up the patch review queue I'll mark this as superseded.

Closing now that bug 727004 is fixed and local_command_line vfuncs work.

The following fix has been pushed:
d689d24 tests: Add tests for GApplication local command line handling

Created attachment 282857 [details] [review]
tests: Add tests for GApplication local command line handling

Add various tests which override Gio.Appliction.do_command_line and
do_local_command_line.