From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x531.google.com (mail-ed1-x531.google.com [IPv6:2a00:1450:4864:20::531]) by sourceware.org (Postfix) with ESMTPS id 0A7D8385840E; Mon, 20 Sep 2021 09:11:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0A7D8385840E Received: by mail-ed1-x531.google.com with SMTP id c21so58214835edj.0; Mon, 20 Sep 2021 02:11:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=4E1ZOkOV/6S5I37WS5Ily6VIHHotAiPV4u7GKw7QwmU=; b=SugoAbVC70lXfWJQnFvV9Bec4CkhAHyqrjBy3jJJdD1kkqCEKg+TXYtVQRMZ2xKlUk MVZR9vn+v/f+6cKC/xYDTdUAjIV3AHMy2uX5jDczDqhA+F7HCHgLSvp15y1WYwUyX5Ry kMggMOuppb34CvBn8H4Ye5UQbDjDMtynQI6D24OLfNFtZIkPAabaOomS5YrTXLqND3ra 5Tgt2l36EskGWlZG1KHBfkl9JkqXhN5aFv8IrH7VcmcGUicEd9dTsAa2U1dTVdlhsF5q mdFy24I9ZtxB/mKLIsL60ANt3XwBhItsK8De8AllDAOPm6cWkSCZa+5VC+wHwzeIuNB/ KiEw== X-Gm-Message-State: AOAM533+XfNXnrv8g7Tm2LMQpjRjcDd9XE0CilnZb/G26k4mqqf4Drxs dBtPuA3SYMc6vEFGIVRHogp/k3cInoeGbycq9KE= X-Google-Smtp-Source: ABdhPJyAYnyO/nBZIcjarQJ02d/CAItzxYT7A78g/boFGFWKq3ciilOfwBjltvjT8L3oWf/OM9jgoLQhPAAjU0xE7z8= X-Received: by 2002:a17:907:244a:: with SMTP id yw10mr26950781ejb.571.1632129093751; Mon, 20 Sep 2021 02:11:33 -0700 (PDT) MIME-Version: 1.0 References: <87r1f6qzmx.fsf@euler.schwinge.homeip.net> <87czqd7e4k.fsf@euler.schwinge.homeip.net> <55126f14-dda1-2040-0976-70b89582a60c@gmail.com> <87h7f7mcqf.fsf@euler.schwinge.homeip.net> <60f2a6ac-764f-470e-2807-acc19c5038a8@gmail.com> <87mtokuag6.fsf@euler.schwinge.homeip.net> <87v92za1t2.fsf@euler.schwinge.homeip.net> <87sfy39p38.fsf@euler.schwinge.homeip.net> In-Reply-To: <87sfy39p38.fsf@euler.schwinge.homeip.net> From: Richard Biener Date: Mon, 20 Sep 2021 11:11:22 +0200 Message-ID: Subject: Re: [PING^2] Re: Fix 'hash_table::expand' to destruct stale Value objects To: Thomas Schwinge Cc: Jonathan Wakely , GCC Patches , GCC Development , Martin Sebor Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Sep 2021 09:11:36 -0000 On Fri, Sep 17, 2021 at 5:52 PM Thomas Schwinge w= rote: > > 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 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