This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3
- From: Jonathan Wakely <jwakely at redhat dot com>
- To: Thomas Schwinge <thomas at codesourcery dot com>
- Cc: Martin Sebor <msebor at gmail dot com>, libstdc++ at gcc dot gnu dot org, gcc-patches at gcc dot gnu dot org, Marek Polacek <polacek at redhat dot com>, "Joseph S. Myers" <joseph at codesourcery dot com>, Jeff Law <law at redhat dot com>
- Date: Tue, 5 Apr 2016 12:01:48 +0100
- Subject: Re: [PR c/68966] Restore atomic builtins usage in libstdc++-v3
- Authentication-results: sourceware.org; auth=none
- References: <567A271B dot 2090403 at gmail dot com> <87zitsqbho dot fsf at kepler dot schwinge dot homeip dot net> <20160321150149 dot GK3805 at redhat dot com> <87zitrbz0o dot fsf at hertz dot schwinge dot homeip dot net>
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.