Skip to content

bpo-35081: Move Include/pyatomic.c to Include/internal/ #10239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 30, 2018
Merged

bpo-35081: Move Include/pyatomic.c to Include/internal/ #10239

merged 2 commits into from
Oct 30, 2018

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 30, 2018

@vstinner
Copy link
Member Author

The only backward incompatible change is that #include "pyatomic.h" no longer works: #include "internal/pyatomic.h" must be used instead... But this header is really specific to Python internals, its full content was surrounded by #ifdef Py_BUILD_CORE. Third-party C extensions must not use Py_BUILD_CORE.

In know that in the past, debuggers like vmprof required to access _PyThreadState_Current, but:

  • _PyThreadState_Current is now an alias to _PyRuntime.gilstate.tstate_current: _PyRuntime is designed to more "private", it is only defined in Include/internal/pystate.h.
  • We now provide _PyThreadState_UncheckedGet(() and _PyGILState_GetInterpreterStateUnsafe() which are exported for such debuggers

I'm talking about this macro which requires pyatomic.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))

We don't want to export pyatomic.h because its the header leaks its implementation, and the implementation highly depend on the compiler and compiler flags. This header is included in Python.h but its content wasn't first protected by #ifdef Py_BUILD_CORE which caused many troubles with C++ compilers for example.

@serhiy-storchaka
Copy link
Member

Would not be better to move files with the content fully surrounded by #ifdef Py_BUILD_CORE out of the Include/ directory?

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update Windows build files?

@vstinner
Copy link
Member Author

Would not be better to move files with the content fully surrounded by #ifdef Py_BUILD_CORE out of the Include/ directory?

Right now, I'm not sure if third party projects require Py_BUILD_CORE code or not... I prefer to continue the previous work to move code into Include/internal/.

It seems like Include/internal/ is not installed by python3-devel on Fedora. So Include/internal/ is not usable by 3rd party modules on Fedora at least.

* Add pyatomic.h to the VS project (it wasn't referenced)
* Don't include pyatomic.c in internal/pystate.h
@vstinner vstinner requested a review from a team as a code owner October 30, 2018 13:23
@vstinner
Copy link
Member Author

Update Windows build files?

pyatomic.h wasn't included in the VS project. I fixed that.

@vstinner vstinner merged commit 31368a4 into python:master Oct 30, 2018
@vstinner vstinner deleted the pyatomic branch October 30, 2018 14:14
@vstinner
Copy link
Member Author

Would not be better to move files with the content fully surrounded by #ifdef Py_BUILD_CORE out of the Include/ directory?

I copied your question at https://bugs.python.org/issue35081#msg328922

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants