This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] [builtins] Port atomic.c to GCC
Needs ReviewPublic

Authored by krytarowski on May 11 2020, 5:23 AM.

Details

Summary

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
enabled warnings

Diff Detail

Event Timeline

krytarowski created this revision.May 11 2020, 5:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2020, 5:23 AM
Herald added subscribers: Restricted Project, jfb, dberris. · View Herald Transcript
theraven requested changes to this revision.May 11 2020, 7:06 AM

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.

This revision now requires changes to proceed.May 11 2020, 7:06 AM
krytarowski edited the summary of this revision. (Show Details)
krytarowski added a reviewer: mgorny.

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.

I've switched to GCC builtins. I had to remove _Atomic() in order to make it buildable.

jfb added a reviewer: ldionne.May 11 2020, 8:36 AM
jfb added a subscriber: __simt__.
jfb added a comment.May 11 2020, 8:41 AM

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.

In D79713#2029514, @jfb wrote:

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 intend to use LLVM (lib)atomic implementation with GCC, instead of the GNU libatomic version.

jfb added a comment.May 11 2020, 9:37 AM
In D79713#2029514, @jfb wrote:

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 intend to use LLVM (lib)atomic implementation with GCC, instead of the GNU libatomic version.

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.

ldionne added inline comments.May 11 2020, 9:49 AM
compiler-rt/lib/builtins/atomic.c
310

Sorry if that's a dumb question (I'm not super familiar with how things are stringed between the compiler and compiler-rt), but doesn't that mean the builtin will now take an additional argument bool weak? IOW, is that an API breaking change?

krytarowski marked an inline comment as done.May 11 2020, 10:48 AM
krytarowski added inline comments.
compiler-rt/lib/builtins/atomic.c
310

I think that there is a bug in Clang/LLVM as it forgets about weak in certain places (CGAtomic.cpp) and notes it in other ones (generally __atomic_compare_exchange_n).

jfb added inline comments.May 11 2020, 11:33 AM
compiler-rt/lib/builtins/atomic.c
310

This does seem like a divergence from GCC. Docs are consistent with the code though:

https://llvm.org/docs/Atomics.html#libcalls-atomic

https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html

I'm not sure that we can unconditionally "fix" this, because it would be an ABI break.

krytarowski marked an inline comment as done.May 11 2020, 12:08 PM
krytarowski added inline comments.
compiler-rt/lib/builtins/atomic.c
310

This still could be fixed unconditionally at this phase as this library is not widely used (not building by default for me either). It also contains missing calls, even if they are generated in Clang (__atomic_fetch_nand_N existing in GNU libatomic or lack of version of __c11_atomic_fetch_min?).

I have got intention to use the same libatomic implementation for both compilers, even if there will be ABI switch in syncing the interface, however I defer this decision to the Clang maintainers.

If there is a decision to support only Clang and be incompatible with GCC on purpose, it would be good to know.

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.

krytarowski edited the summary of this revision. (Show Details)
  • drop ABI change in __atomic_compare_exchange_##n

To give some background (please correct me if I'm wrong): This is initially motivated by having snmalloc working on NetBSD.

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

I'm also somewhat opposed to removing the _Atomic qualifier.

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.

it's quite easy to define a C++ template that switches from lock-free to having an inline lock

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.

ldionne added inline comments.May 12 2020, 5:47 AM
compiler-rt/lib/builtins/atomic.c
310

My question was simply whether this was an API breaking change for existing users of Clang. I'm still not sure of the answer, however it seems you've reverted this part of the patch, so I guess the point is moot.

I don't think anybody claims that libatomic should only support Clang and not GCC, I think we're just trying to figure out what that means and whether we can do it without breaking existing users on Clang. At least, that's what I'm trying to determine.

FWIW, Apple doesn't ship libatomic yet (AFAICT), so whatever we do here should be fine from that perspective. I can't speak for other vendors.

krytarowski marked an inline comment as done.May 12 2020, 7:44 AM
krytarowski added inline comments.
compiler-rt/lib/builtins/atomic.c
310

FreeBSD uses GNU libatomic for the combination with GCC.
OpenBSD doesn't use LLVM atomic.c.
NetBSD doesn't use LLVM atomic.c yet.

Apple doesn't ship it according to you.

After a quick search in Linux distros, there are probably no users of atomic.c, if there are any, they are not in mainstream distros.

NetBSD is apparently the first wannabe user of LLVM atomic and thus I have been first to find out the mismatch in the builtin with GCC.

I don't want to block the entire review on this mismatch and that can be submitted separately.

FreeBSD uses GNU libatomic for the combination with GCC.

I'm not sure what this means. FreeBSD now uses LLVM's atomic.c in our libgcc_s implementation.

jfb added a comment.May 12 2020, 8:16 AM

Still, I think this needs an RFC on LLVM dev. Supporting a new platform should be done deliberately.

krytarowski edited the summary of this revision. (Show Details)

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?

In D79713#2031627, @jfb wrote:

Still, I think this needs an RFC on LLVM dev. Supporting a new platform should be done deliberately.

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.

compiler-rt/lib/builtins/atomic.c
332

Does this really work? I thought you'd need to go through another PP expansion in order for this to expand properly. I'd think you need to write something like CONCAT(ATOMIC_FETCH_, opname, _EXPLICIT) where #define CONCAT(x,y,z) x ## y ## z?

will be "stuck" making sure that libatomic builds on GCC forever.

Generally... we [used to] build everything in compiler-rt with GCC except this single file.

will be "stuck" making sure that libatomic builds on GCC forever.

Generally... we [used to] build everything in compiler-rt with GCC except this single file.

Interesting! Where is the corresponding build system change in this patch?

will be "stuck" making sure that libatomic builds on GCC forever.

Generally... we [used to] build everything in compiler-rt with GCC except this single file.

Interesting! Where is the corresponding build system change in this patch?

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.

I've separated two independent changes out of this revision into a separate review.
D79845 + D79846.

Please review them. I will be easier to focus here on the proper GCC support only.

will be "stuck" making sure that libatomic builds on GCC forever.

Generally... we [used to] build everything in compiler-rt with GCC except this single file.

Interesting! Where is the corresponding build system change in this patch?

In this case my only intention is to reuse atomic.c and construct libatomic with custom build system.

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").

Is this still relevant? If not, let's abandon to clear up the review queue.

It's relevant but I encountered issues to pass the review..

ldionne resigned from this revision.Nov 2 2020, 3:30 PM

It's relevant but I encountered issues to pass the review..

If you're thinking about my comments, they are not blocking. I don't own compiler-rt. I was just giving my opinion.