-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
Confusion between asserts and Py_DEBUG #74127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
There is a bit of confusion in the CPython source between Py_DEBUG and (C) asserts. By default Python builds without Py_DEBUG and without asserts (definining NDEBUG to disable them). Turning on Py_DEBUG also enables asserts. However, it *is* possible to turn on asserts *without* turning on Py_DEBUG, and at Google we routinely build CPython that way. (Doing this with the regular configure/make process can be done by setting CFLAGS=-UNDEBUG when running configure.) This happens to highlight two different problems:
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_DEBUG is enabled it would be better to check for that. The attached 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 in Py_DEBUG mode, that's fine too -- but please make it explicit, by making non-Py_DEBUG builds require NDEBUG. |
Ugh, I logged in with the wrong OpenID without noticing; that was supposed to be me ;-P |
I think we should certainly support asserts regardless of whether Py_DEBUG is in force (although Py_DEBUG should imply asserts run too). And I wish you had stuck to just that much ;-) The argument against, e.g., 'assert(!PyErr_Occurred())', seems exceedingly weak. An Where I draw a hard distinction between assertions and Py_DEBUG is along the "expensive?" axis. The more assertions the merrier, but they better be cheap (and So, to me, 'assert(!PyErr_Occurred())' is fine - it's cheap and catches an error at a point where catching it is possible. Finding the true cause for why the error is set may be arbitrarily more expensive, so _that_ code belongs under Py_DEBUG. Except there is no general way to do that, so no such code exists ;-) |
Perhaps it would be better to raise SystemError for errors in user extensions and left assert() only for checking invariants that can't be broken by user code. But checking the condition takes time, assert() is cheaper. Perhaps it would be better to replace some of asserts in non-critical code with runtime checks and PyErr_BadArgument()/PyErr_BadInternalCall(). |
What happens when you don't have the assert depends on whether the new function call raises an exception or not, and keep in mind *this is what most people see anyway*: if the new call does not raise an exception, a SystemError is raised, with the original exception as cause: Traceback (most recent call last):
File "<stdin>", line 5, in func
TypeError: unsupported operand type(s) for +: 'int' and 'NoneType'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
SystemError: PyEval_EvalFrameEx returned a result with an error set If the new call does raise an exception, the original exception is lost (although this may depend on the exact path through the code here; there's quite a few places that deal with this kind of thing.) I don't mind dropping the assert changes from my PR, but I don't really understand why it is better to be *less* helpful when asserts are enabled :) As I said, the actual assert failure does very little to point to the real problem, as the problem is *some* extension module not clearing the error (or not returning an error value), and the assert does not guard against actual problems -- nothing goes "more wrong" when the assert is not there. I would also argue that an extension module is not *internal* to CPython, any more than arguments passed to a builtin function are. |
So there's more than one issue here. First, should asserts be supported in the absence of Py_DEBUG? It seems, so far, everyone agrees they should be. Second, ...? I'm really not following your argument. It _appears_ to be something along the lines that code "shouldn't be" checking for PyErr_Occurred() at all ... because "nothing goes 'more wrong' when the assert is not there". Maybe, maybe not. For example, if a C function _assumes_ no exception is pending at entry, then it could try some speculative code and deliberately PyErr_Clear() if PyErr_Occurred() is true after - and end up erasing all knowledge of that an exception _was_ in fact pending (upon function entry). An assert at the start prevents such an error when asserts are enabled. Violations of preconditions can have bad consequences. But whatever the second argument is, it seems independent of whether asserts should be supported in the absence of Py_DEBUG. For the rest, I just don't think "internal to CPython" versus "external to CPython". That's a matter of how things happen to be packaged today. I do think "written in C" versus "not written in C". That's the level asserts live in. Any C code (internal or external) mucking with the Python C API has to adhere to a mountain of rules, and asserts are a lightweight way to help check for compliance in cases where it's thought to be "too expensive" to do even cheap unconditional checks all the time. Of course asserts are also useful for verifying invariants and postconditions, but I wouldn't want to rule out using them to verify preconditions too. In short, I'd like to see a patch limited to the obvious win: whatever changes are needed to support asserts in the absence of Py_DEBUG. Anything beyond that is "a crusade" ;-) |
Dropped the Py_DEBUG guards from the dubious asserts in the PR. |
This needs some measure of backporting, now that it's just build-time fixes. I'll take a look. |
FYI, buildbot issues should be fixed by PR #930. |
Buildbots are happy, thanks! |
PR #980 adds a configure flag (--with-assertions), defaulting to the old behaviour (no assertions by default, except when --with-pydebug is passed). I would like to backport that to (at least) 3.6 so that we can set up a buildbot with it, to prevent regressions. Opinions on that? |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: