Patchwork fix libstdc++/56012 - narrowing conversion in std::atomic_flag

login
register
mail settings
Submitter Jonathan Wakely
Date Jan. 17, 2013, 12:21 a.m.
Message ID <CAH6eHdQvPmb=p9FR1xVEmAtCiKRQZ5yzX3BHyHKqgH=TNHpRkw@mail.gmail.com>
Download mbox | patch
Permalink /patch/213111/
State New
Headers show

Comments

Jonathan Wakely - Jan. 17, 2013, 12:21 a.m.
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.
Daniel Krügler - Jan. 17, 2013, 7:53 a.m.
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
Jonathan Wakely - Jan. 17, 2013, 8:22 a.m.
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.

Patch

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;
+}