Improve traceability of assertion errors in common functions

[oddbookworm]

Feature request

(By @picnizx)

While the C API docs explicitly state that functions should usually not be called with an exception set, else some assertion errors may occur and/or the thread state may be invalid, this means that assert(!PyErr_Occurred()) in common macros / functions such as _PyType_LookupRef would just crash. However, in a CI scenario, without a debugger, it's extremely hard to pinpoint the actual cause.

Related: #128159 would give the possibility to show the C stack trace with -X faulthandler.

A crude idea: add assert(!PyErr_Occurred()) inside public common C API functions to be sure that the assertion is checked as well, making debugging a bit easier.

---

Toy Scenario (original request):

I was working on freethread support for pygame-ce, and I finally need to use a python build with debug symbols, and our atexit-registered quit function was causing SIGABRT to be thrown. This took me a while to figure out. Here's all the code you need to run (or just run python -c "import pygame" since pygame.quit() is atexit-registered).

import pygame
pygame.quit()

Here's the guilty block of code in our codebase:

    funcobj = PyObject_GetAttrString(module, "_internal_mod_quit");

    /* If we could not load _internal_mod_quit, load quit function */
    if (!funcobj)
        funcobj = PyObject_GetAttrString(module, "quit");

    /* Silence errors */
    if (PyErr_Occurred())
        PyErr_Clear();

In the 3.12 docs for PyObject_GetAttrString and PyObject_GetAttr. there's no mention at all that there shouldn't be any exceptions set before calling this function, and it works on release versions of python, so nobody has ever questioned this structure. I'll be making a pull request on our end to clear any exceptions between the first call and second call if needed (or use PyObject_GetOptionalAttrString in 3.13+).

The reason for this issue is that I'm not sure throwing a SIGABRT is the best option here.

gdb backtrace on a debug python 3.13t

[gdb.txt](https://github.com/user-attachments/files/18922524/gdb.txt)

CPython versions tested on:

3.12, 3.13, 3.14

Operating systems tested on:

Linux

[picnixz]

The docs say:

Retrieve an attribute named attr_name from object o. Returns the attribute value on success, or NULL on failure.

In general, if NULL is returned, then usually an exception is set. But the docs could perhaps be improved.

there's no mention at all that there shouldn't be any exceptions set before calling this function

Normally, functions shouldn't be called with a set exception, except those clearing exceptions or handling exceptions as they could reset another exception afterwards. I don't know if we documented this or not.

[picnixz]

OTOH, if a missing attribute shouldn't be an error, you should use https://docs.python.org/3/c-api/object.html#c.PyObject_GetOptionalAttr instead as it silences the AttributeError

[oddbookworm]

Yeah, but maybe python could be a bit nicer and not throw an uncatchable critical error for this?
I mentioned using GetOptionalAttrString in 3.13+, but that doesn't help us with the 3.12 and below wheels we provide

[oddbookworm]

Especially since it's something that could, in theory, be filtered out immediately

[oddbookworm]

But actually, I'll probably switch our implementation to this

    if (PyObject_HasAttrString(module, "_internal_mod_quit"))
        funcobj = PyObject_GetAttrString(module, "_internal_mod_quit");
    else
    {
        funcobj = PyObject_GetAttrString(module, "quit");
    }

[picnixz]

Well.. i'm assuming that the crash says that an exception is set. However, that's how the C API usually works (this is documented in https://docs.python.org/3/c-api/intro.html#exceptions) [emphasis mine]

As a general principle, a function that calls another function to perform some task should check whether the called function raised an exception, and if so, pass the exception state on to its caller. It should discard any object references that it owns, and return an error indicator, but it should not set another exception — that would overwrite the exception that was just raised, and lose important information about the exact cause of the error.

Stated otherwise, if the first call to PyObject_GetAttrString raised an exception, you should clear it before calling PyObject_GetAttrString again:

funcobj = PyObject_GetAttrString(module, "_internal_mod_quit");

/* If we could not load _internal_mod_quit, load quit function */
if (!funcobj) {
    PyErr_Clear();
    funcobj = PyObject_GetAttrString(module, "quit");
}

[picnixz]

But actually, I'll probably switch our implementation to this

This is also an alternative.