From 8cef7fd75ac3cbeb8efb767e5ba2dcdb76bf4454 Mon Sep 17 00:00:00 2001 From: Eric Fiselier Date: Tue, 5 Jun 2018 22:32:52 +0000 Subject: [PATCH] Fix PR37694 - std::vector doesn't correctly move construct allocators. C++2a[container.requirements.general]p8 states that when move constructing a container, the allocator is move constructed. Vector previously copy constructed these allocators. This patch fixes that bug. Additionally it cleans up some unnecessary allocator conversions when copy constructing containers. Libc++ uses __internal_allocator_traits::select_on_copy_construction to select the correct allocator during copy construction, but it unnecessarily converted the resulting allocator to the user specified allocator type and back. After this patch list and forward_list no longer do that. Technically we're supposed to be using allocator_traits::select_on_copy_construction, but that should seemingly be addressed as a separate patch, if at all. llvm-svn: 334053 --- libcxx/include/forward_list | 19 ++-- libcxx/include/list | 35 ++++--- libcxx/include/vector | 22 +++-- .../allocator_move.pass.cpp | 97 +++++++++++++++++++ .../sequences/vector.bool/move.pass.cpp | 31 ++++++ .../vector/vector.cons/move.pass.cpp | 33 +++++++ libcxx/test/support/test_allocator.h | 57 ++++++++++- 7 files changed, 264 insertions(+), 30 deletions(-) create mode 100644 libcxx/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp diff --git a/libcxx/include/forward_list b/libcxx/include/forward_list index c98ddc00a037..571afdc925b0 100644 --- a/libcxx/include/forward_list +++ b/libcxx/include/forward_list @@ -457,6 +457,10 @@ protected: typedef typename allocator_traits<__begin_node_allocator>::pointer __begin_node_pointer; + static_assert((!is_same::value), + "internal allocator type must differ from user-specified " + "type; otherwise overload resolution breaks"); + __compressed_pair<__begin_node, __node_allocator> __before_begin_; _LIBCPP_INLINE_VISIBILITY @@ -481,9 +485,11 @@ protected: _NOEXCEPT_(is_nothrow_default_constructible<__node_allocator>::value) : __before_begin_(__begin_node()) {} _LIBCPP_INLINE_VISIBILITY - __forward_list_base(const allocator_type& __a) + explicit __forward_list_base(const allocator_type& __a) : __before_begin_(__begin_node(), __node_allocator(__a)) {} - + _LIBCPP_INLINE_VISIBILITY + explicit __forward_list_base(const __node_allocator& __a) + : __before_begin_(__begin_node(), __a) {} #ifndef _LIBCPP_CXX03_LANG public: _LIBCPP_INLINE_VISIBILITY @@ -954,12 +960,9 @@ forward_list<_Tp, _Alloc>::forward_list(_InputIterator __f, _InputIterator __l, template forward_list<_Tp, _Alloc>::forward_list(const forward_list& __x) - : base(allocator_type( - __node_traits::select_on_container_copy_construction(__x.__alloc()) - ) - ) -{ - insert_after(cbefore_begin(), __x.begin(), __x.end()); + : base( + __node_traits::select_on_container_copy_construction(__x.__alloc())) { + insert_after(cbefore_begin(), __x.begin(), __x.end()); } template diff --git a/libcxx/include/list b/libcxx/include/list index 4b22017f713d..d2e78cd66afb 100644 --- a/libcxx/include/list +++ b/libcxx/include/list @@ -556,6 +556,9 @@ protected: typedef typename __rebind_alloc_helper<__alloc_traits, __node_base>::type __node_base_allocator; typedef typename allocator_traits<__node_base_allocator>::pointer __node_base_pointer; + static_assert((!is_same::value), + "internal allocator type must differ from user-specified " + "type; otherwise overload resolution breaks"); __node_base __end_; __compressed_pair __size_alloc_; @@ -590,6 +593,11 @@ protected: _NOEXCEPT_(is_nothrow_default_constructible<__node_allocator>::value); _LIBCPP_INLINE_VISIBILITY __list_imp(const allocator_type& __a); + _LIBCPP_INLINE_VISIBILITY + __list_imp(const __node_allocator& __a); +#ifndef _LIBCPP_CXX03_LANG + __list_imp(__node_allocator&& __a) _NOEXCEPT; +#endif ~__list_imp(); void clear() _NOEXCEPT; _LIBCPP_INLINE_VISIBILITY @@ -713,9 +721,18 @@ __list_imp<_Tp, _Alloc>::__list_imp(const allocator_type& __a) } template -__list_imp<_Tp, _Alloc>::~__list_imp() -{ - clear(); +inline __list_imp<_Tp, _Alloc>::__list_imp(const __node_allocator& __a) + : __size_alloc_(0, __a) {} + +#ifndef _LIBCPP_CXX03_LANG +template +inline __list_imp<_Tp, _Alloc>::__list_imp(__node_allocator&& __a) _NOEXCEPT + : __size_alloc_(0, std::move(__a)) {} +#endif + +template +__list_imp<_Tp, _Alloc>::~__list_imp() { + clear(); #if _LIBCPP_DEBUG_LEVEL >= 2 __get_db()->__erase_c(this); #endif @@ -1248,10 +1265,8 @@ list<_Tp, _Alloc>::list(_InpIter __f, _InpIter __l, const allocator_type& __a, template list<_Tp, _Alloc>::list(const list& __c) - : base(allocator_type( - __node_alloc_traits::select_on_container_copy_construction( - __c.__node_alloc()))) -{ + : base(__node_alloc_traits::select_on_container_copy_construction( + __c.__node_alloc())) { #if _LIBCPP_DEBUG_LEVEL >= 2 __get_db()->__insert_c(this); #endif @@ -1296,11 +1311,9 @@ list<_Tp, _Alloc>::list(initializer_list __il) } template -inline -list<_Tp, _Alloc>::list(list&& __c) +inline list<_Tp, _Alloc>::list(list&& __c) _NOEXCEPT_(is_nothrow_move_constructible<__node_allocator>::value) - : base(allocator_type(_VSTD::move(__c.__node_alloc()))) -{ + : base(_VSTD::move(__c.__node_alloc())) { #if _LIBCPP_DEBUG_LEVEL >= 2 __get_db()->__insert_c(this); #endif diff --git a/libcxx/include/vector b/libcxx/include/vector index 9e399bdd1f42..74c423ef2241 100644 --- a/libcxx/include/vector +++ b/libcxx/include/vector @@ -355,6 +355,9 @@ protected: __vector_base() _NOEXCEPT_(is_nothrow_default_constructible::value); _LIBCPP_INLINE_VISIBILITY __vector_base(const allocator_type& __a); +#ifndef _LIBCPP_CXX03_LANG + _LIBCPP_INLINE_VISIBILITY __vector_base(allocator_type&& __a) _NOEXCEPT; +#endif ~__vector_base(); _LIBCPP_INLINE_VISIBILITY @@ -438,6 +441,15 @@ __vector_base<_Tp, _Allocator>::__vector_base(const allocator_type& __a) { } +#ifndef _LIBCPP_CXX03_LANG +template +inline _LIBCPP_INLINE_VISIBILITY +__vector_base<_Tp, _Allocator>::__vector_base(allocator_type&& __a) _NOEXCEPT + : __begin_(nullptr), + __end_(nullptr), + __end_cap_(nullptr, std::move(__a)) {} +#endif + template __vector_base<_Tp, _Allocator>::~__vector_base() { @@ -2863,17 +2875,15 @@ vector::operator=(const vector& __v) #ifndef _LIBCPP_CXX03_LANG template -inline _LIBCPP_INLINE_VISIBILITY -vector::vector(vector&& __v) +inline _LIBCPP_INLINE_VISIBILITY vector::vector(vector&& __v) #if _LIBCPP_STD_VER > 14 - _NOEXCEPT + _NOEXCEPT #else - _NOEXCEPT_(is_nothrow_move_constructible::value) + _NOEXCEPT_(is_nothrow_move_constructible::value) #endif : __begin_(__v.__begin_), __size_(__v.__size_), - __cap_alloc_(__v.__cap_alloc_) -{ + __cap_alloc_(std::move(__v.__cap_alloc_)) { __v.__begin_ = nullptr; __v.__size_ = 0; __v.__cap() = 0; diff --git a/libcxx/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp b/libcxx/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp new file mode 100644 index 000000000000..da9b7e646cc0 --- /dev/null +++ b/libcxx/test/std/containers/container.requirements/container.requirements.general/allocator_move.pass.cpp @@ -0,0 +1,97 @@ +//===----------------------------------------------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is dual licensed under the MIT and the University of Illinois Open +// Source Licenses. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +// UNSUPPORTED: c++98, c++03 + +// C++2a[container.requirements.general]p8 +// Move constructors obtain an allocator by move construction from the allocator +// belonging to the container being moved. Such move construction of the +// allocator shall not exit via an exception. + +#include +#include +#include +#include +#include +#include +#include + +#include "test_macros.h" +#include "test_allocator.h" + +template +void test(int expected_num_allocs = 1) { + { + test_alloc_base::clear(); + using AllocT = typename C::allocator_type; + C v(AllocT(42, 101)); + + assert(test_alloc_base::count == expected_num_allocs); + + const int num_stored_allocs = test_alloc_base::count; + { + const AllocT& a = v.get_allocator(); + assert(test_alloc_base::count == 1 + num_stored_allocs); + assert(a.get_data() == 42); + assert(a.get_id() == 101); + } + assert(test_alloc_base::count == num_stored_allocs); + test_alloc_base::clear_ctor_counters(); + + C v2 = std::move(v); + assert(test_alloc_base::count == num_stored_allocs * 2); + assert(test_alloc_base::copied == 0); + assert(test_alloc_base::moved == num_stored_allocs); + { + const AllocT& a = v.get_allocator(); + assert(a.get_id() == test_alloc_base::moved_value); + assert(a.get_data() == test_alloc_base::moved_value); + } + { + const AllocT& a = v2.get_allocator(); + assert(a.get_id() == 101); + assert(a.get_data() == 42); + } + } +} + +int main() { + { // test sequence containers + test > >(); + test > >(); + test > >(); + test > >(); + } + { // test associative containers + test, test_allocator > >(); + test, test_allocator > >(); + + using KV = std::pair; + test, test_allocator > >(); + test, test_allocator > >(); + } + { // test unordered containers + // libc++ stores two allocators in the unordered containers. +#ifdef _LIBCPP_VERSION + int stored_allocators = 2; +#else + int stored_allocators = 1; +#endif + test, std::equal_to, + test_allocator > >(stored_allocators); + test, std::equal_to, + test_allocator > >(stored_allocators); + + using KV = std::pair; + test, std::equal_to, + test_allocator > >(stored_allocators); + test, std::equal_to, + test_allocator > >(stored_allocators); + } +} diff --git a/libcxx/test/std/containers/sequences/vector.bool/move.pass.cpp b/libcxx/test/std/containers/sequences/vector.bool/move.pass.cpp index f189e2b97092..ab2b7ce6d316 100644 --- a/libcxx/test/std/containers/sequences/vector.bool/move.pass.cpp +++ b/libcxx/test/std/containers/sequences/vector.bool/move.pass.cpp @@ -15,6 +15,7 @@ #include #include +#include "test_macros.h" #include "test_allocator.h" #include "min_allocator.h" @@ -59,4 +60,34 @@ int main() assert(l.empty()); assert(l2.get_allocator() == lo.get_allocator()); } + { + test_alloc_base::clear(); + using Vect = std::vector >; + using AllocT = Vect::allocator_type; + Vect v(test_allocator(42, 101)); + assert(test_alloc_base::count == 1); + { + const AllocT& a = v.get_allocator(); + assert(test_alloc_base::count == 2); + assert(a.get_data() == 42); + assert(a.get_id() == 101); + } + assert(test_alloc_base::count == 1); + test_alloc_base::clear_ctor_counters(); + + Vect v2 = std::move(v); + assert(test_alloc_base::count == 2); + assert(test_alloc_base::copied == 0); + assert(test_alloc_base::moved == 1); + { + const AllocT& a = v.get_allocator(); + assert(a.get_id() == test_alloc_base::moved_value); + assert(a.get_data() == test_alloc_base::moved_value); + } + { + const AllocT& a = v2.get_allocator(); + assert(a.get_id() == 101); + assert(a.get_data() == 42); + } + } } diff --git a/libcxx/test/std/containers/sequences/vector/vector.cons/move.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.cons/move.pass.cpp index cd50d5432349..6938aa679eba 100644 --- a/libcxx/test/std/containers/sequences/vector/vector.cons/move.pass.cpp +++ b/libcxx/test/std/containers/sequences/vector/vector.cons/move.pass.cpp @@ -15,10 +15,13 @@ #include #include + +#include "test_macros.h" #include "MoveOnly.h" #include "test_allocator.h" #include "min_allocator.h" #include "asan_testing.h" +#include "verbose_assert.h" int main() { @@ -98,4 +101,34 @@ int main() assert(*j == 3); assert(is_contiguous_container_asan_correct(c2)); } + { + test_alloc_base::clear(); + using Vect = std::vector >; + Vect v(test_allocator(42, 101)); + assert(test_alloc_base::count == 1); + assert(test_alloc_base::copied == 1); + assert(test_alloc_base::moved == 0); + { + const test_allocator& a = v.get_allocator(); + assert(a.get_data() == 42); + assert(a.get_id() == 101); + } + assert(test_alloc_base::count == 1); + test_alloc_base::clear_ctor_counters(); + + Vect v2 = std::move(v); + assert(test_alloc_base::count == 2); + assert(test_alloc_base::copied == 0); + assert(test_alloc_base::moved == 1); + { + const test_allocator& a = v.get_allocator(); + assert(a.get_id() == test_alloc_base::moved_value); + assert(a.get_data() == test_alloc_base::moved_value); + } + { + const test_allocator& a = v2.get_allocator(); + assert(a.get_id() == 101); + assert(a.get_data() == 42); + } + } } diff --git a/libcxx/test/support/test_allocator.h b/libcxx/test/support/test_allocator.h index e77796b676e7..60f9a21b244d 100644 --- a/libcxx/test/support/test_allocator.h +++ b/libcxx/test/support/test_allocator.h @@ -36,12 +36,37 @@ public: static int throw_after; static int count; static int alloc_count; + static int copied; + static int moved; + static int converted; + + const static int destructed_value = -1; + const static int default_value = 0; + const static int moved_value = INT_MAX; + + static void clear() { + assert(count == 0 && "clearing leaking allocator data?"); + count = 0; + time_to_throw = 0; + alloc_count = 0; + throw_after = INT_MAX; + clear_ctor_counters(); + } + + static void clear_ctor_counters() { + copied = 0; + moved = 0; + converted = 0; + } }; int test_alloc_base::count = 0; int test_alloc_base::time_to_throw = 0; int test_alloc_base::alloc_count = 0; int test_alloc_base::throw_after = INT_MAX; +int test_alloc_base::copied = 0; +int test_alloc_base::moved = 0; +int test_alloc_base::converted = 0; template class test_allocator @@ -65,13 +90,35 @@ public: test_allocator() TEST_NOEXCEPT : data_(0), id_(0) {++count;} explicit test_allocator(int i, int id = 0) TEST_NOEXCEPT : data_(i), id_(id) {++count;} - test_allocator(const test_allocator& a) TEST_NOEXCEPT - : data_(a.data_), id_(a.id_) {++count;} - template test_allocator(const test_allocator& a) TEST_NOEXCEPT - : data_(a.data_), id_(a.id_) {++count;} + test_allocator(const test_allocator& a) TEST_NOEXCEPT : data_(a.data_), + id_(a.id_) { + ++count; + ++copied; + assert(a.data_ != destructed_value && a.id_ != destructed_value && + "copying from destroyed allocator"); + } +#if TEST_STD_VER >= 11 + test_allocator(test_allocator&& a) TEST_NOEXCEPT : data_(a.data_), + id_(a.id_) { + ++count; + ++moved; + assert(a.data_ != destructed_value && a.id_ != destructed_value && + "moving from destroyed allocator"); + a.data_ = moved_value; + a.id_ = moved_value; + } +#endif + template + test_allocator(const test_allocator& a) TEST_NOEXCEPT : data_(a.data_), + id_(a.id_) { + ++count; + ++converted; + } ~test_allocator() TEST_NOEXCEPT { assert(data_ >= 0); assert(id_ >= 0); - --count; data_ = -1; id_ = -1; + --count; + data_ = destructed_value; + id_ = destructed_value; } pointer address(reference x) const {return &x;} const_pointer address(const_reference x) const {return &x;}