Re: [PATCH] PR libstdc++/79254 fix exception-safety in std::string::operator=

[Jonathan Wakely <jwakely at redhat dot com>]

On 27/01/17 16:16 +0000, Jonathan Wakely wrote:
> 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.

Whlie working on the backport of this I realised the RAII
commit-and-rollback approach is a lot more complicated than simply
doing the new allocation before making any changes to *this.

This new patch simplifies it. There's no need to tag-dispatch to
_M_copy_assign because the code path for POCCA allocators is behind a
compile-time constant condition anyway, and the allocator assignment
is already conditional because of __alloc_on_copy.

******************************************************************
!!! This removes the new _M_copy_assign members functions that
!!! were exported from libstdc++.so since last Friday.
******************************************************************

Packagers (including at least Fedora rawhide) that have shipped a
gcc-7 build since last Friday will need to update again.  This
shouldn't be a big deal, because I expect the amount of code in a
typical GNU/Linux distro that used the _M_copy_assign(..., true_type)
symbol to be exactly zero, and the _M_copy_assign(..., false_type)
symbol will be inlined with any -Ox level, and is not used at -O0.

Tested powerpc64-linux, committed to trunk.


commit 5e6bb61638e06b51291307e7e21745a55feed5f2
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jan 31 17:56:03 2017 +0000

    PR libstdc++/79254 simplify exception-safety in copy assignment
    
    	PR libstdc++/79254
    	* config/abi/pre/gnu.ver: Remove recently added symbols.
    	* include/bits/basic_string.h [_GLIBCXX_USE_CXX11_ABI]
    	(basic_string::_M_copy_assign): Remove.
    	(basic_string::operator=(const basic_string&)): Don't dispatch to
    	_M_copy_assign. If source object is small just deallocate, otherwise
    	perform new allocation before making any changes.
    	* include/bits/basic_string.tcc [_GLIBCXX_USE_CXX11_ABI]
    	(basic_string::_M_copy_assign(const basic_string&, true_type)):
    	Remove.
    	* testsuite/21_strings/basic_string/allocator/char/copy_assign.cc:
    	Test cases where the allocators are equal or the string is small.
    	* testsuite/21_strings/basic_string/allocator/wchar_t/copy_assign.cc:
    	Likewise.

diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index 1bea4b4..268fb94 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -1955,9 +1955,6 @@ 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 97fe797..981ffc5 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -393,15 +393,6 @@ _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
@@ -636,12 +627,35 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
       operator=(const basic_string& __str)
       {
 #if __cplusplus >= 201103L
-	_M_copy_assign(__str,
-	    typename _Alloc_traits::propagate_on_container_copy_assignment());
-#else
-	this->_M_assign(__str);
+	if (_Alloc_traits::_S_propagate_on_copy_assign())
+	  {
+	    if (!_Alloc_traits::_S_always_equal() && !_M_is_local()
+		&& _M_get_allocator() != __str._M_get_allocator())
+	      {
+		// Propagating allocator cannot free existing storage so must
+		// deallocate it before replacing current allocator.
+		if (__str.size() <= _S_local_capacity)
+		  {
+		    _M_destroy(_M_allocated_capacity);
+		    _M_data(_M_local_data());
+		    _M_set_length(0);
+		  }
+		else
+		  {
+		    const auto __len = __str.size();
+		    auto __alloc = __str._M_get_allocator();
+		    // If this allocation throws there are no effects:
+		    auto __ptr = _Alloc_traits::allocate(__alloc, __len + 1);
+		    _M_destroy(_M_allocated_capacity);
+		    _M_data(__ptr);
+		    _M_capacity(__len);
+		    _M_set_length(__len);
+		  }
+	      }
+	    std::__alloc_on_copy(_M_get_allocator(), __str._M_get_allocator());
+	  }
 #endif
-	return *this;
+	return this->assign(__str);
       }
 
       /**
diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc
index adc8b85..41b7fa1 100644
--- a/libstdc++-v3/include/bits/basic_string.tcc
+++ b/libstdc++-v3/include/bits/basic_string.tcc
@@ -275,70 +275,6 @@ _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 0fc80d7..bea221d 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
@@ -121,6 +121,16 @@ void test03()
   VERIFY( caught );
   VERIFY( v1 == s1 );
   VERIFY( v1.get_allocator() == a1 );
+
+  throw_alloc::set_limit(1); // Allow one more allocation (and no more).
+  test_type v3(s1, a1);
+  // No allocation when allocators are equal and capacity is sufficient:
+  VERIFY( v1.capacity() >= v3.size() );
+  v1 = v3;
+  // No allocation when the contents fit in the small-string buffer:
+  v2 = "sso";
+  v1 = v2;
+  VERIFY( v1.get_allocator() == a2 );
 }
 
 int main()
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 c35e001..e83c4c5 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
@@ -121,6 +121,16 @@ void test03()
   VERIFY( caught );
   VERIFY( v1 == s1 );
   VERIFY( v1.get_allocator() == a1 );
+
+  throw_alloc::set_limit(1); // Allow one more allocation (and no more).
+  test_type v3(s1, a1);
+  // No allocation when allocators are equal and capacity is sufficient:
+  VERIFY( v1.capacity() >= v3.size() );
+  v1 = v3;
+  // No allocation when the contents fit in the small-string buffer:
+  v2 = L"sso";
+  v1 = v2;
+  VERIFY( v1.get_allocator() == a2 );
 }
 
 int main()