diff mbox series

Fix 'hash_table::expand' to destruct stale Value objects (was: 'hash_map<tree, hash_map<tree, tree>>')

Message ID 87h7f7mcqf.fsf@euler.schwinge.homeip.net
State New
Headers show
Series Fix 'hash_table::expand' to destruct stale Value objects (was: 'hash_map<tree, hash_map<tree, tree>>') | expand

Commit Message

Thomas Schwinge Aug. 30, 2021, 10:46 a.m. UTC
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 <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

Comments

Martin Sebor Sept. 2, 2021, 1:31 a.m. UTC | #1
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
>
diff mbox series

Patch

From 2275962abd72acd1636d438d1cb176bbabdcef06 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <thomas@codesourcery.com>
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
<https://stackoverflow.com/questions/6730403/how-to-delete-object-constructed-via-placement-new-operator>
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<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
    -   by 0xA8B373: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1858)
    -   by 0xA8B277: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::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<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
    -   by 0xA8B373: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1858)
    -   by 0xA8B277: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::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<deferred_access_check, va_gc, vl_embed>*, 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<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
    -   by 0xA8B373: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1858)
    -   by 0xA8B277: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::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<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
    -   by 0xA8B373: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::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<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
    -   by 0xA901ED: bool vec_safe_reserve<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:697)
    -   by 0xA8F161: void vec_alloc<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int) (vec.h:718)
    -   by 0xA8D18D: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>::copy() const (vec.h:979)
    -   by 0xA8B0C3: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::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<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
    -   by 0xA901ED: bool vec_safe_reserve<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:697)
    -   by 0xA8F161: void vec_alloc<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int) (vec.h:718)
    -   by 0xA8D18D: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>::copy() const (vec.h:979)
    -   by 0xA8B0C3: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::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<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
    -   by 0xA8B373: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::reserve(unsigned int, bool) (vec.h:1858)
    -   by 0xA8B277: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::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<class_decl_loc_t::class_key_loc_t>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:290)
    -   by 0xA901ED: bool vec_safe_reserve<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int, bool) (vec.h:697)
    -   by 0xA8F161: void vec_alloc<class_decl_loc_t::class_key_loc_t, va_heap>(vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>*&, unsigned int) (vec.h:718)
    -   by 0xA8D18D: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_embed>::copy() const (vec.h:979)
    -   by 0xA8B0C3: vec<class_decl_loc_t::class_key_loc_t, va_heap, vl_ptr>::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<hash_map<tree_decl_hash, class_decl_loc_t, simple_hashmap_traits<default_hash_traits<tree_decl_hash>, class_decl_loc_t> >::hash_entry, false, xcallocator>::expand() (hash-table.h:839)
    -   by 0xA8D4B3: hash_table<hash_map<tree_decl_hash, class_decl_loc_t, simple_hashmap_traits<default_hash_traits<tree_decl_hash>, 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<tree_decl_hash, class_decl_loc_t, simple_hashmap_traits<default_hash_traits<tree_decl_hash>, 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<Descriptor, Lazy, Allocator>::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<int, INT_MIN, INT_MAX> IntHash;
   hash_map<int, int, simple_hashmap_traits<IntHash, int> > 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<Descriptor, Lazy, Allocator>::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 <value_type> ::data_free (oentries);
   else
     ggc_free (oentries);
 }
 
 /* Implements empty() in cases where it isn't a no-op.  */
 
 template<typename Descriptor, bool Lazy,
-- 
2.30.2