From patchwork Mon Nov 19 21:23:44 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Fran=C3=A7ois_Dumont?= X-Patchwork-Id: 1000106 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-490457-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="C3vkdPD7"; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="lONPp4N/"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 42zMKK1GPXz9s3x for ; Tue, 20 Nov 2018 08:24:04 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :subject:to:message-id:date:mime-version:content-type; q=dns; s= default; b=tzUDwg4hI6Vt9aM2l6Dh4dCH/P6tWVwQrCE7G6kmM7sZZNOnzP5Yq vCzoxTCWHGesGYQ1RacXJkD7CXoGHPaEyj4LoEg0yFVD6PaB48oJFW1To0BmzssZ jBoaZPps7We5WuK485XGC5kbfz5ChFay5o2yhX02AEfnd/8ehsCT+8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :subject:to:message-id:date:mime-version:content-type; s= default; bh=Qb+Xt16cPiXK6Y/EFgnvFuE34Qg=; b=C3vkdPD7m3HTo8gBRil2 UkG3NNL6GwpQbCSftv6oNdwUjbKadQNjf8AX/coBHVdlCfSjUQywxFCrEZK8Pwz+ +ja0PtR0CJ9lmil58LKYBomm1oQf1AiMTlsCWh4NwGrN2xu7k2OLTu/8OYu6WPwe D1/qbjAlX9QYuoM7APtH/yM= Received: (qmail 114724 invoked by alias); 19 Nov 2018 21:23:50 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 114704 invoked by uid 89); 19 Nov 2018 21:23:50 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm1-f46.google.com Received: from mail-wm1-f46.google.com (HELO mail-wm1-f46.google.com) (209.85.128.46) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 19 Nov 2018 21:23:48 +0000 Received: by mail-wm1-f46.google.com with SMTP id g131so151857wmg.3; Mon, 19 Nov 2018 13:23:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:subject:to:message-id:date:user-agent:mime-version :content-language; bh=8J/PTYcLbaZ8Bb6rz7AjKVhay8NjkQ/kHTj4ARtTLII=; b=lONPp4N/E7gR2LUnpMB/NZvcTmFeXFHGicMT8RJS86LqerPJkN01ZcwjmzXvJb2BYx xvg+dupptd9Fq0VdTn3BAtZr1spxzsmJz8piL6TNhfcYEQ6EY6Mm3t/Tji4UvAdF+42/ k+fOQ/tUwE0sU4BQ8BeSlD54+1g8/2qBV9zbJxpHzvgD8PP2F2L6SkR8IIhdidFgTqvs CI24Vp5zB1lx+N1vc9+8P0czhxs6X3Qwu9uZfdhgGGdg+2TYRKVybAgizjz9DUh52O67 R8cF5XkhNtZ9SDAbGEgIFzFRhfOjVDOpMRYk4I8bPWubPVwvKFYWTJAiS/U8jLqoq/GS Dz6Q== Received: from [192.168.0.22] (arf62-1-82-237-250-248.fbx.proxad.net. [82.237.250.248]) by smtp.googlemail.com with ESMTPSA id x12sm17994983wrt.20.2018.11.19.13.23.44 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 19 Nov 2018 13:23:45 -0800 (PST) From: =?utf-8?q?Fran=C3=A7ois_Dumont?= Subject: Fix hashtable memory leak To: "libstdc++@gcc.gnu.org" , gcc-patches Message-ID: Date: Mon, 19 Nov 2018 22:23:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 There a memory leak when move assigning between 2 instances with different bucket count and non-propagating and unequal allocator instances (see new test03). I reduced code duplication to fix the issue as 1 of the 2 implementations was fine.     * include/bits/hashtable.h (_Hashtable<>::_M_replicate): 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. I still need to run all tests but new move_assign.cc works fine. Ok to commit once fully tested ? François diff --git a/libstdc++-v3/include/bits/hashtable.h b/libstdc++-v3/include/bits/hashtable.h index efc4c4ab94f..61e177abb45 100644 --- a/libstdc++-v3/include/bits/hashtable.h +++ b/libstdc++-v3/include/bits/hashtable.h @@ -398,6 +398,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_begin() const { return static_cast<__node_type*>(_M_before_begin._M_nxt); } + template + void + _M_replicate(_Ht&&, const _NodeGenerator&); + template void _M_assign(const _Hashtable&, const _NodeGenerator&); @@ -1058,6 +1062,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } // Reuse allocated buckets and nodes. + __reuse_or_alloc_node_type __roan(_M_begin(), *this); + _M_replicate(__ht, + [&__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_replicate(_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(); @@ -1074,14 +1095,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __try { - __hashtable_base::operator=(__ht); + __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, - [&__roan](const __node_type* __n) - { return __roan(__n->_M_v()); }); + _M_assign(__ht, __node_gen); if (__former_buckets) _M_deallocate_buckets(__former_buckets, __former_bucket_count); } @@ -1099,7 +1117,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_bucket_count * sizeof(__bucket_type)); __throw_exception_again; } - return *this; } 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; - } - } } template +#include + #include #include #include @@ -27,7 +29,9 @@ using __gnu_test::counter_type; void test01() { - typedef propagating_allocator alloc_type; + { + typedef propagating_allocator> alloc_type; typedef __gnu_test::counter_type_hasher hash; typedef std::unordered_set, @@ -50,9 +54,15 @@ void test01() VERIFY( counter_type::destructor_count == 2 ); } + // Check there's nothing left allocated or constructed. + __gnu_cxx::annotate_base::check(); +} + void test02() { - typedef propagating_allocator alloc_type; + { + typedef propagating_allocator> alloc_type; typedef __gnu_test::counter_type_hasher hash; typedef std::unordered_set, @@ -80,9 +90,48 @@ void test02() VERIFY( it == v2.begin() ); } + // Check there's nothing left allocated or constructed. + __gnu_cxx::annotate_base::check(); +} + +void test03() +{ + { + 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. + __gnu_cxx::annotate_base::check(); +} + int main() { test01(); test02(); + test03(); return 0; }