[Python-Dev] Rename Include/internals/ to Include/pycore/

[Eric Snow]

On Mon, Oct 29, 2018 at 6:39 AM Victor Stinner <vstinner at redhat.com> wrote:
> Le lun. 29 oct. 2018 à 06:32, Benjamin Peterson <benjamin at python.org> a écrit :
> > How does the current Include/internal/ directory fail at accomplishing your goal?
>
> Hum, let me understand how I came into this issue. I tried to convert
> _PyObject_GC_TRACK() macro to a static inline function. The macro uses
> "_PyGC_generation0" which is defined earlier as: "extern PyGC_Head
> *_PyGC_generation0;". Problem: this symbol has been removed when Eric
> Snow moved it into _PyRuntime which contains "#define
> _PyGC_generation0 _PyRuntime.gc.generation0".

(FTR, _PyGC_generation0 is in Include/internal/mem.h.)

>
> Hum, how is possible that _PyObject_GC_TRACK() of objimpl.h works as expected?
>
> It seems like all C file using this macro explicitly uses #include
> "internal/pystate.h" which uses #include "internal/mem.h". Oh.
>
> To me, it seems wrong that a function or macro defined in
> Include/objimpl.h requires an explicit #include "internal/pystate.h".
> objimpl.h should be self-sufficient.

I agree. :)  FWIW, when I consolidated the runtime state I took the
approach of re-using existing symbols (where possible) to avoid churn.
That's why the code does not reference _PyRuntime directly.

So, the issue is that public header implicitly requires that the
source file include the internal headers in order to get the symbol.
That is definitely too convoluted.  However, it seems like a
consequence of the public header file relying on the internal runtime
state, which is what you're trying to fix.

Shouldn't _PyObject_GC_TRACK() be moved to an internal include file?
IIRC, there were several similar cases where API (under #ifndef
Py_LIMITED_API) relies on internal runtime state and should probably
be moved out of the public headers.  Any such cases should be fixed.

If any public API must rely on the internal runtime state then it
shouldn't rely on it directly like it currently does.  Instead that
state should be hidden behind public API that the public headers can
access.  Right?

> [snip]
>
> > > Py_BUILD_CORE is not only used to select which functions you get.
> > > Py_BUILD_CORE is also commonly used to get a macro instead of a
> > > function call, for best performances.
> >
> > I think the macro and function versions should just have different names then.
>
> I don't want to break the backward compatibility. I would like to make
> small steps towards a better API. Concrete example with pystate.h:
>
> /* Variable and macro for in-line access to current thread state */
>
> /* Assuming the current thread holds the GIL, this is the
>    PyThreadState for the current thread. */
> #ifdef Py_BUILD_CORE
> #  define _PyThreadState_Current _PyRuntime.gilstate.tstate_current
> #  define PyThreadState_GET() \
>              ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current))
> #else
> #  define PyThreadState_GET() PyThreadState_Get()
> #endif

This has a similar problem to _PyObject_GC_TRACK(): it is "public" API
that relies on internal runtime state and on the source code including
the internal headers.

> We cannot remove PyThreadState_GET() from the Python C API. Removing
> any function is likely going to break multiple C extensions.

Right.  There should be very few cases of public API that relies on
internal details.  In this case, could we split things up?  For
example:

"Include/pystate.h"

    // Under Py_BUILD_CORE this is replaced.
    #define PyThreadState_GET() PyThreadState_Get()

"Include/internal/pystate.h"

    #undef PyThreadState_GET
    #define _PyThreadState_Current _PyRuntime.gilstate.tstate_current
    #define PyThreadState_GET() \
               ((PyThreadState*)_Py_atomic_load_relaxed(&_PyThreadState_Current))

-eric