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 652256 - GPtrArray support is missing
GPtrArray support is missing
Status: RESOLVED FIXED
Product: pygobject
Classification: Bindings
Component: introspection
Git master
Other Linux
: Normal normal
: ---
Assigned To: Nobody's working on this now (help wanted and appreciated)
Python bindings maintainers
Depends on:
Blocks:
 
 
Reported: 2011-06-10 05:52 UTC by Alex Eftimie
Modified: 2011-07-05 18:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch handling GPtrArray separately (15.34 KB, patch)
2011-06-10 06:01 UTC, Alex Eftimie
none Details | Review
gobject-introspection marshalling tests for GPtrArrays (8.64 KB, patch)
2011-06-10 06:03 UTC, Alex Eftimie
none Details | Review
pygobject *incomplete* patch against the invoke-rewrite branch (1.95 KB, patch)
2011-06-16 11:15 UTC, Alex Eftimie
none Details | Review

Description Alex Eftimie 2011-06-10 05:52:37 UTC
Currently, pygobject treats all G*Arrays as GArray, which is wrong, since GPtrArray has different API.
Comment 1 Alex Eftimie 2011-06-10 06:01:09 UTC
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).
Comment 2 Alex Eftimie 2011-06-10 06:03:13 UTC
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.
Comment 3 Alex Eftimie 2011-06-10 13:19:22 UTC
Testing the patch with packagekit I have discovered that it crashes when the (element-type ) annotation is missing for the function returning a GPtrArray.
Comment 4 johnp 2011-06-10 15:58:31 UTC
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.
Comment 5 johnp 2011-06-10 17:44:55 UTC
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.
Comment 6 johnp 2011-06-10 17:54:51 UTC
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.
Comment 7 johnp 2011-06-10 17:56:37 UTC
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.
Comment 8 Alex Eftimie 2011-06-16 08:43:43 UTC
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
Comment 9 Alex Eftimie 2011-06-16 08:50:36 UTC
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.
Comment 10 Alex Eftimie 2011-06-16 11:13:53 UTC
(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.
Comment 11 Alex Eftimie 2011-06-16 11:15:29 UTC
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.
Comment 12 johnp 2011-06-17 14:11:58 UTC
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.
Comment 13 johnp 2011-06-17 15:44:22 UTC
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}
Comment 14 Alex Eftimie 2011-06-17 20:12:12 UTC
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.