diff mbox

Fix vector C++11 allocator bug

Message ID 51390699.9040904@gmail.com
State New
Headers show

Commit Message

François Dumont March 7, 2013, 9:28 p.m. UTC 
Hi

     While working on unordered containers C++11 allocator integration I 
used forward_list tests you have done Jon. It reported some problems 
that should have been seen on forward_list or vector allocator tests too 
if those tests were indeed manipulating memory. But there weren't 
because no element was ever injected in the containers.

     So I have started injecting elements and the propagating_allocator 
issue shown up like in my unordered container tests. The problem is that 
there is an assertion in the allocator to check that memory is returned 
to the correct allocator instance thanks to the personality. But when 
forward_list or vector instances with non propagating allocators are 
swapped personnality is not swapped and the issue. So I have introduced 
a bool template parameter to disable all assertions regarding 
personality so that the allocator can still be used to check that 
personality hasn't been exchanged.

     Additional, making the vector tests more functional revealed a bug 
in vector implementation.

2013-03-08  François Dumont  <fdumont@gcc.gnu.org>

     * include/bits/vector.tcc (vector<>operator=(const vector<>&):
     Reset pointers after deallocation when memory can't be reused.
     * testsuite/util/testsuite_allocator.h (uneq_allocator<>): Add
     IgnorePerson template parameter to disable assertions regarding
     allocator personality.
     * testsuite/23_containers/vector/allocator/minimal.cc: Insert
     element to really challenge C++11 allocator integration.
     * testsuite/23_containers/vector/allocator/copy.cc: Likewise.
     * testsuite/23_containers/vector/allocator/copy_assign.cc:
     Likewise.
     * testsuite/23_containers/vector/allocator/move_assign.cc:
     Likewise.
     * testsuite/23_containers/vector/allocator/swap.cc: Disable
     assertions regarding allocator personality when swapping vectors
     with not propagating allocators.
     * testsuite/23_containers/forward_list/allocator/minimal.cc:
     Insert element to really challenge C++11 allocator integration.
     * testsuite/23_containers/forward_list/allocator/copy.cc:
     Likewise.
     * testsuite/23_containers/forward_list/allocator/copy_assign.cc:
     Likewise.
     * testsuite/23_containers/forward_list/allocator/move_assign.cc:
     Likewise.
     * testsuite/23_containers/forward_list/allocator/swap.cc: Disable
     assertions regarding allocator personality when swapping vectors
     with not propagating allocators.


Tested under Linux x86_64.

Ok to commit ?

François
diff mbox

Patch

Index: include/bits/vector.tcc
===================================================================
--- include/bits/vector.tcc	(revision 196526)
+++ include/bits/vector.tcc	(working copy)
@@ -173,6 +173,9 @@ 
 		  _M_deallocate(this->_M_impl._M_start,
 				this->_M_impl._M_end_of_storage
 				- this->_M_impl._M_start);
+		  this->_M_impl._M_start = nullptr;
+		  this->_M_impl._M_finish = nullptr;
+		  this->_M_impl._M_end_of_storage = nullptr;
 		}
 	      std::__alloc_on_copy(_M_get_Tp_allocator(),
 				   __x._M_get_Tp_allocator());
Index: testsuite/23_containers/vector/allocator/minimal.cc
===================================================================
--- testsuite/23_containers/vector/allocator/minimal.cc	(revision 196526)
+++ testsuite/23_containers/vector/allocator/minimal.cc	(working copy)
@@ -35,6 +35,7 @@ 
   typedef std::allocator_traits<alloc_type> traits_type;
   typedef std::vector<T, alloc_type> test_type;
   test_type v(alloc_type{});
+  v.push_back(T());
   VERIFY( v.max_size() == traits_type::max_size(v.get_allocator()) );
 }
 
Index: testsuite/23_containers/vector/allocator/copy.cc
===================================================================
--- testsuite/23_containers/vector/allocator/copy.cc	(revision 196526)
+++ testsuite/23_containers/vector/allocator/copy.cc	(working copy)
@@ -31,6 +31,7 @@ 
   typedef propagating_allocator<T, false> alloc_type;
   typedef std::vector<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_back(T());
   test_type v2(v1);
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(0 == v2.get_allocator().get_personality());
@@ -42,6 +43,7 @@ 
   typedef propagating_allocator<T, true> alloc_type;
   typedef std::vector<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_back(T());
   test_type v2(v1);
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(1 == v2.get_allocator().get_personality());
Index: testsuite/23_containers/vector/allocator/move_assign.cc
===================================================================
--- testsuite/23_containers/vector/allocator/move_assign.cc	(revision 196526)
+++ testsuite/23_containers/vector/allocator/move_assign.cc	(working copy)
@@ -31,7 +31,9 @@ 
   typedef propagating_allocator<T, false> alloc_type;
   typedef std::vector<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_back(T());
   test_type v2(alloc_type(2));
+  v2.push_back(T());
   v2 = std::move(v1);
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(2 == v2.get_allocator().get_personality());
@@ -43,8 +45,10 @@ 
   typedef propagating_allocator<T, true> alloc_type;
   typedef std::vector<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_back(T());
   test_type v2(alloc_type(2));
   v2 = std::move(v1);
+  v2.push_back(T());
   VERIFY(0 == v1.get_allocator().get_personality());
   VERIFY(1 == v2.get_allocator().get_personality());
 }
Index: testsuite/23_containers/vector/allocator/swap.cc
===================================================================
--- testsuite/23_containers/vector/allocator/swap.cc	(revision 196526)
+++ testsuite/23_containers/vector/allocator/swap.cc	(working copy)
@@ -25,30 +25,19 @@ 
 
 using __gnu_test::propagating_allocator;
 
-// It is undefined behaviour to swap() containers wth unequal allocators
-// if the allocator doesn't propagate, so ensure the allocators compare
-// equal, while still being able to test propagation via get_personality().
-bool
-operator==(const propagating_allocator<T, false>&,
-           const propagating_allocator<T, false>&)
-{
-  return true;
-}
-
-bool
-operator!=(const propagating_allocator<T, false>&,
-           const propagating_allocator<T, false>&)
-{
-  return false;
-}
-
 void test01()
 {
   bool test __attribute__((unused)) = true;
-  typedef propagating_allocator<T, false> alloc_type;
+  // It is undefined behaviour to swap() containers wth unequal allocators
+  // if the allocator doesn't propagate, so request allocator to ignore
+  // personality to have equivalent allocators, while still being able to test
+  // propagation via get_personality().
+  typedef propagating_allocator<T, false, true> alloc_type;
   typedef std::vector<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_back(T());
   test_type v2(alloc_type(2));
+  v2.push_back(T());
   std::swap(v1, v2);
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(2 == v2.get_allocator().get_personality());
@@ -60,7 +49,9 @@ 
   typedef propagating_allocator<T, true> alloc_type;
   typedef std::vector<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_back(T());
   test_type v2(alloc_type(2));
+  v2.push_back(T());
   std::swap(v1, v2);
   VERIFY(2 == v1.get_allocator().get_personality());
   VERIFY(1 == v2.get_allocator().get_personality());
Index: testsuite/23_containers/vector/allocator/copy_assign.cc
===================================================================
--- testsuite/23_containers/vector/allocator/copy_assign.cc	(revision 196526)
+++ testsuite/23_containers/vector/allocator/copy_assign.cc	(working copy)
@@ -31,7 +31,9 @@ 
   typedef propagating_allocator<T, false> alloc_type;
   typedef std::vector<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_back(T());
   test_type v2(alloc_type(2));
+  v2.push_back(T());
   v2 = v1;
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(2 == v2.get_allocator().get_personality());
@@ -43,7 +45,9 @@ 
   typedef propagating_allocator<T, true> alloc_type;
   typedef std::vector<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_back(T());
   test_type v2(alloc_type(2));
+  v2.push_back(T());
   v2 = v1;
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(1 == v2.get_allocator().get_personality());
Index: testsuite/23_containers/forward_list/allocator/minimal.cc
===================================================================
--- testsuite/23_containers/forward_list/allocator/minimal.cc	(revision 196526)
+++ testsuite/23_containers/forward_list/allocator/minimal.cc	(working copy)
@@ -37,6 +37,7 @@ 
   typedef std::allocator_traits<alloc_type> traits_type;
   typedef std::forward_list<T, alloc_type> test_type;
   test_type v(alloc_type{});
+  v.push_front(T());
   VERIFY( v.max_size() == traits_type::max_size(v.get_allocator()) );
 }
 
Index: testsuite/23_containers/forward_list/allocator/copy.cc
===================================================================
--- testsuite/23_containers/forward_list/allocator/copy.cc	(revision 196526)
+++ testsuite/23_containers/forward_list/allocator/copy.cc	(working copy)
@@ -31,6 +31,7 @@ 
   typedef propagating_allocator<T, false> alloc_type;
   typedef std::forward_list<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_front(T());
   test_type v2(v1);
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(0 == v2.get_allocator().get_personality());
@@ -42,6 +43,7 @@ 
   typedef propagating_allocator<T, true> alloc_type;
   typedef std::forward_list<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_front(T());
   test_type v2(v1);
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(1 == v2.get_allocator().get_personality());
Index: testsuite/23_containers/forward_list/allocator/move_assign.cc
===================================================================
--- testsuite/23_containers/forward_list/allocator/move_assign.cc	(revision 196526)
+++ testsuite/23_containers/forward_list/allocator/move_assign.cc	(working copy)
@@ -31,7 +31,9 @@ 
   typedef propagating_allocator<T, false> alloc_type;
   typedef std::forward_list<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_front(T());
   test_type v2(alloc_type(2));
+  v2.push_front(T());
   v2 = std::move(v1);
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(2 == v2.get_allocator().get_personality());
@@ -43,7 +45,9 @@ 
   typedef propagating_allocator<T, true> alloc_type;
   typedef std::forward_list<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_front(T());
   test_type v2(alloc_type(2));
+  v2.push_front(T());
   v2 = std::move(v1);
   VERIFY(0 == v1.get_allocator().get_personality());
   VERIFY(1 == v2.get_allocator().get_personality());
Index: testsuite/23_containers/forward_list/allocator/swap.cc
===================================================================
--- testsuite/23_containers/forward_list/allocator/swap.cc	(revision 196526)
+++ testsuite/23_containers/forward_list/allocator/swap.cc	(working copy)
@@ -25,30 +25,19 @@ 
 
 using __gnu_test::propagating_allocator;
 
-// It is undefined behaviour to swap() containers wth unequal allocators
-// if the allocator doesn't propagate, so ensure the allocators compare
-// equal, while still being able to test propagation via get_personality().
-bool
-operator==(const propagating_allocator<T, false>&,
-           const propagating_allocator<T, false>&)
-{
-  return true;
-}
-
-bool
-operator!=(const propagating_allocator<T, false>&,
-           const propagating_allocator<T, false>&)
-{
-  return false;
-}
-
 void test01()
 {
   bool test __attribute__((unused)) = true;
-  typedef propagating_allocator<T, false> alloc_type;
+  // It is undefined behaviour to swap() containers wth unequal allocators
+  // if the allocator doesn't propagate, so request allocator to ignore
+  // personality to have equivalent allocators, while still being able to test
+  // propagation via get_personality().
+  typedef propagating_allocator<T, false, true> alloc_type;
   typedef std::forward_list<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_front(T());
   test_type v2(alloc_type(2));
+  v2.push_front(T());
   std::swap(v1, v2);
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(2 == v2.get_allocator().get_personality());
@@ -60,7 +49,9 @@ 
   typedef propagating_allocator<T, true> alloc_type;
   typedef std::forward_list<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_front(T());
   test_type v2(alloc_type(2));
+  v2.push_front(T());
   std::swap(v1, v2);
   VERIFY(2 == v1.get_allocator().get_personality());
   VERIFY(1 == v2.get_allocator().get_personality());
Index: testsuite/23_containers/forward_list/allocator/copy_assign.cc
===================================================================
--- testsuite/23_containers/forward_list/allocator/copy_assign.cc	(revision 196526)
+++ testsuite/23_containers/forward_list/allocator/copy_assign.cc	(working copy)
@@ -31,7 +31,9 @@ 
   typedef propagating_allocator<T, false> alloc_type;
   typedef std::forward_list<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_front(T());
   test_type v2(alloc_type(2));
+  v2.push_front(T());
   v2 = v1;
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(2 == v2.get_allocator().get_personality());
@@ -43,7 +45,9 @@ 
   typedef propagating_allocator<T, true> alloc_type;
   typedef std::forward_list<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
+  v1.push_front(T());
   test_type v2(alloc_type(2));
+  v2.push_front(T());
   v2 = v1;
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(1 == v2.get_allocator().get_personality());
Index: testsuite/util/testsuite_allocator.h
===================================================================
--- testsuite/util/testsuite_allocator.h	(revision 196526)
+++ testsuite/util/testsuite_allocator.h	(working copy)
@@ -243,7 +243,9 @@ 
     }
   };
 
-  template<typename Tp>
+  // IgnorePerson can be used to disable internal checks on personality while
+  // still allowing to check it in the test.
+  template<typename Tp, bool IgnorePerson = false>
     class uneq_allocator
     : private uneq_allocator_base
     {
@@ -261,8 +263,8 @@ 
 #endif
 
       template<typename Tp1>
-        struct rebind
-	{ typedef uneq_allocator<Tp1> other; };
+	struct rebind
+	{ typedef uneq_allocator<Tp1, IgnorePerson> other; };
 
       uneq_allocator() _GLIBCXX_USE_NOEXCEPT
       : personality(0) { }
@@ -271,7 +273,8 @@ 
       : personality(person) { }
       
       template<typename Tp1>
-        uneq_allocator(const uneq_allocator<Tp1>& b) _GLIBCXX_USE_NOEXCEPT
+      uneq_allocator(const uneq_allocator<Tp1, IgnorePerson>& b)
+	_GLIBCXX_USE_NOEXCEPT
 	: personality(b.get_personality()) { }
 
       ~uneq_allocator() _GLIBCXX_USE_NOEXCEPT
@@ -319,7 +322,7 @@ 
 
 	// Enforce requirements in Table 32 about deallocation vs
 	// allocator equality.
-	VERIFY( it->second == personality );
+	VERIFY( IgnorePerson || it->second == personality );
 
 	get_map().erase(it);
 	::operator delete(p);
@@ -331,8 +334,8 @@ 
 
 #if __cplusplus >= 201103L
       template<typename U, typename... Args>
-        void
-        construct(U* p, Args&&... args) 
+	void
+	construct(U* p, Args&&... args) 
 	{ ::new((void *)p) U(std::forward<Args>(args)...); }
 
       template<typename U>
@@ -357,31 +360,32 @@ 
 #endif
 
     private:
-
       // ... yet swappable!
       friend inline void
       swap(uneq_allocator& a, uneq_allocator& b)
       { std::swap(a.personality, b.personality); } 
       
       template<typename Tp1>
-        friend inline bool
-        operator==(const uneq_allocator& a, const uneq_allocator<Tp1>& b)
-        { return a.personality == b.personality; }
+	friend inline bool
+	operator==(const uneq_allocator& a,
+		   const uneq_allocator<Tp1, IgnorePerson>& b)
+	{ return IgnorePerson || a.personality == b.personality; }
 
       template<typename Tp1>
-        friend inline bool
-        operator!=(const uneq_allocator& a, const uneq_allocator<Tp1>& b)
-        { return !(a == b); }
+	friend inline bool
+	operator!=(const uneq_allocator& a,
+		   const uneq_allocator<Tp1, IgnorePerson>& b)
+	{ return !(a == b); }
       
       int personality;
     };
 
 #if __cplusplus >= 201103L
   // An uneq_allocator which can be used to test allocator propagation.
-  template<typename Tp, bool Propagate>
-    class propagating_allocator : public uneq_allocator<Tp>
+  template<typename Tp, bool Propagate, bool IgnorePerson = false>
+    class propagating_allocator : public uneq_allocator<Tp, IgnorePerson>
     {
-      typedef uneq_allocator<Tp> base_alloc;
+      typedef uneq_allocator<Tp, IgnorePerson> base_alloc;
       base_alloc& base() { return *this; }
       const base_alloc& base() const  { return *this; }
       void swap_base(base_alloc& b) { swap(b, this->base()); }
@@ -392,15 +396,17 @@ 
       // default allocator_traits::rebind_alloc would select
       // uneq_allocator::rebind so we must define rebind here
       template<typename Up>
-	struct rebind { typedef propagating_allocator<Up, Propagate> other; };
+	struct rebind
+	{ typedef propagating_allocator<Up, Propagate, IgnorePerson> other; };
 
       propagating_allocator(int i) noexcept
       : base_alloc(i)
       { }
 
       template<typename Up>
-	propagating_allocator(const propagating_allocator<Up, Propagate>& a)
-       	noexcept
+	propagating_allocator(const propagating_allocator<Up, Propagate,
+							  IgnorePerson>& a)
+	noexcept
 	: base_alloc(a)
 	{ }
 
@@ -417,13 +423,13 @@ 
 	}
 
       template<bool P2>
-  	propagating_allocator&
-  	operator=(const propagating_allocator<Tp, P2>& a) noexcept
-  	{
+	propagating_allocator&
+	operator=(const propagating_allocator<Tp, P2, IgnorePerson>& a) noexcept
+	{
 	  static_assert(P2, "assigning propagating_allocator<T, true>");
 	  propagating_allocator(a).swap_base(*this);
 	  return *this;
-  	}
+	}
 
       // postcondition: a.get_personality() == 0
       propagating_allocator(propagating_allocator&& a) noexcept