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
Implement atomic operations on non-x86 platforms #66237
Comments
The atomic operations listed in the pyatomic.h header file were implemented only for the x86 architecture, this patch uses the atomic bultins available in GCC >= 4.7 to implement such operations, allowing it to work properly in other platforms. |
+ #define __ATOMIC_RELAXED 0 You should use the "_Py_" prefix for these constants, to avoid conflicts in applications. (You may also replace tabs with spaces, the PEP-7 says "Use 4-space indents and no tabs at all." but I also prefer to avoid tabs in other places.) I tested your patch on Fedora 20 (Linux kernel 3.14.8, GCC 4.8.2, glibc 2.18) on x86_64 ("Intel(R) Core(TM) i7-2600 CPU @ 3.40GHz") and the whole Python test suite pass. GCC 4.9 (released a few month ago) provides the <stdatomic.h> header: pyatomic.h contains this comment: /* XXX: When compilers start offering a stdatomic.h with lock-free But using <stdatomic.h> header can be done in a separated issue. |
Implemented a new version of the patch using either gcc builtins or the stdatomic.h header (this is detected by the configure script). |
ping |
Fedora 20 provides GCC 4.7 but no stdatomic.c: HAVE_BUILTIN_ATOMIC set (HAVE_STD_ATOMIC unset). Fedora 21 provides GCC 4.9 with stdatomic.c: HAVE_BUILTIN_ATOMIC and HAVE_STD_ATOMIC are set. I tested atomicv2.patch on Fedora 20 (x86_64) and Fedora 21 (x86_64): the whole test suite pass. |
atomicv2.patch:
Why not using the atomic_int type from stdatomic.h here?
"__atomic_store_n(): The valid memory model variants are __ATOMIC_RELAXED, __ATOMIC_SEQ_CST, and __ATOMIC_RELEASE." I understand that _Py_atomic_store_explicit() only accept some values for order. An assertion should be added here, maybe for any implementation. Something like: #define _Py_atomic_store_explicit(ATOMIC_VAL, NEW_VAL, ORDER) \
(assert((ORDER) == __ATOMIC_RELAXED \
|| (ORDER) == __ATOMIC_SEQ_CST \
|| (ORDER) == __ATOMIC_RELEASE), \
__atomic_store_n(&(ATOMIC_VAL)->_value, NEW_VAL, ORDER)) Same remark for _Py_atomic_load_explicit(): "__atomic_load_n(): The valid memory model variants are __ATOMIC_RELAXED, __ATOMIC_SEQ_CST, __ATOMIC_ACQUIRE, and __ATOMIC_CONSUME." |
@Haypo, done: atomicv3.patch |
New changeset fbe87fb071a6 by Victor Stinner in branch 'default': |
atomicv3.patch is wrong for GCC builtin atomic operations: the parenthesis is not closed. I fixed this typo in the commit. Vitor & Gustavo: Thanks for the patch, it's now applied to Python 3.5. I tested it on Fedora 21 (x86_64). I disabled manually HAVE_STD_ATOMIC in pyconfig.h to test the two new options (stdatomic header & GCC builtins). |
Thank you, Victor! |
I reopen the issue because Python cannot be compiled anymore on the Builder AMD64 FreeBSD 10.0 3.x: checking for stdatomic.h... yes --- Parser/tokenizer_pgen.o --- |
@Haypo, OK, I will investigate the problem. |
@Haypo, done: atomicv4.patch |
Can you please generate a patch for the default branch of Python? |
Gustavo Temple: A patch against newest revision of default branch would be more useful. |
OK, I will do. |
FreeBSD buildbots broken since fbe87fb071a67cb5e638b3496362b5aedc0fc9a7 |
Oops, incomplete comment, apologies. Just noticed haypo has reported the issue here already |
New changeset dacc944641b1 by Victor Stinner in branch 'default': |
I commited atomicv5.patch because it's simple, but I'm not sure that we are using stdatomic.h "correctly". The current code looks to be written for GCC, Clang fails to compile it (FreeBSD 10 now uses Clang instead of GCC). Maybe the "_Atomic void*" type is wrong, and we should use the "atomic_uintptr_t" type instead and cast to the expected type? Attached atomic_pointer.patch implements this idea. It works on FreeBSD 10 with Clang and on Fedora 21 with GCC 4.9, both have stdatomic.h. |
At least, the latest change repaired FreeBSD 10 compilation. |
@Haypo, I checked and your idea and implementation are very good, thank you very much. Yes, there is a Clang-specific implementation of the stdatomic.h header [1]. The Musl libc for example created a stdatomic.h header with full compatibility [2]. [1] http://llvm.org/viewvc/llvm-project?view=revision&revision=218957 |
New changeset eb48295e1f8b by Victor Stinner in branch 'default': |
<stdatomic.h> is not compatible with C++: I disabled completly pyatomic.h on C++. pyatomic.h is only needed by Python core, not to compile Python extensions, so it's not an issue. |
anything left to do on this issue? |
I didn't hear any complain about Include/internal/pycore_atomic.h recently. Previously, it was indirectly included by |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: