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

[Victor Stinner]

Le mer. 31 oct. 2018 à 20:40, Eric Snow <ericsnowcurrently at gmail.com> a écrit :
> 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.)

I pushed a first commit to "cleanup and fix" pystate.h:
https://github.com/python/cpython/commit/9204fb8623896cc5f68ae350784ee25e8a7b2184

* Remove _PyThreadState_Current
* Replace _PyThreadState_Current with _PyRuntime.gilstate.tstate_current

I prefer to be explicit about the reference to _PyRuntime. For
example, in my text editor, I'm able to follow the reference to
_PyRuntime. In a debugger, I'm also able to display
"_PyRuntime.gilstate.tstate_current", whereas gdb fails to display
"_PyThreadState_Current" value since it's a preprocessor only thing.


> > 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.

Well, we can now fix these issues.


> Shouldn't _PyObject_GC_TRACK() be moved to an internal include file?

I'm trying to do that, but I have the #include bug: if I have
Include/objimpl.h and Include/internal/objimpl.h, the compiler is
confused and picked the wrong file... And I cannot use #include
"objimpl.h" to include Include/objdump.h from
Include/internal/objdump.h.

... Again, that's why I would like to rename header files.

I will be even worse when I will add a second directory for the stable
ABI (second part of my plan).


> 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.

Yep. I'm trying to fix issues one by one. I started with the simplest changes.


> 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?

Right. Internal things must be moved to Include/internal/.


> 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.

_PyObject_GC_TRACK() is fine: since it's a private API, we can move it
to Include/internal/ and require an explicit include of the internal
header file.


> > 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))

This API is really the worse. If we do that, it's unclear if we get
the optimized flavor or the PyThreadState_Get() function call... It
depends if "Include/internal/pystate.h" is included or not!?

My idea here is to add a new _PyThreadState_GET() macro which would
only be available in the private API. It would make sure that we pick
the right implementation.

Maybe PyThreadState_GET() can always be an alias to
PyThreadState_Get(). Just make sure that internals use
_PyThreadState_GET().

Victor