[PATCH] PR libstdc++/79254 fix exception-safety in std::string::operator=
Jonathan Wakely
jwakely@redhat.com
Wed Feb 1 12:12:00 GMT 2017
On 01/02/17 11:42 +0000, Jonathan Wakely wrote:
>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.
Here's the backport for the branches, which shows that the new
approach is much closer to the original code and much simpler.
Tested x86_64-linux, committed to gcc-6-branch and gcc-5-branch.
-------------- next part --------------
commit 90d239b147e4f72e9361e10c5d45470c4b52eb7f
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Wed Feb 1 04:27:20 2017 +0000
PR libstdc++/79254 fix exception-safety of std::string copy assignment
PR libstdc++/79254
* include/bits/basic_string.h [_GLIBCXX_USE_CXX11_ABI]
(basic_string::operator=(const basic_string&)): If source object is
small just deallocate, otherwise perform new allocation before
making any changes.
* testsuite/21_strings/basic_string/allocator/wchar_t/copy_assign.cc:
Test exception-safety of copy assignment when allocator propagates.
* testsuite/21_strings/basic_string/allocator/char/copy_assign.cc:
Likewise.
* testsuite/util/testsuite_allocator.h (uneq_allocator::swap): Make
std::swap visible.
diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
index f8f3f88..0352bf4 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -570,10 +570,25 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
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);
+ // 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());
}
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 3c8e440..645e3cb 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
@@ -1,4 +1,4 @@
-// Copyright (C) 2015-2016 Free Software Foundation, Inc.
+// Copyright (C) 2015-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
@@ -20,6 +20,7 @@
#include <string>
#include <testsuite_hooks.h>
#include <testsuite_allocator.h>
+#include <ext/throw_allocator.h>
#if _GLIBCXX_USE_CXX11_ABI
using C = char;
@@ -100,10 +101,44 @@ 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 );
+
+ 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()
{
test01();
test02();
+ test03();
return 0;
}
#else
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 3ac15bb..01cdba4 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
@@ -1,4 +1,4 @@
-// Copyright (C) 2015-2016 Free Software Foundation, Inc.
+// Copyright (C) 2015-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
@@ -20,6 +20,7 @@
#include <string>
#include <testsuite_hooks.h>
#include <testsuite_allocator.h>
+#include <ext/throw_allocator.h>
#if _GLIBCXX_USE_CXX11_ABI
using C = wchar_t;
@@ -100,10 +101,44 @@ 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 );
+
+ 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()
{
test01();
test02();
+ test03();
return 0;
}
#else
diff --git a/libstdc++-v3/testsuite/util/testsuite_allocator.h b/libstdc++-v3/testsuite/util/testsuite_allocator.h
index 4884600..f597a38 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
More information about the Libstdc++
mailing list