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

[Thomas Schwinge]

Hi!

On 2021-09-17T15:03:18+0200, Richard Biener <richard.guenther@gmail.com> wrote:
> On Fri, Sep 17, 2021 at 2:39 PM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>> On Fri, 17 Sept 2021 at 13:08, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>> > On Fri, Sep 17, 2021 at 1:17 PM Thomas Schwinge <thomas@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.

>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-hash_table-expand-to-destruct-stale-Value-object.patch
Type: text/x-diff
Size: 15053 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc/attachments/20210917/1589dc06/attachment-0001.bin>