From 010211394dc2da35487ee0ce9e9f6cb13f343051 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Dumont?= Date: Tue, 27 Nov 2018 21:21:51 +0000 Subject: [PATCH] re PR libstdc++/88199 (memory leak on unordered container move assignment) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 2018-11-27 François Dumont PR libstdc++/88199 * include/bits/hashtable.h (_Hashtable<>::_M_assign_elements): New. (_Hashtable<>::operator=(const _Hashtable&)): Use latter. (_Hashtable<>::_M_move_assign(_Hashtable&&, false_type)): Likewise. * testsuite/23_containers/unordered_set/allocator/move_assign.cc (test03): New. From-SVN: r266528 --- libstdc++-v3/ChangeLog | 11 +- libstdc++-v3/include/bits/hashtable.h | 146 ++++++++---------- .../unordered_set/allocator/move_assign.cc | 128 +++++++++++---- 3 files changed, 171 insertions(+), 114 deletions(-) diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog index 11505c0f31f..4de7cc85174 100644 --- a/libstdc++-v3/ChangeLog +++ b/libstdc++-v3/ChangeLog @@ -1,3 +1,12 @@ +2018-11-27 François Dumont + + PR libstdc++/88199 + * include/bits/hashtable.h (_Hashtable<>::_M_assign_elements): New. + (_Hashtable<>::operator=(const _Hashtable&)): Use latter. + (_Hashtable<>::_M_move_assign(_Hashtable&&, false_type)): Likewise. + * testsuite/23_containers/unordered_set/allocator/move_assign.cc + (test03): New. + 2018-11-26 Jonathan Wakely * testsuite/26_numerics/complex/requirements/more_constexpr.cc: Fix @@ -20,7 +29,7 @@ more_constexpr.cc: New test. * testsuite/26_numerics/headers/complex/synopsis.cc: Add _GLIBCXX20_CONSTEXPR to applicable operators; Add missing proj(). - * testsuite/26_numerics/headers/complex/synopsis.cc: + * testsuite/26_numerics/headers/complex/synopsis.cc: Add _GLIBCXX20_CONSTEXPR to relevant decls. 2018-11-23 Martin Sebor diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h index af16e2c03cb..f6912db3e69 100644 --- a/libstdc++-v3/include/bits/hashtable.h +++ b/libstdc++-v3/include/bits/hashtable.h @@ -388,6 +388,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_begin() const { return static_cast<__node_type*>(_M_before_begin._M_nxt); } + // Assign *this using another _Hashtable instance. Either elements + // are copy or move depends on the _NodeGenerator. + template + void + _M_assign_elements(_Ht&&, const _NodeGenerator&); + template void _M_assign(const _Hashtable&, const _NodeGenerator&); @@ -1042,50 +1048,65 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } // Reuse allocated buckets and nodes. - __bucket_type* __former_buckets = nullptr; - std::size_t __former_bucket_count = _M_bucket_count; - const __rehash_state& __former_state = _M_rehash_policy._M_state(); - - if (_M_bucket_count != __ht._M_bucket_count) - { - __former_buckets = _M_buckets; - _M_buckets = _M_allocate_buckets(__ht._M_bucket_count); - _M_bucket_count = __ht._M_bucket_count; - } - else - __builtin_memset(_M_buckets, 0, - _M_bucket_count * sizeof(__bucket_type)); - - __try - { - __hashtable_base::operator=(__ht); - _M_element_count = __ht._M_element_count; - _M_rehash_policy = __ht._M_rehash_policy; - __reuse_or_alloc_node_type __roan(_M_begin(), *this); - _M_before_begin._M_nxt = nullptr; - _M_assign(__ht, - [&__roan](const __node_type* __n) - { return __roan(__n->_M_v()); }); - if (__former_buckets) - _M_deallocate_buckets(__former_buckets, __former_bucket_count); - } - __catch(...) - { - if (__former_buckets) - { - // Restore previous buckets. - _M_deallocate_buckets(); - _M_rehash_policy._M_reset(__former_state); - _M_buckets = __former_buckets; - _M_bucket_count = __former_bucket_count; - } - __builtin_memset(_M_buckets, 0, - _M_bucket_count * sizeof(__bucket_type)); - __throw_exception_again; - } + _M_assign_elements(__ht, + [](const __reuse_or_alloc_node_type& __roan, const __node_type* __n) + { return __roan(__n->_M_v()); }); return *this; } + template + template + void + _Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, + _H1, _H2, _Hash, _RehashPolicy, _Traits>:: + _M_assign_elements(_Ht&& __ht, const _NodeGenerator& __node_gen) + { + __bucket_type* __former_buckets = nullptr; + std::size_t __former_bucket_count = _M_bucket_count; + const __rehash_state& __former_state = _M_rehash_policy._M_state(); + + if (_M_bucket_count != __ht._M_bucket_count) + { + __former_buckets = _M_buckets; + _M_buckets = _M_allocate_buckets(__ht._M_bucket_count); + _M_bucket_count = __ht._M_bucket_count; + } + else + __builtin_memset(_M_buckets, 0, + _M_bucket_count * sizeof(__bucket_type)); + + __try + { + __hashtable_base::operator=(std::forward<_Ht>(__ht)); + _M_element_count = __ht._M_element_count; + _M_rehash_policy = __ht._M_rehash_policy; + __reuse_or_alloc_node_type __roan(_M_begin(), *this); + _M_before_begin._M_nxt = nullptr; + _M_assign(__ht, + [&__node_gen, &__roan](__node_type* __n) + { return __node_gen(__roan, __n); }); + if (__former_buckets) + _M_deallocate_buckets(__former_buckets, __former_bucket_count); + } + __catch(...) + { + if (__former_buckets) + { + // Restore previous buckets. + _M_deallocate_buckets(); + _M_rehash_policy._M_reset(__former_state); + _M_buckets = __former_buckets; + _M_bucket_count = __former_bucket_count; + } + __builtin_memset(_M_buckets, 0, + _M_bucket_count * sizeof(__bucket_type)); + __throw_exception_again; + } + } + template_M_v())); }); - __ht.clear(); - } - __catch(...) - { - if (__former_buckets) - { - _M_deallocate_buckets(); - _M_rehash_policy._M_reset(__former_state); - _M_buckets = __former_buckets; - _M_bucket_count = __former_bucket_count; - } - __builtin_memset(_M_buckets, 0, - _M_bucket_count * sizeof(__bucket_type)); - __throw_exception_again; - } + _M_assign_elements(std::move(__ht), + [](const __reuse_or_alloc_node_type& __roan, __node_type* __n) + { return __roan(std::move_if_noexcept(__n->_M_v())); }); + __ht.clear(); } } diff --git a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc index ef6c0deb1af..3a553897581 100644 --- a/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc +++ b/libstdc++-v3/testsuite/23_containers/unordered_set/allocator/move_assign.cc @@ -18,71 +18,133 @@ // { dg-do run { target c++11 } } #include + #include #include #include using __gnu_test::propagating_allocator; using __gnu_test::counter_type; +using __gnu_test::tracker_allocator; +using __gnu_test::tracker_allocator_counter; void test01() { - typedef propagating_allocator alloc_type; - typedef __gnu_test::counter_type_hasher hash; - typedef std::unordered_set, - alloc_type> test_type; + tracker_allocator_counter::reset(); + { + typedef propagating_allocator> alloc_type; + typedef __gnu_test::counter_type_hasher hash; + typedef std::unordered_set, + alloc_type> test_type; - test_type v1(alloc_type(1)); - v1.emplace(0); + test_type v1(alloc_type(1)); + v1.emplace(0); - test_type v2(alloc_type(2)); - v2.emplace(1); + test_type v2(alloc_type(2)); + v2.emplace(1); - counter_type::reset(); + counter_type::reset(); - v2 = std::move(v1); + v2 = std::move(v1); - VERIFY( 1 == v1.get_allocator().get_personality() ); - VERIFY( 2 == v2.get_allocator().get_personality() ); + VERIFY( 1 == v1.get_allocator().get_personality() ); + VERIFY( 2 == v2.get_allocator().get_personality() ); - VERIFY( counter_type::move_count == 1 ); - VERIFY( counter_type::destructor_count == 2 ); + VERIFY( counter_type::move_count == 1 ); + VERIFY( counter_type::destructor_count == 2 ); + } + + // Check there's nothing left allocated or constructed. + VERIFY( tracker_allocator_counter::get_construct_count() + == tracker_allocator_counter::get_destruct_count() ); + VERIFY( tracker_allocator_counter::get_allocation_count() + == tracker_allocator_counter::get_deallocation_count() ); } void test02() { - typedef propagating_allocator alloc_type; - typedef __gnu_test::counter_type_hasher hash; - typedef std::unordered_set, - alloc_type> test_type; + tracker_allocator_counter::reset(); + { + typedef propagating_allocator> alloc_type; + typedef __gnu_test::counter_type_hasher hash; + typedef std::unordered_set, + alloc_type> test_type; - test_type v1(alloc_type(1)); - v1.emplace(0); + test_type v1(alloc_type(1)); + v1.emplace(0); - auto it = v1.begin(); + auto it = v1.begin(); - test_type v2(alloc_type(2)); - v2.emplace(0); + test_type v2(alloc_type(2)); + v2.emplace(0); - counter_type::reset(); + counter_type::reset(); - v2 = std::move(v1); + v2 = std::move(v1); - VERIFY(0 == v1.get_allocator().get_personality()); - VERIFY(1 == v2.get_allocator().get_personality()); + VERIFY(0 == v1.get_allocator().get_personality()); + VERIFY(1 == v2.get_allocator().get_personality()); - VERIFY( counter_type::move_count == 0 ); - VERIFY( counter_type::copy_count == 0 ); - VERIFY( counter_type::destructor_count == 1 ); + VERIFY( counter_type::move_count == 0 ); + VERIFY( counter_type::copy_count == 0 ); + VERIFY( counter_type::destructor_count == 1 ); - VERIFY( it == v2.begin() ); + VERIFY( it == v2.begin() ); + } + + // Check there's nothing left allocated or constructed. + VERIFY( tracker_allocator_counter::get_construct_count() + == tracker_allocator_counter::get_destruct_count() ); + VERIFY( tracker_allocator_counter::get_allocation_count() + == tracker_allocator_counter::get_deallocation_count() ); +} + +void test03() +{ + tracker_allocator_counter::reset(); + { + typedef propagating_allocator> alloc_type; + typedef __gnu_test::counter_type_hasher hash; + typedef std::unordered_set, + alloc_type> test_type; + + test_type v1(alloc_type(1)); + v1.emplace(0); + + test_type v2(alloc_type(2)); + int i = 0; + v2.emplace(i++); + for (; v2.bucket_count() == v1.bucket_count(); ++i) + v2.emplace(i); + + counter_type::reset(); + + v2 = std::move(v1); + + VERIFY( 1 == v1.get_allocator().get_personality() ); + VERIFY( 2 == v2.get_allocator().get_personality() ); + + VERIFY( counter_type::move_count == 1 ); + VERIFY( counter_type::destructor_count == i + 1 ); + } + + // Check there's nothing left allocated or constructed. + VERIFY( tracker_allocator_counter::get_construct_count() + == tracker_allocator_counter::get_destruct_count() ); + VERIFY( tracker_allocator_counter::get_allocation_count() + == tracker_allocator_counter::get_deallocation_count() ); } int main() { test01(); test02(); + test03(); return 0; }