Skip to content

3.5: Include/pyatomic.h is incompatible with OpenMP (compilation of the third-party module fails on Python 3.5) #69337

Closed
Closed
3.5: Include/pyatomic.h is incompatible with OpenMP (compilation of the third-party module fails on Python 3.5)#69337
Labels
3.7 (EOL)end of life3.8 (EOL)end of lifebuildThe build process and cross-buildextension-modulesC modules in the Modules dir

Description

@axh
axh
mannequin
opened on Sep 17, 2015
BPO 25150
Nosy @arekm, @pitrou, @scoder, @vstinner, @benjaminp
Files
  • error.txt: error message
  • pyatomic.patch
  • pyatomic-2.patch
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2020-06-05.20:42:45.704>
created_at = <Date 2015-09-17.12:58:59.262>
labels = ['extension-modules', '3.8', 'build', '3.7']
title = '3.5: Include/pyatomic.h is incompatible with OpenMP (compilation of the third-party module fails on Python 3.5)'
updated_at = <Date 2020-06-05.20:42:45.701>
user = 'https://bugs.python.org/axh'

    bugs.python.org fields:

    activity = <Date 2020-06-05.20:42:45.701>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-06-05.20:42:45.704>
closer = 'vstinner'
components = ['Extension Modules']
creation = <Date 2015-09-17.12:58:59.262>
creator = 'axh'
dependencies = []
files = ['40491', '40493', '40497']
hgrepos = ['318']
issue_num = 25150
keywords = ['patch', '3.5regression']
message_count = 30.0
messages = ['250884', '250886', '250888', '250889', '250901', '250919', '250940', '250944', '250945', '250961', '250964', '250972', '250976', '250977', '251157', '251160', '253754', '253755', '254103', '254104', '255229', '255231', '321824', '321825', '321828', '321834', '321860', '321865', '321866', '370783']
nosy_count = 8.0
nosy_names = ['arekm', 'pitrou', 'scoder', 'vstinner', 'benjamin.peterson', 'python-dev', 'axh', 'gul916']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue25150'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    Activity

    1. axh commented on Sep 17, 2015

      @axh
      axhmannequin
      on Sep 17, 2015
      MannequinAuthor

      trying to install the yt package, most recent version 3.2.1 on python 3.5.0 using pip

      pip3 install -U yt

      error output attached. The same package installs fine with python 3.4.3, same compiler and also build from scratch, so the suspicion is that it is related to the new 3.5.0 version. The system installed is the current Fedora 22, gcc 5.1.1-4.

    2. axh
      added
      extension-modulesC modules in the Modules dir
      buildThe build process and cross-build
      on Sep 17, 2015

    3. vstinner commented on Sep 17, 2015

      @vstinner
      vstinner
      on Sep 17, 2015
      Member

      It looks like the problem is that yt uses OpenMP whereas OpenMP doesn't support atomic operations:

      In file included from /home/alex/Python/include/python3.5m/pyatomic.h:12:0,
                 from /home/alex/Python/include/python3.5m/Python.h:53,
                 from build/src.linux-x86_64-3.5/yt/utilities/lib/geometry_utils.c:4:
/usr/lib/gcc/x86_64-redhat-linux/5.1.1/include/stdatomic.h:40:1: sorry, unimplemented: ‘_Atomic’ with OpenMP
 typedef _Atomic _Bool atomic_bool;
 ^

      I'm not sure that Include/pyatomic.h should be included by the main Include/Python.h header. We already skipped Include/pyatomic.h on C++ because this header is incompatible with C++.

    4. vstinner
      changed the title [-]yt package pip compile/install error[/-] [+]3.5: Include/pyatomic.h is incompatible with OpenMP (compilation of the third-party yt module fails on Python 3.5)[/+] on Sep 17, 2015

    5. axh commented on Sep 17, 2015

      @axh
      axhmannequin
      on Sep 17, 2015
      MannequinAuthor

      When I just comment out the

      #include "pyatomic.h"

      line, python 3.5.0 will no longer compile

      gcc -pthread -c -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -Werror=declaration-after-statement -I. -IInclude -I./Include -DPy_BUILD_CORE -o Programs/python.o ./Programs/python.c
      In file included from Include/traceback.h:8:0,
      from Include/Python.h:97,
      from ./Programs/python.c:3:
      Include/pystate.h:186:8: error: unknown type name ‘_Py_atomic_address’
      PyAPI_DATA(_Py_atomic_address) _PyThreadState_Current;
      ^
      Makefile:747: recipe for target 'Programs/python.o' failed
      make: *** [Programs/python.o] Error 1

      pystate.h:

      /* Assuming the current thread holds the GIL, this is the
      PyThreadState for the current thread.

         Issue python/cpython#67832: pyatomic.h is incompatible with C++ (yet). Disable
   PyThreadState_GET() optimization: declare it as an alias to
   PyThreadState_Get(), as done for limited API. */
#if !defined(Py_LIMITED_API) && !defined(__cplusplus)
PyAPI_DATA(_Py_atomic_address) _PyThreadState_Current;
#endif

    6. vstinner commented on Sep 17, 2015

      @vstinner
      vstinner
      on Sep 17, 2015
      Member

      "When I just comment out the << #include "pyatomic.h" >> line, python 3.5.0 will no longer compile"

      Sure. My idea is to "disable" the header with we are not building Python itself. There is a nice define for that: Py_BUILD_CORE. See attached patch.

      Since all symbols in pyatomic.h are prefixed by _Py, this header is fully part of the Python private API and so it's fine to modify it in a bugfix release (3.5.0 => 3.5.1).

    7. brettcannon commented on Sep 17, 2015

      @brettcannon
      brettcannon
      on Sep 17, 2015
      Member

      If everything in the header is a _Py definition then I agree we should hide it in Python.h from the public.

    8. axh commented on Sep 17, 2015

      @axh
      axhmannequin
      on Sep 17, 2015
      MannequinAuthor
      if I just include this patch, some modules don't build:

      (...)

      gcc -pthread -fPIC -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -Werror=declaration-after-statement -Ibuild/temp.linux-x86_64-3.5/libffi/include -Ibuild/temp.linux-x86_64-3.5/libffi -I/home/alex/Python-3.5.0/Modules/_ctypes/libffi/src -I./Include -I/home/alex/Python/include -I. -IInclude -I/usr/local/include -I/home/alex/Python-3.5.0/Include -I/home/alex/Python-3.5.0 -c /home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c -o build/temp.linux-x86_64-3.5/home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.o -Wall -fexceptions
/home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c: In function ‘PyCSimpleType_from_param’:
/home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c:2062:15: error: ‘_PyThreadState_Current’ undeclared (first use in this function)
         if (Py_EnterRecursiveCall("while processing _as_parameter_")) {
               ^
/home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c:2062:15: note: each undeclared identifier is reported only once for each function it appears in
/home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c:2062:28: error: ‘__atomic_load_ptr’ undeclared (first use in this function)
         if (Py_EnterRecursiveCall("while processing _as_parameter_")) {
                            ^
/home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c:2062:66: error: argument 1 of ‘__atomic_load’ must be a non-void pointer type
         if (Py_EnterRecursiveCall("while processing _as_parameter_")) {
                                                                  ^
/home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c:2062:19: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
         if (Py_EnterRecursiveCall("while processing _as_parameter_")) {
                   ^
/home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c:2067:62: error: argument 1 of ‘__atomic_load’ must be a non-void pointer type
/home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c:2067:21: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
         Py_LeaveRecursiveCall();
                     ^
/home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c:2067:62: error: argument 1 of ‘__atomic_load’ must be a non-void pointer type
/home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c:2067:139: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

      Failed to build these modules:
      _ctypes _decimal _json
      _pickle

    9. axh commented on Sep 18, 2015

      @axh
      axhmannequin
      on Sep 18, 2015
      MannequinAuthor

      So, apparently, more than just one spot needs to be fixed. I also tried just modifying the Python.h after install, but that does not do the trick either.

    10. vstinner commented on Sep 18, 2015

      @vstinner
      vstinner
      on Sep 18, 2015
      Member
      /home/alex/Python-3.5.0/Modules/_ctypes/_ctypes.c:2062:15: error: ‘_PyThreadState_Current’ undeclared (first use in this function)
         if (Py_EnterRecursiveCall("while processing _as_parameter_")) {
               ^

      Ah yes, if you check the fix for C++ (changeset cb05b6d7aacd), I also had to modify pystate.h.

      Please try pyatomic-2.patch which hides more CPython internals in public headers.

    11. vstinner commented on Sep 18, 2015

      @vstinner
      vstinner
      on Sep 18, 2015
      Member

      I created a venv with a Python patched with pyatomic-2.patch: I successfully installed yt. yt depends on numpy & Cython: good news, numpy & Cython were compiled correctly. These two libraries are well known users of the Python C API. It's not enough to check if this change breaks modules on PyPI, but it's still a good news :-)

    12. scoder commented on Sep 18, 2015

      @scoder
      scoder
      on Sep 18, 2015
      Contributor

      Would there be a way to expose these internals rather than hiding them?

    13. vstinner commented on Sep 18, 2015

      @vstinner
      vstinner
      on Sep 18, 2015
      Member

      Would there be a way to expose these internals rather than hiding them?

      Here is the issue is that pyatomic.h cannot be compiled on OpenMP. We had the same issue with C++. In fact, it doesn't make sense to compile pyatomic.h differently to access an atomic variable from an extension module. We must always use exactly the same implementation, otherwise bad things will happen.

      A solution for that is to hide the implementation details and only expose high level APIs.

      For example, pyatomic.h must be completly hidden.

      A consequence is that the _PyThreadState_Current variable must be hidden to. _PyThreadState_Current is an implementation detail, you must not access it directly.

      The PyThreadState_GET() macro uses directly the _PyThreadState_Current variable. So the solution to expose the "PyThreadState_GET" symbol (not necessary as a macro) is to define it as an alias to the PyThreadState_Get() *function*.

      The advantage of using a function is that we don't expose implementation details to third-party extensions, it avoids the risk of ABI incompatibilies.

    14. scoder commented on Sep 18, 2015

      @scoder
      scoder
      on Sep 18, 2015
      Contributor

      Understood and agreed. Second patch looks good to me.

      Cython calls PyThreadState_GET() in pretty much every helper function that deals with exceptions, but I doubt that the potential speed difference is going to be relevant in the real world. And we target CPython's API level anyway, not the ABI, so the C code will just adapt at compile time.

    15. 10 remaining items

    16. vstinner commented on Nov 23, 2015

      @vstinner
      vstinner
      on Nov 23, 2015
      Member

      cool

      2015-11-23 22:19 GMT+01:00 Alexander Heger <report@bugs.python.org>:

      Alexander Heger added the comment:

      seems to work now with 3.5.1rc1

      On 5 November 2015 at 23:01, STINNER Victor <report@bugs.python.org> wrote:
      >
      > STINNER Victor added the comment:
      >
      > "Pyatomic-2.patch solved the problem."
      >
      > Great! The good news is that the Python 3.5.1 release has now a schedule: https://www.python.org/dev/peps/pep-0478/
      >
      > "3.5.1 final: December 6, 2015"
      >
      > ----------
      >
      > _______________________________________
      > Python tracker <report@bugs.python.org>
      > <http://bugs.python.org/issue25150\>
      > _______________________________________

      ----------

      Python tracker <report@bugs.python.org>
      <http://bugs.python.org/issue25150\>

    17. pitrou commented on Jul 17, 2018

      @pitrou
      pitrou
      on Jul 17, 2018
      Member

      I think this should be reconsidered. I understand the desire to compile Python with OpenMP. But the resolution here is hiding _Py_atomic symbols all the time, even when OpenMP isn't involved, and even when building a standard extension module.

    18. pitrou commented on Jul 17, 2018

      @pitrou
      pitrou
      on Jul 17, 2018
      Member

      Case in point: if I want to include "internal/pystate.h" from _pickle.c, I get the following error:

      """
      In file included from ./Include/internal/pystate.h:12:0,
      from /home/antoine/cpython/default/Modules/_pickle.c:10:
      ./Include/internal/ceval.h:14:5: error: unknown type name ‘_Py_atomic_int’
      _Py_atomic_int calls_to_do;
      ^
      """

    19. pitrou commented on Jul 17, 2018

      @pitrou
      pitrou
      on Jul 17, 2018
      Member

      (see bpo-34128 for context)

    20. vstinner commented on Jul 17, 2018

      @vstinner
      vstinner
      on Jul 17, 2018
      Member

      This issue is closed. Would you mind to either reopen it or create a new issue?

    21. benjaminp commented on Jul 18, 2018

      @benjaminp
      benjaminp
      on Jul 18, 2018
      Contributor

      Yes, why not fix this by putting the offending code under "#ifndef _OPENMP"?

      What would be even better if is pyatomic.h wasn't included in Python.h, which should be fine since pyatomic.h doesn't have any public APIs. Maybe we can do that for 3.8.

    22. vstinner commented on Jul 18, 2018

      @vstinner
      vstinner
      on Jul 18, 2018
      Member

      (Ah, Benjamin restarted the discussion, so I reopen this issue.)

      I understand the desire to compile Python with OpenMP.

      I'm not sure that I understood the use case. Do you want to only compile Python core ("python3" binary") or just stdlib C extensions, or both?

      But the resolution here is hiding _Py_atomic symbols all the time, even when OpenMP isn't involved, and even when building a standard extension module.

      Sorry, but I don't understand the problem. Why is it an issue to hide _Py_atomic symbols?

    23. vstinner
      reopened this on Jul 18, 2018

    24. vstinner commented on Jul 18, 2018

      @vstinner
      vstinner
      on Jul 18, 2018
      Member

      This issue is very specific to *third party* extensions, not C extensions which are part of the standard library. That's why I asked if it wouldn't be better to open a new issue, if your use case concerns the stdlib and/or Python core.

    25. vstinner
      changed the title [-]3.5: Include/pyatomic.h is incompatible with OpenMP (compilation of the third-party yt module fails on Python 3.5)[/-] [+]3.5: Include/pyatomic.h is incompatible with OpenMP (compilation of the third-party module fails on Python 3.5)[/+] on Jul 18, 2018
    26. terryjreedy
      added
      3.7 (EOL)end of life
      3.8 (EOL)end of life
      on Jul 20, 2018

    27. vstinner commented on Jun 5, 2020

      @vstinner
      vstinner
      on Jun 5, 2020
      Member

      Python.h does not include pyatomic.h in Python 3.8: the header file moved to the internal C API. The initial issue is fixed, so I close again the issue.

    28. vstinner
      closed this as completedon Jun 5, 2020
    29. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.7 (EOL)end of life3.8 (EOL)end of lifebuildThe build process and cross-buildextension-modulesC modules in the Modules dir

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions