<div class="gmail_quote">On Tue, Apr 10, 2012 at 12:58 PM, David Chisnall <span dir="ltr"><<a href="mailto:theraven@sucs.org">theraven@sucs.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On 10 Apr 2012, at 20:22, Richard Smith wrote:<br>
> Hi,<br>
><br>
> 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:<br>

<br>
</div>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++.<div class="im">
</div></blockquote><div><br></div><div>I agree, but I think this is a problem of documentation. On that subject, do we have documentation for our current __atomic_ builtins somewhere?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> 1) In clang, the builtins produce results by value, whereas in gcc, a pointer is passed indicating where the result should be written.<br>
<br>
</div>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.<br>

<div class="im"><br>
> 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).<br>
<br>
</div>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.<br>
<br></blockquote><div>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).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.</blockquote><div><br></div><div>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).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> 3) Clang is missing the __atomic_*_n, __atomic_always_lock_free and __atomic_fetch_nand builtins.<br>
<br>
</div>The _n builtins actually do what the current clang ones do.</blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">__atomic_always_lock_free is probably useful.  __atomic_fetch_nand might be as well, although it isn't needed by C++11 or C11.</blockquote>
<div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> 4) Clang is missing the __atomic_compare_exchange builtin, and instead provides __atomic_compare_exchange_strong and __atomic_compare_exchange_weak.<br>
<br>
</div>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.</blockquote>
<div><br></div><div>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.</div>
<div><br></div><div>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).</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> 5) Clang provides an __atomic_init builtin, which gcc does not provide, and which crashes during CodeGen.<br>
<br>
</div>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...<br>

<br>
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.</blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> My current plan is:<br>
><br>
> For (1), add support for the gcc-style arguments, then port libc++ over to them, then remove support for the current arguments.<br>
<br>
</div>Please don't.</blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> For (2), relax the requirement to the type being either _Atomic or trivially copyable.<br>
<br>
</div>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.</blockquote>
<div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> 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.<br>

<br>
</div>As with the other clang builtins, they make stdatomic.h much easier to implement.<div class="im"></div></blockquote><div>[...]</div><div><br></div><div>As I said, I don't intend to remove the strong/weak variants.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> For (5), remove this builtin.<br>
<br>
</div>Again, please don't, it is required for C11 and makes achieving the correct semantics for C++11 much easier.<br></blockquote></div><br><div>Agreed.</div><div><br></div><div>Many thanks for your feedback!</div><div>
Richard</div>