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

Tue Apr 10 14:11:20 PDT 2012

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?

> 1) In clang, the builtins produce results by value, whereas in gcc, a
> pointer is passed indicating where the result should be written.
>
> This means that the things required by C11's stdatomic.h can be trivially
> implemented as macros (just dropping the __ prefix) with the clang
> builtins.  They can not with the GCC ones in their standard form, although
> the _n versions can be used this way.  I am not sure what the perceived
> advantage of the GCC non-_n ones is.
>
> > 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) 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).

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

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

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

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

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

> For (2), relax the requirement to the type being either _Atomic or
> trivially copyable.
>
> I think this would be a serious mistake.  Allowing atomic operations on
> non-_Atomic types in the intrinsics makes it insanely hard to correctly
> implement C11 stdatomic.h and will cause all sorts of problems for users of
> these builtins later.

Given the above approach to (1), I think the best plan here would be to
leave the C11 intrinsics alone and require the argument for the GNU
intrinsics to be trivially-copyable. Thus the GNU intrinsics would not work
with _Atomic types, and the _Atomic intrinsics would not work with
non-_Atomic types.

> For (3) and (4), add the missing builtins. I don't intend to remove the
> __atomic_compare_exchange_strong/__atomic_compare_exchange_weak builtins,
> but will do so if anyone particularly wants to see them gone.
>
> As with the other clang builtins, they make stdatomic.h much easier to
> implement.
>
[...]

As I said, I don't intend to remove the strong/weak variants.

> > For (5), remove this builtin.
>
> Again, please don't, it is required for C11 and makes achieving the
> correct semantics for C++11 much easier.
>

Agreed.

Many thanks for your feedback!
Richard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120410/f456f7e5/attachment.html>