diff mbox

[v3] PR libstdc++/78389

Message ID CAFk2RUZCr_QEd_SkOb=tpduN38+RWmpbNyydOWx755vQn65nPg@mail.gmail.com
State New
Headers show

Commit Message

Ville Voutilainen Jan. 13, 2017, 8:27 a.m. UTC
On 13 January 2017 at 10:09, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
>>> Ah, I think I see what you're saying. Just splice them back in any
>>> order. Ok, I'll give that a spin.
>>
>> Right, list::sort doesn't promise strong exception safety, so
>> "unsorting" is not needed.
>>
>> Also, shouldn't merge() rethrow the caught exception rather than swallow it?
>
> Ha, yes, well spotted. I'll cook up an improved patch.

Thus:

2017-01-13  Ville Voutilainen  <ville.voutilainen@gmail.com>

    PR libstdc++/78389
    * include/bits/list.tcc (merge(list&&)):
    Adjust list sizes if the comparator throws.
    (merge(list&&, _StrictWeakOrdering)): Likewise.
    (sort()): Splice elements back from the scratch buffers
    if the comparator throws.
    (sort(_StrictWeakOrdering)): Likewise.
    * testsuite/23_containers/list/operations/78389.cc: New.

Comments

Ville Voutilainen Jan. 13, 2017, 8:29 a.m. UTC | #1
On 13 January 2017 at 10:27, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> On 13 January 2017 at 10:09, Ville Voutilainen
> <ville.voutilainen@gmail.com> wrote:
>>>> Ah, I think I see what you're saying. Just splice them back in any
>>>> order. Ok, I'll give that a spin.
>>>
>>> Right, list::sort doesn't promise strong exception safety, so
>>> "unsorting" is not needed.
>>>
>>> Also, shouldn't merge() rethrow the caught exception rather than swallow it?
>>
>> Ha, yes, well spotted. I'll cook up an improved patch.
>
> Thus:
>
> 2017-01-13  Ville Voutilainen  <ville.voutilainen@gmail.com>
>
>     PR libstdc++/78389
>     * include/bits/list.tcc (merge(list&&)):
>     Adjust list sizes if the comparator throws.
>     (merge(list&&, _StrictWeakOrdering)): Likewise.
>     (sort()): Splice elements back from the scratch buffers
>     if the comparator throws.
>     (sort(_StrictWeakOrdering)): Likewise.
>     * testsuite/23_containers/list/operations/78389.cc: New.


..and yes, sigh, that patch has whitespace damage in it. I have
already fixed that, so that'll be corrected before
committing.
Ville Voutilainen Jan. 13, 2017, 8:30 a.m. UTC | #2
On 13 January 2017 at 10:29, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> ..and yes, sigh, that patch has whitespace damage in it. I have
> already fixed that, so that'll be corrected before
> committing.

..as well as replacing those asserts with VERIFY.
Tim Song Jan. 13, 2017, 8:41 a.m. UTC | #3
On Fri, Jan 13, 2017 at 3:27 AM, Ville Voutilainen
<ville.voutilainen@gmail.com> wrote:
> On 13 January 2017 at 10:09, Ville Voutilainen
> <ville.voutilainen@gmail.com> wrote:
>>>> Ah, I think I see what you're saying. Just splice them back in any
>>>> order. Ok, I'll give that a spin.
>>>
>>> Right, list::sort doesn't promise strong exception safety, so
>>> "unsorting" is not needed.
>>>
>>> Also, shouldn't merge() rethrow the caught exception rather than swallow it?
>>
>> Ha, yes, well spotted. I'll cook up an improved patch.
>
> Thus:
>
> 2017-01-13  Ville Voutilainen  <ville.voutilainen@gmail.com>
>
>     PR libstdc++/78389
>     * include/bits/list.tcc (merge(list&&)):
>     Adjust list sizes if the comparator throws.
>     (merge(list&&, _StrictWeakOrdering)): Likewise.
>     (sort()): Splice elements back from the scratch buffers
>     if the comparator throws.
>     (sort(_StrictWeakOrdering)): Likewise.
>     * testsuite/23_containers/list/operations/78389.cc: New.

Shouldn't __carry be spliced back as well?
diff mbox

Patch

diff --git a/libstdc++-v3/include/bits/list.tcc b/libstdc++-v3/include/bits/list.tcc
index c4f397f..46bb9d2 100644
--- a/libstdc++-v3/include/bits/list.tcc
+++ b/libstdc++-v3/include/bits/list.tcc
@@ -386,20 +386,30 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	  iterator __last1 = end();
 	  iterator __first2 = __x.begin();
 	  iterator __last2 = __x.end();
-	  while (__first1 != __last1 && __first2 != __last2)
-	    if (*__first2 < *__first1)
-	      {
-		iterator __next = __first2;
-		_M_transfer(__first1, __first2, ++__next);
-		__first2 = __next;
-	      }
-	    else
-	      ++__first1;
-	  if (__first2 != __last2)
-	    _M_transfer(__last1, __first2, __last2);
+	  size_t __orig_size = __x.size();
+	  __try {
+	    while (__first1 != __last1 && __first2 != __last2)
+	      if (*__first2 < *__first1)
+		{
+		  iterator __next = __first2;
+		  _M_transfer(__first1, __first2, ++__next);
+		  __first2 = __next;
+		}
+	      else
+		++__first1;
+	    if (__first2 != __last2)
+	      _M_transfer(__last1, __first2, __last2);
 
-	  this->_M_inc_size(__x._M_get_size());
-	  __x._M_set_size(0);
+	    this->_M_inc_size(__x._M_get_size());
+	    __x._M_set_size(0);
+	  }
+	  __catch(...)
+	    {
+	      size_t __dist = distance(__first2, __last2);
+	      this->_M_inc_size(__dist);
+	      __x._M_set_size(__orig_size - __dist);
+	      __throw_exception_again;
+	    }
 	}
     }
 
@@ -423,20 +433,31 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	    iterator __last1 = end();
 	    iterator __first2 = __x.begin();
 	    iterator __last2 = __x.end();
-	    while (__first1 != __last1 && __first2 != __last2)
-	      if (__comp(*__first2, *__first1))
-		{
-		  iterator __next = __first2;
-		  _M_transfer(__first1, __first2, ++__next);
-		  __first2 = __next;
-		}
-	      else
-		++__first1;
-	    if (__first2 != __last2)
-	      _M_transfer(__last1, __first2, __last2);
-
-	    this->_M_inc_size(__x._M_get_size());
-	    __x._M_set_size(0);
+	    size_t __orig_size = __x.size();
+	    __try
+	      {
+		while (__first1 != __last1 && __first2 != __last2)
+		  if (__comp(*__first2, *__first1))
+		    {
+		      iterator __next = __first2;
+		      _M_transfer(__first1, __first2, ++__next);
+		      __first2 = __next;
+		    }
+		  else
+		    ++__first1;
+		if (__first2 != __last2)
+		  _M_transfer(__last1, __first2, __last2);
+
+		this->_M_inc_size(__x._M_get_size());
+		__x._M_set_size(0);
+	      }
+	    __catch(...)
+	      {
+		size_t __dist = distance(__first2, __last2);
+		this->_M_inc_size(__dist);
+		__x._M_set_size(__orig_size - __dist);
+		__throw_exception_again;
+	      }
 	  }
       }
 
@@ -453,27 +474,35 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
         list __tmp[64];
         list * __fill = __tmp;
         list * __counter;
-
-        do
+	__try
 	  {
-	    __carry.splice(__carry.begin(), *this, begin());
-
-	    for(__counter = __tmp;
-		__counter != __fill && !__counter->empty();
-		++__counter)
+	    do
 	      {
-		__counter->merge(__carry);
+		__carry.splice(__carry.begin(), *this, begin());
+		
+		for(__counter = __tmp;
+		    __counter != __fill && !__counter->empty();
+		    ++__counter)
+		  {
+		    __counter->merge(__carry);
+		    __carry.swap(*__counter);
+		  }
 		__carry.swap(*__counter);
+		if (__counter == __fill)
+		  ++__fill;
 	      }
-	    __carry.swap(*__counter);
-	    if (__counter == __fill)
-	      ++__fill;
+	    while ( !empty() );
+	    
+	    for (__counter = __tmp + 1; __counter != __fill; ++__counter)
+	      __counter->merge(*(__counter - 1));
+	    swap( *(__fill - 1) );
+	  }
+	__catch(...)
+	  {
+	    for (int i = 0; i < sizeof(__tmp)/sizeof(__tmp[0]); ++i)
+	      this->splice(this->end(), __tmp[i]);
+	    __throw_exception_again;
 	  }
-	while ( !empty() );
-
-        for (__counter = __tmp + 1; __counter != __fill; ++__counter)
-          __counter->merge(*(__counter - 1));
-        swap( *(__fill - 1) );
       }
     }
 
@@ -530,27 +559,35 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 	    list __tmp[64];
 	    list * __fill = __tmp;
 	    list * __counter;
-
-	    do
+	    __try
 	      {
-		__carry.splice(__carry.begin(), *this, begin());
-
-		for(__counter = __tmp;
-		    __counter != __fill && !__counter->empty();
-		    ++__counter)
+		do
 		  {
-		    __counter->merge(__carry, __comp);
+		    __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;
 		  }
-		__carry.swap(*__counter);
-		if (__counter == __fill)
-		  ++__fill;
+		while ( !empty() );
+		
+		for (__counter = __tmp + 1; __counter != __fill; ++__counter)
+		  __counter->merge(*(__counter - 1), __comp);
+		swap(*(__fill - 1));
+	      }
+	    __catch(...)
+	      {
+		for (int i = 0; i < sizeof(__tmp)/sizeof(__tmp[0]); ++i)
+		  this->splice(this->end(), __tmp[i]);
+		__throw_exception_again;
 	      }
-	    while ( !empty() );
-
-	    for (__counter = __tmp + 1; __counter != __fill; ++__counter)
-	      __counter->merge(*(__counter - 1), __comp);
-	    swap(*(__fill - 1));
 	  }
       }
 
diff --git a/libstdc++-v3/testsuite/23_containers/list/operations/78389.cc b/libstdc++-v3/testsuite/23_containers/list/operations/78389.cc
new file mode 100644
index 0000000..38ff2c1
--- /dev/null
+++ b/libstdc++-v3/testsuite/23_containers/list/operations/78389.cc
@@ -0,0 +1,85 @@ 
+// { dg-do run { target c++11 } }
+
+// Copyright (C) 2017 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/>.
+
+// 23.2.2.4 list operations [lib.list.ops]
+
+#include <testsuite_hooks.h>
+
+#include <list>
+
+struct ThrowingComparator
+{
+    unsigned int throw_after = 0;
+    unsigned int count = 0;
+    bool operator()(int, int) {
+        if (++count >= throw_after) {
+            throw 666;
+        }
+        return true;
+    }
+};
+
+struct X
+{
+  X() = default;
+  X(int) {}
+};
+
+unsigned int throw_after_X = 0;
+unsigned int count_X = 0;
+
+bool operator<(const X&, const X&) {
+  if (++count_X >= throw_after_X) {
+    throw 666;
+  }
+  return true;
+}
+
+
+int main()
+{
+    std::list<int> a{1, 2, 3, 4};
+    std::list<int> b{5, 6, 7, 8, 9, 10, 11, 12};
+    try {
+        a.merge(b, ThrowingComparator{5});
+    } catch (...) {
+    }
+    VERIFY(a.size() == 8 && b.size() == 4);
+    std::list<X> ax{1, 2, 3, 4};
+    std::list<X> bx{5, 6, 7, 8, 9, 10, 11, 12};
+    throw_after_X = 5;
+    try {
+        ax.merge(bx);
+    } catch (...) {
+    }
+    VERIFY(ax.size() == 8 && bx.size() == 4);
+    std::list<int> ay{5, 6, 7, 8, 9, 10, 11, 12};
+    try {
+        ay.sort(ThrowingComparator{5});
+    } catch (...) {
+    }
+    assert(ay.size() == 8);
+    std::list<X> az{5, 6, 7, 8, 9, 10, 11, 12};
+    throw_after_X = 5;
+    try {
+        az.sort();
+    } catch (...) {
+    }
+    assert(az.size() == 8);
+}