[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Pär Björklund added the comment: The HLE variants were simply chosen to match the semantics on other platforms with regard to aquire/release. If Intel engineers say the plain versions are better that's good enough for me. It would be interesting seeing some benchmarks but I don't have any idea

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Antoine Pitrou added the comment: I would be ok with reverting to the non-HLE variants. Does anyone want to test the performance implications on TSX-enabled hardware? -- ___ Python tracker

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Ethan Smith added the comment: When working on clang-cl support, I was advised here https://reviews.llvm.org/D47672#1131325 that we may be using hardware lock elision incorrectly. Copying from there: > I also spoke to Andi Kleen here at Intel to make sure I got these inline > assembly

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Antoine Pitrou added the comment: Thanks to Segev Finer for submitting the PR which fixed the warnings! -- nosy: +Segev Finer ___ Python tracker ___

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Changes by Antoine Pitrou : -- resolution: -> fixed stage: needs patch -> resolved status: open -> closed ___ Python tracker ___

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Antoine Pitrou added the comment: New changeset 0267128aa4dc7c383c341c19833c5d506eae9c93 by Antoine Pitrou (Segev Finer) in branch 'master': bpo-9566 & bpo-30747: Silence warnings from pyatomic.h macros (#3140) https://github.com/python/cpython/commit/0267128aa4dc7c383c341c19833c5d506eae9c93

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Changes by Segev Finer : -- pull_requests: +3175 ___ Python tracker ___ ___

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Pär Björklund added the comment: My guess would be the cast from uintptr_t to intptr_t in the return type. I'll look into this, when possible, should have some time later this week or over the weekend. -- ___ Python tracker

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Steve Dower added the comment: This commit added *a lot* of warnings - http://buildbot.python.org/all/builders/AMD64%20Windows10%203.x/builds/1056 Most are of the form: warning C4133: 'function': incompatible types - from 'volatile uintptr_t *' to 'volatile int *' I can't obviously see

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Changes by Antoine Pitrou : -- resolution: -> fixed stage: patch review -> backport needed status: open -> closed ___ Python tracker ___

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Antoine Pitrou added the comment: New changeset e664d7f89d2b9960d9049237136396e824795cac by Antoine Pitrou (Pär Björklund) in branch 'master': bpo-30747: Attempt to fix atomic load/store (#2383) https://github.com/python/cpython/commit/e664d7f89d2b9960d9049237136396e824795cac --

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Pär Björklund added the comment: Microsoft don't spend much time on the C compiler features, still lacking C99 features so I don't have much hope of getting C11 support anytime soon or at all. One could of course implement a cross platform stdatomic library that matches the C11 spec.

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Antoine Pitrou added the comment: Unfortunately not: """Python versions greater than or equal to 3.6 use C89 with several select C99 features: [...]""" (PEP 7). -- ___ Python tracker

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Jeffrey Yasskin added the comment: Has enough time passed that you can use the C11 atomic types and operations instead of special-casing these for each compiler? (e.g. http://en.cppreference.com/w/c/atomic/atomic_store) -- ___ Python tracker

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Steve Dower added the comment: > Would there be any interest of implementing them for MSVC/ARM as well Sure, since you're there. It's not easy to test, but I know people who are doing it, so it'll get noticed eventually. Maybe there's some sort of stress test we can write that is likely to

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Pär Björklund added the comment: Antoine said it best. It's very hard to prove that this code is correct or incorrect as it requires multiple threads accessing the same variable and very specific timings to produce an actual issue. My PR only solved half of the issue because I didn't really

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Antoine Pitrou added the comment: The general issue those macros want to prevent is that modern CPUs have a tendency to execute a lot of stuff out-of-order, *including* memory operations. From the perspective of a single hardware core (or thread, really), that's fine since it has a

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Changes by Serhiy Storchaka : -- nosy: +mark.dickinson ___ Python tracker ___

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Steve Dower added the comment: Unless you've got an example of this causing actual issues, it should only go into 3.7. All platforms supported by Windows guarantee atomicity for aligned, pointer-sized reads and writes, but there appear to be memory fencing semantics in here too. I don't

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Changes by Antoine Pitrou : -- nosy: +paul.moore, steve.dower, tim.golden, zach.ware stage: -> patch review type: enhancement -> behavior versions: +Python 3.5 ___ Python tracker

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

Changes by Pär Björklund : -- pull_requests: +2433 ___ Python tracker ___ ___

[issue30747] _Py_atomic_* not actually atomic on Windows with MSVC

New submission from Pär Björklund: _Py_atomic_store and _Py_atomic_load are not implemented as atomic operations on Windows when using Visual Studio to build. This might cause hard to troubleshoot behaviour, especially in third party hosting applications.. -- components: Interpreter