Dag Sverre Seljebotn wrote: > Andrew Straw wrote: > >> Hi, >> >> I tracked down an issue that was giving me segfaults with a poor >> __getbuffer__ implementation and modified the compiler to catch this case. >> > > Background first: > > The situation is that this patch guards against returning erronous data > from a GetBuffer call. According to the PEP (3118), I think it is pretty > clear that data should either be set or an exception raised, so this > patch only helps detect when you create an invalid __getbuffer__ > implementation. > I understand your reasoning, but Cython's __getbuffer__ works outside the scope of PEP 3118. The PEP doesn't specify Cython behavior, and a great aspect of Cython, IMO, is that it makes Python/C bridges easier, not to adhere to the minimum standards specified by a PEP. Thus a small amount of additional error checking seems OK to me. Also, as another significant difference, Cython's implementation is working in Python earlier than 2.6, whereas the Python development process will only bring this out with 2.6. (I'm certainly not complaining -- it's just to point out that the PEP doesn't directly apply.) > (If I am wrong and the PEP says that the buf field can be set to NULL on > return then I'll of course apply the patch!) > > I'd like input from others on this before making up my mind permanently, > but I'm -1 so far. The reason is that this belongs in a unit test that > you would write for your new buffer implementation, without adding > another redundtant if-test to all Cython buffer acquisitions that works > against correct getbuffer implementations. > Yes, that unit test would be good to have, regardless. > With a unit test like that, you avoid runtime penalty everywhere, and > you also get to test strides, shape etc. for which you cannot insert > validation in a generic way in Cython (and so your patch check for > correct "buf" but not correct "strides" or "shape"). > Yes, there is the runtime penalty of checking in C whether a pointer is NULL. Given that using buffers often saves copying large amounts of memory, I think that's an acceptable cost. Furthermore, there is already a runtime check in the code (checking the number of dimesions in the buffer using ndim, line 534 of Cython/Compiler/Buffer.py). According to your reasoning above, that check should be removed, too. I would be against that -- my position is that a couple sanity checks costing a single C comparison each is worth fighting segfaults and other undefined behavior.
> So what I would propose instead (though I'll never get time for it :-) ) > is to have a reusable "buffer tester" library, i.e. use Cython to create > an application which checks that a given buffer can be acquired and > read, and then feed your custom buffer to it. We could thus create a > "standard PEP 3118 test" or similar. > Yes, as mentioned above, I agree this is a good idea, but I think orthogonal to the issue here. > > *However*, thanks a lot for creating the patch and bringing up the > issue! (And I don't make the final call here either, it may go in.) My pleasure. Thank you for writing this stuff in the first place -- it is very exciting. I especially like the possibility that I can basically start using PEP 3118 now without waiting for Python 2.6 to become standard (or for it to be implemented by the Numpy C API, either). Also, don't forget about the tiny issue of the ErrorBuffer implementation in tests/run/bufaccess.pyx addressed in my patch. -Andrew _______________________________________________ Cython-dev mailing list [email protected] http://codespeak.net/mailman/listinfo/cython-dev
