Assertion in _PyManagedBuffer_FromObject()
Hi, I just stumbled over this assertion in _PyManagedBuffer_FromObject() in the latest Py3.3 branch: """ 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; } """ I'm not saying that this is likely to happen, but I could imagine code that wants to use a different object for the cleanup than itself, possibly for keeping a certain kind of state when it delivers more than one buffer, or for remembering what kind of allocation was used, or ... Given that the buffer will eventually get released by the object pointed to by the view->obj field in the Py_buffer struct, is there a reason why it should be asserted that this is the same as the object that originally provided the buffer? Stefan
Stefan Behnel <stefan_ml@behnel.de> wrote:
I /think/ a different cleanup object would be possible, but memoryview now has the m.obj attribute that let's you see easily which object the view actually references. That attribute would then point to the cleanup handler. Note that the complexity is such that I would have to go through the whole code again to be *sure* that it's possible. So I'd rather see that people just don't use such schemes (unless there is a storm of protest). The assumption is clearly documented in: http://docs.python.org/dev/c-api/buffer.html#Py_buffer http://docs.python.org/dev/c-api/typeobj.html#buffer-object-structures Since the Py_buffer.obj filed was undocumented in 3.2, I think we're within out rights to restrict the field to the exporter. Stefan Krah
Stefan Krah, 02.03.2012 12:53:
Careful. There are tons of code out there that use the buffer interface, and the "obj" field has been the way to handle the buffer release ever since the interface actually worked (somewhere around the release of Py3.0, IIRC). Personally, I never read the documentation above (which was written way after the design and implementation of the buffer interface). I initially looked at the (outdated) PEP, and then switched to reading the code once it started to divert substantially from the PEP. I'm sure there are many users out there who have never seen the second link above, and still some who aren't aware that the "exporting object" in the first link is required to be identical with the one that "__getbuffer__()" was called on. Just think of an object that acts as a façade to different buffers. I'm well aware of the complexity of the implementation. However, even if the assert was (appropriately, as Nick noted) replaced by an exception, it's still not all that unlikely that it breaks user code (assuming that it currently works). The decision to enforce this restriction should not be taken lightly. Stefan
Stefan Behnel <stefan_ml@behnel.de> wrote:
The documentation has been largely re-written for 3.3.
That's exactly what the ndarray test object from Modules/_testbuffer.c can do. You can push new buffers onto a linked list and present different ones to each consumer. [Note that IMO that's a questionable design, but it's a test object.] The recommended way of keeping track of resources is to use Py_buffer.internal. I think that part is also appropriately mentioned in the original PEP, though I can perfectly understand if someone misses it due to the huge amount of information that needs to be absorbed.
As I said, user code using the (also undocumented) Py_buffer.smalltable will also be broken. Stefan Krah
Stefan Krah <stefan@bytereef.org> wrote:
But even for 3.0 it's not obvious to me why 'obj' should refer to anything but the exporter: http://docs.python.org/release/3.0/c-api/typeobj.html "obj is the object to export ..." Stefan Krah
On Fri, Mar 2, 2012 at 8:19 PM, Stefan Behnel <stefan_ml@behnel.de> wrote:
Supporting that kind of behaviour is what the "internal" field is for. However, given the lack of control, an assert() isn't the appropriate tool here - PyObject_GetBuffer itself should be *checking* the constraint and then reporting an error if the check fails. Otherwise a misbehaving extension module could trivially crash the Python interpreter by returning a bad Py_buffer. Regards, Nick -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Nick Coghlan <ncoghlan@gmail.com> wrote:
I'm not so sure. Extension modules that use the C-API in wrong or undocumented ways can always crash the interpreter. This assert() should be triggered in the first unit test of the module. Now, if the module does not have unit tests or they don't test against a new Python version is that really our problem? Modules do need to be recompiled anyway due to the removal of Py_buffer.smalltable, otherwise they will almost certainly crash. Perhaps an addition to whatsnew/3.3 would be sufficient. Stefan Krah
On Fri, Mar 2, 2012 at 10:55 PM, Stefan Krah <stefan@bytereef.org> wrote:
Crashing out with a C assert when we can easily give them a nice Python traceback instead is unnecessarily unfriendly. As Stefan Behnel pointed out, by tightening up the API semantics, we're already running the risk of breaking applications that relied on looking at what the old code *did*, since it clearly deviated from both spec (the PEP) and the documentation (which didn't explain how ReleaseBuffer works at all).
Modules do need to be recompiled anyway due to the removal of Py_buffer.smalltable, otherwise they will almost certainly crash.
Perhaps an addition to whatsnew/3.3 would be sufficient.
That, updating the 2.7 and 3.2 docs with a reference to the fleshed out 3.3 semantics and converting the assert() to a Python exception should cover it. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Nick Coghlan, 02.03.2012 14:22:
One problem here: if the code raises an exception, it should properly clean up after itself. Meaning that it must call PyBuffer_Release() on the already acquired buffer - thus proving that the code actually works, except that it decides to raise an exception. I keep failing to see the interest in making this an error in the first place. Why would the object that bf_getbuffer() is being called on have to be identical with the one that exports the buffer? Stefan
On Sat, Mar 3, 2012 at 12:39 AM, Stefan Behnel <stefan_ml@behnel.de> wrote:
OK, I misunderstood your suggestion. So you actually want to just remove the assert altogether, thus allowing delegation of the buffer API by defining *only* the getbuffer slot and setting obj to point to a different object? I don't see any obvious problems with that, either. It would need new test cases and some documentation updates, though. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Stefan Behnel <stefan_ml@behnel.de> wrote:
I keep failing to see the interest in making this an error in the first place.
First, it is meant to guard against random pointers in the view.obj field, precisely because view.obj was undocumented and exporters might not fill in the field. Then, as I said, the exporter is exposed on the Python level now:
Why would the object that bf_getbuffer() is being called on have to be identical with the one that exports the buffer?
It doesn't have to be. This is now possible:
Stefan Krah
Stefan Krah <stefan@bytereef.org> wrote:
Stefan (Behnel), do you have an existing example object that does what you described? If I understand correctly, in the above example the ndarray would redirect the buffer request to 'exporter' and set m.obj to 'exporter'. It would be nice to know if people are actually using this. The reason why this scheme was not chosen for a chain of memoryviews was that 'exporter' (in theory) could implement a slideshow of buffers, which means that in the face of redirecting requests m might not be equal to nd. Stefan Krah
Stefan Krah, 02.03.2012 17:42:
Yes, that's a suitable example. It would take the ndarray out of the loop - after all, it has nothing to do with what the memoryview wants, and won't need to do any cleanup for the memoryview's buffer view either. Keeping it explicitly alive in the memoryview is just a waste of resources. It's also related to this issue, which asks for an equivalent at the Python level: http://bugs.python.org/issue13797
It would be nice to know if people are actually using this.
I'm not using this anywhere. My guess is that it would be more of a feature than something to provide legacy code support for, but I can't speak for anyone else. In general, the NumPy mailing list is a good place to ask about these things.
Right. Then it's only safe when the intermediate provider knows what the underlying buffer providers do. Not unlikely in an application setting, though, and it could just be an option at creation time to activate the delegation for the ndarray above. Stefan
On Sat, Mar 3, 2012 at 3:14 AM, Stefan Behnel <stefan_ml@behnel.de> wrote:
OK, my take on the discussion so far: 1. assert() is the wrong tool for this job (it should trigger a Python error message) 2. the current check is too strict (it should just check for obj != NULL, not obj == &exporter) 3. the current check is in the wrong place (it should be in PyObject_GetBuffer) Sound about right? Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Nick Coghlan, 03.03.2012 00:49:
Absolutely.
2. the current check is too strict (it should just check for obj != NULL, not obj == &exporter)
I don't know. The documentation isn't very clear on the cases where obj may be NULL. Definitely on error, ok, but otherwise, the bf_getbuffer() docs do not explicitly say that it must not be NULL (they just mention a "standard" case): http://docs.python.org/dev/c-api/typeobj.html#buffer-object-structures and the Py_buffer docs say explicitly that the field either refers to the exporter or is NULL, without saying if this has any implications or specific meaning: http://docs.python.org/dev/c-api/buffer.html#Py_buffer Personally, I don't see a NULL (or None) value being a problem - it would just mean that the buffer does not need any release call (i.e. no cleanup), e.g. because it was statically allocated in an extension module. PyBuffer_Release() has the appropriate checks in place anyway. But I don't care either way, as long as it's documented. Stefan
Stefan Behnel <stefan_ml@behnel.de> wrote:
1. assert() is the wrong tool for this job
Absolutely.
I disagree. This assert() is meant for extension authors and not end users. I can't see how a reasonable release procedure would fail to trigger the assert(). My procedure as a C extension author is to test against a new Python version and *then* set the PyPI classifier for that version. If I download a C extension that doesn't have the 3.3 classifier set, then as a user I would not be upset if the extension throws an assert or, as Thomas Wouters pointed out, continues to work as before if not compiled in debug mode.
How about this: "The value of view.obj is the equivalent of the return value of any C-API function that returns a new reference. The value must be NULL on error or a valid new reference to an exporting object. For a chain or a tree of views, there are two possible schemes: 1) Re-export: Each member of the tree pretends to be the exporting object and sets view.obj to a new reference to itself. 2) Redirect: The buffer request is redirected to the root object of the tree. Here view.obj will be a reference to the root object." I think it's better not to complicate this familiar scheme of owning a reference by allowing view.obj==NULL for the general case. view.obj==NULL was introduced for temporary wrapping of ad-hoc memoryviews via PyBuffer_FillInfo() and now also PyMemoryView_FromMemory(). That's why I explicitly wrote the following in the documentation of PyBuffer_FillInfo(): "If this function is used as part of a getbufferproc, exporter MUST be set to the exporting object. Otherwise, exporter MUST be NULL." Stefan Krah
On Sat, Mar 3, 2012 at 03:08, Stefan Krah <stefan@bytereef.org> wrote:
Do you test against pydebug builds of Python, or otherwise a build that actually enables asserts? Because I suspect most people don't, so they don't trigger the assert. Python is normally (that is, a release build on Windows or a regular, non-pydebug build on the rest) built without asserts. Asserts are disabled by the NDEBUG symbol, which Python passes for regular builds. Even that aside, asserts are for internal invariants, not external ones. You can use asserts in your extension module to check that your own code is passing what you think it should pass, but you shouldn't really use them to check that a library or API you use is, and Python certainly shouldn't be using it to check what code outside of the core is giving it. Aborting (which is what failed asserts do) is just not the right thing to do.
-- Thomas Wouters <thomas@python.org> Hi! I'm a .signature virus! copy me into your .signature file to help me spread!
Thomas Wouters <thomas@python.org> wrote:
Do you test against pydebug builds of Python, or otherwise a build that actually enables asserts?
Yes, I do (and much more than that): http://hg.python.org/features/cdecimal/file/40917e4b51aa/Modules/_decimal/py... http://hg.python.org/features/cdecimal/file/40917e4b51aa/Modules/_decimal/py... It's automated, so it's not a big deal. You get 100% coverage, with and without threads, all machine configurations, pydebug, refleaks, release build and release build with Valgrind. The version on PyPI has had the same tests for a long time (i.e. also before I became involved with core development).
If many C-extension authors don't know the benefits of --with-pydebug and the consensus here is to protect these authors and their users, then of course I agree with the exception approach for a (now hypothetical) API change. I would have some comments about valid uses of explicit aborts in a library that essentially perform the same function as compiling said library with -D_FORTIFY_SOURCE=2 and -ftrapv (i.e. crash when an external program violates a function contract), but I suspect that would be OT now. Stefan Krah
Stefan Krah, 04.03.2012 12:33:
Same for Cython. We continuously test against the debug builds of all CPython branches since 2.4 (the oldest we support), as well as the latest developer branch, using both our own test suite and Python's regression test suite. https://sage.math.washington.edu:8091/hudson/ https://sage.math.washington.edu:8091/hudson/view/python/ BTW, I can warmly recommend Jenkins' matrix builds for this kind of compatibility testing. Here's an example: https://sage.math.washington.edu:8091/hudson/job/cython-devel-tests/ Basically, you write a build script and Jenkins configures it using environment variables that define the specific setup, e.g. Python 2.7 with C backend. It'll then run all combinations in parallel (optionally filtering out nonsense combinations or preferring combinations that should fail the build early) and present the results both as an aggregated view and in separate per-setup views. It also uses file hashes to remember where the dependencies came from, e.g. which build created the CPython installation that was used for testing, so that you can jump right to the build log of the dependency to check for relevant changes that may have triggered a test failure. Oh, and you can just copy such a job config to set up a separate set of test jobs for a developer's branch, for example. A huge help in distributed developer settings, or when you want to get a GSoC student up and running. Stefan
Nick Coghlan <ncoghlan@gmail.com> wrote:
2. the current check is too strict (it should just check for obj != NULL, not obj == &exporter)
Yes. For anyone who is interested, see issue #14181.
3. the current check is in the wrong place (it should be in PyObject_GetBuffer)
Agreed, since it's not memoryview specific. But I don't think we even need to check for obj != NULL. view.obj was undocumented, and since 3.0 Include/object.h contains this: typedef struct bufferinfo { void *buf; PyObject *obj; /* owned reference */ So it would be somewhat audacious to set this field to NULL. But even if existing code uses the view.obj==NULL scheme from PyBuffer_FillInfo() correctly, it will still work in the new implementation. I'd just prefer to forbid this in the documentation, because it's much easier to remember: getbuffer "returns" a new reference or NULL. Stefan Krah
Stefan Behnel <stefan_ml@behnel.de> wrote:
Yes, this should be supported. The "cleanup handler" in the earlier example got me on the wrong track, that's why I kept insisting this wasn't necessary.
NumPy re-exports, this was confirmed in issue #10181. That's actually the main reason why I considered re-exporting rather than redirecting the standard model and built the test suite around it. Stefan Krah
On Fri, Mar 2, 2012 at 05:22, Nick Coghlan <ncoghlan@gmail.com> wrote:
But you should keep in mind that for non-debug builds, asserts are generally off. So the behaviour most people see isn't actually a crash, but silent acceptance. -- Thomas Wouters <thomas@python.org> Hi! I'm a .signature virus! copy me into your .signature file to help me spread!
Stefan Behnel <stefan_ml@behnel.de> wrote:
I /think/ a different cleanup object would be possible, but memoryview now has the m.obj attribute that let's you see easily which object the view actually references. That attribute would then point to the cleanup handler. Note that the complexity is such that I would have to go through the whole code again to be *sure* that it's possible. So I'd rather see that people just don't use such schemes (unless there is a storm of protest). The assumption is clearly documented in: http://docs.python.org/dev/c-api/buffer.html#Py_buffer http://docs.python.org/dev/c-api/typeobj.html#buffer-object-structures Since the Py_buffer.obj filed was undocumented in 3.2, I think we're within out rights to restrict the field to the exporter. Stefan Krah
Stefan Krah, 02.03.2012 12:53:
Careful. There are tons of code out there that use the buffer interface, and the "obj" field has been the way to handle the buffer release ever since the interface actually worked (somewhere around the release of Py3.0, IIRC). Personally, I never read the documentation above (which was written way after the design and implementation of the buffer interface). I initially looked at the (outdated) PEP, and then switched to reading the code once it started to divert substantially from the PEP. I'm sure there are many users out there who have never seen the second link above, and still some who aren't aware that the "exporting object" in the first link is required to be identical with the one that "__getbuffer__()" was called on. Just think of an object that acts as a façade to different buffers. I'm well aware of the complexity of the implementation. However, even if the assert was (appropriately, as Nick noted) replaced by an exception, it's still not all that unlikely that it breaks user code (assuming that it currently works). The decision to enforce this restriction should not be taken lightly. Stefan
Stefan Behnel <stefan_ml@behnel.de> wrote:
The documentation has been largely re-written for 3.3.
That's exactly what the ndarray test object from Modules/_testbuffer.c can do. You can push new buffers onto a linked list and present different ones to each consumer. [Note that IMO that's a questionable design, but it's a test object.] The recommended way of keeping track of resources is to use Py_buffer.internal. I think that part is also appropriately mentioned in the original PEP, though I can perfectly understand if someone misses it due to the huge amount of information that needs to be absorbed.
As I said, user code using the (also undocumented) Py_buffer.smalltable will also be broken. Stefan Krah
Stefan Krah <stefan@bytereef.org> wrote:
But even for 3.0 it's not obvious to me why 'obj' should refer to anything but the exporter: http://docs.python.org/release/3.0/c-api/typeobj.html "obj is the object to export ..." Stefan Krah
On Fri, Mar 2, 2012 at 8:19 PM, Stefan Behnel <stefan_ml@behnel.de> wrote:
Supporting that kind of behaviour is what the "internal" field is for. However, given the lack of control, an assert() isn't the appropriate tool here - PyObject_GetBuffer itself should be *checking* the constraint and then reporting an error if the check fails. Otherwise a misbehaving extension module could trivially crash the Python interpreter by returning a bad Py_buffer. Regards, Nick -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Nick Coghlan <ncoghlan@gmail.com> wrote:
I'm not so sure. Extension modules that use the C-API in wrong or undocumented ways can always crash the interpreter. This assert() should be triggered in the first unit test of the module. Now, if the module does not have unit tests or they don't test against a new Python version is that really our problem? Modules do need to be recompiled anyway due to the removal of Py_buffer.smalltable, otherwise they will almost certainly crash. Perhaps an addition to whatsnew/3.3 would be sufficient. Stefan Krah
On Fri, Mar 2, 2012 at 10:55 PM, Stefan Krah <stefan@bytereef.org> wrote:
Crashing out with a C assert when we can easily give them a nice Python traceback instead is unnecessarily unfriendly. As Stefan Behnel pointed out, by tightening up the API semantics, we're already running the risk of breaking applications that relied on looking at what the old code *did*, since it clearly deviated from both spec (the PEP) and the documentation (which didn't explain how ReleaseBuffer works at all).
Modules do need to be recompiled anyway due to the removal of Py_buffer.smalltable, otherwise they will almost certainly crash.
Perhaps an addition to whatsnew/3.3 would be sufficient.
That, updating the 2.7 and 3.2 docs with a reference to the fleshed out 3.3 semantics and converting the assert() to a Python exception should cover it. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Nick Coghlan, 02.03.2012 14:22:
One problem here: if the code raises an exception, it should properly clean up after itself. Meaning that it must call PyBuffer_Release() on the already acquired buffer - thus proving that the code actually works, except that it decides to raise an exception. I keep failing to see the interest in making this an error in the first place. Why would the object that bf_getbuffer() is being called on have to be identical with the one that exports the buffer? Stefan
On Sat, Mar 3, 2012 at 12:39 AM, Stefan Behnel <stefan_ml@behnel.de> wrote:
OK, I misunderstood your suggestion. So you actually want to just remove the assert altogether, thus allowing delegation of the buffer API by defining *only* the getbuffer slot and setting obj to point to a different object? I don't see any obvious problems with that, either. It would need new test cases and some documentation updates, though. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Stefan Behnel <stefan_ml@behnel.de> wrote:
I keep failing to see the interest in making this an error in the first place.
First, it is meant to guard against random pointers in the view.obj field, precisely because view.obj was undocumented and exporters might not fill in the field. Then, as I said, the exporter is exposed on the Python level now:
Why would the object that bf_getbuffer() is being called on have to be identical with the one that exports the buffer?
It doesn't have to be. This is now possible:
Stefan Krah
Stefan Krah <stefan@bytereef.org> wrote:
Stefan (Behnel), do you have an existing example object that does what you described? If I understand correctly, in the above example the ndarray would redirect the buffer request to 'exporter' and set m.obj to 'exporter'. It would be nice to know if people are actually using this. The reason why this scheme was not chosen for a chain of memoryviews was that 'exporter' (in theory) could implement a slideshow of buffers, which means that in the face of redirecting requests m might not be equal to nd. Stefan Krah
Stefan Krah, 02.03.2012 17:42:
Yes, that's a suitable example. It would take the ndarray out of the loop - after all, it has nothing to do with what the memoryview wants, and won't need to do any cleanup for the memoryview's buffer view either. Keeping it explicitly alive in the memoryview is just a waste of resources. It's also related to this issue, which asks for an equivalent at the Python level: http://bugs.python.org/issue13797
It would be nice to know if people are actually using this.
I'm not using this anywhere. My guess is that it would be more of a feature than something to provide legacy code support for, but I can't speak for anyone else. In general, the NumPy mailing list is a good place to ask about these things.
Right. Then it's only safe when the intermediate provider knows what the underlying buffer providers do. Not unlikely in an application setting, though, and it could just be an option at creation time to activate the delegation for the ndarray above. Stefan
On Sat, Mar 3, 2012 at 3:14 AM, Stefan Behnel <stefan_ml@behnel.de> wrote:
OK, my take on the discussion so far: 1. assert() is the wrong tool for this job (it should trigger a Python error message) 2. the current check is too strict (it should just check for obj != NULL, not obj == &exporter) 3. the current check is in the wrong place (it should be in PyObject_GetBuffer) Sound about right? Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Nick Coghlan, 03.03.2012 00:49:
Absolutely.
2. the current check is too strict (it should just check for obj != NULL, not obj == &exporter)
I don't know. The documentation isn't very clear on the cases where obj may be NULL. Definitely on error, ok, but otherwise, the bf_getbuffer() docs do not explicitly say that it must not be NULL (they just mention a "standard" case): http://docs.python.org/dev/c-api/typeobj.html#buffer-object-structures and the Py_buffer docs say explicitly that the field either refers to the exporter or is NULL, without saying if this has any implications or specific meaning: http://docs.python.org/dev/c-api/buffer.html#Py_buffer Personally, I don't see a NULL (or None) value being a problem - it would just mean that the buffer does not need any release call (i.e. no cleanup), e.g. because it was statically allocated in an extension module. PyBuffer_Release() has the appropriate checks in place anyway. But I don't care either way, as long as it's documented. Stefan
Stefan Behnel <stefan_ml@behnel.de> wrote:
1. assert() is the wrong tool for this job
Absolutely.
I disagree. This assert() is meant for extension authors and not end users. I can't see how a reasonable release procedure would fail to trigger the assert(). My procedure as a C extension author is to test against a new Python version and *then* set the PyPI classifier for that version. If I download a C extension that doesn't have the 3.3 classifier set, then as a user I would not be upset if the extension throws an assert or, as Thomas Wouters pointed out, continues to work as before if not compiled in debug mode.
How about this: "The value of view.obj is the equivalent of the return value of any C-API function that returns a new reference. The value must be NULL on error or a valid new reference to an exporting object. For a chain or a tree of views, there are two possible schemes: 1) Re-export: Each member of the tree pretends to be the exporting object and sets view.obj to a new reference to itself. 2) Redirect: The buffer request is redirected to the root object of the tree. Here view.obj will be a reference to the root object." I think it's better not to complicate this familiar scheme of owning a reference by allowing view.obj==NULL for the general case. view.obj==NULL was introduced for temporary wrapping of ad-hoc memoryviews via PyBuffer_FillInfo() and now also PyMemoryView_FromMemory(). That's why I explicitly wrote the following in the documentation of PyBuffer_FillInfo(): "If this function is used as part of a getbufferproc, exporter MUST be set to the exporting object. Otherwise, exporter MUST be NULL." Stefan Krah
On Sat, Mar 3, 2012 at 03:08, Stefan Krah <stefan@bytereef.org> wrote:
Do you test against pydebug builds of Python, or otherwise a build that actually enables asserts? Because I suspect most people don't, so they don't trigger the assert. Python is normally (that is, a release build on Windows or a regular, non-pydebug build on the rest) built without asserts. Asserts are disabled by the NDEBUG symbol, which Python passes for regular builds. Even that aside, asserts are for internal invariants, not external ones. You can use asserts in your extension module to check that your own code is passing what you think it should pass, but you shouldn't really use them to check that a library or API you use is, and Python certainly shouldn't be using it to check what code outside of the core is giving it. Aborting (which is what failed asserts do) is just not the right thing to do.
-- Thomas Wouters <thomas@python.org> Hi! I'm a .signature virus! copy me into your .signature file to help me spread!
Thomas Wouters <thomas@python.org> wrote:
Do you test against pydebug builds of Python, or otherwise a build that actually enables asserts?
Yes, I do (and much more than that): http://hg.python.org/features/cdecimal/file/40917e4b51aa/Modules/_decimal/py... http://hg.python.org/features/cdecimal/file/40917e4b51aa/Modules/_decimal/py... It's automated, so it's not a big deal. You get 100% coverage, with and without threads, all machine configurations, pydebug, refleaks, release build and release build with Valgrind. The version on PyPI has had the same tests for a long time (i.e. also before I became involved with core development).
If many C-extension authors don't know the benefits of --with-pydebug and the consensus here is to protect these authors and their users, then of course I agree with the exception approach for a (now hypothetical) API change. I would have some comments about valid uses of explicit aborts in a library that essentially perform the same function as compiling said library with -D_FORTIFY_SOURCE=2 and -ftrapv (i.e. crash when an external program violates a function contract), but I suspect that would be OT now. Stefan Krah
Stefan Krah, 04.03.2012 12:33:
Same for Cython. We continuously test against the debug builds of all CPython branches since 2.4 (the oldest we support), as well as the latest developer branch, using both our own test suite and Python's regression test suite. https://sage.math.washington.edu:8091/hudson/ https://sage.math.washington.edu:8091/hudson/view/python/ BTW, I can warmly recommend Jenkins' matrix builds for this kind of compatibility testing. Here's an example: https://sage.math.washington.edu:8091/hudson/job/cython-devel-tests/ Basically, you write a build script and Jenkins configures it using environment variables that define the specific setup, e.g. Python 2.7 with C backend. It'll then run all combinations in parallel (optionally filtering out nonsense combinations or preferring combinations that should fail the build early) and present the results both as an aggregated view and in separate per-setup views. It also uses file hashes to remember where the dependencies came from, e.g. which build created the CPython installation that was used for testing, so that you can jump right to the build log of the dependency to check for relevant changes that may have triggered a test failure. Oh, and you can just copy such a job config to set up a separate set of test jobs for a developer's branch, for example. A huge help in distributed developer settings, or when you want to get a GSoC student up and running. Stefan
Nick Coghlan <ncoghlan@gmail.com> wrote:
2. the current check is too strict (it should just check for obj != NULL, not obj == &exporter)
Yes. For anyone who is interested, see issue #14181.
3. the current check is in the wrong place (it should be in PyObject_GetBuffer)
Agreed, since it's not memoryview specific. But I don't think we even need to check for obj != NULL. view.obj was undocumented, and since 3.0 Include/object.h contains this: typedef struct bufferinfo { void *buf; PyObject *obj; /* owned reference */ So it would be somewhat audacious to set this field to NULL. But even if existing code uses the view.obj==NULL scheme from PyBuffer_FillInfo() correctly, it will still work in the new implementation. I'd just prefer to forbid this in the documentation, because it's much easier to remember: getbuffer "returns" a new reference or NULL. Stefan Krah
Stefan Behnel <stefan_ml@behnel.de> wrote:
Yes, this should be supported. The "cleanup handler" in the earlier example got me on the wrong track, that's why I kept insisting this wasn't necessary.
NumPy re-exports, this was confirmed in issue #10181. That's actually the main reason why I considered re-exporting rather than redirecting the standard model and built the test suite around it. Stefan Krah
On Fri, Mar 2, 2012 at 05:22, Nick Coghlan <ncoghlan@gmail.com> wrote:
But you should keep in mind that for non-debug builds, asserts are generally off. So the behaviour most people see isn't actually a crash, but silent acceptance. -- Thomas Wouters <thomas@python.org> Hi! I'm a .signature virus! copy me into your .signature file to help me spread!
participants (4)
-
Nick Coghlan -
Stefan Behnel -
Stefan Krah -
Thomas Wouters