bpo-29941: Assert fixes #886
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…means making sure helper functions are defiend when NDEBUG is not defined, not just when Py_DEBUG is defined. Also fix a division-by-zero in obmalloc.c: in Py_DEBUG mode, elsize is never 0, so this assert is never triggered.
…Unlike other asserts this is easy to trigger indirectly, and the assertion failure will not point to the actual problem.
|
@Yhg1s, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @serhiy-storchaka and @tim-one to be potential reviewers. |
benjaminp
reviewed
Mar 30, 2017
Objects/obmalloc.c
Outdated
| @@ -1227,7 +1227,7 @@ _PyObject_Alloc(int use_calloc, void *ctx, size_t nelem, size_t elsize) | |||
|
|
|||
| _Py_AllocatedBlocks++; | |||
|
|
|||
| assert(nelem <= PY_SSIZE_T_MAX / elsize); | |||
| assert(elsize == 0 || nelem <= PY_SSIZE_T_MAX / elsize); | |||
| nbytes = nelem * elsize; | |||
There was a problem hiding this comment.
Probably this whole block should go under after line 1242 instead.
There was a problem hiding this comment.
Moved line 1242 up instead.
… build." This reverts commit 7333298.
|
Dropped the change to add Py_DEBUG guards around dubious asserts, as Tim doesn't think the're dubious enough :) |
benjaminp
approved these changes
Mar 31, 2017
Yhg1s
added a commit
that referenced
this pull request
Mar 31, 2017
Yhg1s
added a commit
to Yhg1s/cpython
that referenced
this pull request
Mar 31, 2017
Yhg1s
added a commit
that referenced
this pull request
Mar 31, 2017
Yhg1s
added a commit
to Yhg1s/cpython
that referenced
this pull request
Mar 31, 2017
Make a non-Py_DEBUG, asserts-enabled build of CPython possible. This means making sure helper functions are defined when NDEBUG is not defined, not just when Py_DEBUG is defined. Also fix a division-by-zero in obmalloc.c that went unnoticed because in Py_DEBUG mode, elsize is never zero. (cherry picked from commit a00c3fd and 06bb487)
Yhg1s
added a commit
to Yhg1s/cpython
that referenced
this pull request
Mar 31, 2017
Make a non-Py_DEBUG, asserts-enabled build of CPython possible. This means making sure helper functions are defined when NDEBUG is not defined, not just when Py_DEBUG is defined. Also fix a division-by-zero in obmalloc.c that went unnoticed because in Py_DEBUG mode, elsize is never zero. (cherry picked from commit a00c3fd and 06bb487)
|
I wrote most _PyXXX_CheckConsistency() methods. To be clear: these methods are not written for performances, but to catch bugs earlier. Be careful on slowdown if you enable assertions on production ;-) |
|
We enable them for tests (which we run a lot, all the time, but that's okay), not for deployed code. |
Yhg1s
added a commit
to Yhg1s/cpython
that referenced
this pull request
Apr 1, 2017
Make a non-Py_DEBUG, asserts-enabled build of CPython possible. This means making sure helper functions are defined when NDEBUG is not defined, not just when Py_DEBUG is defined. Also fix a division-by-zero in obmalloc.c that went unnoticed because in Py_DEBUG mode, elsize is never zero. (cherry picked from commit a00c3fd and 06bb487)
Yhg1s
added a commit
that referenced
this pull request
Apr 2, 2017
Make a non-Py_DEBUG, asserts-enabled build of CPython possible. This means making sure helper functions are defined when NDEBUG is not defined, not just when Py_DEBUG is defined. Also fix a division-by-zero in obmalloc.c that went unnoticed because in Py_DEBUG mode, elsize is never zero. (cherry picked from commit a00c3fd and 06bb487)
Yhg1s
added a commit
that referenced
this pull request
Apr 2, 2017
Make a non-Py_DEBUG, asserts-enabled build of CPython possible. This means making sure helper functions are defined when NDEBUG is not defined, not just when Py_DEBUG is defined. Also fix a division-by-zero in obmalloc.c that went unnoticed because in Py_DEBUG mode, elsize is never zero. (cherry picked from commit a00c3fd and 06bb487)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is a bit of confusion in the CPython source between
Py_DEBUGand (C) asserts. By default Python builds withoutPy_DEBUGand without asserts (defininingNDEBUGto disable them). Turning onPy_DEBUGalso enables asserts. However, it is possible to turn on asserts without turning onPy_DEBUG, and at Google we routinely build CPython that way. (Doing this with the regularconfigure/makeprocess can be done by settingCFLAGS=-UNDEBUGwhen runningconfigure.) This happens to highlight two different problems:Code being defined in
Py_DEBUGblocks but used in assertions:_PyDict_CheckConsistency()is defined in dictobject.c in an#ifdef Py_DEBUG, but then used in assert without a check forPy_DEBUG. This is a compile-time error.Assertions checking for things that are outside of CPython's control, like whether an exception is set before calling something that might clobber it. Generally speaking assertions should be for internal invariants; things that should be a specific way, and it's an error in CPython itself when it's not (I think Tim Peters originally expressed this view of C asserts). For example, PyObject_Call() (and various other flavours of it) does
assert(!PyErr_Occurred()), which is easily triggered and the cause of which is not always apparent.The second case is useful, mind you, as it exposes bugs in extension modules, but the way it does it is not very helpful (it displays no traceback), and if the intent is to only do this when
Py_DEBUGis enabled it would be better to check for that. This PR fixes both issues.I think what our codebase does (enable assertions by default, without enabling
Py_DEBUG) is useful, even when applied to CPython, and I would like CPython to keep working that way. However, if it's deemed more appropriate to make assertions only work inPy_DEBUGmode, that's fine too -- but please make it explicit, by making non-Py_DEBUGbuilds requireNDEBUG.