[cfe-dev] RFC: __atomic_* support for gcc 4.7 compatibility

[David Chisnall]

On 10 Apr 2012, at 22:11, Richard Smith wrote:

> On Tue, Apr 10, 2012 at 12:58 PM, David Chisnall <theraven at sucs.org> wrote:
> On 10 Apr 2012, at 20:22, Richard Smith wrote:
> > Hi,
> >
> > I'm looking into adding support for gcc 4.7's __atomic_* builtins for the clang 3.1 release (these are necessary to support various parts of libstdc++4.7). Our current set of __atomic_* builtins is incompatible with gcc 4.7's builtins in a few ways:
> 
> While it would be nice to build libstdc++, it should be noted that the gcc builtins are poorly designed in a number of ways and I don't think we should be encouraging their use by supporting them in anything other than the specific case of supporting libstdc++.
> 
> I agree, but I think this is a problem of documentation. On that subject, do we have documentation for our current __atomic_ builtins somewhere?

The documentation is the C11 spec.  The builtins have a 1:1 correspondence with the C11 generic functions in stdatomic.h.

> > 2) In clang, the argument type must be an _Atomic. gcc does not have this restriction (but also fails to require that the type is trivially copyable, which is probably a bug).
> 
> This is a horrible bug in GCC, because it means that you can have atomic and non-atomic accesses to the same memory address (something explicitly not allowed by C11).  This is part of the reason for the _Atomic() qualifier.  It also changes the alignment requirements of certain types, in both gcc and clang, which can lead to really hard-to-track-down bugs as a result of sloppy casting.
> 
> I don't see that this is obviously a bug in gcc -- it does not support _Atomic, and (as you suggested above)

In that case, it is an additional bug, because its documentation on its atomic implementation indicates that it does.

> presumably expects its builtins to be used only to implement <stdatomic.h> and <atomic> (or by people who know what they're doing and want to circumvent the normal protections).

As I said, it's very difficult to enforce the requirement that _Atomic operations only be accessed atomically in C (not so hard in C++, where you have operator overloading).  For example, in C11 this:

_Atomic(int) i;
...
i++;

Is an atomic increment.  And so is this:

atomic_fetch_add_explicit(&i, 1, memory_order_seq_cst);

But this i++ is not atomic if i is not _Atomic-qualified, and atomic_fetch_add_explicit() is undefined in this case.

> I would strongly oppose supporting using the builtins with types that are not _Atomic-qualified, because doing so makes it trivial to introduce a large number of very subtle bugs.
> 
> Such support is necessary to support libstdc++'s <atomic>. I think you've made a reasonable case that we should have two sets of builtins, a C11-style set (the current builtins) and a GCC-compatible set. To this end, I would strongly prefer for them to have different names, so that the trivial mapping of <stdatomic.h> names to builtins never accidentally uses the GCC-compatible builtins. Unfortunately, that seems to require renaming the C11-style builtins (unless someone has a better suggestion).

I believe that it's possible to implement the GCC ones in a terms of the clang ones in a header, if they're required for libstdc++.  If libstdc++ is the only consumer of them, then an <atomic> that did this and then #include_next'd the libstdc++ one might be a better solution.

> > 3) Clang is missing the __atomic_*_n, __atomic_always_lock_free and __atomic_fetch_nand builtins.
> 
> The _n builtins actually do what the current clang ones do.
> 
> Functionally, yes, but the _Atomic check makes clang's implementations unusable for libstdc++, and the _n variants are 
> required to check that the argument type is a 1, 2, 4, 8 or 16-byte integer or pointer type.

The latter requirement is not required by either C++11 or C11.

> __atomic_always_lock_free is probably useful.  __atomic_fetch_nand might be as well, although it isn't needed by C++11 or C11.
> 
> Right. I consider __atomic_fetch_nand and __atomic_always_lock_free to be a low priority (not necessary for 3.1) since libstdc++4.7 does not use them.
> 
> > 4) Clang is missing the __atomic_compare_exchange builtin, and instead provides __atomic_compare_exchange_strong and __atomic_compare_exchange_weak.
> 
> I'm not sure what the point of the GCC version is.  The strong / weak option affects code generation, it's not something that can be selected at run time, so having a bool parameter just makes code less readable.
> 
> GCC's (documented) approach is to use the strong version if it cannot prove the 'weak' bool is true. I agree that this seems like a strange choice (and indeed in libstdc++ the 'weak' flag is always one of 0, 1, false, or true), but it's necessary for libstdc++ compatibility, so if we want such compatibility, our aesthetic preferences are somewhat irrelevant.

#define __atomic_compare_exchage(a,b,c,d,e) __atomic_compare_exchange_strong(a,b,d,e)

> The same objection also applies to the memory order parameter (where GCC always uses seq_cst if it can't determine the argument at build-time, and clang generates a branch).

The difference being that the C11 and C++11 specs make the memory order parameters explicit, but they make the strong / weak version part of the operation name.

> > 5) Clang provides an __atomic_init builtin, which gcc does not provide, and which crashes during CodeGen.
> 
> This is required for C11.  I'll take a look at the crash.  It's also used by libc++.  __atomic_init() initialises an _Atomic type without performing an atomic operation (as per both C++11 and C11).  Or, at least, it should...
> 
> I believe that libstdc++ gets around this by not _Atomic-qualifying the types and then using normal initialisation.  This is... problematic, to say the least, as an approach.
> 
> libc++ does not currently use __atomic_init; it uses a sequentially-consistent __atomic_store instead.

That's a bug then.  The C++11 spec explicitly states that initialisation is NOT atomic (as Howard pointed out to me when I tried to introduce this bug in libc++).

> I agree that that seems highly suboptimal, and that we should keep this builtin.
> > My current plan is:
> >
> > For (1), add support for the gcc-style arguments, then port libc++ over to them, then remove support for the current arguments.
> 
> Please don't.
> 
> Your objections thus far have not been strong enough, in my opinion, to justify abandoning libstdc++4.7 support for clang, nor to justify abandoning g++ support for libc++ (although naturally that will be Howard's choice). I'm not opposed to keeping the existing builtins too, as distasteful as it is to have two competing, and very similar, sets.

I'm not opposing supporting libstdc++, but I am not convinced that introducing a set of poorly designed intrinsics is the correct way of doing it, especially since we would be forced to support them long-term.

-- Sent from my Cray X1