On Wed, Oct 31, 2018 at 2:05 PM Victor Stinner <vstin...@redhat.com> wrote: > > Le mer. 31 oct. 2018 à 20:40, Eric Snow <ericsnowcurren...@gmail.com> a écrit > : > > On Mon, Oct 29, 2018 at 6:39 AM Victor Stinner <vstin...@redhat.com> wrote: > > > Le lun. 29 oct. 2018 à 06:32, Benjamin Peterson <benja...@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 _______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
