libstdc++: Allow stateful allocators in std::list::sort [PR 66742]

The temporary lists used by std::list::sort are default constructed,
which means they use default constructed allocators. The sort operation
is defined in terms of merge and splice operations, which have undefined
behaviour (and abort) if the allocators do not compare equal. This means
it is not possible to sort a list that uses an allocator that compares
unequal to an default constructed allocator.

The solution is to avoid using temporary std::list objects at all. We do
not need to be able to allocate memory because no nodes are allocated,
only spliced from one list to another. That means the temporary lists
don't need an allocator at all, so whether it would compare equal
doesn't matter.

Instead of temporary std::list objects, we can just use a collection of
_List_node_base objects that nodes can be spliced onto as needed. Those
objects are wrapped in a _Scratch_list type that implements the splicing
and merging operations used by list::sort.

We also don't need to update the list size during the sort, because
sorting doesn't alter the number of nodes. Although we move nodes in and
out of the scratch lists, at the end of the function all nodes are back
in the original std::list and the scratch lists are empty.  So for the
cxx11 ABI we can avoid the _M_size modifications usually done when
splicing nodes.

Signed-off-by: Jonathan Wakely <jwakely@redhat.com>
libstdc++-v3/ChangeLog:

	PR libstdc++/66742
	* include/bits/list.tcc (list::sort()): Use _Scratch_list
	objects for splicing and merging.
	(list::sort(StrictWeakOrdering)): Likewise.
	* include/bits/stl_list.h (__detail::_Scratch_list): New type.
	* src/c++98/list.cc (_List_node_base::_M_transfer): Add
	assertion for --enable-libstdcxx-debug library.
	* testsuite/23_containers/list/operations/66742.cc: New test.

diff --git a/libstdc++-v3/include/bits/list.tcc b/libstdc++-v3/include/bits/list.tcc
index 62b0ba1063abf06d3248e71da14769ada2562b60..7f4e1569ab1a96e3e19ba07ad315ea891f04ae1d 100644 (file)
--- a/libstdc++-v3/include/bits/list.tcc
+++ b/libstdc++-v3/include/bits/list.tcc
@@ -485,21 +485,34 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       if (this->_M_impl._M_node._M_next != &this->_M_impl._M_node
          && this->_M_impl._M_node._M_next->_M_next != &this->_M_impl._M_node)
       {
-        list __carry;
-        list __tmp[64];
-        list * __fill = __tmp;
-        list * __counter;
+       using __detail::_Scratch_list;
+       // The algorithm used here is largely unchanged from the SGI STL
+       // and is described in The C++ Standard Template Library by Plauger,
+       // Stepanov, Lee, Musser.
+       // Each element of *this is spliced out and merged into one of the
+       // sorted lists in __tmp, then all the lists in __tmp are merged
+       // together and then swapped back into *this.
+       // Because all nodes end up back in *this we do not need to update
+       // this->size() while nodes are temporarily moved out.
+       _Scratch_list __carry;
+       _Scratch_list __tmp[64];
+       _Scratch_list* __fill = __tmp;
+       _Scratch_list* __counter;
+
+       _Scratch_list::_Ptr_cmp<const_iterator, void> __ptr_comp;
+
        __try
          {
            do
              {
-               __carry.splice(__carry.begin(), *this, begin());
+               __carry._M_take_one(begin()._M_node);
 
                for(__counter = __tmp;
                    __counter != __fill && !__counter->empty();
                    ++__counter)
                  {
-                   __counter->merge(__carry);
+
+                   __counter->merge(__carry, __ptr_comp);
                    __carry.swap(*__counter);
                  }
                __carry.swap(*__counter);
@@ -509,14 +522,15 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
            while ( !empty() );
 
            for (__counter = __tmp + 1; __counter != __fill; ++__counter)
-             __counter->merge(*(__counter - 1));
-           swap( *(__fill - 1) );
+             __counter->merge(__counter[-1], __ptr_comp);
+           __fill[-1].swap(this->_M_impl._M_node);
          }
        __catch(...)
          {
-           this->splice(this->end(), __carry);
+           // Move all nodes back into *this.
+           __carry._M_put_all(end()._M_node);
            for (int __i = 0; __i < sizeof(__tmp)/sizeof(__tmp[0]); ++__i)
-             this->splice(this->end(), __tmp[__i]);
+             __tmp[__i]._M_put_all(end()._M_node);
            __throw_exception_again;
          }
       }
@@ -602,42 +616,49 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
        // Do nothing if the list has length 0 or 1.
        if (this->_M_impl._M_node._M_next != &this->_M_impl._M_node
            && this->_M_impl._M_node._M_next->_M_next != &this->_M_impl._M_node)
-         {
-           list __carry;
-           list __tmp[64];
-           list * __fill = __tmp;
-           list * __counter;
-           __try
-             {
-               do
-                 {
-                   __carry.splice(__carry.begin(), *this, begin());
-
-                   for(__counter = __tmp;
-                       __counter != __fill && !__counter->empty();
-                       ++__counter)
-                     {
-                       __counter->merge(__carry, __comp);
-                       __carry.swap(*__counter);
-                     }
-                   __carry.swap(*__counter);
-                   if (__counter == __fill)
-                     ++__fill;
-                 }
-               while ( !empty() );
+       {
+         using __detail::_Scratch_list;
+         _Scratch_list __carry;
+         _Scratch_list __tmp[64];
+         _Scratch_list* __fill = __tmp;
+         _Scratch_list* __counter;
 
-               for (__counter = __tmp + 1; __counter != __fill; ++__counter)
-                 __counter->merge(*(__counter - 1), __comp);
-               swap(*(__fill - 1));
-             }
-           __catch(...)
-             {
-               this->splice(this->end(), __carry);
-               for (int __i = 0; __i < sizeof(__tmp)/sizeof(__tmp[0]); ++__i)
-                 this->splice(this->end(), __tmp[__i]);
-               __throw_exception_again;
-             }
-         }
+       _Scratch_list::_Ptr_cmp<const_iterator, _StrictWeakOrdering> __ptr_comp
+         = { __comp };
+
+         __try
+           {
+             do
+               {
+                 __carry._M_take_one(begin()._M_node);
+
+                 for(__counter = __tmp;
+                     __counter != __fill && !__counter->empty();
+                     ++__counter)
+                   {
+
+                     __counter->merge(__carry, __ptr_comp);
+                     __carry.swap(*__counter);
+                   }
+                 __carry.swap(*__counter);
+                 if (__counter == __fill)
+                   ++__fill;
+               }
+             while ( !empty() );
+
+             for (__counter = __tmp + 1; __counter != __fill; ++__counter)
+               __counter->merge(__counter[-1], __ptr_comp);
+             __fill[-1].swap(this->_M_impl._M_node);
+           }
+         __catch(...)
+           {
+             // Move all nodes back into *this.
+             __carry._M_put_all(end()._M_node);
+             for (int __i = 0; __i < sizeof(__tmp)/sizeof(__tmp[0]); ++__i)
+               __tmp[__i]._M_put_all(end()._M_node);
+             __throw_exception_again;
+           }
+       }
       }
 
 _GLIBCXX_END_NAMESPACE_CONTAINER

diff --git a/libstdc++-v3/include/bits/stl_list.h b/libstdc++-v3/include/bits/stl_list.h
index e11603c015762dc8789e49cbf1e8c5578e21ee4d..81361dfa4d5b68977bd9c3940748a5fcff13dc23 100644 (file)
--- a/libstdc++-v3/include/bits/stl_list.h
+++ b/libstdc++-v3/include/bits/stl_list.h
@@ -158,6 +158,73 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     private:
       _List_node_base* _M_base() { return this; }
     };
+
+    // Used by list::sort to hold nodes being sorted.
+    struct _Scratch_list : _List_node_base
+    {
+      _Scratch_list() { _M_next = _M_prev = this; }
+
+      bool empty() const { return _M_next == this; }
+
+      void swap(_List_node_base& __l) { _List_node_base::swap(*this, __l); }
+
+      template<typename _Iter, typename _Cmp>
+       struct _Ptr_cmp
+       {
+         _Cmp _M_cmp;
+
+         bool
+         operator()(const __detail::_List_node_base* __lhs,
+                    const __detail::_List_node_base* __rhs) /* not const */
+         { return _M_cmp(*_Iter(__lhs), *_Iter(__rhs)); }
+       };
+
+      template<typename _Iter>
+       struct _Ptr_cmp<_Iter, void>
+       {
+         bool
+         operator()(const __detail::_List_node_base* __lhs,
+                    const __detail::_List_node_base* __rhs) const
+         { return *_Iter(__lhs) < *_Iter(__rhs); }
+       };
+
+      // Merge nodes from __x into *this. Both lists must be sorted wrt _Cmp.
+      template<typename _Cmp>
+       void
+       merge(_List_node_base& __x, _Cmp __comp)
+       {
+         _List_node_base* __first1 = _M_next;
+         _List_node_base* const __last1 = this;
+         _List_node_base* __first2 = __x._M_next;
+         _List_node_base* const __last2 = std::__addressof(__x);
+
+         while (__first1 != __last1 && __first2 != __last2)
+           {
+             if (__comp(__first2, __first1))
+               {
+                 _List_node_base* __next = __first2->_M_next;
+                 __first1->_M_transfer(__first2, __next);
+                 __first2 = __next;
+               }
+             else
+               __first1 = __first1->_M_next;
+           }
+         if (__first2 != __last2)
+           this->_M_transfer(__first2, __last2);
+       }
+
+      // Splice the node at __i into *this.
+      void _M_take_one(_List_node_base* __i)
+      { this->_M_transfer(__i, __i->_M_next); }
+
+      // Splice all nodes from *this after __i.
+      void _M_put_all(_List_node_base* __i)
+      {
+       if (!empty())
+         __i->_M_transfer(_M_next, this);
+      }
+    };
+
   } // namespace detail
 
 _GLIBCXX_BEGIN_NAMESPACE_CONTAINER

diff --git a/libstdc++-v3/src/c++98/list.cc b/libstdc++-v3/src/c++98/list.cc
index 18f0e136c4769cac875a5b9f7a078710df22de9f..c2f5c43622ea8a201b185574af5ffebb419ef5e1 100644 (file)
--- a/libstdc++-v3/src/c++98/list.cc
+++ b/libstdc++-v3/src/c++98/list.cc
@@ -94,6 +94,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     _M_transfer(_List_node_base * const __first,
                _List_node_base * const __last) _GLIBCXX_USE_NOEXCEPT
     {
+      __glibcxx_assert(__first != __last);
+
       if (this != __last)
        {
          // Remove [first, last) from its old position.

diff --git a/libstdc++-v3/testsuite/23_containers/list/operations/66742.cc b/libstdc++-v3/testsuite/23_containers/list/operations/66742.cc
new file mode 100644 (file)
index 0000000..24bda39
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/list/operations/66742.cc
@@ -0,0 +1,55 @@
+// { dg-do run { target c++11 } }
+
+#include <list>
+#include <testsuite_allocator.h>
+#include <testsuite_hooks.h>
+
+// PR libstdc++/66742
+// abort on sorting list with custom allocator that is not stateless
+
+template<typename List, typename Cmp = std::less<typename List::value_type>>
+bool is_sorted(const List& l, Cmp cmp = {})
+{
+  auto it = l.begin();
+  auto next = it;
+  const auto end = l.end();
+  if (it == end)
+    return true;
+  while (++next != end)
+    if (cmp(*next, *it))
+      return false;
+    else
+      it = next;
+  return true;
+}
+
+void
+test01()
+{
+  using Alloc = __gnu_test::uneq_allocator<int>;
+  Alloc a1(1);
+  std::list<int, Alloc> l(a1);
+  for (int i = 0; i < 1000; ++i)
+  {
+    l.push_front(i);
+    l.push_back(i + (i % 3));
+  }
+  const auto orig = l;
+
+  l.sort();
+  VERIFY( is_sorted(l) );
+  l.sort();
+  VERIFY( is_sorted(l) );
+
+  l = orig;
+  l.sort(std::less<int>());
+  VERIFY( is_sorted(l) );
+  l.sort(std::greater<int>());
+  VERIFY( is_sorted(l, std::greater<int>()) );
+}
+
+int
+main()
+{
+  test01();
+}