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

[Eric Snow]

On Wed, Oct 31, 2018 at 2:05 PM Victor Stinner <vstinner at redhat.com> wrote:
>
> 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

Thanks for that.  I don't have much time to review (typing w/left
hand, sleeping babe in right), so it may make sense to get feedback
from others (Nick?) before going too far down this road, just for a
sanity check.

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

As long as we avoid pulling internal API into public header files,
which is exactly the point of your efforts. :)

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

+1

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

+1

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

+1

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

OK, good.

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

Fair enough.  we just need a way to provide the optimized impl without
depending on internal API (e.g. _PyRuntime) in public header files,
right?  There should only be two or three cases where it's an issue
(i.e. involving actual public API).

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

+1

FWIW, I think you're on the right track with all this. :)

-eric

>
> Victor