[google-4.6]Backport r183875 to fix incorrect increment/decrement of atomic pointers

[jingyu, 2012-07-19 22:11:34]

Backport r183875 from trunk and gcc-4.7 to fix PR51811 ([C++0x] Incorrect
increment/decrement of atomic pointers).

Tested:
1) --testers=crosstool.
2) unit test in Google ref b/6702865

OK for google-4_6 branch?

Thanks,
Jing


2012-07-19  Jing Yu  <jingyu@google.com>

	Backport r183875 to fix wrong atomic<_Tp*> add_fetch.
	PR libstdc++/51811, Google ref b/6702865
	* include/bits/atomic_0.h (atomic<_Tp*>): Fix offsets.
	* include/bits/atomic_2.h: Likewise.
	* testsuite/29_atomics/atomic/operators/51811.cc: New.
	* testsuite/29_atomics/atomic/operators/pointer_partial_void.cc: New.

Index: include/bits/atomic_0.h
===================================================================
--- include/bits/atomic_0.h	(revision 189589)
+++ include/bits/atomic_0.h	(working copy)
@@ -620,7 +620,7 @@ namespace __atomic0
       __return_pointer_type
       fetch_add(ptrdiff_t __d, memory_order __m = memory_order_seq_cst)
       {
-	void* __v = _ATOMIC_MODIFY_(this, +=, __d, __m);
+	void* __v = _ATOMIC_MODIFY_(this, +=, __d * sizeof(_PTp), __m);
 	return reinterpret_cast<__return_pointer_type>(__v);
       }
 
@@ -628,14 +628,14 @@ namespace __atomic0
       fetch_add(ptrdiff_t __d,
 		memory_order __m = memory_order_seq_cst) volatile
       {
-	void* __v = _ATOMIC_MODIFY_(this, +=, __d, __m);
+	void* __v = _ATOMIC_MODIFY_(this, +=, __d * sizeof(_PTp), __m);
 	return reinterpret_cast<__return_pointer_type>(__v);
       }
 
       __return_pointer_type
       fetch_sub(ptrdiff_t __d, memory_order __m = memory_order_seq_cst)
       {
-	void* __v = _ATOMIC_MODIFY_(this, -=, __d, __m);
+	void* __v = _ATOMIC_MODIFY_(this, -=, __d * sizeof(_PTp), __m);
 	return reinterpret_cast<__return_pointer_type>(__v);
       }
 
@@ -643,7 +643,7 @@ namespace __atomic0
       fetch_sub(ptrdiff_t __d,
 		memory_order __m = memory_order_seq_cst) volatile
       {
-	void* __v = _ATOMIC_MODIFY_(this, -=, __d, __m);
+	void* __v = _ATOMIC_MODIFY_(this, -=, __d * sizeof(_PTp), __m);
 	return reinterpret_cast<__return_pointer_type>(__v);
       }
     };
Index: include/bits/atomic_2.h
===================================================================
--- include/bits/atomic_2.h	(revision 189589)
+++ include/bits/atomic_2.h	(working copy)
@@ -644,21 +644,21 @@ namespace __atomic2
 
       __pointer_type
       fetch_add(ptrdiff_t __d, memory_order __m = memory_order_seq_cst)
-      { return __sync_fetch_and_add(&_M_p, __d); }
+      { return __sync_fetch_and_add(&_M_p, __d * sizeof(_PTp)); }
 
       __pointer_type
       fetch_add(ptrdiff_t __d,
 		memory_order __m = memory_order_seq_cst) volatile
-      { return __sync_fetch_and_add(&_M_p, __d); }
+      { return __sync_fetch_and_add(&_M_p, __d * sizeof(_PTp)); }
 
       __pointer_type
       fetch_sub(ptrdiff_t __d, memory_order __m = memory_order_seq_cst)
-      { return __sync_fetch_and_sub(&_M_p, __d); }
+      { return __sync_fetch_and_sub(&_M_p, __d * sizeof(_PTp)); }
 
       __pointer_type
       fetch_sub(ptrdiff_t __d,
 		memory_order __m = memory_order_seq_cst) volatile
-      { return __sync_fetch_and_sub(&_M_p, __d); }
+      { return __sync_fetch_and_sub(&_M_p, __d * sizeof(_PTp)); }
     };
 
 } // namespace __atomic2
Index: testsuite/29_atomics/atomic/operators/pointer_partial_void.cc
===================================================================
--- testsuite/29_atomics/atomic/operators/pointer_partial_void.cc	(revision 0)
+++ testsuite/29_atomics/atomic/operators/pointer_partial_void.cc	(revision 0)
@@ -0,0 +1,71 @@
+// { dg-require-atomic-builtins "" }
+// { dg-options "-std=gnu++0x" }
+
+// Copyright (C) 2012 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>
+#include <cstdlib> //std::abs
+#include <testsuite_hooks.h>
+
+// pointer arithimetic vs. atomic<void*>.
+// atomic<void*> vs. explicitly specialized w/o operators, like atomic_bool?
+int main(void)
+{
+  // bool test __attribute__((unused)) = true;
+
+  using namespace std;
+
+  typedef int 	value_type;
+  const size_t n = 2;
+  value_type value = 42;
+  value_type* p = &value;
+  void* vp = p;
+  ptrdiff_t dist(0);
+
+  atomic<void*> a(vp);
+
+  // operator++
+  void* vp2(a);
+  a++;
+  void* vp3(a);
+  dist = reinterpret_cast<char*>(vp2) - reinterpret_cast<char*>(vp3);
+  // VERIFY ( std::abs(dist) == sizeof(void*));
+
+  // operator--
+  void* vp4(a);
+  a--;
+  void* vp5(a);
+  dist = reinterpret_cast<char*>(vp4) - reinterpret_cast<char*>(vp5);
+  // VERIFY ( std::abs(dist) == sizeof(void*));
+
+  // operator+=
+  void* vp6(a);
+  a+=n;
+  void* vp7(a);
+  dist = reinterpret_cast<char*>(vp6) - reinterpret_cast<char*>(vp7);
+  // VERIFY ( std::abs(dist) == sizeof(void*) * n);
+
+  // operator-=
+  void* vp8(a);
+  a-=n;
+  void* vp9(a);
+  dist = reinterpret_cast<char*>(vp8) - reinterpret_cast<char*>(vp9);
+  //VERIFY ( std::abs(dist) == sizeof(void*) * n);
+
+  return 0;
+}
Index: testsuite/29_atomics/atomic/operators/51811.cc
===================================================================
--- testsuite/29_atomics/atomic/operators/51811.cc	(revision 0)
+++ testsuite/29_atomics/atomic/operators/51811.cc	(revision 0)
@@ -0,0 +1,90 @@
+// { dg-options "-std=gnu++0x" }
+
+// Copyright (C) 2012 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>
+#include <cstdlib> //std::abs
+#include <testsuite_hooks.h>
+
+// libstdc++/51811
+// pointer arithimetic vs. atomic<_Tp*> specialization
+int main(void)
+{
+  bool test __attribute__((unused)) = true;
+
+  using namespace std;
+
+  typedef int 	value_type;
+  const size_t n = 2;
+  value_type value = 42;
+  atomic<value_type*> p, p2, p3;
+
+  // operator++
+  {
+    p = &value;
+    p2 = p++;
+    VERIFY (p != p2);
+    
+    value_type* vp(p);
+    value_type* vp2(p2);
+    ptrdiff_t dist = reinterpret_cast<char*>(vp) -
reinterpret_cast<char*>(vp2);
+    VERIFY ( std::abs(dist) == sizeof(value_type));
+  
+    p = &value;
+    p3 = ++p;
+    VERIFY (p == p3);
+  }
+
+  // operator--
+  {
+    p = &value;
+    p2 = p--;
+    VERIFY (p != p2);
+
+    value_type* vp(p);
+    value_type* vp2(p2);
+    ptrdiff_t dist = reinterpret_cast<char*>(vp) -
reinterpret_cast<char*>(vp2);
+    VERIFY ( std::abs(dist) == sizeof(value_type));
+
+    p = &value;
+    p3 = --p;
+    VERIFY (p == p3);
+  }
+
+  // operator+=
+  {
+    p = &value;
+    value_type* vp(p);
+    p+=n;
+    value_type* vp2(p);
+    ptrdiff_t dist = reinterpret_cast<char*>(vp) -
reinterpret_cast<char*>(vp2);
+    VERIFY ( std::abs(dist) == sizeof(value_type) * n);
+  }
+
+  // operator-=
+  {
+    p = &value;
+    value_type* vp(p);
+    p-=n;
+    value_type* vp2(p);
+    ptrdiff_t dist = reinterpret_cast<char*>(vp) -
reinterpret_cast<char*>(vp2);
+    VERIFY ( std::abs(dist) == sizeof(value_type) * n);
+  }
+
+  return 0;
+}

--
This patch is available for review at http://codereview.appspot.com/6428056

[dougkwan, 2012-07-19 22:29:32]

This seems to be different from r183875.  Are the parts chaing is_look_free() in
r183875 necessary? If not why?

[jingyu, 2012-07-19 22:48:14]

It is not a straightforward backport.
<atomic> has changed a lot in gcc-4.7. is_lock_free() body is entirely
different between gcc-4.6 and r183875. In gcc-4.6, is_lock_free()
simply returns false or true. Notice that gcc-4.6 defines two
namesapce __atomic0, __atomic2 in separate files (atomic_0.h,
atomic_2.h), which disappear in gcc-4.7.

Even though, the idea behind the bug is the same. The new unit tests
will fail without this patch.

Thanks,
Jing

Even though, the bug is the same. the new unit tests will

On Thu, Jul 19, 2012 at 3:29 PM,  <dougkwan@google.com> wrote:
> This seems to be different from r183875.  Are the parts chaing
> is_look_free() in r183875 necessary? If not why?
>
> http://codereview.appspot.com/6428056/

[dougkwan, 2012-07-19 23:55:07]

LGTM.