From: Martin Sebor <msebor@gmail.com>
To: Thomas Schwinge <thomas@codesourcery.com>,
Jonathan Wakely <jwakely.gcc@gmail.com>,
Richard Biener <richard.guenther@gmail.com>,
gcc@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: Re: Fix 'hash_table::expand' to destruct stale Value objects
Date: Wed, 1 Sep 2021 19:31:19 -0600 [thread overview]
Message-ID: <60f2a6ac-764f-470e-2807-acc19c5038a8@gmail.com> (raw)
In-Reply-To: <87h7f7mcqf.fsf@euler.schwinge.homeip.net>
On 8/30/21 4:46 AM, Thomas Schwinge wrote:
> Hi!
>
> Ping -- we still need to plug the memory leak; see patch attached, and/or
> long discussion here:
Thanks for answering my questions. I have no concerns with going
forward with the patch as is. Just a suggestion/request: unless
this patch fixes all the outstanding problems you know of or suspect
in this area (leaks/missing dtor calls) and unless you plan to work
on those in the near future, please open a bug for them with a brain
dump of what you learned. That should save us time when the day
comes to tackle those.
Martin
>
> On 2021-08-16T14:10:00-0600, Martin Sebor <msebor@gmail.com> wrote:
>> On 8/16/21 6:44 AM, Thomas Schwinge wrote:
>>> On 2021-08-12T17:15:44-0600, Martin Sebor via Gcc <gcc@gcc.gnu.org> 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<tree, tree> 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<tree, field_map_t> record_field_map_t;
>>>>>
>>>>> Thus, that's a 'hash_map<tree, hash_map<tree, tree>>'. (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.)
>
> (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 = [...];
>>>>> [...]
>>>>> bool existed;
>>>>> field_map_t &fields
>>>>> = 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<hash_map<tree_node*, tree_node*, simple_hashmap_traits<default_hash_traits<tree_node*>, 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*, tree_node*, simple_hashmap_traits<default_hash_traits<tree_node*>, tree_node*> >::hash_map(unsigned long, bool, bool, bool) (hash-map.h:143)
>>>>> by 0x15DEE87: hash_map<tree_node*, hash_map<tree_node*, tree_node*, simple_hashmap_traits<default_hash_traits<tree_node*>, tree_node*> >, simple_hashmap_traits<default_hash_traits<tree_node*>, hash_map<tree_node*, tree_node*, simple_hashmap_traits<default_hash_traits<tree_node*>, tree_node*> > > >::get_or_insert(tree_node* const&, bool*) (hash-map.h:205)
>>>>> by 0x15DD52C: execute_omp_oacc_neuter_broadcast() (omp-oacc-neuter-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<Descriptor, Lazy, Allocator>::expand ()
>>>>> {
>>>>> value_type *q = 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 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 primitive
>>> types; "pseudo destructor") is the right thing to do; see
>>> <https://stackoverflow.com/questions/6730403/how-to-delete-object-constructed-via-placement-new-operator>
>>> 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üße
> Thomas
>
>
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
>
next prev parent reply other threads:[~2021-09-02 1:31 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <87r1f6qzmx.fsf@euler.schwinge.homeip.net>
[not found] ` <af8fa221-b555-c192-bd99-6eb73db3935f@gmail.com>
2021-08-16 12:44 ` 'hash_map<tree, hash_map<tree, tree>>' Thomas Schwinge
2021-08-16 20:10 ` Martin Sebor
2021-08-17 6:40 ` Expensive selftests (was: 'hash_map<tree, hash_map<tree, tree>>') Thomas Schwinge
2021-08-17 8:57 ` Richard Biener
2021-08-18 11:34 ` Add more self-tests for 'hash_map' with Value type with non-trivial constructor/destructor (was: Expensive selftests) Thomas Schwinge
2021-08-18 13:35 ` Expensive selftests (was: 'hash_map<tree, hash_map<tree, tree>>') David Edelsohn
2021-08-18 15:34 ` Make 'gcc/hash-map-tests.c:test_map_of_type_with_ctor_and_dtor_expand' work on 32-bit architectures [PR101959] Thomas Schwinge
2021-08-18 16:12 ` Richard Biener
2021-08-17 15:01 ` Expensive selftests Martin Sebor
2021-08-30 10:46 ` Fix 'hash_table::expand' to destruct stale Value objects (was: 'hash_map<tree, hash_map<tree, tree>>') Thomas Schwinge
2021-09-02 1:31 ` Martin Sebor [this message]
2021-09-10 8:00 ` [PING] Re: Fix 'hash_table::expand' to destruct stale Value objects Thomas Schwinge
2021-09-17 11:17 ` [PING^2] " Thomas Schwinge
2021-09-17 12:08 ` Richard Biener
2021-09-17 12:39 ` Jonathan Wakely
2021-09-17 13:03 ` Richard Biener
2021-09-17 15:52 ` Thomas Schwinge
2021-09-17 19:08 ` Jonathan Wakely
2021-09-20 9:11 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=60f2a6ac-764f-470e-2807-acc19c5038a8@gmail.com \
--to=msebor@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=gcc@gcc.gnu.org \
--cc=jwakely.gcc@gmail.com \
--cc=richard.guenther@gmail.com \
--cc=thomas@codesourcery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).