Skip to content
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

swift-atomics uses non-standard _Atomic(bool) operations #37

Closed
lorentey opened this issue Sep 8, 2021 · 6 comments
Closed

swift-atomics uses non-standard _Atomic(bool) operations #37

lorentey opened this issue Sep 8, 2021 · 6 comments
Labels
bug Something isn't working

Comments

@lorentey
Copy link
Member

lorentey commented Sep 8, 2021

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 lorentey added the bug Something isn't working label Sep 8, 2021
@lorentey
Copy link
Member Author

lorentey commented Sep 8, 2021

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

lorentey added a commit to lorentey/swift-atomics that referenced this issue Sep 9, 2021
Per apple#37, we currently happen to use some non-standard C atomics that only work in Clang; to prevent issues when this module gets compiled using other compilers, `#ifdef` out all the actual shims, leaving only the definitions for `retain_n`/`release_n`.

(For example, CMake builds the shims module using the system C compiler by default, which is usually GCC on Linux.)
lorentey added a commit to lorentey/swift-atomics that referenced this issue Sep 9, 2021
Per apple#37, we currently happen to use some non-standard C atomics that only work in Clang; to prevent issues when this module gets compiled using other compilers, `#ifdef` out all the actual shims, leaving only the definitions for `retain_n`/`release_n`.

(For example, CMake builds the shims module using the system C compiler by default, which is usually GCC on Linux.)
@glessard
Copy link
Contributor

glessard commented Sep 9, 2021

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

@stephentyrone
Copy link
Member

stephentyrone commented Sep 10, 2021

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.

@compnerd
Copy link
Contributor

@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.

@lorentey
Copy link
Member Author

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

Meanwhile, #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
Copy link
Member Author

#74 refactored things so that Bool now goes through Int8 atomics instead, even when we're using C atomics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants