On 8 June 2010 14:57, Chuck Blake <[email protected]> wrote: > Say you want to implement an extension in Cython exporting a Buffer API. > This is a great canonical use-case for cython. So, you write something > (stripped to its bare essentials) like > > cdef class Foo: > def __getreadbuffer__ (Foo f, long s, void **A): pass > def __getwritebuffer__(Foo f, long s, void **A): pass > def __getsegcount__ (Foo f, int *C): > if C: C[0] = 1 > return 1 > def __getbuffer__(Foo f, Py_buffer *b, int flags): pass > > Currently, Cython generates a PyBufferProcs initializer like > > static PyBufferProcs __pyx_tp_as_buffer_Foo = { > #if PY_MAJOR_VERSION < 3 > __pyx_pf_3int_3Foo___getreadbuffer__, /*bf_getreadbuffer*/ > #endif > > with appropriate method definitions like > > static int __pyx_pf_3int_3Foo___getsegcount__(PyObject *__pyx_v_f, int > *__pyx_v_C) { > > That is fine for Python 2.3/2.4 and Python 3.0. > > > However, for Python 2.5 and 2.6, object.h changed the type of the function > arguments for segment counts (and a lot of other things) to Py_ssize_t > which can be 64-bits. > > For the read/write/charbuffer methods this isn't that big a deal if you > have less than 4 giga-segments since the arguments are values and they > should be converted, though you do get a C compiler warning. > > However, for the [get]segcountproc method (the typedef name dropped the > "get" for 2.5 and added it back in for 2.6), it does actually matter to > get the type right. Otherwise, Cython defines a function taking an int* > but calls are made through a function pointer the C compiler thinks is > an Py_ssize_t*. So, assuming the call site really uses the address of > a Py_ssize_t variable, the generated assembly from the assignment through > the int* will only alter 4 of the 8 bytes at the region passed to it. > > On little-endian 64-bit architectures, it still works out all right. > On big-endian 64-bit architectures (e.g. PPC/IBM's POWER), it will > erroneously convert some small number of segments like "1" into some > crazy giant number of giga-segments (which will likely translate into > some kind of segmentation violation downstream). So, it's a rare and > kind of subtle bug. Anyway, one reason to pay attention to certain > C compiler warnings for the generated code. > > So, we really want to instead generate a signature like > > static Py_ssize_t __pyx_pf_7ssize_t_3Foo___getsegcount__(PyObject > *__pyx_v_f, Py_ssize_t *__pyx_v_C) { > > > The following patch does exactly this. Note that the generated code is > correct for all Python 2,3 versions because of Cython generating typedefs > of Py_ssize_t to "int" for backward compatibility. So for those versions > the Py_ssize_t will be effectively int like it is in the current generation > interface, though it will be spelled differently in emitted code. > > We also fix the signatures for other PyBufferProcs entries to be complete. > > The only real issue I had was that it may err too much on the side of > backward compatibility within Cython itself instead of being more > internally consistent. The Z/z choice reverses the format_map convention > of "more capitialization = more indirection" as used by p/P, i/I, s/S. > Capital Z was already taken. > > On the other hand, Z is only used by about 6 signatures that I see > anywhere in all of Cython and format_map itself seems isolated. > So, it is cleaner to go with the more consistent approach and flip > the case of the Z in those 6 sigs and make z=Py_ssize_t and Z=Py_ssize_t*. > > If you want, I'm happy to prepare a patch that does z/Z the way i/I are. > It's pretty trivial text editing, though, to search for Signature.*Z and > make the change in TypeSlots.py. For what it's worth, I've tested this > patch and the code it generates on Py 2.4, 2.5, 2.6, and 3.1 and it > seems fine. 3.0 was harder as my Gentoo isn't currently letting me > install that simultaneously with 3.1, but I don't think there is any > issue. > > Thanks! > > -------------------------------------------------------------------------- > diff -r dd29215de4a0 Cython/Compiler/TypeSlots.py > --- a/Cython/Compiler/TypeSlots.py Mon Jun 07 11:06:45 2010 -0700 > +++ b/Cython/Compiler/TypeSlots.py Mon Jun 07 16:59:49 2010 -0400 > @@ -49,6 +49,7 @@ > 'I': PyrexTypes.c_int_ptr_type, > 'l': PyrexTypes.c_long_type, > 'Z': PyrexTypes.c_py_ssize_t_type, > + 'z': PyrexTypes.c_py_ssize_t_ptr_type, # Z here z above would be > more conventional but invasive > 's': PyrexTypes.c_char_ptr_type, > 'S': PyrexTypes.c_char_ptr_ptr_type, > 'r': PyrexTypes.c_returncode_type, > @@ -506,7 +507,7 @@ > getcharbufferproc = Signature("TiS", 'i') # typedef int > (*getcharbufferproc)(PyObject *, int, const char **); > readbufferproc = Signature("TZP", "Z") # typedef Py_ssize_t > (*readbufferproc)(PyObject *, Py_ssize_t, void **); > writebufferproc = Signature("TZP", "Z") # typedef Py_ssize_t > (*writebufferproc)(PyObject *, Py_ssize_t, void **); > -segcountproc = Signature("TZ", "Z") # typedef Py_ssize_t > (*segcountproc)(PyObject *, Py_ssize_t *); > +segcountproc = Signature("Tz", "Z") # typedef Py_ssize_t > (*segcountproc)(PyObject *, Py_ssize_t *); > charbufferproc = Signature("TZS", "Z") # typedef Py_ssize_t > (*charbufferproc)(PyObject *, Py_ssize_t, char **); > objargproc = Signature("TO", 'r') # typedef int > (*objobjproc)(PyObject *, PyObject *); > # typedef int > (*visitproc)(PyObject *, void *); > @@ -625,10 +626,10 @@ > ) > > PyBufferProcs = ( > - MethodSlot(getreadbufferproc, "bf_getreadbuffer", "__getreadbuffer__", > py3 = False), > - MethodSlot(getwritebufferproc, "bf_getwritebuffer", > "__getwritebuffer__", py3 = False), > - MethodSlot(getsegcountproc, "bf_getsegcount", "__getsegcount__", py3 = > False), > - MethodSlot(getcharbufferproc, "bf_getcharbuffer", "__getcharbuffer__", > py3 = False), > + MethodSlot(readbufferproc, "bf_getreadbuffer", "__getreadbuffer__", py3 > = False), > + MethodSlot(writebufferproc, "bf_getwritebuffer", "__getwritebuffer__", > py3 = False), > + MethodSlot(segcountproc, "bf_getsegcount", "__getsegcount__", py3 = > False), > + MethodSlot(charbufferproc, "bf_getcharbuffer", "__getcharbuffer__", py3 > = False), > > MethodSlot(getbufferproc, "bf_getbuffer", "__getbuffer__", ifdef = > "PY_VERSION_HEX >= 0x02060000"), > MethodSlot(releasebufferproc, "bf_releasebuffer", "__releasebuffer__", > ifdef = "PY_VERSION_HEX >= 0x02060000") > _______________________________________________ > Cython-dev mailing list > [email protected] > http://codespeak.net/mailman/listinfo/cython-dev >
Looks fine, but I think you should follow the i/I rules. Any other opinion? -- Lisandro Dalcin --------------- CIMEC (INTEC/CONICET-UNL) Predio CONICET-Santa Fe Colectora RN 168 Km 472, Paraje El Pozo Tel: +54-342-4511594 (ext 1011) Tel/Fax: +54-342-4511169 _______________________________________________ Cython-dev mailing list [email protected] http://codespeak.net/mailman/listinfo/cython-dev
