GNOME Bugzilla – Bug 657040
g_struct_info_get_size is incorrect for GValue
Last modified: 2015-02-07 16:55:59 UTC
g_struct_info_get_size called on a non-pointer GValue interface type returns 12 bytes, but GValue is actually 20 bytes: struct _GValue { /*< private >*/ GType g_type; /* public for GTypeValueTable methods */ union { gint v_int; guint v_uint; glong v_long; gulong v_ulong; gint64 v_int64; guint64 v_uint64; gfloat v_float; gdouble v_double; gpointer v_pointer; } data[2]; }; I'm guessing the size calculation is thrown off by the two-element array of unions?
I can't reproduce it (I'm on fedora15 x86_64). g_struct_info_get_size() for GObject.Value returns 24 for me, which is correct on 64b system. I'm on gobject-introspection git master.
<field name="data" writable="1"> <array zero-terminated="0" c:type="gpointer" fixed-size="2"> <type name="gpointer" c:type="gpointer"/> </array> </field> This is the definition of GValue::data field in generated gir. Strangely, scanner understood array part correctly, but failed to parse the union. Now it is clear that on 32b systems the size calculation is really incorrect, because sizeof(gpointer) != sizeof(union GValue::data). One way to fix this could be adding some annotation - annotating data field to (array fixed-size=2)(element-type gint64) is not strictly correct, but should work for most normal systems. Fixing the scanner to generate proper union inside would be better fix, although probably harder to do.
I tried to fix scanner to handle struct members which are fixed-size arrays of anonymous structs/unions, but failed. The main obstacle was that ast.Array requires element type to be of ast.Type, and I did not find any way how to stick anonymous ast.Union node into ast.Type.
Created attachment 194547 [details] [review] Move GValue:data out of GValue definition, make it external named one Workaround for g-ir-scanner inability to handle struct member which is fixed-size array of anonymous union - make the union named one, externally visible. g-ir-scanner handles this construct just fine.
Created attachment 196178 [details] [review] scanner: correctly handle structs with arrays of anon unions This applies mainly to GValue, which is defined as: struct _GValue { /*< private >*/ GType g_type; /* public for GTypeValueTable methods */ union { gint v_int; guint v_uint; glong v_long; gulong v_ulong; gint64 v_int64; guint64 v_uint64; gfloat v_float; gdouble v_double; gpointer v_pointer; } data[2]; }; Previously, the scanner did not understand the array of unions. This resulted in g_struct_info_get_size returning an incorrect size for GValue (at least on 32bit systems). Fix this by making up a separate union declaration for the GIR that can be referenced by the array.
Here's the complete diff of the installed GIR files: # diff -ur ~/Desktop/gir-1.0/ /opt/gnomes/gnome/share/gir-1.0/ diff -ur /home/torsten/Desktop/gir-1.0//GObject-2.0.gir /opt/gnomes/gnome/share/gir-1.0//GObject-2.0.gir --- /home/torsten/Desktop/gir-1.0//GObject-2.0.gir 2011-09-10 16:55:54.300151265 +0200 +++ /opt/gnomes/gnome/share/gir-1.0//GObject-2.0.gir 2011-09-10 17:09:32.120151514 +0200 @@ -6465,8 +6465,8 @@ <type name="GType" c:type="GType"/> </field> <field name="data" writable="1"> - <array zero-terminated="0" c:type="gpointer" fixed-size="2"> - <type name="gpointer" c:type="gpointer"/> + <array zero-terminated="0" fixed-size="2"> + <type name="_Value__data__type" c:type="_GValue__data__type"/> </array> </field> <method name="copy" c:identifier="g_value_copy"> @@ -7447,6 +7447,35 @@ </parameter> </parameters> </callback> + <union name="_Value__data__type" c:type="_GValue__data__type"> + <field name="v_int" writable="1"> + <type name="gint" c:type="gint"/> + </field> + <field name="v_uint" writable="1"> + <type name="guint" c:type="guint"/> + </field> + <field name="v_long" writable="1"> + <type name="glong" c:type="glong"/> + </field> + <field name="v_ulong" writable="1"> + <type name="gulong" c:type="gulong"/> + </field> + <field name="v_int64" writable="1"> + <type name="gint64" c:type="gint64"/> + </field> + <field name="v_uint64" writable="1"> + <type name="guint64" c:type="guint64"/> + </field> + <field name="v_float" writable="1"> + <type name="gfloat" c:type="gfloat"/> + </field> + <field name="v_double" writable="1"> + <type name="gdouble" c:type="gdouble"/> + </field> + <field name="v_pointer" writable="1"> + <type name="gpointer" c:type="gpointer"/> + </field> + </union> <function name="boxed_copy" c:identifier="g_boxed_copy" introspectable="0"> <doc xml:whitespace="preserve">Provide a copy of a boxed structure @src_boxed which is of type @boxed_type.</doc> <return-value> Which is the expected change. And gtk+'s GIR files are not affected by this change.
Review of attachment 196178 [details] [review]: Great work Torsten, just a few nitnicks. Looks good to me, you can commit it with or without the changes I propose, you decide. ::: giscanner/transformer.py @@ +319,1 @@ assert isinstance(symbol, SourceSymbol), symbol Just parent perhaps? @@ +443,3 @@ yield self._create_parameter(child) + def _create_member(self, symbol, parent_symbol=None): Just parent perhaps? @@ +460,3 @@ + if (source_type.base_type.type == CTYPE_UNION and + source_type.base_type.name is None): + # Create a fake union so that the array can reference it as I'd but this whole block in a separate method, self._create_anonymous_union() @@ +465,3 @@ + assert parent_name + fake_ident = parent_symbol.ident + "__" + symbol.ident + "__type" + # Create a fake union so that the array can reference it as I'd say that __union makes more sense here. @@ +698,3 @@ def _parse_fields(self, symbol, compound): for child in symbol.base_type.child_list: + child_node = self._traverse_one(child, None, symbol) self._traverse_one(child, parent_symbol=symbol) is the same thing ::: tests/scanner/Regress-1.0-expected.gir @@ +1174,3 @@ + </field> + </record> + <array zero-terminated="0" fixed-size="2"> Is it possible to place this before the record in mind?
Created attachment 196181 [details] [review] New version of the patch. It's sufficiently different that I'd prefer another review before I commit. Particularly regarding the new FIXME comment.
Review of attachment 196181 [details] [review]: Same as last time, looks good, here are optional comments you can address. ::: giscanner/transformer.py @@ +444,3 @@ + def _synthesize_union_type(self, symbol, parent_symbol): + # Synthesize a named union so that it can be referenced. # This is necessary because we need to include all field names to be able # to properly calculate the size of the struct, see https://bugzilla.gnome.org/show_bug.cgi?id=657040 @@ +454,3 @@ + assert namespace and parent_name + fake_name = parent_name + '__' + symbol.ident + '__union' + if hidden: union = ast.Union('_%s__%s__union' % (parent_name, symbol.ident)) @@ +460,3 @@ + self._parse_fields(symbol.base_type, fake_union) + self._append_new_node(fake_union) + (namespace, parent_name) = matches[-1] target_giname="%s.%s" % (namespace.name, union.name) @@ +478,3 @@ + # If the array contains anonymous unions, like in the GValue + # struct, we need to handle this specially. + if (source_type.base_type.type == CTYPE_UNION and Both branches ends up creating an array with None as the first argument, might make sense to avoid duplicating that. ::: tests/scanner/Regress-1.0-expected.gir @@ +1173,3 @@ + </field> + </record> + <field name="some_union" writable="1"> Pondering about this again, should this be marked as private somehow? Perhaps just by placing a _ in front of the struct name? ::: tests/scanner/regress.h @@ +245,3 @@ +/* This one has an array of anonymous unions, inspired by GValue */ +struct RegressTestStructE RegressValue ?
Alright, committed with slight modifications. > ::: tests/scanner/Regress-1.0-expected.gir > @@ +1173,3 @@ > + </field> > + </record> > + <field name="some_union" writable="1"> > > Pondering about this again, should this be marked as private somehow? > Perhaps just by placing a _ in front of the struct name? I don't think it should be marked private. All the information about the union is publicly and intentionally available in the header, it just doesn't have a name.
Torsten: FYI, this patch broke arm/powerpc builds, and presumably other arhictectures. I reported this as bug 660338, investigating now.
Sorry, that might have come out wrong: I meant to say that the test fails on powerpc/armel. I suppose it's just some harmless padding issue or so. This wasn't meant to be a blame :)
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]