Author gregory.p.smith
Recipients gregory.p.smith, serhiy.storchaka, sfreilich, vstinner
Date 2019-01-19.00:25:38
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1547857539.11.0.0878038947953.issue35711@roundup.psfhosted.org>
In-reply-to
Content
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
```
History
Date User Action Args
2019-01-19 00:25:40gregory.p.smithsetrecipients: + gregory.p.smith, vstinner, serhiy.storchaka, sfreilich
2019-01-19 00:25:39gregory.p.smithsetmessageid: <1547857539.11.0.0878038947953.issue35711@roundup.psfhosted.org>
2019-01-19 00:25:39gregory.p.smithlinkissue35711 messages
2019-01-19 00:25:38gregory.p.smithcreate