[PING^2] Re: Fix 'hash_table::expand' to destruct stale Value objects

[Thomas Schwinge]

Hi!

On 2021-09-10T10:00:25+0200, I wrote:
> On 2021-09-01T19:31:19-0600, Martin Sebor via Gcc-patches 
>  wrote:
>> On 8/30/21 4:46 AM, Thomas Schwinge wrote:
>>> 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.
>
> Thanks, Martin.  Ping for formal approval (and review for using proper
> C++ terminology in the 'gcc/hash-table.h:hash_table::expand' source code
> comment that I'm adding).  Patch again attached, for easy reference.

Ping, once again.


Grüße
 Thomas


>> 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.
>
> ACK.  I'm not aware of any additional known problems.  (In our email
> discussion, we did have some "vague ideas" for opportunities of
> clarification/clean-up, but these aren't worth filing PRs for; needs
> someone to gain understanding, taking a look.)
>
>
> Grüße
>  Thomas
>
>
>>> 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  
> wrote:
>> On 8/6/21 10:57 AM, Thomas Schwinge wrote:
>>> So I'm trying to do some C++...  ;-)
>>>
>>> Given:
>>>
>>>/* A map from SSA names or var decls to record fields.  */
>>>typedef hash_map field_map_t;
>>>
>>>/* For each propagation record type, this is a map from SSA 
>>> names 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.
>>
>> 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

Re: [PING^2] Re: Fix 'hash_table::expand' to destruct stale Value objects

[Richard Biener via Gcc-patches]

On Fri, Sep 17, 2021 at 1:17 PM Thomas Schwinge  wrote:
>
> Hi!
>
> On 2021-09-10T10:00:25+0200, I wrote:
> > On 2021-09-01T19:31:19-0600, Martin Sebor via Gcc-patches 
> >  wrote:
> >> On 8/30/21 4:46 AM, Thomas Schwinge wrote:
> >>> 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.
> >
> > Thanks, Martin.  Ping for formal approval (and review for using proper
> > C++ terminology in the 'gcc/hash-table.h:hash_table::expand' source code
> > comment that I'm adding).  Patch again attached, for easy reference.
>
> Ping, once again.

I'm happy when a C++ literate approves the main change which I quote as

  new ((void*) q) value_type (std::move (x));
+
+ /* Manually invoke destructor of original object, to counterbalance
+object constructed via placement new.  */
+ x.~value_type ();

but I had the impression that std::move already "moves away" from the source?
That said, the dance above looks iffy, there must be a nicer way to "move"
an object in C++?

What happens if the dtor is deleted btw?  Shouldn't you use sth
like a placement 'delete' instead of invoking a DTOR?

But the above clearly shows I know nothing of C++ :P

Richard.

>
> Grüße
>  Thomas
>
>
> >> 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.
> >
> > ACK.  I'm not aware of any additional known problems.  (In our email
> > discussion, we did have some "vague ideas" for opportunities of
> > clarification/clean-up, but these aren't worth filing PRs for; needs
> > someone to gain understanding, taking a look.)
> >
> >
> > Grüße
> >  Thomas
> >
> >
> >>> 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  
> > wrote:
> >> On 8/6/21 10:57 AM, Thomas Schwinge wrote:
> >>> So I'm trying to do some C++...  ;-)
> >>>
> >>> Given:
> >>>
> >>>/* A map from SSA names or var decls to record fields.  */
> >>>typedef hash_map field_map_t;
> >>>
> >>>/* For each propagation record type, this is a map from SSA 
> >>> names 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.
> >>
> >> 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 bra

Re: [PING^2] Re: Fix 'hash_table::expand' to destruct stale Value objects

[Jonathan Wakely via Gcc-patches]

On Fri, 17 Sept 2021 at 13:08, Richard Biener
 wrote:
>
> On Fri, Sep 17, 2021 at 1:17 PM Thomas Schwinge  
> wrote:
> >
> > Hi!
> >
> > On 2021-09-10T10:00:25+0200, I wrote:
> > > On 2021-09-01T19:31:19-0600, Martin Sebor via Gcc-patches 
> > >  wrote:
> > >> On 8/30/21 4:46 AM, Thomas Schwinge wrote:
> > >>> 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.
> > >
> > > Thanks, Martin.  Ping for formal approval (and review for using proper
> > > C++ terminology in the 'gcc/hash-table.h:hash_table::expand' source code
> > > comment that I'm adding).  Patch again attached, for easy reference.
> >
> > Ping, once again.
>
> I'm happy when a C++ literate approves the main change which I quote as
>
>   new ((void*) q) value_type (std::move (x));
> +
> + /* Manually invoke destructor of original object, to counterbalance
> +object constructed via placement new.  */
> + x.~value_type ();
>
> but I had the impression that std::move already "moves away" from the source?

It just casts the argument to an rvalue reference, which allows the
value_type constructor to steal its guts.

> That said, the dance above looks iffy, there must be a nicer way to "move"
> an object in C++?

The code above is doing two things: transfer the resources from x to a
new object at location *q, and then destroy x.

The first part (moving its resources) has nothing to do with
destruction. An object still needs to be destroyed, even if its guts
have been moved to another object.

The second part is destroying the object, to end its lifetime. You
wouldn't usually call a destructor explicitly, because it would be
done automatically at the end of scope for objects on the stack, or
done by delete when you free obejcts on the heap. This is a special
case where the object's lifetime is manually managed in storage that
is manually managed.

>
> What happens if the dtor is deleted btw?

If the destructor is deleted you have created an unusable type that
cannot be stored in containers. It can only be created using new, and
then never destroyed. If you play stupid games, you win stupid prizes.
Don't do that.

> Shouldn't you use sth
> like a placement 'delete' instead of invoking a DTOR?

No, there is no placement delete. This is exactly the right way to
destroy an object in-place.

I haven't read the rest of the patch, but the snippet above looks fine.

Re: [PING^2] Re: Fix 'hash_table::expand' to destruct stale Value objects

[Richard Biener via Gcc-patches]

On Fri, Sep 17, 2021 at 2:39 PM Jonathan Wakely  wrote:
>
> On Fri, 17 Sept 2021 at 13:08, Richard Biener
>  wrote:
> >
> > On Fri, Sep 17, 2021 at 1:17 PM Thomas Schwinge  
> > wrote:
> > >
> > > Hi!
> > >
> > > On 2021-09-10T10:00:25+0200, I wrote:
> > > > On 2021-09-01T19:31:19-0600, Martin Sebor via Gcc-patches 
> > > >  wrote:
> > > >> On 8/30/21 4:46 AM, Thomas Schwinge wrote:
> > > >>> 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.
> > > >
> > > > Thanks, Martin.  Ping for formal approval (and review for using proper
> > > > C++ terminology in the 'gcc/hash-table.h:hash_table::expand' source code
> > > > comment that I'm adding).  Patch again attached, for easy reference.
> > >
> > > Ping, once again.
> >
> > I'm happy when a C++ literate approves the main change which I quote as
> >
> >   new ((void*) q) value_type (std::move (x));
> > +
> > + /* Manually invoke destructor of original object, to 
> > counterbalance
> > +object constructed via placement new.  */
> > + x.~value_type ();
> >
> > but I had the impression that std::move already "moves away" from the 
> > source?
>
> It just casts the argument to an rvalue reference, which allows the
> value_type constructor to steal its guts.
>
> > That said, the dance above looks iffy, there must be a nicer way to "move"
> > an object in C++?
>
> The code above is doing two things: transfer the resources from x to a
> new object at location *q, and then destroy x.
>
> The first part (moving its resources) has nothing to do with
> destruction. An object still needs to be destroyed, even if its guts
> have been moved to another object.
>
> The second part is destroying the object, to end its lifetime. You
> wouldn't usually call a destructor explicitly, because it would be
> done automatically at the end of scope for objects on the stack, or
> done by delete when you free obejcts on the heap. This is a special
> case where the object's lifetime is manually managed in storage that
> is manually managed.
>
> >
> > What happens if the dtor is deleted btw?
>
> If the destructor is deleted you have created an unusable type that
> cannot be stored in containers. It can only be created using new, and
> then never destroyed. If you play stupid games, you win stupid prizes.
> Don't do that.
>
> > Shouldn't you use sth
> > like a placement 'delete' instead of invoking a DTOR?
>
> No, there is no placement delete. This is exactly the right way to
> destroy an object in-place.
>
> I haven't read the rest of the patch, but the snippet above looks fine.

OK, thanks for clarifying.

The patch is OK then.

Thanks,
Richard.

Re: [PING^2] Re: Fix 'hash_table::expand' to destruct stale Value objects

[Thomas Schwinge]

Hi!

On 2021-09-17T15:03:18+0200, Richard Biener  wrote:
> On Fri, Sep 17, 2021 at 2:39 PM Jonathan Wakely  wrote:
>> On Fri, 17 Sept 2021 at 13:08, Richard Biener
>>  wrote:
>> > On Fri, Sep 17, 2021 at 1:17 PM Thomas Schwinge  
>> > wrote:
>> > > On 2021-09-10T10:00:25+0200, I wrote:
>> > > > On 2021-09-01T19:31:19-0600, Martin Sebor via Gcc-patches 
>> > > >  wrote:
>> > > >> On 8/30/21 4:46 AM, Thomas Schwinge wrote:
>> > > >>> Ping -- we still need to plug the memory leak; see patch attached, 
>> > > >>> [...]

>> > > > Ping for formal approval (and review for using proper
>> > > > C++ terminology in the 'gcc/hash-table.h:hash_table::expand' source 
>> > > > code
>> > > > comment that I'm adding).  Patch again attached, for easy reference.

>> > I'm happy when a C++ literate approves the main change which I quote as
>> >
>> >   new ((void*) q) value_type (std::move (x));
>> > +
>> > + /* Manually invoke destructor of original object, to 
>> > counterbalance
>> > +object constructed via placement new.  */
>> > + x.~value_type ();
>> >
>> > but I had the impression that std::move already "moves away" from the 
>> > source?
>>
>> It just casts the argument to an rvalue reference, which allows the
>> value_type constructor to steal its guts.
>>
>> > That said, the dance above looks iffy, there must be a nicer way to "move"
>> > an object in C++?
>>
>> The code above is doing two things: transfer the resources from x to a
>> new object at location *q, and then destroy x.
>>
>> The first part (moving its resources) has nothing to do with
>> destruction. An object still needs to be destroyed, even if its guts
>> have been moved to another object.
>>
>> The second part is destroying the object, to end its lifetime. You
>> wouldn't usually call a destructor explicitly, because it would be
>> done automatically at the end of scope for objects on the stack, or
>> done by delete when you free obejcts on the heap. This is a special
>> case where the object's lifetime is manually managed in storage that
>> is manually managed.

ACK, and happy that I understood this correctly.

And, thanks for providing some proper C++-esque wording, which I
summarized to replace my original source code comment.

>> > What happens if the dtor is deleted btw?
>>
>> If the destructor is deleted you have created an unusable type that
>> cannot be stored in containers. It can only be created using new, and
>> then never destroyed. If you play stupid games, you win stupid prizes.
>> Don't do that.

Haha!  ;-)

And, by the way, as I understood this: if the destructor is "trivial"
(which includes POD types, for example), the explicit destructor call
here is a no-op.

>> I haven't read the rest of the patch, but the snippet above looks fine.
>
> OK, thanks for clarifying.
>
> The patch is OK then.

Thanks, pushed to master branch
commit 89be17a1b231ade643f28fbe616d53377e069da8
"Fix 'hash_table::expand' to destruct stale Value objects".

Should this be backported to release branches, after a while?


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
>From 89be17a1b231ade643f28fbe616d53377e069da8 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

Re: [PING^2] Re: Fix 'hash_table::expand' to destruct stale Value objects

[Jonathan Wakely via Gcc-patches]

On Fri, 17 Sep 2021, 16:52 Thomas Schwinge,  wrote:

> Hi!
>
> On 2021-09-17T15:03:18+0200, Richard Biener 
> wrote:
> > On Fri, Sep 17, 2021 at 2:39 PM Jonathan Wakely 
> wrote:
> >> On Fri, 17 Sept 2021 at 13:08, Richard Biener
> >>  wrote:
> >> > On Fri, Sep 17, 2021 at 1:17 PM Thomas Schwinge <
> tho...@codesourcery.com> wrote:
> >> > > On 2021-09-10T10:00:25+0200, I wrote:
> >> > > > On 2021-09-01T19:31:19-0600, Martin Sebor via Gcc-patches <
> gcc-patches@gcc.gnu.org> wrote:
> >> > > >> On 8/30/21 4:46 AM, Thomas Schwinge wrote:
> >> > > >>> Ping -- we still need to plug the memory leak; see patch
> attached, [...]
>
> >> > > > Ping for formal approval (and review for using proper
> >> > > > C++ terminology in the 'gcc/hash-table.h:hash_table::expand'
> source code
> >> > > > comment that I'm adding).  Patch again attached, for easy
> reference.
>
> >> > I'm happy when a C++ literate approves the main change which I quote
> as
> >> >
> >> >   new ((void*) q) value_type (std::move (x));
> >> > +
> >> > + /* Manually invoke destructor of original object, to
> counterbalance
> >> > +object constructed via placement new.  */
> >> > + x.~value_type ();
> >> >
> >> > but I had the impression that std::move already "moves away" from the
> source?
> >>
> >> It just casts the argument to an rvalue reference, which allows the
> >> value_type constructor to steal its guts.
> >>
> >> > That said, the dance above looks iffy, there must be a nicer way to
> "move"
> >> > an object in C++?
> >>
> >> The code above is doing two things: transfer the resources from x to a
> >> new object at location *q, and then destroy x.
> >>
> >> The first part (moving its resources) has nothing to do with
> >> destruction. An object still needs to be destroyed, even if its guts
> >> have been moved to another object.
> >>
> >> The second part is destroying the object, to end its lifetime. You
> >> wouldn't usually call a destructor explicitly, because it would be
> >> done automatically at the end of scope for objects on the stack, or
> >> done by delete when you free obejcts on the heap. This is a special
> >> case where the object's lifetime is manually managed in storage that
> >> is manually managed.
>
> ACK, and happy that I understood this correctly.
>
> And, thanks for providing some proper C++-esque wording, which I
> summarized to replace my original source code comment.
>
> >> > What happens if the dtor is deleted btw?
> >>
> >> If the destructor is deleted you have created an unusable type that
> >> cannot be stored in containers. It can only be created using new, and
> >> then never destroyed. If you play stupid games, you win stupid prizes.
> >> Don't do that.
>
> Haha!  ;-)
>
> And, by the way, as I understood this: if the destructor is "trivial"
> (which includes POD types, for example), the explicit destructor call
> here is a no-op.
>

Right.

And you can even do x.~value_type(); for things which aren't classes and
don't have any destructor at all, not even a trivial one. So in a template
function, if the template argument T is int or char or long*, it's ok to do
t.~T(). This is called a pseudo-destructor call (because scalar types like
int don't actually have a destructor). This will also be a no-op.

This allows you to write the same template code for any types* and it will
correctly destroy them, whether they have a non-trivial destructor that
does real work, or a trivial one, or if they are not even classes and have
no destructor at all.

* Well, nearly any types... You can't do it if the destructor is deleted,
as Richi asked about, or private, and you can't do it for non-object types
(references, functions, void) but that's ok because you can't store them in
a container anyway.

Re: [PING^2] Re: Fix 'hash_table::expand' to destruct stale Value objects

[Richard Biener via Gcc-patches]

On Fri, Sep 17, 2021 at 5:52 PM Thomas Schwinge  wrote:
>
> Hi!
>
> On 2021-09-17T15:03:18+0200, Richard Biener  
> wrote:
> > On Fri, Sep 17, 2021 at 2:39 PM Jonathan Wakely  
> > wrote:
> >> On Fri, 17 Sept 2021 at 13:08, Richard Biener
> >>  wrote:
> >> > On Fri, Sep 17, 2021 at 1:17 PM Thomas Schwinge 
> >> >  wrote:
> >> > > On 2021-09-10T10:00:25+0200, I wrote:
> >> > > > On 2021-09-01T19:31:19-0600, Martin Sebor via Gcc-patches 
> >> > > >  wrote:
> >> > > >> On 8/30/21 4:46 AM, Thomas Schwinge wrote:
> >> > > >>> Ping -- we still need to plug the memory leak; see patch attached, 
> >> > > >>> [...]
>
> >> > > > Ping for formal approval (and review for using proper
> >> > > > C++ terminology in the 'gcc/hash-table.h:hash_table::expand' source 
> >> > > > code
> >> > > > comment that I'm adding).  Patch again attached, for easy reference.
>
> >> > I'm happy when a C++ literate approves the main change which I quote as
> >> >
> >> >   new ((void*) q) value_type (std::move (x));
> >> > +
> >> > + /* Manually invoke destructor of original object, to 
> >> > counterbalance
> >> > +object constructed via placement new.  */
> >> > + x.~value_type ();
> >> >
> >> > but I had the impression that std::move already "moves away" from the 
> >> > source?
> >>
> >> It just casts the argument to an rvalue reference, which allows the
> >> value_type constructor to steal its guts.
> >>
> >> > That said, the dance above looks iffy, there must be a nicer way to 
> >> > "move"
> >> > an object in C++?
> >>
> >> The code above is doing two things: transfer the resources from x to a
> >> new object at location *q, and then destroy x.
> >>
> >> The first part (moving its resources) has nothing to do with
> >> destruction. An object still needs to be destroyed, even if its guts
> >> have been moved to another object.
> >>
> >> The second part is destroying the object, to end its lifetime. You
> >> wouldn't usually call a destructor explicitly, because it would be
> >> done automatically at the end of scope for objects on the stack, or
> >> done by delete when you free obejcts on the heap. This is a special
> >> case where the object's lifetime is manually managed in storage that
> >> is manually managed.
>
> ACK, and happy that I understood this correctly.
>
> And, thanks for providing some proper C++-esque wording, which I
> summarized to replace my original source code comment.
>
> >> > What happens if the dtor is deleted btw?
> >>
> >> If the destructor is deleted you have created an unusable type that
> >> cannot be stored in containers. It can only be created using new, and
> >> then never destroyed. If you play stupid games, you win stupid prizes.
> >> Don't do that.
>
> Haha!  ;-)
>
> And, by the way, as I understood this: if the destructor is "trivial"
> (which includes POD types, for example), the explicit destructor call
> here is a no-op.
>
> >> I haven't read the rest of the patch, but the snippet above looks fine.
> >
> > OK, thanks for clarifying.
> >
> > The patch is OK then.
>
> Thanks, pushed to master branch
> commit 89be17a1b231ade643f28fbe616d53377e069da8
> "Fix 'hash_table::expand' to destruct stale Value objects".
>
> Should this be backported to release branches, after a while?

You'd have to cross-check the status of C++ support in our containers there.
If it is a memory leak fix then yes, but as said, something older than
GCC 11 needs
double-checking if it is a) affected and b) has other bits already.

Richard.

>
> 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