| Message ID | 87zitsqbho.fsf@kepler.schwinge.homeip.net |
|---|---|
| State | New |
| Headers | show |
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 ... 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. 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. >That said, I have not looked into why the libstdc++-v3 configure script >checks short, int, and long long atomics, but not long. But then, only >the short and int results are used to decide on _GLIBCXX_ATOMIC_BUILTINS, >and on the other hand there actually are "typedef long _Atomic_word" >definitions, but long atomics are not explicitly checked for. Ugh. Those checks have a long and messy history. If we're now only using _GLIBCXX_ATOMIC_BUILTINS to decide how to operate on _Atomic_word then it should really only be testing whether atomics are supported for that type. I'm not brave enough to change that now though.
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.) > 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 (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. ;-) > >That said, I have not looked into why the libstdc++-v3 configure script > >checks short, int, and long long atomics, but not long. But then, only > >the short and int results are used to decide on _GLIBCXX_ATOMIC_BUILTINS, > >and on the other hand there actually are "typedef long _Atomic_word" > >definitions, but long atomics are not explicitly checked for. > > Ugh. Those checks have a long and messy history. > > If we're now only using _GLIBCXX_ATOMIC_BUILTINS to decide how to > operate on _Atomic_word then it should really only be testing whether > atomics are supported for that type. I'm not brave enough to change > that now though. "Don't shoot the messenger", who quickly runs away to stay out of this mess. ;-) Grüße Thomas
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.
On 05/04/16 12:01 +0100, Jonathan Wakely wrote: >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. This is now https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70554
diff --git libstdc++-v3/acinclude.m4 libstdc++-v3/acinclude.m4 index 95df24a..145d0f9 100644 --- libstdc++-v3/acinclude.m4 +++ libstdc++-v3/acinclude.m4 @@ -3282,25 +3282,6 @@ AC_DEFUN([GLIBCXX_ENABLE_ATOMIC_BUILTINS], [ CXXFLAGS="$CXXFLAGS -fno-exceptions" - AC_MSG_CHECKING([for atomic builtins for bool]) - AC_CACHE_VAL(glibcxx_cv_atomic_bool, [ - AC_TRY_LINK( - [ ], - [typedef bool atomic_type; - atomic_type c1; - atomic_type c2; - atomic_type c3(0); - __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); - __atomic_compare_exchange_n(&c1, &c2, c3, true, __ATOMIC_ACQ_REL, - __ATOMIC_RELAXED); - __atomic_test_and_set(&c1, __ATOMIC_RELAXED); - __atomic_load_n(&c1, __ATOMIC_RELAXED); - ], - [glibcxx_cv_atomic_bool=yes], - [glibcxx_cv_atomic_bool=no]) - ]) - AC_MSG_RESULT($glibcxx_cv_atomic_bool) - AC_MSG_CHECKING([for atomic builtins for short]) AC_CACHE_VAL(glibcxx_cv_atomic_short, [ AC_TRY_LINK( @@ -3371,35 +3352,6 @@ AC_DEFUN([GLIBCXX_ENABLE_ATOMIC_BUILTINS], [ [#]line __oline__ "configure" int main() { - typedef bool atomic_type; - atomic_type c1; - atomic_type c2; - atomic_type c3(0); - __atomic_fetch_add(&c1, c2, __ATOMIC_RELAXED); - __atomic_compare_exchange_n(&c1, &c2, c3, true, __ATOMIC_ACQ_REL, - __ATOMIC_RELAXED); - __atomic_test_and_set(&c1, __ATOMIC_RELAXED); - __atomic_load_n(&c1, __ATOMIC_RELAXED); - - return 0; -} -EOF - - AC_MSG_CHECKING([for atomic builtins for bool]) - if AC_TRY_EVAL(ac_compile); then - if grep __atomic_ conftest.s >/dev/null 2>&1 ; then - glibcxx_cv_atomic_bool=no - else - glibcxx_cv_atomic_bool=yes - fi - fi - AC_MSG_RESULT($glibcxx_cv_atomic_bool) - rm -f conftest* - - cat > conftest.$ac_ext << EOF -[#]line __oline__ "configure" -int main() -{ typedef short atomic_type; atomic_type c1; atomic_type c2; @@ -3490,8 +3442,7 @@ EOF AC_LANG_RESTORE # Set atomicity_dir to builtins if all but the long long test above passes. - if test "$glibcxx_cv_atomic_bool" = yes \ - && test "$glibcxx_cv_atomic_short" = yes \ + if test "$glibcxx_cv_atomic_short" = yes \ && test "$glibcxx_cv_atomic_int" = yes; then AC_DEFINE(_GLIBCXX_ATOMIC_BUILTINS, 1, [Define if the compiler supports C++11 atomics.])