Problem with Py_buffer struct definition in Builtin.py
Hi, the builtin Py_buffer struct type is currently defined as follows: """ builtin_structs_table = [ ('Py_buffer', 'Py_buffer', [("buf", PyrexTypes.c_void_ptr_type), ("obj", PyrexTypes.py_object_type), ("len", PyrexTypes.c_py_ssize_t_type), ... """ I hadn't noticed this before, but when you actually use it in a "__getbuffer__()" special method, you have to assign the buffer owner (i.e. self) to the .obj field, which is currently defined as "object". Meaning, Cython will do reference counting for it. That's fine for the self reference being assigned to it. However, when "__getbuffer__()" gets called, .obj is not assigned, i.e. it may be NULL, or it may contain garbage, depending on the caller. So, Cython will generate broken code for it when trying to DECREF the original value as part of the assignment. """ * buffer.obj = self # <<<<<<<<<<<<<< */ __Pyx_INCREF(((PyObject *)__pyx_v_self)); __Pyx_GIVEREF(((PyObject *)__pyx_v_self)); __Pyx_GOTREF(__pyx_v_buffer->obj); __Pyx_DECREF(__pyx_v_buffer->obj); // please crash for me __pyx_v_buffer->obj = ((PyObject *)__pyx_v_self); """ I see a couple of ways to fix this: 1) Change the field to PyObject* instead of object (and finally make PyObject a known type in Cython). Problem: may break existing code, although that's currently broken anyway. Also makes it more annoying for users who then have to do the manual cast+INCREF dance. 2) Special case this field and do not do any reference counting for its current value. PyBuffer_Release() will properly do the XDECREF cleanup anyway, and it's unlikely that user code will assign to it more than once. Problem: very unexpected behaviour for users in case they ever *do* multiple assignments. 3) Wrap the user provided "__getbuffer__()" special method in code that always assigns "self" to the .obj field before entering the user code and also clears it on error return. That would mean that it will always contain a valid value, so that ref-counting will work as expected. Problem: this complicates the implementation and may not be obvious for users (who don't read the documentation). It also won't fix any code that uses Py_buffer outside of "__getbuffer__()" or that calls "__releasebuffer__()" directly without going through PyBuffer_Release(). However, this actually makes it easier for users, who can then even skip setting the value in all but some very rare cases and just care about the actual buffer setup in their "__getbuffer__()" code. I'm leaning towards 3). What do you think? I also think that this is worth fixing for 0.16 if possible, because it currently generates crash code from innocent and correct looking user code. Stefan
Stefan Behnel, 02.03.2012 10:45:
the builtin Py_buffer struct type is currently defined as follows:
""" builtin_structs_table = [ ('Py_buffer', 'Py_buffer', [("buf", PyrexTypes.c_void_ptr_type), ("obj", PyrexTypes.py_object_type), ("len", PyrexTypes.c_py_ssize_t_type), ... """
I hadn't noticed this before, but when you actually use it in a "__getbuffer__()" special method, you have to assign the buffer owner (i.e. self) to the .obj field, which is currently defined as "object".
Oh, well, I should really learn to read code before composing a lengthy e-mail... "__getbuffer__()" is already special-cased and sets the value to None. I think I even recall that we discussed this back when Dag implemented support for buffers. The reason I had originally noticed this was this recent change in Py3.3: """ static PyObject * _PyManagedBuffer_FromObject(PyObject *base) { _PyManagedBufferObject *mbuf; mbuf = mbuf_alloc(); if (mbuf == NULL) return NULL; if (PyObject_GetBuffer(base, &mbuf->master, PyBUF_FULL_RO) < 0) { /* mbuf->master.obj must be NULL. */ Py_DECREF(mbuf); return NULL; } /* Assume that master.obj is a new reference to base. */ assert(mbuf->master.obj == base); return (PyObject *)mbuf; } """ Note the assertion (which is unreleased as of now, so it may still be subject to changes). Is there any reason the value should be set to None by Cython's special casing code instead of self? Stefan
On 2 March 2012 10:09, Stefan Behnel <stefan_ml@behnel.de> wrote:
Stefan Behnel, 02.03.2012 10:45:
the builtin Py_buffer struct type is currently defined as follows:
""" builtin_structs_table = [ ('Py_buffer', 'Py_buffer', [("buf", PyrexTypes.c_void_ptr_type), ("obj", PyrexTypes.py_object_type), ("len", PyrexTypes.c_py_ssize_t_type), ... """
I hadn't noticed this before, but when you actually use it in a "__getbuffer__()" special method, you have to assign the buffer owner (i.e. self) to the .obj field, which is currently defined as "object".
Oh, well, I should really learn to read code before composing a lengthy e-mail...
"__getbuffer__()" is already special-cased and sets the value to None. I think I even recall that we discussed this back when Dag implemented support for buffers. The reason I had originally noticed this was this recent change in Py3.3:
""" static PyObject * _PyManagedBuffer_FromObject(PyObject *base) { _PyManagedBufferObject *mbuf;
mbuf = mbuf_alloc(); if (mbuf == NULL) return NULL;
if (PyObject_GetBuffer(base, &mbuf->master, PyBUF_FULL_RO) < 0) { /* mbuf->master.obj must be NULL. */ Py_DECREF(mbuf); return NULL; }
/* Assume that master.obj is a new reference to base. */ assert(mbuf->master.obj == base);
return (PyObject *)mbuf; } """
Note the assertion (which is unreleased as of now, so it may still be subject to changes). Is there any reason the value should be set to None by Cython's special casing code instead of self?
It sets it to None to it can later reset it to NULL. Python 3.3 is the first version to document the 'obj' Py_buffer attribute, and it mentions that the exporter may set it to NULL. The code above is not the PyObject_GetBuffer function, so setting it to NULL should still work. Defaulting it to 'self' instead would work equally well I'd think.
Stefan _______________________________________________ cython-devel mailing list cython-devel@python.org http://mail.python.org/mailman/listinfo/cython-devel
mark florisson, 02.03.2012 17:33:
On 2 March 2012 10:09, Stefan Behnel wrote:
Stefan Behnel, 02.03.2012 10:45:
the builtin Py_buffer struct type is currently defined as follows:
""" builtin_structs_table = [ ('Py_buffer', 'Py_buffer', [("buf", PyrexTypes.c_void_ptr_type), ("obj", PyrexTypes.py_object_type), ("len", PyrexTypes.c_py_ssize_t_type), ... """
I hadn't noticed this before, but when you actually use it in a "__getbuffer__()" special method, you have to assign the buffer owner (i.e. self) to the .obj field, which is currently defined as "object".
Oh, well, I should really learn to read code before composing a lengthy e-mail...
"__getbuffer__()" is already special-cased and sets the value to None. I think I even recall that we discussed this back when Dag implemented support for buffers. The reason I had originally noticed this was this recent change in Py3.3:
""" static PyObject * _PyManagedBuffer_FromObject(PyObject *base) { _PyManagedBufferObject *mbuf;
mbuf = mbuf_alloc(); if (mbuf == NULL) return NULL;
if (PyObject_GetBuffer(base, &mbuf->master, PyBUF_FULL_RO) < 0) { /* mbuf->master.obj must be NULL. */ Py_DECREF(mbuf); return NULL; }
/* Assume that master.obj is a new reference to base. */ assert(mbuf->master.obj == base);
I asked on python-dev and I think this assert will be removed. http://thread.gmane.org/gmane.comp.python.devel/130365 There is now a ticket to allow buffer redirection by "__getbuffer__()" as a general feature. http://bugs.python.org/issue14181
return (PyObject *)mbuf; } """
Note the assertion (which is unreleased as of now, so it may still be subject to changes). Is there any reason the value should be set to None by Cython's special casing code instead of self?
It sets it to None to it can later reset it to NULL. Python 3.3 is the first version to document the 'obj' Py_buffer attribute, and it mentions that the exporter may set it to NULL.
I don't find it very clear on that part, though. The way I read it, NULL is only to be set on error (which Cython does already).
The code above is not the PyObject_GetBuffer function, so setting it to NULL should still work. Defaulting it to 'self' instead would work equally well I'd think.
I'll wait and see what the discussion on python-dev brings up. If the outcome is that NULL is for errors and that it should have a value otherwise, I think 'self' is a better default. Stefan
participants (2)
-
mark florisson -
Stefan Behnel