swift-atomics icon indicating copy to clipboard operation
swift-atomics copied to clipboard

swift-atomics uses non-standard `_Atomic(bool)` operations

Open lorentey opened this issue 2 years ago • 4 comments

Swift Atomics currently exposes and/xor/or operations on booleans, and maps them to atomic_fetch_XXX_explicit calls in C. However, C17 does not define these operations on _Atomic(_Bool); it only defines them for other atomic integer types.

This is silently accepted by Clang, but it causes build issues when the _AtomicsShims module is compiled with GCC, such as with CMake.

Review Bool's atomic operations and figure out if we should deprecate loadThenLogicalOr and friends, or if we can continue exposing them (with this implementation or through compareExchange).

lorentey avatar Sep 08 '21 21:09 lorentey

For now, I'm going to #ifdef out the shims when compiled outside Swift's Clang Importer.

lorentey avatar Sep 08 '21 21:09 lorentey

Interesting. I had relied on cppreference to be correct, and assumed I did not need to read the standard.

glessard avatar Sep 09 '21 18:09 glessard

This is silently accepted by Clang, but it causes build issues when the _AtomicsShims module is compiled with GCC, such as with CMake.

CMake builds not using a swift-toolchain-compatible clang seems like a bigger problem, since System and Atomics and Numerics would all benefit considerably from a known C environment.

stephentyrone avatar Sep 10 '21 01:09 stephentyrone

@stephentyrone - at the very least, MSVC needs to be supported as a C compiler. There are subtle bugs in clang/LLVM when it comes to some platforms, and so assuming a swift toolchain compatible clang is just as big an issue IMO.

compnerd avatar Nov 28 '21 19:11 compnerd

The current #if __swift__ clauses should nicely step around C compiler problems.

Meanwhile, https://github.com/apple/swift-atomics/pull/74 starts walking towards getting rid of the C module altogether. It'll require compiler and stdlib changes to land; I'm going to do my best to deliver them.

The use of non-standard _Atomic(bool) operations is an issue specific to the current _AtomicsShims module. We could easily fix that, but it does not currently cause problems, so I'm not going to bother.

Closing.

lorentey avatar Mar 20 '23 19:03 lorentey

https://github.com/apple/swift-atomics/pull/74 refactored things so that Bool now goes through Int8 atomics instead, even when we're using C atomics.

lorentey avatar Mar 25 '23 00:03 lorentey