| Message ID | CAH6eHdQvPmb=p9FR1xVEmAtCiKRQZ5yzX3BHyHKqgH=TNHpRkw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
2013/1/17 Jonathan Wakely <jwakely.gcc@gmail.com>: > This fixes a regression since 4.6 when -Wsystem-headers is used. The > initialization of the __atomic_flag_base base class has a narrowing > conversion from int (the macro) to either bool or unsigned char. The > patch fixes it by calling a constexpr function which implicitly > converts the value to the return type instead of doing the conversion > inside a braced-init-list. Doing that requires naming the return > type, so I defined a new typedef for to avoid duplicating the > preprocessor conditional. The patch also adds a missing assignment > operator in atomic<bool>. > > PR libstdc++/56012 > * include/bits/atomic_base.h (atomic_flag): Fix narrowing conversion. > * testsuite/29_atomics/atomic/operators/56012.cc: New. > > PR libstdc++/56011 > * include/std/atomic (atomic<bool>::operator=(bool) volatile): Add > missing overload. > * testsuite/29_atomics/atomic/operators/56011.cc: New. > > Tested x86_64-linux, it's a regression so I want to commit it to the > trunk and 4.7 branch, any objections from the atomics experts? Isn't here a typedef missing: + /* The target's "set" value for test-and-set may not be exactly 1. */ +#if __GCC_ATOMIC_TEST_AND_SET_TRUEVAL == 1 + typedef bool __atomic_flag_data_type; +#else + unsigned char __atomic_flag_data_type; +#endif I would expect that this looked like: + /* The target's "set" value for test-and-set may not be exactly 1. */ +#if __GCC_ATOMIC_TEST_AND_SET_TRUEVAL == 1 + typedef bool __atomic_flag_data_type; +#else + typedef unsigned char __atomic_flag_data_type; +#endif - Daniel
On 17 January 2013 07:53, Daniel Krügler <daniel.kruegler@gmail.com> wrote: > 2013/1/17 Jonathan Wakely <jwakely.gcc@gmail.com>: >> This fixes a regression since 4.6 when -Wsystem-headers is used. The >> initialization of the __atomic_flag_base base class has a narrowing >> conversion from int (the macro) to either bool or unsigned char. The >> patch fixes it by calling a constexpr function which implicitly >> converts the value to the return type instead of doing the conversion >> inside a braced-init-list. Doing that requires naming the return >> type, so I defined a new typedef for to avoid duplicating the >> preprocessor conditional. The patch also adds a missing assignment >> operator in atomic<bool>. >> >> PR libstdc++/56012 >> * include/bits/atomic_base.h (atomic_flag): Fix narrowing conversion. >> * testsuite/29_atomics/atomic/operators/56012.cc: New. >> >> PR libstdc++/56011 >> * include/std/atomic (atomic<bool>::operator=(bool) volatile): Add >> missing overload. >> * testsuite/29_atomics/atomic/operators/56011.cc: New. >> >> Tested x86_64-linux, it's a regression so I want to commit it to the >> trunk and 4.7 branch, any objections from the atomics experts? > > Isn't here a typedef missing: > > + /* The target's "set" value for test-and-set may not be exactly 1. */ > +#if __GCC_ATOMIC_TEST_AND_SET_TRUEVAL == 1 > + typedef bool __atomic_flag_data_type; > +#else > + unsigned char __atomic_flag_data_type; > +#endif Gah! quite right, thanks. I'll try to find a target to test it on where the set value isn't 1, just to be sure it works.
diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h index 8ce5553..959f524 100644 --- a/libstdc++-v3/include/bits/atomic_base.h +++ b/libstdc++-v3/include/bits/atomic_base.h @@ -1,6 +1,6 @@ // -*- C++ -*- header. -// Copyright (C) 2008-2012 Free Software Foundation, Inc. +// Copyright (C) 2008-2013 Free Software Foundation, Inc. // // This file is part of the GNU ISO C++ Library. This library is free // software; you can redistribute it and/or modify it under the @@ -212,6 +212,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template<typename _Tp> struct atomic<_Tp*>; + /* The target's "set" value for test-and-set may not be exactly 1. */ +#if __GCC_ATOMIC_TEST_AND_SET_TRUEVAL == 1 + typedef bool __atomic_flag_data_type; +#else + unsigned char __atomic_flag_data_type; +#endif /** * @brief Base type for atomic_flag. @@ -227,12 +233,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION struct __atomic_flag_base { - /* The target's "set" value for test-and-set may not be exactly 1. */ -#if __GCC_ATOMIC_TEST_AND_SET_TRUEVAL == 1 - bool _M_i; -#else - unsigned char _M_i; -#endif + __atomic_flag_base_type _M_i; }; _GLIBCXX_END_EXTERN_C @@ -250,7 +251,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // Conversion to ATOMIC_FLAG_INIT. constexpr atomic_flag(bool __i) noexcept - : __atomic_flag_base({ __i ? __GCC_ATOMIC_TEST_AND_SET_TRUEVAL : 0 }) + : __atomic_flag_base{ _S_init(__i) } { } bool @@ -284,6 +285,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __atomic_clear (&_M_i, __m); } + + private: + static constexpr __atomic_flag_data_type + _S_init(bool __i) + { return __i ? __GCC_ATOMIC_TEST_AND_SET_TRUEVAL : 0; } }; diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic index 4012f7d..7cc5c89 100644 --- a/libstdc++-v3/include/std/atomic +++ b/libstdc++-v3/include/std/atomic @@ -69,6 +69,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION operator=(bool __i) noexcept { return _M_base.operator=(__i); } + bool + operator=(bool __i) volatile noexcept + { return _M_base.operator=(__i); } + operator bool() const noexcept { return _M_base.load(); } diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/operators/56011.cc b/libstdc++-v3/testsuite/29_atomics/atomic/operators/56011.cc new file mode 100644 index 0000000..1d77a55 --- /dev/null +++ b/libstdc++-v3/testsuite/29_atomics/atomic/operators/56011.cc @@ -0,0 +1,29 @@ +// { dg-options "-std=gnu++0x" } +// { dg-do compile } + +// Copyright (C) 2013 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +#include <atomic> +void test01() +{ + using namespace std; + volatile atomic<bool> ab1 __attribute__((unused)); + ab1 = true; + volatile atomic_bool ab2 __attribute__((unused)); + ab2 = true; +} diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/56012.cc b/libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/56012.cc new file mode 100644 index 0000000..73da933 --- /dev/null +++ b/libstdc++-v3/testsuite/29_atomics/atomic_flag/cons/56012.cc @@ -0,0 +1,26 @@ +// { dg-options "-std=gnu++0x -Wsystem-headers -Wnarrowing" } +// { dg-do compile } + +// Copyright (C) 2008-2013 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +#include <atomic> +void test01() +{ + using namespace std; + atomic_flag af __attribute__((unused)) = ATOMIC_FLAG_INIT; +}
This fixes a regression since 4.6 when -Wsystem-headers is used. The initialization of the __atomic_flag_base base class has a narrowing conversion from int (the macro) to either bool or unsigned char. The patch fixes it by calling a constexpr function which implicitly converts the value to the return type instead of doing the conversion inside a braced-init-list. Doing that requires naming the return type, so I defined a new typedef for to avoid duplicating the preprocessor conditional. The patch also adds a missing assignment operator in atomic<bool>. PR libstdc++/56012 * include/bits/atomic_base.h (atomic_flag): Fix narrowing conversion. * testsuite/29_atomics/atomic/operators/56012.cc: New. PR libstdc++/56011 * include/std/atomic (atomic<bool>::operator=(bool) volatile): Add missing overload. * testsuite/29_atomics/atomic/operators/56011.cc: New. Tested x86_64-linux, it's a regression so I want to commit it to the trunk and 4.7 branch, any objections from the atomics experts? commit e32b21316534398d2b45d4b1b4c06bb12ec17e84 Author: Jonathan Wakely <jwakely.gcc@gmail.com> Date: Wed Jan 16 23:54:37 2013 +0000 PR libstdc++/56012 * include/bits/atomic_base.h (atomic_flag): Fix narrowing conversion. * testsuite/29_atomics/atomic/operators/56012.cc: New. PR libstdc++/56011 * include/std/atomic (atomic<bool>::operator=(bool) volatile): Add missing overload. * testsuite/29_atomics/atomic/operators/56011.cc: New.