<div class="gmail_quote">On Tue, Apr 10, 2012 at 2:25 PM, David Chisnall <span dir="ltr"><<a href="mailto:csdavec@swan.ac.uk">csdavec@swan.ac.uk</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 22:11, Richard Smith wrote:<br> > On Tue, Apr 10, 2012 at 12:58 PM, David Chisnall <<a href="mailto:theraven@sucs.org">theraven@sucs.org</a>> wrote:<br>
> 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>
> 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++.<br>

><br>
> I agree, but I think this is a problem of documentation. On that subject, do we have documentation for our current __atomic_ builtins somewhere?<br>
<br>
</div>The documentation is the C11 spec.  The builtins have a 1:1 correspondence with the C11 generic functions in stdatomic.h.<br></blockquote><div><br></div><div>The C11 spec does not talk about __atomic_*. A statement in our documentation saying that these builtins implement the <stdatomic.h> operations of the same name would be useful (especially since our builtins have the same name as, but different behaviour from, the GCC builtins).</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">> 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.</div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
><br>
> 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).<br>

<br>
</div>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.</blockquote>
<div><br></div><div>I assume you're suggesting that we cast the argument from T* to _Atomic(T*) prior to passing it into the builtin? That's ugly, but seems like it might be workable if _Atomic(T) always has the same size and alignment as T. I don't know if those properties hold, but I understand the intent of _Atomic() was that they need not. (We would also need to ensure TBAA knows that T and _Atomic(T) can alias.)</div>
<div><br></div><div>This approach, including the need to interpose in front of <atomic> and <shared_ptr> (and any other user of the atomics builtins in libstdc++) and detect whether we're about to include a header from libstdc++, seems sufficiently fragile that I'm still in favour of introducing GNU-compatible builtins.</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">
> > 3) Clang is missing the __atomic_*_n, __atomic_always_lock_free and __atomic_fetch_nand builtins.<br>
><br>
> The _n builtins actually do what the current clang ones do.<br>
><br>
> Functionally, yes, but the _Atomic check makes clang's implementations unusable for libstdc++, and the _n variants are<br>
> required to check that the argument type is a 1, 2, 4, 8 or 16-byte integer or pointer type.<br>
<br>
</div>The latter requirement is not required by either C++11 or C11.</blockquote><div><br></div><div>If we want to fully support the GNU-style atomics, we should implement them correctly. (Naturally, if we're using your compatibility-shim approach, we needn't worry about this.)</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:</div><div class="im">
> ><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>
> Please don't.<br>
><br>
> 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.<br>

<br>
</div>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.</blockquote>
<div><br></div><div>I share your concern. On the other hand, we have a long history of supporting gcc features, even when we think their design is questionable, for compatibility.</div><div><br></div><div>- Richard</div></div>