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 7DCFF3858D35; Mon, 30 Aug 2021 10:46:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7DCFF3858D35 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: BWgzgrYFNtSLdfA6l0hS+7wnU3Yqdoumj73rd2hnu9EMyT4k2t6ZbwVviK9ZAOYoB4kFQum9Bg ht9aS4AS1BBOZigKA2tOdSm+wSwzf8YFrVUpDvHN4r+rID+9qf1knirzugoU+4vZASi67Rpj7k 78VpFD32gnASLYCk8gI7rRAPNNvXGlc9aH8s7g4d9oh+2+Vz3ImpLFxZ9isSr52lgu4Xok3Xy3 wQJ5AghH3abiwUl7mGfLAjubYwcueYJAX0h6TE2GN0DNL/j8Y6NjI2TkZi0yf4WOSR9EWiTFQE vyd+ZgsVIk1yD8KKUTONQhon X-IronPort-AV: E=Sophos;i="5.84,363,1620720000"; d="scan'208,223";a="67756837" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa1.mentor.iphmx.com with ESMTP; 30 Aug 2021 02:46:45 -0800 IronPort-SDR: 6g9nZnaORz9PdRdt0Ed9L4r/XWH//G90JRFNsZglBTT5GmZHFr6YF48hp1erE4VYfoullMueQU kXUVZcrQmXm1z1oeu/7sGN/p32VEQwQD23K7sLdxx/JnL1uOQZ9FjKd3gaqy4VLLgwGuSJe3pz xS1gFCb32VixxszpMTtGbe+jHLAX32I83+MrdMCNQQf/McIHi+i40c9c8N2iz6aXUY8ZRVNFH9 nEcYa+RzdI6O+o4vd263THCzLzvGz/T5djTsu21Yqtha2DW4IOcGYVgPTJRzcwqGEx1rtc1MnJ /v4= From: Thomas Schwinge To: Martin Sebor , Jonathan Wakely , Richard Biener , , Subject: Fix 'hash_table::expand' to destruct stale Value objects (was: 'hash_map>') In-Reply-To: <55126f14-dda1-2040-0976-70b89582a60c@gmail.com> References: <87r1f6qzmx.fsf@euler.schwinge.homeip.net> <87czqd7e4k.fsf@euler.schwinge.homeip.net> <55126f14-dda1-2040-0976-70b89582a60c@gmail.com> User-Agent: Notmuch/0.29.3+94~g74c3f1b (https://notmuchmail.org) Emacs/27.1 (x86_64-pc-linux-gnu) Date: Mon, 30 Aug 2021 12:46:32 +0200 Message-ID: <87h7f7mcqf.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-05.mgc.mentorg.com (139.181.222.5) 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 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@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Aug 2021 10:46:56 -0000 --=-=-= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi! Ping -- we still need to plug the memory leak; see patch attached, and/or long discussion here: On 2021-08-16T14:10:00-0600, Martin Sebor wrote: > On 8/16/21 6:44 AM, Thomas Schwinge wrote: >> On 2021-08-12T17:15:44-0600, Martin Sebor via Gcc wrot= e: >>> 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 name= s or var decls >>>> to propagate, to the field in the record type that should be = used 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 us= es >>>> 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, whic= h >> 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 i= n >> 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.) (Attached "Fix 'hash_table::expand' to destruct stale Value objects" again.) >> 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" (We've done that in commit e4f16e9f357a38ec702fb69a0ffab9d292a6af9b "Add more self-tests for 'hash_map' with Value type with non-trivial constructor/destructor", quickly followed by bug fix commit bb04a03c6f9bacc890118b9e12b657503093c2f8 "Make 'gcc/hash-map-tests.c:test_map_of_type_with_ctor_and_dtor_expand' work on 32-bit architectures [PR101959]". >> (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 hap= py. >>>> 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_entr= y, false, xcallocator>::hash_table(unsigned long, bool, bool, bool, mem_all= oc_origin) (hash-table.h:275) >>>> by 0x15E0120: hash_map, tree_node*> >::hash_map(unsigned l= ong, bool, bool, bool) (hash-map.h:143) >>>> by 0x15DEE87: hash_map, tree_node*> >= , simple_hashmap_traits, hash_map, tree_= node*> > > >::get_or_insert(tree_node* const&, bool*) (hash-map.h:205) >>>> by 0x15DD52C: execute_omp_oacc_neuter_broadcast() (omp-oacc-n= euter-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 happe= n), >>>> 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::exp= and () >>>> { >>>> value_type *q =3D find_empty_slot_for_expand (Descrip= tor::hash (x)); >>>> new ((void*) q) value_type (std::move (x)); >>>> + //BAD Descriptor::remove (x); // (doesn't make sense and) = a ton 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.) > > I find all these these traits pretty much impenetrable. (Indeed. ... which triggered this reflex with me, to try simplifying this by getting rid of 'typed_free_remove' etc...) > I guess > intuitively, I'd expect Descriptor::remove() to destroy an element, > but I have no idea if that would be right given the design. So 'typed_delete_remove' does destruct via 'delete'. 'typed_free_remove' doesn't -- but is only used via 'free_ptr_hash', where this isn't relevant? 'typed_noop_remove' I haven't considered yet regarding that issue. Anyway, that's all 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 primitiv= e >> 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.) > > Yes, of course, all elements need to be eventually be destroyed. > My only hesitation was whether it should be done via one of these > traits classes (like it's done in C++ containers via allocators) > rather than directly Given that there is (apparently) no issue to do a placement new in 'hash_table::expand', I'd asumme a -- corresponding -- explicit destructor call would be likewise appropriate? (But I'll of course route this through a (new) "traits" function if someone explains why this is the right thing to do.) > and whether there might be other places > where the destruction night need to happen. (Possibly, yes, per discussion above -- but that's a separate issue?) >> 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? Ping. > Shouldn't the hash_table dtor (and perhaps also empty()) also > destroy the elements? (Otherwise, what destroys the elements > newly constructed here, or anywhere else for that matter, such > as in the hash_table ctor?) Per my understanding, per discussion above, they (eventually) do get destroyed via 'delete' in 'typed_delete_remove', for example, via 'Descriptor::remove' calls for all/relevant entries in 'hash_table::~hash_table', 'hash_table::empty_slow', 'hash_table::clear_slot', 'hash_table::remove_elt_with_hash'. (This means that if there has been an intermediate 'expand', this may (eventually) destroy objects at a different memory location from where they originally have been created -- but that's fine.) The 'expand' case itself is different: any (live) entries are *moved* into a new storage memory object via placement new. (And then the hollow entries in the old storage memory object linger.) > Also, shouldn't the destroyed slot be marked either deleted or > empty?) Per my understanding, irrelevant in 'expand': working through 'oentries', the *move* is only done 'if (!is_empty (x) && !is_deleted (x))' (so combined with the item above, there is no memory leak for any entries that have already been 'remove'd -- they have already been destructed), and the whole (old) storage memory object will be deallocated right after the 'oentries' loop. > (Sorry, I realize I have more questions than answers.) No worries, happens to me most of the times! Thanks for looking into this. 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-Fix-hash_table-expand-to-destruct-stale-Value-object.patch" >From 2275962abd72acd1636d438d1cb176bbabdcef06 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 | 10 ++++++---- gcc/hash-table.h | 4 ++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/gcc/hash-map-tests.c b/gcc/hash-map-tests.c index 6acc0d4337e..511d4342087 100644 --- a/gcc/hash-map-tests.c +++ b/gcc/hash-map-tests.c @@ -305,31 +305,32 @@ 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 aspects of 'hash_table::expand'. */ +/* Verify aspects of 'hash_table::expand', in particular that it doesn't leak + Value objects. */ static void test_map_of_type_with_ctor_and_dtor_expand (bool remove_some_inline) { /* Configure, so that hash table expansion triggers a few times. */ const size_t N_init = 0; const int N_elem = 70; size_t expand_c_expected = 4; size_t expand_c = 0; /* For stability of this testing, we need all Key values 'k' to produce unique hash values 'Traits::hash (k)', as otherwise the dynamic insert/remove behavior may diverge across different architectures. This is, for example, a problem when using the standard 'pointer_hash::hash', which is simply doing a 'k >> 3' operation, which is fine on 64-bit @@ -388,69 +389,70 @@ 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' here, but via 'm_n_deleted' does influence any following one. */ 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); } } ASSERT_EQ (expand_c, expand_c_expected); 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