GNOME Bugzilla – Bug 652256
GPtrArray support is missing
Last modified: 2011-07-05 18:01:00 UTC
Currently, pygobject treats all G*Arrays as GArray, which is wrong, since GPtrArray has different API.
Created attachment 189598 [details] [review] proposed patch handling GPtrArray separately proposed patch handling GPtrArray separately. It needs more work, especially checking for memory leaks; It depends on marshalling tests https://github.com/alexef/gobject-introspection (will add it as another patch to gobject-introspection).
Created attachment 189599 [details] [review] gobject-introspection marshalling tests for GPtrArrays gobject-introspection marshalling tests for GPtrArrays, basically the same tests as for GArray, with GPtrArray api.
Testing the patch with packagekit I have discovered that it crashes when the (element-type ) annotation is missing for the function returning a GPtrArray.
It looks good but I would wait until the invoke-rewrite branch gets merged and then re-factor this for pygobject 3.0. There is a lot of interesting memory issues with the current marshalling code and arrays. I will take a look at re-factoring. Thanks.
Question, can an interface take a ptr array? In which case object to array marshalling is not in this patch. I don't think we can marshal to pointer arrays but I don't know what the API is used for.
Looking into this more, garrays are all the same and can use the garray API. GPtrArray struct is a public mask over the _GRealArray struct so there shouldn't be any need for this patch or it can be greatly simplified.
What I think you are running into is the fact that the element size might not be encoded correctly or we have a bug with encoding the element size in the current code. I think this may be fixed in the invoke-rewrite branch as we take element size into account.
Sorry for the long delay, after some printf debugging I found the problem to be here: http://git.gnome.org/browse/pygobject/tree/gi/pygi-argument.c#n1510 for GPtrArrays the _g_array_index macro doesn't return the correct address; what's surprising me is that neither g_ptr_array_index works, instead it returns the correct address - 0x78 (by correct address I mean the valid pointer inside the PackageKit g_ptr_array). #define _g_array_index(a,t,i) \ *(t *)((a)->data + g_array_get_element_size(a) * (i)) I think you're right, the problem is with the element size - but does the invoke branch solve it? For what I see, the same macro is used there as well. http://git.gnome.org/browse/pygobject/tree/gi/pygi-private.h?h=invoke-rewrite#n82
Ah, the answer is no, compiled and ran invoke-rewrite branch, same issues at converting GPtrArray argument to object. The item_size reported is 2.
(In reply to comment #8) > for GPtrArrays the _g_array_index macro doesn't return the correct address; > what's surprising me is that neither g_ptr_array_index works, instead it > returns the correct address - 0x78 (by correct address I mean the valid pointer > inside the PackageKit g_ptr_array). Please ignore this comment, the 0x78 difference was my fault, different objects. g_ptr_array_index works correctly on GPtrArrays and _g_array_index does not. I'm attaching an incomplete patch against the invoke-rewrite branch.
Created attachment 190029 [details] [review] pygobject *incomplete* patch against the invoke-rewrite branch this patch is incomplete and just a proof-of-concept, showing where the indexing should be changed.
I figured out what is going on. Don't have a fix yet but I should today, if not a complete one. The issue after indexing is fixed is that GPtrArray holds pointers to pointers. There is no way to annotate this so we will have to special case PtrArrays where we deref the pointer at the index given. I have no idea why we aren't just sending an array of (PkDetails *) and what the need is for the indirection but that is how it is.
Ok, walters pointed out to me that ptrarrays are gpointer * (so void **) while other arrays are simply gpointer (so void *) which is why we need the dereferencing. Since this is consistent behaviour and I don't have to worry about other arrays being pointers to pointers I special cased PtrArrays to use the PtrArray API. Sorry to run you through all of this to come back to the original solution but I had to understand how big the scope of the problem was. We also fixed array indexing in the process. I committed the patch to the invoke-rewrite branch. I don't think it needs to be backported this second as pygobject3 should come out for GNOME 3.2. I still need to fix in marshalling. Your test now works but crashes on package.get_name() (null deref). To me it looks like an issue inside of PackageKit and not GI though we could be freeing something we shouldn't but everything looks good except that package->priv->package_id_split is NULL. Here is the gdb output: . . . <Package object at 0x9a7050 (PkPackage at 0x9eaa10)> <Package object at 0x990af0 (PkPackage at 0x9e20b0)> Program received signal SIGSEGV, Segmentation fault. 0x00007fffee25b05a in pk_package_get_name (package=0x9e20b0) at pk-package.c:263 263 return package->priv->package_id_split[PK_PACKAGE_ID_NAME]; (gdb) p *package $1 = {parent = {parent = {g_type_instance = {g_class = 0x9e1710}, ref_count = 3, qdata = 0x9df010}, priv = 0x9e20e0}, priv = 0x9e20f0} (gdb) p *package->priv $2 = {info = PK_INFO_ENUM_INSTALLED, package_id = 0x9e28a0 "gnome-power-manager;3.0.0-2.fc15;x86_64;installed:fedora", package_id_split = 0x0, summary = 0x9e28f0 "GNOME power management service", license = 0x0, group = PK_GROUP_ENUM_UNKNOWN, description = 0x0, url = 0x0, size = 0, update_updates = 0x0, update_obsoletes = 0x0, update_vendor_url = 0x0, update_bugzilla_url = 0x0, update_cve_url = 0x0, update_restart = PK_RESTART_ENUM_UNKNOWN, update_text = 0x0, update_changelog = 0x0, update_state = PK_UPDATE_STATE_ENUM_UNKNOWN, update_issued = 0x0, update_updated = 0x0}
Hooray, these are great news, thanks for the fix, can't wait for the 3.2 release to come out! (In reply to comment #13) > Your test now works but crashes on package.get_name() (null deref). To me it > looks like an issue inside of PackageKit and not GI though we could be freeing > something we shouldn't but everything looks good except that > package->priv->package_id_split is NULL. Here is the gdb output: Yes, it's PK's fault, but this should not happen with PackageKit from master branch, since hugsie and I figured it out in http://gitorious.org/packagekit/packagekit/commit/a6a527f8e117d3b8758a30975e87931b8f75d9d5 . I will test it on my computer when I get home.