I can reproduce the bug with Rawhide:
  $ rpm -q python3-gobject pygobject3
  python3-gobject-3.7.4-1.fc19.i686
  pygobject3-3.7.4-1.fc19.i686

As before, on a 32 bits machine, Python 2 works but Python 3 doesn't with the same error message (but the integer overflows to a different value though)

I'm not sure of this, but as help was requested on #python, here is my best guess:

Values are assigned to the enum Python object here:

http://git.gnome.org/browse/pygobject/tree/gi/module.py#n166

That get_value method is implemented here (assuming get_values returns ValueInfos):

http://git.gnome.org/browse/pygobject/tree/gi/pygi-info.c#n1279

So I would check why that PYGLIB_PyLong_FromLong call isn't doing what it should.

Thanks Tomeu, I was actually just wondering how the enum vars were stuck on the the class as the static bindings didn't do it.

I don't have a 32 bit machine to test this on but running the following might give some hints:

Gdk.ModifierType.mro()

for value in Gdk.ModifierType.__info__.get_values():
    if value.get_name_unescaped() == 'release_mask':
        print('%s = 0x%02x' % (value.get_name_unescaped(), value.get_value()))

hex(Gdk.ModifierType.RELEASE_MASK)

hex(GObject.G_MAXLONG)


As Tomeu pointed out, it is most likely a problem in PYGLIB_PyLong_FromLong. The output of loop printing will at least verify the "get_value" method is returning a bad value and it is not caused by wrapping it with the Flags object. The correct answer is that they should both read "0x40000000".

Hah!

I think you're onto something Simon.

Here's what I get on Python 3:

>>> Gdk.ModifierType.mro()
[<class 'gi.repository.Gdk.GdkModifierType'>, <class 'gobject.GFlags'>, <class 'int'>, <class 'object'>]
>>> for value in Gdk.ModifierType.__info__.get_values():
...     if value.get_name_unescaped() == 'release_mask':
...         print('%s = 0x%02x' % (value.get_name_unescaped(), value.get_value()))
... 
release_mask = 0x40000000
>>> hex(Gdk.ModifierType.RELEASE_MASK)
0x37f200000000
>>> hex(GObject.G_MAXLONG)
0x7fffffff

Notice how it's different from what I get on Python 2:
>>> Gdk.ModifierType.mro()
[<class 'gi.repository.Gdk.GdkModifierType'>, <type 'gobject.GFlags'>, <type 'int'>, <type 'object'>]
>>> for value in Gdk.ModifierType.__info__.get_values():
...     if value.get_name_unescaped() == 'release_mask':
...         print('%s = 0x%02x' % (value.get_name_unescaped(), value.get_value()))
... 
release_mask = 0x40000000
>>> hex(Gdk.ModifierType.RELEASE_MASK)
0x40000000
>>> hex(GObject.G_MAXLONG)
0x7fffffff

That first hex value on Python 3 looks very wrong. Also, I get a different value every time I run it. (on Python 3 only, it is stable on Python 2)

By the way, the results from commt 4 were obtained with the latest pygobject:
  $ rpm -q python3-gobject pygobject3
  python3-gobject-3.7.5.1-1.fc19.i686
  pygobject3-3.7.5.1-1.fc19.i686

I built an i386 schroot and can reproduce.

Created attachment 237444 [details]
current diff for debugging (BROKEN!)

This initially seemed very easy, but it seems I opened a can of worms here.

First of all, GFlagsValue.value is a guint, so everything that handles flags right now is currently doing it wrong. It happens to work on 64 bit because long just happens to be 64 bit, and we don't have that large flag values. So I defined two new aliases PYGLIB_PyLong_FromUnsignedLong() and PYGLIB_PyLong_AsUnsignedLong() (for Python 3 for now, I'll worry about Python 2 once this works at all), and fixed pygflags.c and some other places to use unsigned longs.

However, this is still not enough. The crux of the problem is in pyg_flags_val_new(). When this is being called for Gdk.ModifierType.RELEASE_MASK (0x40000000, first value to exceed MAX_INT), stepping through gives:

43	    item =  (&PYGLIB_PyLong_Type)->tp_new((PyTypeObject*)subclass, args, NULL);
(gdb) call PyLong_AsUnsignedLong(item)
$18 = 1073741824
(gdb) call PyLong_AsUnsignedLongMask(item)
$19 = 1073741824
(gdb) call PyErr_Occurred()
$20 = (PyObject *) 0x0

which is the correct value. Things go haywire as soon as gtype is assigned a bit further down:

47	    ((PyGEnum*)item)->gtype = gtype;

Right before this, PyLong_AsUnsignedLong{,Mask}() both returned correct values. But as soon as ->gtype gets assigned, they both fail:

(gdb) n
49	    return item;
(gdb) call PyLong_AsUnsignedLongMask(item)
$31 = 0
(gdb) call PyErr_Occurred()
$32 = (PyObject *) 0x0
(gdb) call PyLong_AsUnsignedLong(item)
$33 = 4294967295
(gdb) call PyErr_Print()
OverflowError: python int too large to convert to C unsigned long

I. e. assigning the ->gtype field somehow breaks integer conversion.

I was thinking that this would require implementing __int__/__long__, so I extended pyg_flags_as_number to point to a new pyg_flags_int() method. Note that this is totally unclean, but as the function is currently not called at all (I ensured this by adding an abort() at the top), that's not the problem either.

So we need to debug what's breaking conversion of a PyGFlags to an int.

For the record, I removed nb_coerce for Python 3 from the patch (http://docs.python.org/3.3/c-api/typeobj.html#number-structs), pyg_flags_int() is still not being called.

Simon, as author of http://git.gnome.org/browse/pygobject/commit/?id=3e3525e93d8 you can perhaps explain this to me:

+ item = (&PYGLIB_PyLong_Type)->tp_new((PyTypeObject*)subclass, args, NULL);

item later gets cast into PyGFlags, although it's not really clear what type subclass is. For example, when being called from pyg_flags_add() it is this weird "stub" type, which looks like PyGFlags_Type:

    stub = PyObject_CallFunction((PyObject *)&PyType_Type, "s(O)O",
                                 typename, (PyObject *)&PyGFlags_Type,
                                 instance_dict);

I don't know what this does.

However, I experimented with various types in pyg_flags_val_new() including hardcoding PyGFlags_Type, and none of that made the bug disappear, so I guess it's not due to mis-casting a different object into a PyGFlags and partially overwriting its data with the gtype.

I know it is confusing, hence bug 682405.

(In reply to comment #9)
> Simon, as author of
> http://git.gnome.org/browse/pygobject/commit/?id=3e3525e93d8 you can perhaps
> explain this to me:
> 
> + item = (&PYGLIB_PyLong_Type)->tp_new((PyTypeObject*)subclass, args, NULL);
>
> item later gets cast into PyGFlags, although it's not really clear what type
> subclass is. For example, when being called from pyg_flags_add() it is this
> weird "stub" type, which looks like PyGFlags_Type:
> 
>     stub = PyObject_CallFunction((PyObject *)&PyType_Type, "s(O)O",
>                                  typename, (PyObject *)&PyGFlags_Type,
>                                  instance_dict);
> 
> I don't know what this does.

Crazy right? it is dynamically generating a subclass, pure Python would look something like this:

stub = type('Name', (GObject.GFlags,), {})

If you look at my commit, I added had actually added a comment to pyg_enum_add stating this (but not pyg_flags_add). I recall it took some time to decode what the that was :)

A side note, there is a copy paste but in my commit, although it should have no affect:
http://git.gnome.org/browse/pygobject/tree/gi/_gobject/pygflags.c?h=pygobject-3-4#n47

Shows a cast of the "item" into a PyGEnum (should be PyGFlags). That probably shouldn't matter so much because they are basically the same thing.

I had a quick read through: http://www.python.org/dev/peps/pep-0253/
Specifically the section "Creating a subtype of a built-in type in C"

Specifically I noticed we do not set the "tp_basicsize" slot of the the PyGFlags or PyGEnum types, this needs to be set to sizeof(PyGFlags) and sizeof(PyGEnum) which might be the problem here.

Another note is we probably don't even need to store the GType per instance of these items because
1) we should be able to store it on the class
2) we probably don't need such strict type checking or could just use python type checking (issubclass, etc)

but that can be saved for another time.

(In reply to comment #9)
> Simon, as author of
> http://git.gnome.org/browse/pygobject/commit/?id=3e3525e93d8 you can perhaps
> explain this to me:
> 
> + item = (&PYGLIB_PyLong_Type)->tp_new((PyTypeObject*)subclass, args, NULL);
> 
> item later gets cast into PyGFlags, although it's not really clear what type
> subclass is.

It should always be a sub-type of PyGFlags_Type. Specifying this explicitly as the argument type or asserting about it would not be a bad thing.


(In reply to comment #10)
> Specifically I noticed we do not set the "tp_basicsize" slot of the the
> PyGFlags or PyGEnum types, this needs to be set to sizeof(PyGFlags) and
> sizeof(PyGEnum) which might be the problem here.

Turns out the tp_basicsize is already setup by PYGLIB_DEFINE_TYPE.

Not sure if any of this is helpful but here are some additional details about size attributes on my machine:

Python 3.3 64 bit:
sizeof(PyLongObject)         int         32	
PyLong_Type.tp_basicsize     Py_ssize_t  24	
PyLong_Type.tp_itemsize      Py_ssize_t  4	
sizeof(PyGFlags)	     int         40	
sizeof(GType)                int         8	
PyGFlags_Type.tp_basicsize   Py_ssize_t  40	
PyGFlags_Type.tp_itemsize    Py_ssize_t  0	


Python 2.7 64 bit:
sizeof(PyIntObject)          int         24	
PyInt_Type.tp_basicsize      Py_ssize_t  24	
PyInt_Type.tp_itemsize       Py_ssize_t  0	
sizeof(PyGFlags)             int         32	
sizeof(GType)                int         8	
PyGFlags_Type.tp_basicsize   Py_ssize_t  32	
PyGFlags_Type.tp_itemsize    Py_ssize_t  0	


PyIntObject and PyLongObject have a different overall size due to PyIntObject using PyObject_HEAD and PyLongObject using PyObject_VAR_HEAD. It would be interesting to see what this looks like for 32 bits but it might not be relevant.

Created attachment 237496 [details] [review]
Fix signedness, overflow checking, and 32 bit overflow of GFlags

I got it working now, tested on 32 and 64 bit architectures with Python 2.7 and 3.3.

The bogus numbers were due to PyLong_FromUnsignedLong() taking the adjacent GType value into account when interpreting a PyLong subclass such as PyGFlags. I don't fully understand why this happens, as I would expect it to have an internal representation how many digits the struct has. My hunch is that it's actually taking the size of the data type into account, according to /usr/include/python3.3m/longintrepr.h:

/* Long integer representation.
   The absolute value of a number is equal to
        SUM(for i=0 through abs(ob_size)-1) ob_digit[i] * 2**(SHIFT*i)

So I added an extra zero padding to the struct, which helps:

 typedef struct {
     PYGLIB_PyLongObject parent;
+    int zero_pad; /* must always be 0 */
     GType gtype;
 } PyGFlags;

I'm not sure whether that's guaranteed to succeed for all possible corner cases, but at least the test cases now cover representation and arithmetic with flags whose values exceed MAX_INT.

Review of attachment 237496 [details] [review]:

Looks good to me. Thanks for tackling this!

This patch appears to break pygobject on big-endian 64-bit architectures (seen on ppc64 box with 3.7.92).

[I think for any python class subclassing GObject.GObject, due to 
PARAM_READABLE, PARAM_WRITABLE and PARAM_READWRITE all erroneously being 0 and thus
(process:39448): GLib-GObject-CRITICAL **: g_object_class_install_property: assertion `pspec->flags & (G_PARAM_READABLE | G_PARAM_WRITABLE)' failed
]

See https://bugzilla.redhat.com/show_bug.cgi?id=924425 for more details (the hard failure goes away if I undo caeeeb7e4282e183eefc3c53b2d53c8c2bb7de89)

Hi Dave,
I just fixed a potentially related stack corruption bug (bug 696892). Please try the latest pygobject-3-8 or master branch to see if it helps.

Closing as the patch mentioned seemed to have fixed the problem:
https://bugzilla.redhat.com/show_bug.cgi?id=924425