[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Change by Thomas Wouters :


--
stage:  -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Changes by Thomas Wouters :


--
pull_requests: +1829

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Changes by Thomas Wouters :


--
pull_requests: +1821

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[STINNER Victor]

STINNER Victor added the comment:

> since the commit a00c3fd12d421e41b769debd7df717d17b0deed5 *or later* (who 
> knows?)...

Hum, my sentence is unclear: I mean that I am not sure that this
commit is related to issue #30169 crash.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[STINNER Victor]

STINNER Victor added the comment:

Please see the issue #30169 "test_multiprocessing_spawn crashed on AMD64 
Windows8.1 Non-Debug 3.x buildbot", a mysterious and random bug on Windows 
which started to occur since the commit 
a00c3fd12d421e41b769debd7df717d17b0deed5 *or later* (who knows?)...

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Thomas Wouters added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Changes by Thomas Wouters :


--
pull_requests: +1153

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Thomas Wouters added the comment:


New changeset 553275d125478a6563dde7523f4f28c92f1861b4 by T. Wouters in branch 
'3.5':
bpo-29941: Assert fixes (#886) (#956)
https://github.com/python/cpython/commit/553275d125478a6563dde7523f4f28c92f1861b4


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Thomas Wouters added the comment:


New changeset 90e3518225bafaff01469ed48c472ae7db5686f0 by T. Wouters in branch 
'3.6':
bpo-29941: Assert fixes (#886) (#955)
https://github.com/python/cpython/commit/90e3518225bafaff01469ed48c472ae7db5686f0


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Changes by Thomas Wouters :


--
pull_requests: +1138

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Changes by Thomas Wouters :


--
pull_requests: +1137

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Changes by Thomas Wouters :


--
pull_requests: +1119

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Changes by Thomas Wouters :


--
pull_requests: +1118

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Zachary Ware]

Zachary Ware added the comment:

Buildbots are happy, thanks!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Thomas Wouters added the comment:

FYI, buildbot issues should be fixed by PR #930.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Changes by Thomas Wouters :


--
pull_requests: +1115

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Changes by Thomas Wouters :


--
pull_requests: +1114

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Zachary Ware]

Zachary Ware added the comment:

This seems to have seriously broken a few buildbots:

http://buildbot.python.org/all/builders/AMD64%20Debian%20PGO%203.x/builds/539
http://buildbot.python.org/all/builders/AMD64%20FreeBSD%20CURRENT%20Non-Debug%203.x/builds/120
http://buildbot.python.org/all/builders/AMD64%20Windows8.1%20Non-Debug%203.x/builds/578

--
nosy: +zach.ware

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Thomas Wouters added the comment:

This needs some measure of backporting, now that it's just build-time fixes. 
I'll take a look.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Thomas Wouters added the comment:


New changeset a00c3fd12d421e41b769debd7df717d17b0deed5 by T. Wouters in branch 
'master':
bpo-29941: Assert fixes (#886)
https://github.com/python/cpython/commit/a00c3fd12d421e41b769debd7df717d17b0deed5


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Thomas Wouters added the comment:

Dropped the Py_DEBUG guards from the dubious asserts in the PR.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Tim Peters]

Tim Peters added the comment:

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" ;-)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Thomas Wouters added the comment:

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 "", 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 "", line 1, in 
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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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().

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Tim Peters]

Tim Peters added the comment:

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 `assert()` is to catch 
things that are never supposed to happen.  It's an error in the implementation 
if such a thing ever does happen.  But whether that error is in the Pytnon core 
or an external C extension is a distinction that only matters to assigning 
blame - it's "an error" all the same.  It's nothing but good to catch errors 
ASAP.

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 `PyErr_Occurred()` is pretty cheap).  
Py_DEBUG does all sorts of stuff  that's expensive and intrusive - that's for 
heavy duty verification.

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 ;-)

--
nosy: +tim.peters

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
nosy: +haypo, serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

Thomas Wouters added the comment:

Ugh, I logged in with the wrong OpenID without noticing; that was supposed to 
be me ;-P

--
nosy: +twouters

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue29941] Confusion between asserts and Py_DEBUG

[Thomas Wouters]

New submission from Thomas Wouters:

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:

 - Code being defined in Py_DEBUG blocks but used in assertions: 
_PyDict_CheckConsistency() is defined in dictobject.c in an #ifdef Py_DEBUG, 
but then used in assert without a check for Py_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_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.

--
messages: 290784
nosy: Thomas Wouters, gregory.p.smith
priority: normal
pull_requests: 788
severity: normal
status: open
title: Confusion between asserts and Py_DEBUG
type: crash

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com