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