On Wed, Oct 31, 2018 at 2:05 PM Victor Stinner <vstinner@redhat.com> wrote:
Le mer. 31 oct. 2018 à 20:40, Eric Snow <ericsnowcurrently@gmail.com> a écrit :
On Mon, Oct 29, 2018 at 6:39 AM Victor Stinner <vstinner@redhat.com> wrote:
Le lun. 29 oct. 2018 à 06:32, Benjamin Peterson <benjamin@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/9204fb8623896cc5f68ae350784ee25e8a7...
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