From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa1.mentor.iphmx.com (esa1.mentor.iphmx.com [68.232.129.153]) by sourceware.org (Postfix) with ESMTPS id 50881394AC18; Mon, 16 Aug 2021 12:44:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 50881394AC18 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com IronPort-SDR: ur0eTq5Jv/M0YRHtrHLzsXnBQOWlP0n4pQZa1BXS8IhBamNAFH/P+Xnjqg3lY9uk5DYKyhWYtz 4QzBScyhPS/QijVM/bwUgGFiH99OmnSBOjmU9dDFUV71Hjb1rGo0zzuWm/Yv6zAHIofIZHnelQ EGR+2Nd9xqU9qhDJ3qgTb1Br3b9D/z5Kzre1yYCcAO5tPrAfVcfNGnqPKsSY8Unl1DeyRfweob msr0rGDEIMg9tI/HPo9zaxj+5a7QaokujW5M/1Dd+Ji2wStYiJ4MfXN6o3VWdz67xKfVIJDPkm b63ko65u4+8pU0u+P3wMtcBC X-IronPort-AV: E=Sophos;i="5.84,324,1620720000"; d="scan'208,223";a="67223133" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa1.mentor.iphmx.com with ESMTP; 16 Aug 2021 04:44:52 -0800 IronPort-SDR: 1zpLZ5zGPh2BHnJYfGqlb81LoytYX9go9fhtn9v1+bzb80N9F2/FUCPPJkMWcPICywXvKtf1gR CJd+1h/QtsM5fXu6aY8rgudTXRlojYYBuOu0qkTEsllvrmutfHvGuulAql5obRRJrN0vkiBEFx B28Ulm0utpcsKHi+Pi0r+6GmnVeyNK96V1L0UwhZfTfUrglQY1UxUOqFUcnLYkO+C51mZ3Tvb9 wvVK8CGai4KVX7GYUAmA5H4VGD10Sf61zhzOlIVbbclqQ9iWxQDCRrbiCgQblc/5pSSGLgr1OJ 6JY= From: Thomas Schwinge To: Jonathan Wakely , Richard Biener , Martin Sebor , , Subject: Re: 'hash_map>' In-Reply-To: References: <87r1f6qzmx.fsf@euler.schwinge.homeip.net> User-Agent: Notmuch/0.29.3+94~g74c3f1b (https://notmuchmail.org) Emacs/27.1 (x86_64-pc-linux-gnu) Date: Mon, 16 Aug 2021 14:44:43 +0200 Message-ID: <87czqd7e4k.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-06.mgc.mentorg.com (139.181.222.6) To svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP, URIBL_SBL_A autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Aug 2021 12:45:03 -0000 --=-=-= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi! On 2021-08-12T17:15:44-0600, Martin Sebor via Gcc wrote: > On 8/6/21 10:57 AM, Thomas Schwinge wrote: >> So I'm trying to do some C++... ;-) >> >> Given: >> >> /* A map from SSA names or var decls to record fields. */ >> typedef hash_map field_map_t; >> >> /* For each propagation record type, this is a map from SSA names o= r var decls >> to propagate, to the field in the record type that should be use= d for >> transmission and reception. */ >> typedef hash_map record_field_map_t; >> >> Thus, that's a 'hash_map>'. (I may do that, >> right?) Looking through GCC implementation files, very most of all uses >> of 'hash_map' boil down to pointer key ('tree', for example) and >> pointer/integer value. > > Right. Because most GCC containers rely exclusively on GCC's own > uses for testing, if your use case is novel in some way, chances > are it might not work as intended in all circumstances. > > I've wrestled with hash_map a number of times. A use case that's > close to yours (i.e., a non-trivial value type) is in cp/parser.c: > see class_to_loc_map_t. Indeed, at the time you sent this email, I already had started looking into that one! (The Fortran test cases that I originally analyzed, which triggered other cases of non-POD/non-trivial destructor, all didn't result in a memory leak, because the non-trivial constructor doesn't actually allocate any resources dynamically -- that's indeed different in this case here.) ..., and indeed: > (I don't remember if I tested it for leaks > though. It's used to implement -Wmismatched-tags so compiling > a few tests under Valgrind should show if it does leak.) ... it does leak memory at present. :-| (See attached commit log for details for one example.) To that effect, to document the current behavior, I propose to "Add more self-tests for 'hash_map' with Value type with non-trivial constructor/destructor", see attached. OK to push to master branch? (Also cherry-pick into release branches, eventually?) >> Then: >> >> record_field_map_t field_map ([...]); // see below >> for ([...]) >> { >> tree record_type =3D [...]; >> [...] >> bool existed; >> field_map_t &fields >> =3D field_map.get_or_insert (record_type, &existed); >> gcc_checking_assert (!existed); >> [...] >> for ([...]) >> fields.put ([...], [...]); >> [...] >> } >> [stuff that looks up elements from 'field_map'] >> field_map.empty (); >> >> This generally works. >> >> If I instantiate 'record_field_map_t field_map (40);', Valgrind is happy= . >> If however I instantiate 'record_field_map_t field_map (13);' (where '13= ' >> would be the default for 'hash_map'), Valgrind complains: >> >> 2,080 bytes in 10 blocks are definitely lost in loss record 828 of = 876 >> at 0x483DD99: calloc (vg_replace_malloc.c:762) >> by 0x175F010: xcalloc (xmalloc.c:162) >> by 0xAF4A2C: hash_table, tree_node*> >::hash_entry, = false, xcallocator>::hash_table(unsigned long, bool, bool, bool, mem_alloc_= origin) (hash-table.h:275) >> by 0x15E0120: hash_map, tree_node*> >::hash_map(unsigned long= , bool, bool, bool) (hash-map.h:143) >> by 0x15DEE87: hash_map, tree_node*> >, s= imple_hashmap_traits, hash_map, tree_nod= e*> > > >::get_or_insert(tree_node* const&, bool*) (hash-map.h:205) >> by 0x15DD52C: execute_omp_oacc_neuter_broadcast() (omp-oacc-neut= er-broadcast.cc:1371) >> [...] >> >> (That's with '#pragma GCC optimize "O0"' at the top of the 'gcc/*.cc' >> file.) >> >> My suspicion was that it is due to the 'field_map' getting resized as it >> incrementally grows (and '40' being big enough for that to never happen)= , >> and somehow the non-POD (?) value objects not being properly handled >> during that. Working my way a bit through 'gcc/hash-map.*' and >> 'gcc/hash-table.*' (but not claiming that I understand all that, off >> hand), it seems as if my theory is right: I'm able to plug this memory >> leak as follows: >> >> --- gcc/hash-table.h >> +++ gcc/hash-table.h >> @@ -820,6 +820,8 @@ hash_table::expand= () >> { >> value_type *q =3D find_empty_slot_for_expand (Descriptor= ::hash (x)); >> new ((void*) q) value_type (std::move (x)); >> + //BAD Descriptor::remove (x); // (doesn't make sense and) a t= on of "Invalid read [...] inside a block of size [...] free'd" >> + x.~value_type (); //GOOD This seems to work! -- but does it = make sense? >> } >> >> p++; >> >> However, that doesn't exactly look like a correct fix, does it? I'd >> expect such a manual destructor call in combination with placement new >> (that is being used here, obviously) -- but this is after 'std::move'? >> However, this also survives a smoke-test-like run of parts of the GCC >> testsuite, bootstrap and complete run now ongoing. > > If explicitly calling the dtor on the moved object is the right thing > to do I'd expect to see such invocations elsewhere in hash_table but > I don't. It does seem like removed elements ought to be destroyed, > but it also seems like the destruction should go through some traits > class (e.g., call Descriptor::remove and mark_deleted or do some > similar dance), and be called from other member functions that move > elements. So, we shall assume that any "real C++" use case (that is, C++ class) is using the appropriate C++ method, that is, 'typed_delete_remove', which does 'delete', which does destroy the C++ object, if applicable, else 'typed_noop_remove'. (Shall we look into the few places that use 'typed_free_remove' via 'free_ptr_hash', and potentially replace them with 'typed_delete_remove'? Or is there a reason for the two schemes to co-exist, other than for legacy reasons? Anyway, that's for another day.) What is different in the 'hash_table::expand' case is that all the Value objects do get 'std::move'd into a new blob of memory via placement new (so a subsequent 'delete' via 'typed_delete_remove' is not appropriate), but then the stale Value objects never get destructed. And indeed an explicit destructor call (which, as I understand is a no-op for primitive types; "pseudo destructor") is the right thing to do; see and others, for example. (Therefore, I don't think this needs to be routed through a "traits" function, but can rather be done in-line here, after each placement new, before deallocation of the original blob of memory. Also, I argue it's the right thing to do also for 'm_ggc', because even if in that case we're not going to leak memory (GC will reclaim), but we still may leak other resources dynamically allocated in a non-trivial constructor.) The attached "Fix 'hash_table::expand' to destruct stale Value objects" does prevent my original problem, does address the current 'class2loc' memory leak in 'cp/parser.c' (see commit log for one example), and adjusts the new 'gcc/hash-map-tests.c:test_map_of_type_with_ctor_and_dtor_expand' test case such that number of constructor calls matches the number of destructor calls. After some careful review regarding C++ details (please!), OK to push to master branch? (Also cherry-pick into release branches, eventually?) Is the source code comment that I'm adding sufficiently explanatory and correct in C++ terms? Gr=C3=BC=C3=9Fe Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename="0001-Add-more-self-tests-for-hash_map-with-Value-type-wit.patch" >From 12fda2ece45ea477cdc9a697b5efc6e51c9da229 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Fri, 13 Aug 2021 17:53:12 +0200 Subject: [PATCH] Add more self-tests for 'hash_map' with Value type with non-trivial constructor/destructor ... to document the current behavior. gcc/ * hash-map-tests.c (test_map_of_type_with_ctor_and_dtor): Extend. (test_map_of_type_with_ctor_and_dtor_expand): Add function. (hash_map_tests_c_tests): Call it. --- gcc/hash-map-tests.c | 142 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/gcc/hash-map-tests.c b/gcc/hash-map-tests.c index 5b6b192cd28..3c79a13c1a8 100644 --- a/gcc/hash-map-tests.c +++ b/gcc/hash-map-tests.c @@ -278,6 +278,146 @@ test_map_of_type_with_ctor_and_dtor () ASSERT_TRUE (val_t::ndefault + val_t::ncopy == val_t::ndtor); } + + + /* Verify basic construction and destruction of Value objects. */ + { + const int N_elem = 1234; + void *a[N_elem]; + for (int i = 0; i < N_elem; ++i) + a[i] = &a[i]; + + const int N_init = 44; + + val_t::ndefault = 0; + val_t::ncopy = 0; + val_t::nassign = 0; + val_t::ndtor = 0; + Map m (N_init); + ASSERT_EQ (val_t::ndefault + + val_t::ncopy + + val_t::nassign + + val_t::ndtor, 0); + + for (int i = 0; i < N_elem; ++i) + { + m.get_or_insert (a[i]); + ASSERT_EQ (val_t::ndefault, 1 + i); + ASSERT_EQ (val_t::ncopy, 0); + ASSERT_EQ (val_t::nassign, 0); + ASSERT_EQ (val_t::ndtor, i); + + m.remove (a[i]); + ASSERT_EQ (val_t::ndefault, 1 + i); + ASSERT_EQ (val_t::ncopy, 0); + ASSERT_EQ (val_t::nassign, 0); + ASSERT_EQ (val_t::ndtor, 1 + i); + } + } +} + +/* Verify 'hash_table::expand'. */ + +static void +test_map_of_type_with_ctor_and_dtor_expand (bool remove_some_inline) +{ + typedef hash_map Map; + + /* Note that we are starting with a fresh 'Map'. Even if an existing one has + been cleared out completely, there remain 'deleted' elements, and these + would disturb the following logic, where we don't have access to the + actual 'm_n_deleted' value. */ + size_t m_n_deleted = 0; + + const int N_elem = 1234; + void *a[N_elem]; + for (int i = 0; i < N_elem; ++i) + a[i] = &a[i]; + + const int N_init = 4; + + val_t::ndefault = 0; + val_t::ncopy = 0; + val_t::nassign = 0; + val_t::ndtor = 0; + Map m (N_init); + + /* In the following, in particular related to 'expand', we're adapting from + the internal logic of 'hash_table', glossing over "some details" not + relevant for this testing here. */ + + size_t m_size; + { + unsigned int size_prime_index_ = hash_table_higher_prime_index (N_init); + m_size = prime_tab[size_prime_index_].prime; + } + int n_expand_moved = 0; + + for (int i = 0; i < N_elem; ++i) + { + int elts = m.elements (); + + /* Per 'hash_table::find_slot_with_hash'. */ + size_t m_n_elements = elts + m_n_deleted; + bool expand = m_size * 3 <= m_n_elements * 4; + m.get_or_insert (a[i]); + if (expand) + { + /* Per 'hash_table::expand'. */ + { + unsigned int nindex = hash_table_higher_prime_index (elts * 2); + m_size = prime_tab[nindex].prime; + } + m_n_deleted = 0; + + /* All non-deleted elements have been moved. */ + n_expand_moved += i; + if (remove_some_inline) + n_expand_moved -= (i + 2) / 3; + } + + ASSERT_EQ (val_t::ndefault, 1 + i); + ASSERT_EQ (val_t::ncopy, n_expand_moved); + ASSERT_EQ (val_t::nassign, 0); + if (remove_some_inline) + ASSERT_EQ (val_t::ndtor, (i + 2) / 3); + else + ASSERT_EQ (val_t::ndtor, 0); + + /* Remove some inline. This never triggers an 'expand', but does + influence any following via 'm_n_deleted'. */ + if (remove_some_inline + && !(i % 3)) + { + m.remove (a[i]); + /* Per 'hash_table::remove_elt_with_hash'. */ + m_n_deleted++; + + ASSERT_EQ (val_t::ndefault, 1 + i); + ASSERT_EQ (val_t::ncopy, n_expand_moved); + ASSERT_EQ (val_t::nassign, 0); + ASSERT_EQ (val_t::ndtor, 1 + (i + 2) / 3); + } + } + + int ndefault = val_t::ndefault; + int ncopy = val_t::ncopy; + int nassign = val_t::nassign; + int ndtor = val_t::ndtor; + + for (int i = 0; i < N_elem; ++i) + { + if (remove_some_inline + && !(i % 3)) + continue; + + m.remove (a[i]); + ++ndtor; + ASSERT_EQ (val_t::ndefault, ndefault); + ASSERT_EQ (val_t::ncopy, ncopy); + ASSERT_EQ (val_t::nassign, nassign); + ASSERT_EQ (val_t::ndtor, ndtor); + } } /* Test calling empty on a hash_map that has a key type with non-zero @@ -309,6 +449,8 @@ hash_map_tests_c_tests () test_map_of_strings_to_int (); test_map_of_int_to_strings (); test_map_of_type_with_ctor_and_dtor (); + test_map_of_type_with_ctor_and_dtor_expand (false); + test_map_of_type_with_ctor_and_dtor_expand (true); test_nonzero_empty_key (); } -- 2.30.2 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename="0002-Fix-hash_table-expand-to-destruct-stale-Value-object.patch" >From 404d5b985c24d2a15d1908229456e6a8663b1ff3 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Fri, 13 Aug 2021 18:03:38 +0200 Subject: [PATCH] Fix 'hash_table::expand' to destruct stale Value objects Thus plugging potentional memory leaks if these have non-trivial constructor/destructor. See and others. As one example, compilation of 'g++.dg/warn/Wmismatched-tags.C' per 'valgrind --leak-check=full' improves as follows: [...] - -104 bytes in 1 blocks are definitely lost in loss record 399 of 519 - at 0x483DFAF: realloc (vg_replace_malloc.c:836) - by 0x223B62C: xrealloc (xmalloc.c:179) - by 0xA8D848: void va_heap::reserve(vec*&, unsigned int, bool) (vec.h:290) - by 0xA8B373: vec::reserve(unsigned int, bool) (vec.h:1858) - by 0xA8B277: vec::safe_push(class_decl_loc_t::class_key_loc_t const&) (vec.h:1967) - by 0xA57481: class_decl_loc_t::add_or_diag_mismatched_tag(tree_node*, tag_types, bool, bool) (parser.c:32967) - by 0xA573E1: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32941) - by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819) - by 0xA3AD12: cp_parser_elaborated_type_specifier(cp_parser*, bool, bool) (parser.c:20227) - by 0xA37EF2: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18942) - by 0xA31CDD: cp_parser_decl_specifier_seq(cp_parser*, int, cp_decl_specifier_seq*, int*) (parser.c:15517) - by 0xA43C71: cp_parser_parameter_declaration(cp_parser*, int, bool, bool*) (parser.c:24242) - -168 bytes in 3 blocks are definitely lost in loss record 422 of 519 - at 0x483DFAF: realloc (vg_replace_malloc.c:836) - by 0x223B62C: xrealloc (xmalloc.c:179) - by 0xA8D848: void va_heap::reserve(vec*&, unsigned int, bool) (vec.h:290) - by 0xA8B373: vec::reserve(unsigned int, bool) (vec.h:1858) - by 0xA8B277: vec::safe_push(class_decl_loc_t::class_key_loc_t const&) (vec.h:1967) - by 0xA57481: class_decl_loc_t::add_or_diag_mismatched_tag(tree_node*, tag_types, bool, bool) (parser.c:32967) - by 0xA573E1: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32941) - by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819) - by 0xA3AD12: cp_parser_elaborated_type_specifier(cp_parser*, bool, bool) (parser.c:20227) - by 0xA37EF2: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18942) - by 0xA31CDD: cp_parser_decl_specifier_seq(cp_parser*, int, cp_decl_specifier_seq*, int*) (parser.c:15517) - by 0xA53385: cp_parser_single_declaration(cp_parser*, vec*, bool, bool, bool*) (parser.c:31072) - -488 bytes in 7 blocks are definitely lost in loss record 449 of 519 - at 0x483DFAF: realloc (vg_replace_malloc.c:836) - by 0x223B62C: xrealloc (xmalloc.c:179) - by 0xA8D848: void va_heap::reserve(vec*&, unsigned int, bool) (vec.h:290) - by 0xA8B373: vec::reserve(unsigned int, bool) (vec.h:1858) - by 0xA8B277: vec::safe_push(class_decl_loc_t::class_key_loc_t const&) (vec.h:1967) - by 0xA57481: class_decl_loc_t::add_or_diag_mismatched_tag(tree_node*, tag_types, bool, bool) (parser.c:32967) - by 0xA573E1: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32941) - by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819) - by 0xA3AD12: cp_parser_elaborated_type_specifier(cp_parser*, bool, bool) (parser.c:20227) - by 0xA37EF2: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18942) - by 0xA31CDD: cp_parser_decl_specifier_seq(cp_parser*, int, cp_decl_specifier_seq*, int*) (parser.c:15517) - by 0xA49508: cp_parser_member_declaration(cp_parser*) (parser.c:26440) - -728 bytes in 7 blocks are definitely lost in loss record 455 of 519 - at 0x483B7F3: malloc (vg_replace_malloc.c:309) - by 0x223B63F: xrealloc (xmalloc.c:177) - by 0xA8D848: void va_heap::reserve(vec*&, unsigned int, bool) (vec.h:290) - by 0xA8B373: vec::reserve(unsigned int, bool) (vec.h:1858) - by 0xA57508: class_decl_loc_t::add_or_diag_mismatched_tag(tree_node*, tag_types, bool, bool) (parser.c:32980) - by 0xA573E1: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32941) - by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819) - by 0xA48BC6: cp_parser_class_head(cp_parser*, bool*) (parser.c:26090) - by 0xA4674B: cp_parser_class_specifier_1(cp_parser*) (parser.c:25302) - by 0xA47D76: cp_parser_class_specifier(cp_parser*) (parser.c:25680) - by 0xA37E27: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18912) - by 0xA31CDD: cp_parser_decl_specifier_seq(cp_parser*, int, cp_decl_specifier_seq*, int*) (parser.c:15517) - -832 bytes in 8 blocks are definitely lost in loss record 458 of 519 - at 0x483B7F3: malloc (vg_replace_malloc.c:309) - by 0x223B63F: xrealloc (xmalloc.c:177) - by 0xA8D848: void va_heap::reserve(vec*&, unsigned int, bool) (vec.h:290) - by 0xA901ED: bool vec_safe_reserve(vec*&, unsigned int, bool) (vec.h:697) - by 0xA8F161: void vec_alloc(vec*&, unsigned int) (vec.h:718) - by 0xA8D18D: vec::copy() const (vec.h:979) - by 0xA8B0C3: vec::copy() const (vec.h:1824) - by 0xA896B1: class_decl_loc_t::operator=(class_decl_loc_t const&) (parser.c:32697) - by 0xA571FD: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32899) - by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819) - by 0xA3AD12: cp_parser_elaborated_type_specifier(cp_parser*, bool, bool) (parser.c:20227) - by 0xA37EF2: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18942) - -1,144 bytes in 11 blocks are definitely lost in loss record 466 of 519 - at 0x483B7F3: malloc (vg_replace_malloc.c:309) - by 0x223B63F: xrealloc (xmalloc.c:177) - by 0xA8D848: void va_heap::reserve(vec*&, unsigned int, bool) (vec.h:290) - by 0xA901ED: bool vec_safe_reserve(vec*&, unsigned int, bool) (vec.h:697) - by 0xA8F161: void vec_alloc(vec*&, unsigned int) (vec.h:718) - by 0xA8D18D: vec::copy() const (vec.h:979) - by 0xA8B0C3: vec::copy() const (vec.h:1824) - by 0xA896B1: class_decl_loc_t::operator=(class_decl_loc_t const&) (parser.c:32697) - by 0xA571FD: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32899) - by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819) - by 0xA48BC6: cp_parser_class_head(cp_parser*, bool*) (parser.c:26090) - by 0xA4674B: cp_parser_class_specifier_1(cp_parser*) (parser.c:25302) - -1,376 bytes in 10 blocks are definitely lost in loss record 467 of 519 - at 0x483DFAF: realloc (vg_replace_malloc.c:836) - by 0x223B62C: xrealloc (xmalloc.c:179) - by 0xA8D848: void va_heap::reserve(vec*&, unsigned int, bool) (vec.h:290) - by 0xA8B373: vec::reserve(unsigned int, bool) (vec.h:1858) - by 0xA8B277: vec::safe_push(class_decl_loc_t::class_key_loc_t const&) (vec.h:1967) - by 0xA57481: class_decl_loc_t::add_or_diag_mismatched_tag(tree_node*, tag_types, bool, bool) (parser.c:32967) - by 0xA573E1: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32941) - by 0xA56C52: cp_parser_check_class_key(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32819) - by 0xA3AD12: cp_parser_elaborated_type_specifier(cp_parser*, bool, bool) (parser.c:20227) - by 0xA37EF2: cp_parser_type_specifier(cp_parser*, int, cp_decl_specifier_seq*, bool, int*, bool*) (parser.c:18942) - by 0xA31CDD: cp_parser_decl_specifier_seq(cp_parser*, int, cp_decl_specifier_seq*, int*) (parser.c:15517) - by 0xA301E0: cp_parser_simple_declaration(cp_parser*, bool, tree_node**) (parser.c:14772) - -3,552 bytes in 33 blocks are definitely lost in loss record 483 of 519 - at 0x483B7F3: malloc (vg_replace_malloc.c:309) - by 0x223B63F: xrealloc (xmalloc.c:177) - by 0xA8D848: void va_heap::reserve(vec*&, unsigned int, bool) (vec.h:290) - by 0xA901ED: bool vec_safe_reserve(vec*&, unsigned int, bool) (vec.h:697) - by 0xA8F161: void vec_alloc(vec*&, unsigned int) (vec.h:718) - by 0xA8D18D: vec::copy() const (vec.h:979) - by 0xA8B0C3: vec::copy() const (vec.h:1824) - by 0xA8964A: class_decl_loc_t::class_decl_loc_t(class_decl_loc_t const&) (parser.c:32689) - by 0xA8F515: hash_table, class_decl_loc_t> >::hash_entry, false, xcallocator>::expand() (hash-table.h:839) - by 0xA8D4B3: hash_table, class_decl_loc_t> >::hash_entry, false, xcallocator>::find_slot_with_hash(tree_node* const&, unsigned int, insert_option) (hash-table.h:1008) - by 0xA8B1DC: hash_map, class_decl_loc_t> >::get_or_insert(tree_node* const&, bool*) (hash-map.h:200) - by 0xA57128: class_decl_loc_t::add(cp_parser*, unsigned int, tag_types, tree_node*, bool, bool) (parser.c:32888) [...] LEAK SUMMARY: - definitely lost: 8,440 bytes in 81 blocks + definitely lost: 48 bytes in 1 blocks indirectly lost: 12,529 bytes in 329 blocks possibly lost: 0 bytes in 0 blocks still reachable: 1,644,376 bytes in 768 blocks gcc/ * hash-table.h (hash_table::expand): Destruct stale Value objects. * hash-map-tests.c (test_map_of_type_with_ctor_and_dtor_expand): Update. --- gcc/hash-map-tests.c | 9 +++++---- gcc/hash-table.h | 4 ++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/gcc/hash-map-tests.c b/gcc/hash-map-tests.c index 3c79a13c1a8..d49eafa0bb9 100644 --- a/gcc/hash-map-tests.c +++ b/gcc/hash-map-tests.c @@ -304,31 +304,31 @@ test_map_of_type_with_ctor_and_dtor () m.get_or_insert (a[i]); ASSERT_EQ (val_t::ndefault, 1 + i); ASSERT_EQ (val_t::ncopy, 0); ASSERT_EQ (val_t::nassign, 0); ASSERT_EQ (val_t::ndtor, i); m.remove (a[i]); ASSERT_EQ (val_t::ndefault, 1 + i); ASSERT_EQ (val_t::ncopy, 0); ASSERT_EQ (val_t::nassign, 0); ASSERT_EQ (val_t::ndtor, 1 + i); } } } -/* Verify 'hash_table::expand'. */ +/* Verify that 'hash_table::expand' doesn't leak Value objects. */ static void test_map_of_type_with_ctor_and_dtor_expand (bool remove_some_inline) { typedef hash_map Map; /* Note that we are starting with a fresh 'Map'. Even if an existing one has been cleared out completely, there remain 'deleted' elements, and these would disturb the following logic, where we don't have access to the actual 'm_n_deleted' value. */ size_t m_n_deleted = 0; const int N_elem = 1234; void *a[N_elem]; for (int i = 0; i < N_elem; ++i) @@ -368,68 +368,69 @@ test_map_of_type_with_ctor_and_dtor_expand (bool remove_some_inline) unsigned int nindex = hash_table_higher_prime_index (elts * 2); m_size = prime_tab[nindex].prime; } m_n_deleted = 0; /* All non-deleted elements have been moved. */ n_expand_moved += i; if (remove_some_inline) n_expand_moved -= (i + 2) / 3; } ASSERT_EQ (val_t::ndefault, 1 + i); ASSERT_EQ (val_t::ncopy, n_expand_moved); ASSERT_EQ (val_t::nassign, 0); if (remove_some_inline) - ASSERT_EQ (val_t::ndtor, (i + 2) / 3); + ASSERT_EQ (val_t::ndtor, n_expand_moved + (i + 2) / 3); else - ASSERT_EQ (val_t::ndtor, 0); + ASSERT_EQ (val_t::ndtor, n_expand_moved); /* Remove some inline. This never triggers an 'expand', but does influence any following via 'm_n_deleted'. */ if (remove_some_inline && !(i % 3)) { m.remove (a[i]); /* Per 'hash_table::remove_elt_with_hash'. */ m_n_deleted++; ASSERT_EQ (val_t::ndefault, 1 + i); ASSERT_EQ (val_t::ncopy, n_expand_moved); ASSERT_EQ (val_t::nassign, 0); - ASSERT_EQ (val_t::ndtor, 1 + (i + 2) / 3); + ASSERT_EQ (val_t::ndtor, n_expand_moved + 1 + (i + 2) / 3); } } int ndefault = val_t::ndefault; int ncopy = val_t::ncopy; int nassign = val_t::nassign; int ndtor = val_t::ndtor; for (int i = 0; i < N_elem; ++i) { if (remove_some_inline && !(i % 3)) continue; m.remove (a[i]); ++ndtor; ASSERT_EQ (val_t::ndefault, ndefault); ASSERT_EQ (val_t::ncopy, ncopy); ASSERT_EQ (val_t::nassign, nassign); ASSERT_EQ (val_t::ndtor, ndtor); } + ASSERT_EQ (val_t::ndefault + val_t::ncopy, val_t::ndtor); } /* Test calling empty on a hash_map that has a key type with non-zero "empty" value. */ static void test_nonzero_empty_key () { typedef int_hash IntHash; hash_map > x; for (int i = 1; i != 32; ++i) x.put (i, i); ASSERT_EQ (x.get (0), NULL); diff --git a/gcc/hash-table.h b/gcc/hash-table.h index a6e0ac8eea9..8c2a0589fbd 100644 --- a/gcc/hash-table.h +++ b/gcc/hash-table.h @@ -808,30 +808,34 @@ hash_table::expand () m_entries = nentries; m_size = nsize; m_size_prime_index = nindex; m_n_elements -= m_n_deleted; m_n_deleted = 0; value_type *p = oentries; do { value_type &x = *p; if (!is_empty (x) && !is_deleted (x)) { value_type *q = find_empty_slot_for_expand (Descriptor::hash (x)); new ((void*) q) value_type (std::move (x)); + + /* Manually invoke destructor of original object, to counterbalance + object constructed via placement new. */ + x.~value_type (); } p++; } while (p < olimit); if (!m_ggc) Allocator ::data_free (oentries); else ggc_free (oentries); } /* Implements empty() in cases where it isn't a no-op. */ template