This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Fix vector C++11 allocator bug


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

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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]