Replace debug runtime checks in release mode with assertions in debug mode
Hi, I would like to replace runtime checks on C function arguments with assertions. For example, replace: PyObject * PyDict_GetItemWithError(PyObject *op, PyObject *key) { if (!PyDict_Check(op)) { PyErr_BadInternalCall(); return NULL; } ... } with: PyObject * PyDict_GetItemWithError(PyObject *op, PyObject *key) { assert(PyDict_Check(op)); ... } I'm not asking to remove all of them. For example, PyList_GetItem(list, index) will continuing raising an IndexError if the index is out of range. I'm asking to replace runtime checks with assertions when the C API is "obviously" misused: replace PyErr_BadInternalCall(), _Py_CheckFunctionResult() and _Py_CheckSlotResult() with assertions. The exact scope should be defined. -- Python tries to be nice to C extensions authors and calls PyErr_BadInternalCall() to emit a nice SystemError exception when the C API is misused. There are many sanity checks run at runtime. We pay the cost of these runtime checks even if a C extension is well written (has no bug). In Python 3.6, I added the _Py_CheckFunctionResult() function to check the result of any function call. A function must return a non-NULL object and not raises an exception, or return NULL and raises an exception. Otherwise, SystemError is raised. For example, PyObject_Call() uses it to check the PyTypeObject.tp_call result. In Python 3.10, I added a similar _Py_CheckSlotResult() function to check the result of slot functions. For example, PyObject_Size() checks the result of the PyTypeObject.tp_as_sequence.sq_length slot function. -- I modified Python 3.8 to be able to use a debug Python build as a drop-in replacement of a Python release build: debug and release builds are now ABI compatible. It is no longer needed to rebuild C extensions in debug mode. My first goal was to be able to use gdb on a Python binary built with -O0: https://developers.redhat.com/articles/2021/09/08/debugging-python-c-extensi... A debug Python build adds many other debug features useful to develop C extensions: https://docs.python.org/dev/using/configure.html#python-debug-build IMO with Python 3.11, it's now easy enough to get a Python debug build to develop or debug your C extensions. End users should not have to pay the price of these runtime checks. On most Linux distribution, a Python debug build is available as "python3-debug" executable. To check if you are using a debug build, check if the sys.gettotalrefcount() function is available. By the way, it is also possible to build Python in release mode with assertions using "./configure --with-assertions": https://docs.python.org/dev/using/configure.html#cmdoption-with-assertions So, what do you think? -- In 2019, I already proposed this idea on the bug tracker, but I abandoned my idea: https://bugs.python.org/issue37406 Victor -- Night gathers, and now my watch begins. It shall not end until my death.
ISTM this is better discussed on a case-by-case basis than as a blanket policy change. (The latter could end up causing a flood of trivial PRs from wannabe-contributors who found and fix yet another violation of the policy, which is both a nuisance for reviewers and a risk of introducing bugs due to being over-zealous.) On Mon, Feb 7, 2022 at 7:59 AM Victor Stinner <vstinner@python.org> wrote:
-- --Guido van Rossum (python.org/~guido) *Pronouns: he/him **(why is my pronoun here?)* <http://feministing.com/2015/02/03/how-using-they-as-a-singular-pronoun-can-c...>
On Mon, Feb 7, 2022 at 5:38 PM Guido van Rossum <guido@python.org> wrote:
ISTM this is better discussed on a case-by-case basis than as a blanket policy change. (The latter could end up causing a flood of trivial PRs from wannabe-contributors who found and fix yet another violation of the policy, which is both a nuisance for reviewers and a risk of introducing bugs due to being over-zealous.)
That's why I propose to only change code using these 3 functions: * PyErr_BadInternalCall(), * _Py_CheckFunctionResult() * _Py_CheckSlotResult() Victor -- Night gathers, and now my watch begins. It shall not end until my death.
So you're proposing to completely get rid of those three? And you're sure that each and every single call to any of those is better off being an assert()? (I still haven't gotten into the habit of building in debug mode by default, in part because it *isn't* the default when you invoke ./configure or PCbuild/build.bat.) On Mon, Feb 7, 2022 at 8:45 AM Victor Stinner <vstinner@python.org> wrote:
-- --Guido van Rossum (python.org/~guido) *Pronouns: he/him **(why is my pronoun here?)* <http://feministing.com/2015/02/03/how-using-they-as-a-singular-pronoun-can-c...>
On Mon, Feb 7, 2022 at 5:48 PM Guido van Rossum <guido@python.org> wrote:
So you're proposing to completely get rid of those three?
I don't propose to remove them, but only call them if Python is built in debug mode. Or remove them from the release build, unless ./configure --with-assertions is used.
And you're sure that each and every single call to any of those is better off being an assert()?
For many years, many C extensions raised an exception *and* returned a result: that's a bug. The strange part is that in some cases, the exceptions is somehow ignored and the program continues running fine. That's why I added _Py_CheckFunctionResult() and _Py_CheckSlotResult() which helped to catch such bugs. But before that, these programs were running fine :-) So it's not fully clear to me there was really a bug or it's just that Python became more pedantic :-) About PyErr_BadInternalCall(): in 10 years, I saw a few SystemError raised by this function, but usually when I hacked on Python. It's really rare to hit such bug.
(I still haven't gotten into the habit of building in debug mode by default, in part because it *isn't* the default when you invoke ./configure or PCbuild/build.bat.)
If you don't develop C extensions, the release mode is faster and enough ;-) Ah. I don't know if CIs like GitHub Actions and Azure Pipelines provide Python debug builds. If if it's not the case, it would be nice to have the choice :-) Victor -- Night gathers, and now my watch begins. It shall not end until my death.
On Mon, Feb 7, 2022 at 8:59 AM Victor Stinner <vstinner@python.org> wrote:
They do not: https://github.com/actions/python-versions/blob/797eb71c41e47d194f563c7ef017... .
What does pyperformance say about --enable-optimizations builds with all of those compiled out vs today? I like the runtime safety checking for purposes of making the lives of Python C API users easier. But without the use of assertion enabled CPython builds being the norm during development (we should try to shift that norm in the community and make it the popular CI system default regardless), I'm concerned this would hurt the developers who need it most. Having some idea of what it actually gains us performance wise would be useful. -gps On Mon, Feb 7, 2022 at 3:30 PM Brett Cannon <brett@python.org> wrote:
Brett Cannon <brett@python.org> writes:
You can get a debug variant using from https://github.com/deadsnakes/action. bye, lele. -- nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia. lele@metapensiero.it | -- Fortunato Depero, 1929.
On 2/7/2022 11:48 AM, Guido van Rossum wrote:
On Windows, I have build.bat in the directory containing the repository and maintenance workspaces. It calls PCbuild\build.bat -d. I usually build with "..\build" rather than the direct call above. Easier to type and does the usual right thing. -- Terry Jan Reedy
On Tue, Feb 8, 2022 at 9:49 AM Serhiy Storchaka <storchaka@gmail.com> wrote:
It's unrelated to what I said, but yeah it sounds like a good idea :-) I already did it manually (local patch) sometimes to help me debugging C extensions bugs, or when I ran a fuzzer on Python. Otherwise, the SystemError exception raised by PyErr_BadInternalCall() can be "hidden silently": a function can catch it and ignores it. _Py_CheckFunctionResult() already uses Py_FatalError() if Python is build with Py_DEBUG macro defined (debug build). Maybe we can do something similar than iobase_finalize(): change the behavior (log an exception) if Py_DEBUG or if dev_mode config is non-zero (python3 -X dev / PYTHONDEVMODE=1). Victor -- Night gathers, and now my watch begins. It shall not end until my death.
ISTM this is better discussed on a case-by-case basis than as a blanket policy change. (The latter could end up causing a flood of trivial PRs from wannabe-contributors who found and fix yet another violation of the policy, which is both a nuisance for reviewers and a risk of introducing bugs due to being over-zealous.) On Mon, Feb 7, 2022 at 7:59 AM Victor Stinner <vstinner@python.org> wrote:
-- --Guido van Rossum (python.org/~guido) *Pronouns: he/him **(why is my pronoun here?)* <http://feministing.com/2015/02/03/how-using-they-as-a-singular-pronoun-can-c...>
On Mon, Feb 7, 2022 at 5:38 PM Guido van Rossum <guido@python.org> wrote:
ISTM this is better discussed on a case-by-case basis than as a blanket policy change. (The latter could end up causing a flood of trivial PRs from wannabe-contributors who found and fix yet another violation of the policy, which is both a nuisance for reviewers and a risk of introducing bugs due to being over-zealous.)
That's why I propose to only change code using these 3 functions: * PyErr_BadInternalCall(), * _Py_CheckFunctionResult() * _Py_CheckSlotResult() Victor -- Night gathers, and now my watch begins. It shall not end until my death.
So you're proposing to completely get rid of those three? And you're sure that each and every single call to any of those is better off being an assert()? (I still haven't gotten into the habit of building in debug mode by default, in part because it *isn't* the default when you invoke ./configure or PCbuild/build.bat.) On Mon, Feb 7, 2022 at 8:45 AM Victor Stinner <vstinner@python.org> wrote:
-- --Guido van Rossum (python.org/~guido) *Pronouns: he/him **(why is my pronoun here?)* <http://feministing.com/2015/02/03/how-using-they-as-a-singular-pronoun-can-c...>
On Mon, Feb 7, 2022 at 5:48 PM Guido van Rossum <guido@python.org> wrote:
So you're proposing to completely get rid of those three?
I don't propose to remove them, but only call them if Python is built in debug mode. Or remove them from the release build, unless ./configure --with-assertions is used.
And you're sure that each and every single call to any of those is better off being an assert()?
For many years, many C extensions raised an exception *and* returned a result: that's a bug. The strange part is that in some cases, the exceptions is somehow ignored and the program continues running fine. That's why I added _Py_CheckFunctionResult() and _Py_CheckSlotResult() which helped to catch such bugs. But before that, these programs were running fine :-) So it's not fully clear to me there was really a bug or it's just that Python became more pedantic :-) About PyErr_BadInternalCall(): in 10 years, I saw a few SystemError raised by this function, but usually when I hacked on Python. It's really rare to hit such bug.
(I still haven't gotten into the habit of building in debug mode by default, in part because it *isn't* the default when you invoke ./configure or PCbuild/build.bat.)
If you don't develop C extensions, the release mode is faster and enough ;-) Ah. I don't know if CIs like GitHub Actions and Azure Pipelines provide Python debug builds. If if it's not the case, it would be nice to have the choice :-) Victor -- Night gathers, and now my watch begins. It shall not end until my death.
On Mon, Feb 7, 2022 at 8:59 AM Victor Stinner <vstinner@python.org> wrote:
They do not: https://github.com/actions/python-versions/blob/797eb71c41e47d194f563c7ef017... .
What does pyperformance say about --enable-optimizations builds with all of those compiled out vs today? I like the runtime safety checking for purposes of making the lives of Python C API users easier. But without the use of assertion enabled CPython builds being the norm during development (we should try to shift that norm in the community and make it the popular CI system default regardless), I'm concerned this would hurt the developers who need it most. Having some idea of what it actually gains us performance wise would be useful. -gps On Mon, Feb 7, 2022 at 3:30 PM Brett Cannon <brett@python.org> wrote:
Brett Cannon <brett@python.org> writes:
You can get a debug variant using from https://github.com/deadsnakes/action. bye, lele. -- nickname: Lele Gaifax | Quando vivrò di quello che ho pensato ieri real: Emanuele Gaifas | comincerò ad aver paura di chi mi copia. lele@metapensiero.it | -- Fortunato Depero, 1929.
On 2/7/2022 11:48 AM, Guido van Rossum wrote:
On Windows, I have build.bat in the directory containing the repository and maintenance workspaces. It calls PCbuild\build.bat -d. I usually build with "..\build" rather than the direct call above. Easier to type and does the usual right thing. -- Terry Jan Reedy
On Tue, Feb 8, 2022 at 9:49 AM Serhiy Storchaka <storchaka@gmail.com> wrote:
It's unrelated to what I said, but yeah it sounds like a good idea :-) I already did it manually (local patch) sometimes to help me debugging C extensions bugs, or when I ran a fuzzer on Python. Otherwise, the SystemError exception raised by PyErr_BadInternalCall() can be "hidden silently": a function can catch it and ignores it. _Py_CheckFunctionResult() already uses Py_FatalError() if Python is build with Py_DEBUG macro defined (debug build). Maybe we can do something similar than iobase_finalize(): change the behavior (log an exception) if Py_DEBUG or if dev_mode config is non-zero (python3 -X dev / PYTHONDEVMODE=1). Victor -- Night gathers, and now my watch begins. It shall not end until my death.
participants (8)
-
Antoine Pitrou -
Brett Cannon -
Gregory P. Smith -
Guido van Rossum -
Lele Gaifax -
Serhiy Storchaka -
Terry Reedy -
Victor Stinner