Created attachment 244241 [details] [review]
girepository: fix offset computation with callback fields

Review of attachment 244241 [details] [review]:

Hmm.  Ugh =(  I wish we could go back and do something like synthesize types at the toplevel.  This is really an artifact of the fact that the typelib doesn't nest.

Anyways, this is another patch where it'd be *very* useful for me to know what program actually broke.  Was it that some library added an anonymous callback to a structure and all of a sudden things started crashing because we were looking at invalid field offsets?  If it's that kind of thing, what structure, what change?

Hmm.  Also it's not just that we're making field offset computation linear, we're also making *method* lookups linear in the number of fields since they come after fields.  Eww =/  Not your fault though.

So...just marking this as reviewed, let's keep talking about it.

::: girepository/girnode.c
@@ +817,3 @@
           size += _g_ir_node_get_full_size_internal (node, (GIrNode *)field->callback);
+        else
+          size += - sizeof (SimpleTypeBlob) +

The subtraction here looked really strange/wrong to me until I saw we're doing it elsewhere, in e.g. the FIELD case of _g_ir_node_build_typelib().  So...let's keep this as is I guess.

This broke in my face when I was debugging bug 700347, which adds support for g_object_class_* in gjs.
It would call g_struct_info_find_method (not g_struct_info_get_method, used by g-ir-generate) on GObjectClass, which is a struct with plenty of callbacks.

I don't know if this would bite us should bug 700347 (or a pygobject equivalent) not land, but to me it looks like an incident waiting to happen, especially with structure types. It is less of a problem with GObject types, because usually you just have parent_instance and priv.

*** Bug 706004 has been marked as a duplicate of this bug. ***

I am working on making pygobject use lazy resolution of methods. This involves using g_struct_info_find_method() at some points.

I haven't finished this work yet, as I haven't got the testsuite passing yet. But I'm making progress. One of the issues that was in the way was a crash, didn't happen every time and could be influenced by meaningless print statements. Running under gdb found it right away though, the bug found here was causing a strcmp() of bad addresses.

Please do apply this patch...

Blah.  Okay, I shouldn't have sat on this one so long.  Now we're here at .90.  This patch from last I looked was probably correct, but two things make me nervous:

1) There's no test coverage of this inside g-i itself
2) There can't be test coverage inside the bindings yet since they'd crash

I can take a look at doing #1.

So I just committed a very similar change in bug 725198 (I should have searched for similar bugs first). The commit did not however change the code for g_struct_info_find_method. Out of curiosity what was the problem with updating g_struct_info_find_method anyhow?

FWIW I like Giovanni's approach to sharing _g_base_info_get_field_offset better than what I did. I'd be happy to incorporate that as a refactoring pass if others agree.

In terms of linear time for the computation, we could grab one of a 16 bit reserved values on the blob and start writing out "n_field_callbacks" as the number of fields which are also callbacks. Then the computation becomes:

// n_fields includes n_field_callbacks (unsure if we'd want this)
(n_fields - n_field_callbacks) * sizeof(FieldBlob) + 
n_field_callbacks * (sizeof(FieldBlob) + sizeof(CallbackBlob))

That would at least allow a constant time skip over the fields section for ObjectBlobs when accessing anything after fields. This could work now in a compatible way since callback fields crashed the compiler prior to bug 725198 being fixed anyhow...

Created attachment 270526 [details] [review]
girepository: Use constant time calculation for sections after Object fields

Add "n_field_callbacks" to ObjectBlob which represents the number of object
fields which are also callbacks. This a allows a constant time computation
for accessing sections after fields. Track writing of this field by passing
an extra argument through the girnode writers recursive call structure. This
essentally reverts a portion of commit 7027bb256d0d1ab which added a linear
time computation for accessing sections after fields.
Update typelib validator to also ensure n_field_callbacks is properly set.

Notes:
I verified the PyGObject test suite works well with this: Regress.TestObj is 
heavily relied on and problems here causes everything to break. Still if 
this approach seems ok, it would be a good idea to add more callback fields 
to TestObj which are also interleaved with regular fields to be more 
thorough.

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]

Review of attachment 270526 [details] [review]:

(Sorry I never got around to review this, in more than one year...)

I think this makes sense to me.

Review of attachment 270526 [details] [review]:

::: girepository/giobjectinfo.c
@@ -77,3 @@
-      if (field_blob->has_embedded_type)
-        offset += header->callback_blob_size;
-    }

Can we preserve this function, just make it constant time again?  Would reduce the diff noise.

Actually nevermind, that previous comment doesn't really make sense.

But...we aren't handling the case where there are intermixed fields and callbacks, right?

struct foo {
  guint x;
  void (*foo) (void);
  guint y;
}

?

There's not really a good reason to do this.  I suspect it exists though.  Could possibly query gir files for this.

Review of attachment 270526 [details] [review]:

OK, I reworked this patch to preserve the get_field_offset() for field lookups, but return everything else to constant time.   https://git.gnome.org/browse/gobject-introspection/commit/?id=df21d1f362a810f48a23b7c121bf09ce398539c7

(In reply to Colin Walters from comment #12)
> Actually nevermind, that previous comment doesn't really make sense.
> 
> But...we aren't handling the case where there are intermixed fields and
> callbacks, right?
> 
> struct foo {
>   guint x;
>   void (*foo) (void);
>   guint y;
> }

Indeed this was a bug, nice catch.

At this point the patch may cause a compatibility problem. Note from comment #7:

"This could work now in a compatible way since callback fields crashed the compiler prior to bug 725198 being fixed anyhow..."

That statement is no longer true since over a year has passed. typelib files created between then and now with field callbacks will have n_field_callbacks set to zero. Skipping over the fields section worked due to the linear search for fields with "has_embedded_type". With this removed, the typelibs will not be compatible with new versions of gi which are expecting n_field_callbacks to be correct.

Sorry for not giving a clearer warning in the original patch. Does this need to be backed out or can we bump the typelib version and add backwards compatibility?

(In reply to Simon Feltman from comment #15)
> At this point the patch may cause a compatibility problem. Note from comment
> #7:
> 
> "This could work now in a compatible way since callback fields crashed the
> compiler prior to bug 725198 being fixed anyhow..."
> 
> That statement is no longer true since over a year has passed. typelib files
> created between then and now with field callbacks will have
> n_field_callbacks set to zero. Skipping over the fields section worked due
> to the linear search for fields with "has_embedded_type". With this removed,
> the typelibs will not be compatible with new versions of gi which are
> expecting n_field_callbacks to be correct.

Oh, ugh.  Maybe we can support both typelib formats, and increment the minor version?

We don't have any really good way to test this stuff at the moment =/  Maybe --format-version=4.0 for g-ir-compiler?

Simon, do you want to take this?  I can take a look possibly if not.

(In reply to Colin Walters from comment #17)
> Simon, do you want to take this?  I can take a look possibly if not.

Sure, I can take a look. Is idea with "--format-version" that we would be able to write out the earlier version in our test suite, then verify it loads and offset calculations work correctly?

(In reply to Simon Feltman from comment #18)
> (In reply to Colin Walters from comment #17)
> > Simon, do you want to take this?  I can take a look possibly if not.
> 
> Sure, I can take a look. Is idea with "--format-version" that we would be
> able to write out the earlier version in our test suite, then verify it
> loads and offset calculations work correctly?

Something like that; we don't necessarily have to do this now, it could be tested by hand (old copy of g-ir-compiler).

It looks like g_struct_info_find_method is still broken for class structs due to the problems described here.  For example, it crashes when you try to find "find_style_property" on GtkWidgetClass.  I attach a patch that fixes the problem for me by mimicing what g_struct_info_get_method does (calling g_struct_get_field_offset).  But I don't know if this a complete fix; in particular, I don't have a clear understanding of the backwards compatibility concerns you discussed.

Created attachment 338295 [details] [review]
g_struct_info_find_method: fix offset computation

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gobject-introspection/issues/86.