[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc
Subject:    Re: [PING^2] Re: Fix 'hash_table::expand' to destruct stale Value objects
From:       Richard Biener via Gcc <gcc () gcc ! gnu ! org>
Date:       2021-09-20 9:11:22
Message-ID: CAFiYyc14c5O5gcCDJ1O7c_0thB7-f7roucTBk9WjDEV6oRONSg () mail ! gmail ! com
[Download RAW message or body]

On Fri, Sep 17, 2021 at 5:52 PM Thomas Schwinge <thomas@codesourcery.com> w=
rote:
>
> 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-p=
atches@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 attac=
hed, [...]
>
> >> > > > Ping for formal approval (and review for using proper
> >> > > > C++ terminology in the 'gcc/hash-table.h:hash_table::expand' sou=
rce code
> >> > > > comment that I'm adding).  Patch again attached, for easy refere=
nce.
>
> >> > 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 count=
erbalance
> >> > +            object constructed via placement new.  */
> >> > +         x.~value_type ();
> >> >
> >> > but I had the impression that std::move already "moves away" from th=
e 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=C3=BC=C3=9Fe
>  Thomas
>
>
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 2=
01, 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=
=C3=A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellsc=
haft: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955
[prev in list] [next in list] [prev in thread] [next in thread]