diff mbox

PR libstdc++/79254 fix exception-safety in std::string::operator=

Message ID 20170127161656.GA10816@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Jan. 27, 2017, 4:16 p.m. UTC 
This implements the strong exception-safety guarantee that is required
by [string.require] p2, which the new string can fail to meet when
propagate_on_container_copy_assignment (POCCA) is true.

The solution is to define a helper that takes ownership of the
string's memory (and also the associated allocator, length and
capacity) and either deallocates it after the assignment, or swaps it
back in if an exception happens (i.e. commit or rollback).

	PR libstdc++/79254
	* config/abi/pre/gnu.ver: Add new symbols.
	* include/bits/basic_string.h [_GLIBCXX_USE_CXX11_ABI]
	(basic_string::_M_copy_assign): New overloaded functions to perform
	copy assignment.
	(basic_string::operator=(const basic_string&)): Dispatch to
	_M_copy_assign.
	* include/bits/basic_string.tcc [_GLIBCXX_USE_CXX11_ABI]
	(basic_string::_M_copy_assign(const basic_string&, true_type)):
	Define, performing rollback on exception.
	* testsuite/21_strings/basic_string/allocator/char/copy_assign.cc:
	Test exception-safety guarantee.
	* testsuite/21_strings/basic_string/allocator/wchar_t/copy_assign.cc:
	Likewise.
	* testsuite/util/testsuite_allocator.h (uneq_allocator::swap): Make
	std::swap visible.

The backports for the branches will be a bit different, as we can't
add new exports to closed symbol versions, so I'll keep everything in
operator= instead of tag dispatching. The POCCA code path will still
be dependent on a constant expression, so should be optimized away for
most allocators.

Tested powerpc64le-linux, committed to trunk.
commit 229dc84648b0ac0374292a5c2dd54e14e737b0c5
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Jan 27 12:44:53 2017 +0000

    PR libstdc++/79254 fix exception-safety in std::string::operator=
    
    	PR libstdc++/79254
    	* config/abi/pre/gnu.ver: Add new symbols.
    	* include/bits/basic_string.h [_GLIBCXX_USE_CXX11_ABI]
    	(basic_string::_M_copy_assign): New overloaded functions to perform
    	copy assignment.
    	(basic_string::operator=(const basic_string&)): Dispatch to
    	_M_copy_assign.
    	* include/bits/basic_string.tcc [_GLIBCXX_USE_CXX11_ABI]
    	(basic_string::_M_copy_assign(const basic_string&, true_type)):
    	Define, performing rollback on exception.
    	* testsuite/21_strings/basic_string/allocator/char/copy_assign.cc:
    	Test exception-safety guarantee.
    	* testsuite/21_strings/basic_string/allocator/wchar_t/copy_assign.cc:
    	Likewise.
    	* testsuite/util/testsuite_allocator.h (uneq_allocator::swap): Make
    	std::swap visible.
diff mbox

Patch

diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index 3e6e70b..1bea4b4 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -1700,7 +1700,9 @@  GLIBCXX_3.4.21 {
     _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE1[01]**;
     _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE12_Alloc_hiderC[12]EP[cw]RKS3_;
     _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE12_M*;
-    _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE1[3-9]*;
+    _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE13*;
+    _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE14_M_replace_aux*;
+    _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE1[5-9]*;
     _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE[2-9]*;
     _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EEC[12]EOS4_*;
     _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EEC[12]EPK*;
@@ -1953,6 +1955,9 @@  GLIBCXX_3.4.23 {
     _ZNSsC[12]ERKSs[jmy]RKSaIcE;
     _ZNSbIwSt11char_traitsIwESaIwEEC[12]ERKS2_mRKS1_;
 
+    # basic_string<C, T, A>::_M_copy_assign(const basic_string&, {true,false}_type)
+    _ZNSt7__cxx1112basic_stringI[cw]St11char_traitsI[cw]ESaI[cw]EE14_M_copy_assign*;
+
 #ifndef HAVE_EXCEPTION_PTR_SINCE_GCC46
     # std::future symbols are exported in the first version to support
     # std::exception_ptr
diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
index 9dffcf9..97fe797 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -384,7 +384,7 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
       }
 
       void
-      _M_assign(const basic_string& __rcs);
+      _M_assign(const basic_string&);
 
       void
       _M_mutate(size_type __pos, size_type __len1, const _CharT* __s,
@@ -393,6 +393,15 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
       void
       _M_erase(size_type __pos, size_type __n);
 
+#if __cplusplus >= 201103L
+      void
+      _M_copy_assign(const basic_string& __str, /* pocca = */ true_type);
+
+      void
+      _M_copy_assign(const basic_string& __str, /* pocca = */ false_type)
+      { this->_M_assign(__str); }
+#endif
+
     public:
       // Construct/copy/destroy:
       // NB: We overload ctors in some cases instead of using default
@@ -627,20 +636,12 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
       operator=(const basic_string& __str)
       {
 #if __cplusplus >= 201103L
-	if (_Alloc_traits::_S_propagate_on_copy_assign())
-	  {
-	    if (!_Alloc_traits::_S_always_equal() && !_M_is_local()
-		&& _M_get_allocator() != __str._M_get_allocator())
-	      {
-		// replacement allocator cannot free existing storage
-		_M_destroy(_M_allocated_capacity);
-		_M_data(_M_local_data());
-		_M_set_length(0);
-	      }
-	    std::__alloc_on_copy(_M_get_allocator(), __str._M_get_allocator());
-	  }
+	_M_copy_assign(__str,
+	    typename _Alloc_traits::propagate_on_container_copy_assignment());
+#else
+	this->_M_assign(__str);
 #endif
-	return this->assign(__str);
+	return *this;
       }
 
       /**
diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc
index 41b7fa1..adc8b85 100644
--- a/libstdc++-v3/include/bits/basic_string.tcc
+++ b/libstdc++-v3/include/bits/basic_string.tcc
@@ -275,6 +275,70 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	}
     }
 
+#if __cplusplus >= 201103L
+  template<typename _CharT, typename _Traits, typename _Alloc>
+    void
+    basic_string<_CharT, _Traits, _Alloc>::
+    _M_copy_assign(const basic_string& __str, true_type)
+    {
+      struct _Guard // RAII type for strong exception-safety guarantee.
+      {
+	// Takes ownership of string's original state.
+	_Guard(basic_string* __self)
+	: _M_self(__self), _M_alloc(std::move(__self->_M_get_allocator())),
+	  _M_ptr(__self->_M_data()),
+	  _M_capacity(__self->_M_allocated_capacity), _M_len(__self->length())
+	{
+	  __self->_M_data(__self->_M_local_data());
+	  __self->_M_length(0);
+	}
+
+	// Restores string's original state if _M_release() was not called.
+	~_Guard()
+	{
+	  if (_M_ptr)
+	    {
+	      _M_self->_M_get_allocator() = std::move(_M_alloc);
+	      _M_self->_M_data(_M_ptr);
+	      _M_self->_M_capacity(_M_capacity);
+	      _M_self->_M_length(_M_len);
+	    }
+	}
+
+	_Guard(const _Guard&) = delete;
+	_Guard& operator=(const _Guard&) = delete;
+
+	void _M_release()
+	{
+	  // Original state can be freed now.
+	  _Alloc_traits::deallocate(_M_alloc, _M_ptr, _M_capacity + 1);
+	  _M_ptr = nullptr;
+	}
+
+	basic_string*	_M_self;
+	allocator_type	_M_alloc;
+	pointer		_M_ptr;
+	size_type	_M_capacity;
+	size_type	_M_len;
+      };
+
+      if (!_Alloc_traits::_S_always_equal() && !_M_is_local()
+	  && _M_get_allocator() != __str._M_get_allocator())
+	{
+	  // The propagating allocator cannot free existing storage.
+	  _Guard __guard(this);
+	  _M_get_allocator() = __str._M_get_allocator();
+	  this->_M_assign(__str);
+	  __guard._M_release();
+	}
+      else
+	{
+	  _M_get_allocator() = __str._M_get_allocator();
+	  this->_M_assign(__str);
+	}
+    }
+#endif
+
   template<typename _CharT, typename _Traits, typename _Alloc>
     void
     basic_string<_CharT, _Traits, _Alloc>::
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/allocator/char/copy_assign.cc b/libstdc++-v3/testsuite/21_strings/basic_string/allocator/char/copy_assign.cc
index 32ee708..0fc80d7 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/allocator/char/copy_assign.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/allocator/char/copy_assign.cc
@@ -22,6 +22,7 @@ 
 #include <string>
 #include <testsuite_hooks.h>
 #include <testsuite_allocator.h>
+#include <ext/throw_allocator.h>
  
 using C = char;
 const C c = 'a';
@@ -99,9 +100,33 @@  void test02()
   VERIFY(1 == v5.get_allocator().get_personality());
 }
 
+void test03()
+{
+  // PR libstdc++/79254
+  using throw_alloc = __gnu_cxx::throw_allocator_limit<C>;
+  typedef propagating_allocator<C, true, throw_alloc> alloc_type;
+  typedef std::basic_string<C, traits, alloc_type> test_type;
+  alloc_type a1(1), a2(2);
+  throw_alloc::set_limit(2); // Throw on third allocation (during assignment).
+  const C* s1 = "a string that is longer than a small string";
+  const C* s2 = "another string that is longer than a small string";
+  test_type v1(s1, a1);
+  test_type v2(s2, a2);
+  bool caught = false;
+  try {
+    v1 = v2;
+  } catch (__gnu_cxx::forced_error&) {
+    caught = true;
+  }
+  VERIFY( caught );
+  VERIFY( v1 == s1 );
+  VERIFY( v1.get_allocator() == a1 );
+}
+
 int main()
 {
   test01();
   test02();
+  test03();
   return 0;
 }
diff --git a/libstdc++-v3/testsuite/21_strings/basic_string/allocator/wchar_t/copy_assign.cc b/libstdc++-v3/testsuite/21_strings/basic_string/allocator/wchar_t/copy_assign.cc
index 89593ba..c35e001 100644
--- a/libstdc++-v3/testsuite/21_strings/basic_string/allocator/wchar_t/copy_assign.cc
+++ b/libstdc++-v3/testsuite/21_strings/basic_string/allocator/wchar_t/copy_assign.cc
@@ -22,6 +22,7 @@ 
 #include <string>
 #include <testsuite_hooks.h>
 #include <testsuite_allocator.h>
+#include <ext/throw_allocator.h>
  
 using C = wchar_t;
 const C c = L'a';
@@ -99,9 +100,33 @@  void test02()
   VERIFY(1 == v5.get_allocator().get_personality());
 }
 
+void test03()
+{
+  // PR libstdc++/79254
+  using throw_alloc = __gnu_cxx::throw_allocator_limit<C>;
+  typedef propagating_allocator<C, true, throw_alloc> alloc_type;
+  typedef std::basic_string<C, traits, alloc_type> test_type;
+  alloc_type a1(1), a2(2);
+  throw_alloc::set_limit(2); // Throw on third allocation (during assignment).
+  const C* s1 = L"a string that is longer than a small string";
+  const C* s2 = L"another string that is longer than a small string";
+  test_type v1(s1, a1);
+  test_type v2(s2, a2);
+  bool caught = false;
+  try {
+    v1 = v2;
+  } catch (__gnu_cxx::forced_error&) {
+    caught = true;
+  }
+  VERIFY( caught );
+  VERIFY( v1 == s1 );
+  VERIFY( v1.get_allocator() == a1 );
+}
+
 int main()
 {
   test01();
   test02();
+  test03();
   return 0;
 }
diff --git a/libstdc++-v3/testsuite/util/testsuite_allocator.h b/libstdc++-v3/testsuite/util/testsuite_allocator.h
index 20387a8..813fc81 100644
--- a/libstdc++-v3/testsuite/util/testsuite_allocator.h
+++ b/libstdc++-v3/testsuite/util/testsuite_allocator.h
@@ -287,7 +287,7 @@  namespace __gnu_test
 
       Alloc& base() { return *this; }
       const Alloc& base() const  { return *this; }
-      void swap_base(Alloc& b) { swap(b, this->base()); }
+      void swap_base(Alloc& b) { using std::swap; swap(b, this->base()); }
 
     public:
       typedef typename check_consistent_alloc_value_type<Tp, Alloc>::value_type