On Fri, Sep 17, 2021 at 5:52 PM Thomas Schwinge <tho...@codesourcery.com> wrote: > > Hi! > > On 2021-09-17T15:03:18+0200, Richard Biener <richard.guent...@gmail.com> > wrote: > > On Fri, Sep 17, 2021 at 2:39 PM Jonathan Wakely <jwakely....@gmail.com> > > wrote: > >> On Fri, 17 Sept 2021 at 13:08, Richard Biener > >> <richard.guent...@gmail.com> 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. > > >> 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
