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 E4965384801E for ; Sat, 7 Aug 2021 08:08:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E4965384801E 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: k/oX////xbOc7BTpWpt0cbfhaPONq5MvuCD93SnCqiC+kXqMPOWy8IcbF2EmHFMSTonaK+Cta/ oecQ9MWuoy2YFKxYthE6vKW/v4RrQSgP4MQnDxHjMvIUjfv/9dfMAvHEECRT/2IoFP23HfA8DM 8Q3+13eDEvnwd9vbe3xjRAOEoKJNx1qkTcwg4CiQB2AkWw/Ba3mAhyAIZezUQrO1qNgzD7fgel h04to3I3EcXVttQI0KxSxgCSu02Ldq//mDDZ9VAFD7Qm6ASXOOHs1UtQrgyNmGDml2w3TFVBJR 45ZWm/wwJJw4krwsGJ5HRxjo X-IronPort-AV: E=Sophos;i="5.84,301,1620720000"; d="scan'208";a="66867156" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa1.mentor.iphmx.com with ESMTP; 07 Aug 2021 00:08:11 -0800 IronPort-SDR: Ynt6o2/1OP1aoYG330NQtBjvItKUMVyURaQ7aD3H+iKNQwL5MtUX9vnNkWcHziFABaoicuM4aT l1XvvlqzsJ91jssKicTU3sh1BO9DPTjh6jpmxvoOoLow6a/HqbwXm2jIZqGgpHn5jZZjqIvvvf AXsQoJjbB2miKeyY+DGl8NJ+tSkN8FPyz86D93PHCvJZ9uvRrWqEQuWthKlFFvtrsJ2w/fKpHb TA1xeBN3voQgBP4zd9uFj1tGSYmkW/QXk+k+g7/e+ua1pP9XExi03NGJC/Kwds3TVt1XROMt6W jFQ= From: Thomas Schwinge To: Jonathan Wakely , 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: Sat, 7 Aug 2021 10:08:03 +0200 Message-ID: <87lf5dr82k.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: SVR-IES-MBX-07.mgc.mentorg.com (139.181.222.7) To svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=no 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: Sat, 07 Aug 2021 08:08:14 -0000 Hi! On 2021-08-06T19:37:58+0100, Jonathan Wakely wrote: > On Fri, 6 Aug 2021, 17:58 Thomas Schwinge, wrot= e: >> 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 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 uses >> of 'hash_map' boil down to pointer key ('tree', for example) and >> pointer/integer value. >> >> 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 8= 76 >> at 0x483DD99: calloc (vg_replace_malloc.c:762) >> by 0x175F010: xcalloc (xmalloc.c:162) >> by 0xAF4A2C: hash_table, tree_node*> >::hash_entry, f= alse, xcallocator>::hash_table(unsigned long, bool, bool, bool, mem_alloc_o= rigin) (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*> >, si= mple_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-neute= r-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 to= n of "Invalid read [...] inside a block of size [...] free'd" >> + x.~value_type (); //GOOD This seems to work! -- but does it m= ake 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. That testing came back without any issues. > Does GCC's hash_map assume you only use it to store POD (plain old data) > types Don't you disappoint me, C++! > which don't need to be destroyed, because they don't have any > dynamically allocated memory or other resources? > > A hash_map is not a POD, because it does have dynamically allocated memor= y. ACK, that's what I tried to say above in my "layman's terms". ;-) > If my guess is right, then hash_map should really use a static_assert to > enforce that requirement, instead of letting you use it in a way that wil= l > leak. Eh, yes, at the very least! Or, of course, make it work? I mean GCC surely isn't the first software project to desire implementing a 'hash_map' storing non-POD objects? Don't you disappoint me, C++! Alternative to that manual destructor call (per my patch/hack above) -- is maybe something wrong in the 'value_type' constructor implementation or any other bits related to the 'std::move'? (Is that where the non-POD source data ought to be destructed; via "move" instead of "copy" semantics?) "Learning C++ by actual need." ;-D 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