Add support for GCC builtins. This is a step towards
reusing LLVM atomic.c as libatomic compatible with GCC.
Add public function prototypes to avoid build errors with
krytarowski on May 11 2020, 5:23 AM.Authored by
I don't think that this should be including <stdatomic.h>. These routines are used to implement interfaces in <stdatomic.h> and it would be completely valid for a conforming implementation to use _Generic macros to directly call these functions from things in <stdatomic.h> that we then depend call, giving a circular dependency.
I don't particularly object to moving this to using the GCC builtins, since clang supports both the GCC ones and the __c11 ones, but the only two valid implementations of the routines in this file are to use compiler builtins or inline assembly.
Without looking at the details, this generally seems fine... but why is this desirable? The commit message should have that information. If there's not really a reason then I'd rather leave it alone and not change something that's fine.
I think that's a fine motivation. libatomic from LLVM and GCC have the same API, and it's not big. However, this is a new thing for LLVM to support. I'd suggest sending an RFC, basically asking "are we OK supporting libatomic for non-LLVM compilers?". I suspect the answer is "yes" because it seems like very low overhead. However I might be missing details that make this complicated, where for example there's non-trivial and non-portable tie-in between the compiler and the runtime's implementation.
To give some background (please correct me if I'm wrong): This is initially motivated by having snmalloc working on NetBSD. Snmalloc relies on two-pointer compare-and-exchange. When passing -mcx16, clang will happily emit these instructions for the atomic builtins used by <atomic> but GCC currently contains a bug that will emit these only for the old-style __sync_* builtins and not for the __atomic_ ones. As a result, the gcc builds of snmalloc include a call to __atomic_compare_exchange_16, which is not found on NetBSD because they don't ship the relevant support functions. NetBSD would like to ship chunks of compiler-rt, compiled with their default toolchain (GCC).
Unfortunately, I don't believe these changes will actually work as expected. Because of the aforementioned GCC but, __atomic_compare_exchange_16 will compile to the version with a lock on x86-64 with GCC, not the lock-free version. This means that _Atomic-qualified 16-byte values will not be correctly synchronised between GCC code, clang code, and clang code compiled with -mcx16.
I'm also somewhat opposed to removing the _Atomic qualifier. I'd rather that we did a #define _Atomic(x) x guarded by a check for broken compilers. Unfortunately, the C11 spec is somewhat broken in this regard. _Atomic qualifiers were added to allow Atomic(T) and T to have different padding and alignment requirements (and even different representations, such as requiring an inline lock for large values of T), but then the <stdatomic.h> functions were defined to take volatile T* instead of _Atomic(T) * arguments, violating the intent. The porting of C++11's atomics to C was not handled well at all by WG14. Part of the early goal was for _Atomic(T) and std::atomic<T> to be ABI-compatible (or, at least, for ABIs where these two were compatible to be possible), but it's quite easy to define a C++ template that switches from lock-free to having an inline lock. but it's impossible to do that with the eventual APIs in <stdatomic.h>. Supporting the eventual mess is likely to cause pain for many years to come.
There is a growing number of software that requires libatomic, especially for older and 32-bit CPUs and we cannot keep forcing usage of shiny CPU types in all circumstances. snmalloc is only one of them, but not the only one.
We would want use the same library in the basesystem for both compilers. Something along these lines was also originally proposed in the snmalloc repository to deliver a basesystem version of libatomic.
GCC will likely fix -mcx16 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80878
Would you like to fix Clang? _Atomic() specifiers shall not be at least rejected.
I have removed from the proposed patch ABI change in __atomic_compare_exchange_##n in order to make it easier to merge, that one shall be fixed too.
While there... LLDB forces std::atomic<uint64_t> in https://github.com/llvm/llvm-project/blob/cdc514e4c67f268b07863bbac3d8d7e0d088186c/lldb/include/lldb/Utility/Timer.h#L32 This makes import of LLDB into the NetBSD basesystem harder.
Should we have llvm::atomic<> that reimplements libatomic? Have you got an idea how to design it? As I looked into std::atomic<> the implementation is rather too large for reimplementation (around 3000 LOC). That's not the first time when we see std::atomic<> in the LLDB codebase and this will eventually return back.
I presume that it's best to just deliver libatomic at this point, as it is a part of the C11/C++14 runtime.
I rewrote this code again to support GCC. This time without removal of _Atomic().
Now, there is a separation for Clang and GCC builtins. It's easier to handle them separately as they are not compatible sufficiently.
Is this version looking fine?
Once I will get something acceptable to merge, I can send a mail. I will speak up for merging the __atomic_compare_exchange_##n semantics.
It's not super pretty, but I think this would work (assuming my concern about PP expansions above is wrong).
That being said, I somewhat agree that adding support for another platform or building with another compiler is something that should be done deliberately. In this case, for example, it means that the compiler-rt maintainers (who are they?) will be "stuck" making sure that libatomic builds on GCC forever. For instance, do we have a buildbot that does that? This is the kind of thing that would cause me to say "nope, we don't support that compiler because we can't test such support".
I'm not a compiler-rt maintainer, but I would definitely hold that position for libc++ and libc++abi.
In this case my only intention is to reuse atomic.c and construct libatomic with custom build system.
I'm kind to upstream local modifications, before doing it, for the general benefit.
The point I'm trying to make is that it seems this change allows you to build this file with GCC at this point in time, which is great. But it also appears like none of this is stringed into the rest of LLVM (build system, build bots, etc.). That's kind of what I'm worried about. To make my concern clearer, after you commit this, what would prevent someone from unknowingly removing the prototypes before the function definitions and breaking you? Nothing AFAICT. That's the sort of thing that tells me this change is "lazy", in the sense that it allows you to solve your problem right now, but it does introduce latent tech debt.
Anyway, I'm not trying to block you, I'm not even a maintainer of compiler-rt. It just seems like this is flaky.
In all cases, this looks OK for libc++ as long as you don't break the builtins when compiled with Clang (and I include changing their API in "break").