classification
Title: 3.5: Include/pyatomic.h is incompatible with OpenMP (compilation of the third-party module fails on Python 3.5)
Type: compile error Stage:
Components: Extension Modules Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: arekm, axh, benjamin.peterson, brett.cannon, gul916, pitrou, python-dev, scoder, vstinner
Priority: normal Keywords: 3.5regression, patch

Created on 2015-09-17 12:58 by axh, last changed 2018-07-20 18:32 by terry.reedy.

Files
File name Uploaded Description Edit
error.txt axh, 2015-09-17 12:58 error message
pyatomic.patch vstinner, 2015-09-17 14:08 review
pyatomic-2.patch vstinner, 2015-09-18 06:51 review
Repositories containing patches
3.5.0 release
Messages (29)
msg250884 - (view) Author: Alexander Heger (axh) * Date: 2015-09-17 12:58
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.
msg250886 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-17 13:13
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++.
msg250888 - (view) Author: Alexander Heger (axh) * Date: 2015-09-17 14:05
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 #23644: 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
msg250889 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-17 14:08
"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).
msg250901 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-09-17 17:24
If everything in the header is a _Py definition then I agree we should hide it in Python.h from the public.
msg250919 - (view) Author: Alexander Heger (axh) * Date: 2015-09-17 20:14
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
msg250940 - (view) Author: Alexander Heger (axh) * Date: 2015-09-18 06:07
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.
msg250944 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-18 06:51
/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.
msg250945 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-18 07:01
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 :-)
msg250961 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-09-18 11:13
Would there be a way to expose these internals rather than hiding them?
msg250964 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-18 11:34
> 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.
msg250972 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-09-18 12:25
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.
msg250976 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-09-18 13:09
New changeset d4fcb362f7c6 by Victor Stinner in branch '3.5':
Issue #25150: Hide the private _Py_atomic_xxx symbols from the public
https://hg.python.org/cpython/rev/d4fcb362f7c6
msg250977 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-18 13:09
I pushed my fix pyatomic-2.patch to Python 3.5 and 3.6.

Thanks for the bug report Alexander Heger!
msg251157 - (view) Author: Alexander Heger (axh) * Date: 2015-09-20 08:54
Dear Victor,

yes, you patch seems to fix the yt install.  Thank you very much!

I hope this can be included in 3.5.1.

Best wishes,
Alexander
msg251160 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-09-20 09:33
Yes my fix will be part of Python 3.5.1 release.
msg253754 - (view) Author: Arkadiusz Miskiewicz Arkadiusz Miskiewicz (arekm) Date: 2015-10-30 20:28
Should it work with /configure '--with-cxx-main=g++' && make?

Because currently it doesn't:

g++ -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
cc1plus: warning: command line option ‘-Wstrict-prototypes’ is valid for C/ObjC but not for C++
In file included from Include/pyatomic.h:10:0,
                 from Include/Python.h:53,
                 from ./Programs/python.c:3:
/usr/lib64/gcc/x86_64-pld-linux/5.2.0/include/stdatomic.h:40:9: error: ‘_Atomic’ does not name a type
[...]

(testing 3.5.0 + patch for this issue from hg)
msg253755 - (view) Author: Arkadiusz Miskiewicz Arkadiusz Miskiewicz (arekm) Date: 2015-10-30 20:32
Same for 3.5 branch from hg (git mirror actually).
msg254103 - (view) Author: (gul916) Date: 2015-11-05 11:56
Hi,
Just a few words to tell you that I had the same problem to compile scipy 0.16.0 with mkl libraries under python 3.5 and linux fedora 22. Pyatomic-2.patch solved the problem.
Thanks
msg254104 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-11-05 12:01
"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"
msg255229 - (view) Author: Alexander Heger (axh) * Date: 2015-11-23 21:19
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>
> _______________________________________
msg255231 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-11-23 21:32
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>
> _______________________________________
msg321824 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-07-17 13:37
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.
msg321825 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-07-17 13:38
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;
     ^
"""
msg321828 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-07-17 13:50
(see issue34128 for context)
msg321834 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-17 14:52
This issue is closed. Would you mind to either reopen it or create a new issue?
msg321860 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-07-18 06:16
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.
msg321865 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-18 08:08
(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?
msg321866 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-07-18 08:09
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.
History
Date User Action Args
2018-07-20 18:32:19terry.reedysetversions: + Python 3.7, Python 3.8, - Python 3.5
2018-07-18 08:09:37vstinnersetmessages: + msg321866
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)
2018-07-18 08:08:43vstinnersetstatus: closed -> open
resolution: fixed ->
messages: + msg321865
2018-07-18 06:16:46benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg321860
2018-07-17 14:52:17vstinnersetmessages: + msg321834
2018-07-17 13:50:42pitrousetmessages: + msg321828
2018-07-17 13:38:57pitrousetmessages: + msg321825
2018-07-17 13:37:05pitrousetnosy: + pitrou
messages: + msg321824
2015-11-23 21:32:36vstinnersetmessages: + msg255231
2015-11-23 21:19:37axhsetmessages: + msg255229
2015-11-05 12:01:19vstinnersetmessages: + msg254104
2015-11-05 11:56:21gul916setnosy: + gul916
messages: + msg254103
2015-10-30 20:32:56arekmsetmessages: + msg253755
2015-10-30 20:28:21arekmsetnosy: + arekm
messages: + msg253754
2015-09-21 07:18:56vstinnersetkeywords: + 3.5regression
2015-09-20 09:33:39vstinnersetmessages: + msg251160
2015-09-20 08:54:24axhsetmessages: + msg251157
2015-09-18 13:19:00vstinnersetstatus: open -> closed
resolution: fixed
2015-09-18 13:09:58vstinnersetmessages: + msg250977
2015-09-18 13:09:14python-devsetnosy: + python-dev
messages: + msg250976
2015-09-18 12:25:14scodersetmessages: + msg250972
2015-09-18 11:34:14vstinnersetmessages: + msg250964
2015-09-18 11:13:44scodersetnosy: + scoder
messages: + msg250961
2015-09-18 07:01:37vstinnersetmessages: + msg250945
2015-09-18 06:51:53vstinnersetfiles: + pyatomic-2.patch

messages: + msg250944
2015-09-18 06:07:43axhsetmessages: + msg250940
2015-09-17 20:14:01axhsetmessages: + msg250919
2015-09-17 17:24:44brett.cannonsetnosy: + brett.cannon
messages: + msg250901
2015-09-17 14:08:50vstinnersetfiles: + pyatomic.patch
keywords: + patch
messages: + msg250889
2015-09-17 14:05:21axhsetmessages: + msg250888
2015-09-17 13:13:53vstinnersettitle: 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)
versions: + Python 3.6
2015-09-17 13:13:00vstinnersetnosy: + vstinner
messages: + msg250886
2015-09-17 12:58:59axhcreate