Gregory P. Smith <g...@krypto.org> added the comment:
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
```
----------
assignee: -> gregory.p.smith
nosy: +gregory.p.smith
resolution: fixed ->
status: closed -> open
_______________________________________
Python tracker <rep...@bugs.python.org>
<https://bugs.python.org/issue35711>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
