Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3

[Jonathan Wakely <jwakely@redhat.com>]

On 21/03/16 17:01 +0100, Thomas Schwinge wrote:
>Hi!
>
>On Mon, 21 Mar 2016 15:01:49 +0000, Jonathan Wakely <jwakely@redhat.com> wrote:
>> On 21/03/16 13:08 +0100, Thomas Schwinge wrote:
>> >Per my (admittedly, not in-depth) reading of libstdc++-v3 source code,
>> >the _GLIBCXX_ATOMIC_BUILTINS conditional is only used in combination with
>> >the _Atomic_word data type, which in
>> >libstdc++-v3/doc/xml/manual/concurrency_extensions.xml is described as "a
>> >signed integral type" (so, matching the semantics as clarified by your
>> >patch).  That makes sense: it's used to keep reference counts, for
>> >example.  So, it seems sound to just remove the bool atomics check.
>>
>> I agree that it doesn't make any sense to check whether atomics work
>> for bool when we only care about them for _Atomic_word, however ...
>
>(Please review that it really is used only for that; I have only done a
>quick scan of the libstdc++-v3 sources.)

My own checking agreed.

>> This would change the value of _GLIBCXX_ATOMIC_BUILTINS for any target
>> which was already failing the check for bool but passing it for the
>> other types. We would now switch to using atomic builtins where we
>> previously didn't use them, which could be a problem. I don't know if
>> there are any targets that would be affected, and if it would cause an
>> actual problem.
>
>Assuming there are no other reasons that could have caused the bool
>atomics checks to fail

A target without 1-byte atomics might fail the bool checks, but pass
the int and short ones.

>(under the condition that the short and int ones
>did and still do succeed), my patch just restores the state of a few
>months ago, before Martin's bool atomics warning patch got committed.
>So, I think it is safe to commit.
>
>> Would leaving the bool check in place, but just removing the
>> __atomic_fetch_add() part be better? It should still fix the
>> regression, but is less likely to change behaviour for targets that
>> were never using the builtins.
>
>Yes, we could do that, but while I have not verified this, I assume that
>it's very unlikely that there exists a configuration where the bool
>atomics checks already used to fail but the short and int ones did and
>still do succeed.  Anyway, that's not my decision to make.  ;-)

Well I guess it's mine, and this is a fairly serious regression (is it
tracked in Bugzilla anywhere?) so the patch is OK for trunk.