classification
Title: Print information about an unexpectedly pending error before crashing
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: gregory.p.smith, serhiy.storchaka, sfreilich, vstinner
Priority: normal Keywords: patch, patch, patch

Created on 2019-01-10 20:57 by sfreilich, last changed 2019-01-19 11:03 by serhiy.storchaka.

Pull Requests
URL Status Linked Edit
PR 11513 closed python-dev, 2019-01-10 20:59
PR 11513 closed python-dev, 2019-01-10 20:59
PR 11513 closed python-dev, 2019-01-10 20:59
Messages (7)
msg333418 - (view) Author: Samuel Freilich (sfreilich) * Date: 2019-01-10 20:57
_PyObject_FastCallDict and _PyObject_FastCallKeywords assert that there is no pending exception before calling functions that might otherwise clobber the exception state. However, that doesn't produce very clear output for debugging, since the assert failure doesn't say anything about what the pending exception actually was. It would be better to print the pending exception first.
msg333442 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-01-11 07:37
Calling PyErr_Occurred() on every function call adds some overhead. It should be called only in the debug build.

In addition to the traceback, it would be nice to output also some information about the callable.

Note also that PyErr_Print() calls sys.excepthook and exits the interpreter if SystemExit is raised. It is not a proper function for using here.
msg333450 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-11 10:38
I added _Py_CheckFunctionResult() to every C calls. This function calls PyErr_Occurred() after a function call. This change has been made in Python 3.5: bpo-23571. I made this change because it was very difficult to identify bugs when an exception was raised but the bug was only spotted "later". Some exceptions were ignored by mistakes.

I also added a *lot* of assertions like the following one, but only when Python is built in debug mode:

#ifdef Py_DEBUG
    /* type_call() must not be called with an exception set,
       because it may clear it (directly or indirectly) and so the
       caller loses its exception */
    assert(!PyErr_Occurred());
#endif

commit 4a7cc8847276df27c8f52987cda619ca279687c2
Author: Victor Stinner <victor.stinner@gmail.com>
Date:   Fri Mar 6 23:35:27 2015 +0100

    Issue #23571: PyObject_Call(), PyCFunction_Call() and call_function() now
    raise a SystemError if a function returns a result and raises an exception.
    The SystemError is chained to the previous exception.
    
    Refactor also PyObject_Call() and PyCFunction_Call() to make them more readable.
    
    Remove some checks which became useless (duplicate checks).
    
    Change reviewed by Serhiy Storchaka.

commit efde146b0c42f2643f96d00896c99a90d501fb69
Author: Victor Stinner <victor.stinner@gmail.com>
Date:   Sat Mar 21 15:04:43 2015 +0100

    Issue #23571: _Py_CheckFunctionResult() now gives the name of the function
    which returned an invalid result (result+error or no result without error) in
    the exception message.
    
    Add also unit test to check that the exception contains the name of the
    function.
    
    Special case: the final _PyEval_EvalFrameEx() check doesn't mention the
    function since it didn't execute a single function but a whole frame.
msg333451 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-11 10:42
I'm a supporter to add *more* checks to the debug build but also provide a Python runtime compiled in debug mode which would be ABI compatible (no need to rebuild C extensions).

See my large project:

   https://pythoncapi.readthedocs.io/

Especially:

   https://pythoncapi.readthedocs.io/runtimes.html#debug-build

It would avoid any overhead at runtime for the regular runtime (compiled in release mode).

Only we would have such "debug runtime" available, I even plan to remove runtime checks from the release build :-)

https://pythoncapi.readthedocs.io/optimization_ideas.html#remove-debug-checks
msg333687 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-01-15 12:04
I close the issue, see:

https://github.com/python/cpython/pull/11513#issuecomment-454369637
msg334025 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2019-01-19 00:25
I'm re-opening this.  The original intent of the PR was missed even though the change in the PR as is was not how we want to do this in the CPython VM.

Background: We build our default non-production test interpreter in `#undef NDEBUG` mode at Google which is why sfreilich@ ran into this while working on some code.

What we have today are a bunch of assert(!PyErr_Occurred()); statements around the CPython internals.

A failure (crash) from such asserts provides no information on its own about the unhandled exception itself to help the developer track down the C API use error that led to this state.

It'd be useful if all of these provided more information about the untracked/unhandled exception before crashing.

So instead of

```c
  assert(!PyErr_Occurred());
```

Something equivalent to:

```c
#ifndef NDEBUG
  if (PyErr_Occurred()) {
    PyErr_Print();
    assert(!PyErr_Occurred());  // crash
  }
#endif
```

were used.  But that is extremely verbose, so we wouldn't want to update all of the existing asserts into that.  Instead that should be encapsulated within something that, as with assert, compiles as a non-existant no-op in the NDEBUG (release) build state.  But otherwise does that.

Given this is C, a conditional macro is reasonable.  (i'd suggest a void function who's body is empty... but that still has consequences and would not be removed in all compilation and linking situations) potentially impacting performance.  A conditional macro similar to this would not:

```c
#ifdef NDEBUG
# define _Assert_No_PyErr_Occurred() ((void)0)
#else  /* Not NDEBUG (assertions enabled). */
# define _Assert_No_PyErr_Occurred()  do { \
        int _unhandled_error = PyErr_Occurred();
        if (_unhandled_error) {
            PyErr_Print();
            assert(!_unhandled_error);  // crash.
        }
    } while(0)
#endif
```

along with replacing all ~38 of our current `assert(!PyErr_Occurred());` statements with a call to that macro.

If, as vstinner noted in the PR comments, rather than just PyErr_Print() for this... using the new `_PyObject_ASSERT()` from Include/cpython/object.h macro giving it the unhandled exception object itself as obj would be good as it would produce even more useful debugging information.

Code doing that gets complicated but probably looks something like (untested):

```c
#ifdef NDEBUG
# define _Assert_No_PyErr_Occurred() ((void)0)
#else  /* Not NDEBUG (assertions enabled). */
# define _Assert_No_PyErr_Occurred()  do {  \
        int _unhandled_exception = PyErr_Occurred();  \
        if (_unhandled_exception) {  ]
            PyObject *sys_module, *exception_object;  \
            PyErr_Print();  /* Clears PyErr_Occurred() and sets sys.last_value. */  \
            sys_module = PyImport_ImportModule("sys");  \
            assert(sys_module /* within _Assert_No_PyErr_Occurred() */);  \
            exception_object = PyObject_GetAttrString(sys_module, "last_value");  \
            if (exception_object == NULL) {  \
                PyErr_Clear();  /* AttributeError */  \
                /* Given PyErr_Print probably instantiated a value of there was  */  \
                /* only a type, I don't even know if this code path is possible. */  \
                /* Playing it safe and useful in debugging code. */  \
                exception_object = PyObject_GetAttrString(sys_module, "last_type");  \
            }  \
            Py_DECREF(sys_module);  \
            _PyObject_ASSERT(exception_object, _unhandled_exception);  \
            /* Unreachable code, _PyObject_ASSERT should have aborted. */  \
            Py_XDECREF(exception_object);  \
            abort();  \
        }  \
    } while(0)
#endif
```
msg334054 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-01-19 11:03
Not PyErr_Print(). Maybe PyErr_WriteUnraisable().
History
Date User Action Args
2019-01-19 11:03:49serhiy.storchakasetkeywords: patch, patch, patch

messages: + msg334054
2019-01-19 00:25:39gregory.p.smithsetstatus: closed -> open

assignee: gregory.p.smith

keywords: patch, patch, patch
nosy: + gregory.p.smith
messages: + msg334025
resolution: fixed ->
2019-01-15 12:04:55vstinnersetstatus: open -> closed
messages: + msg333687

keywords: patch, patch, patch
resolution: fixed
stage: patch review -> resolved
2019-01-11 10:42:12vstinnersetkeywords: patch, patch, patch

messages: + msg333451
2019-01-11 10:38:07vstinnersetkeywords: patch, patch, patch

messages: + msg333450
2019-01-11 07:37:53serhiy.storchakasettype: enhancement
components: + Interpreter Core
versions: + Python 3.8
keywords: patch, patch, patch
nosy: + vstinner, serhiy.storchaka

messages: + msg333442
2019-01-10 20:59:51python-devsetkeywords: + patch
stage: patch review
pull_requests: + pull_request11073
2019-01-10 20:59:47python-devsetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11072
2019-01-10 20:59:44python-devsetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11071
2019-01-10 20:57:59sfreilichcreate