Message ID | 20191201155220.15971-1-philippe.waroquiers@skynet.be |
---|---|
State | New, archived |
Headers |
Received: (qmail 55973 invoked by alias); 1 Dec 2019 15:52:33 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 55958 invoked by uid 89); 1 Dec 2019 15:52:32 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.1 spammy=H*F:D*be, finds, HX-Languages-Length:1963, HContent-Transfer-Encoding:8bit X-HELO: mailsec112.isp.belgacom.be Received: from mailsec112.isp.belgacom.be (HELO mailsec112.isp.belgacom.be) (195.238.20.108) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 01 Dec 2019 15:52:29 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1575215549; x=1606751549; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=QwFxf9cuosvC1KDgpCu3bBaSFkyTCMHIrkzZZXNLV/8=; b=cBTCoidOD+GAm5ZgimDRdR762F8TITInGJjcidAdvx6elBAArvYWwb2O R2OrTNbT4d9sOimoIqfUjB3IFa1vaw==; IronPort-SDR: BNd5OebccMl8DI1Q5O+2eiC7GD07Rb5CFhcRRU9S/CICx8/Fq2vaN2nrGHevuZFMQWVM1saK49 TbiHXgt8mLx/RcJ+xBmVdbNAg8Nfz4J0x5cK9gXMDh3pXjs1QPZXQGSYQ97QOOltOWBFNF4CSL ZNts7jpyffA0t8NNSu/HMPDjbSdFicpqy+1KnpK9723bVzBlH3IwoZ4Hg1TIQlAwyJyA96ED60 Fy9GV0fPRwZvvAR8JKrl3hQUB7aXdmSinjkNb48116CspRs93TTtoWnVTndoIhOUhvT3ojOYwg 0ME= Received: from 136.173-134-109.adsl-dyn.isp.belgacom.be (HELO md.home) ([109.134.173.136]) by relay.skynet.be with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256; 01 Dec 2019 16:52:27 +0100 From: Philippe Waroquiers <philippe.waroquiers@skynet.be> To: gdb-patches@sourceware.org Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be> Subject: [RFA] Fix leaks when pruning inferiors. Date: Sun, 1 Dec 2019 16:52:20 +0100 Message-Id: <20191201155220.15971-1-philippe.waroquiers@skynet.be> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
Commit Message
Philippe Waroquiers
Dec. 1, 2019, 3:52 p.m. UTC
Valgrind detects various inferior related leaks, such as: ==31877== 5,530 (56 direct, 5,474 indirect) bytes in 1 blocks are definitely lost in loss record 7,131 of 7,355 ==31877== at 0x4C2E18C: calloc (vg_replace_malloc.c:760) ==31877== by 0x23E580: xcalloc (alloc.c:100) ==31877== by 0x4794A9: xcnewvec<void*> (poison.h:158) ==31877== by 0x4794A9: registry_alloc_data(registry_data_registry*, registry_fields*) (registry.c:51) ==31877== by 0x3A537C: inferior_alloc_data (inferior.c:43) ==31877== by 0x3A537C: inferior::inferior(int) (inferior.c:92) ==31877== by 0x3A5426: add_inferior_silent(int) (inferior.c:98) ==31877== by 0x3A5530: add_inferior(int) (inferior.c:122) ... Origin of the leaks is in prune_inferiors: prune_inferiors is first removing the inferior to prune from the inferior list, then calls delete_inferior. But delete_inferior will only really destroy the inferior when it finds it into the inferior list. As delete_inferior is removing the inferior to delete from the inferior list, ensure prune_inferiors only calls delete_inferior, without touching the inferior list. gdb/ChangeLog YYYY-MM-DD Philippe Waroquiers <philippe.waroquiers@skynet.be> * inferior.c (prune_inferiors): Only call delete_inferior, Do not modify the inferior list. --- gdb/inferior.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
Comments
On 12/1/19 3:52 PM, Philippe Waroquiers wrote: > Origin of the leaks is in prune_inferiors: prune_inferiors is first removing > the inferior to prune from the inferior list, then calls delete_inferior. > But delete_inferior will only really destroy the inferior when it finds > it into the inferior list. > As delete_inferior is removing the inferior to delete from the inferior list, > ensure prune_inferiors only calls delete_inferior, without touching the > inferior list. Whoops. > @@ -370,24 +370,22 @@ have_live_inferiors (void) > void > prune_inferiors (void) > { > - struct inferior *ss, **ss_link; > + struct inferior *ss, *ss_next; > > > - *ss_link = ss->next; > + ss_next = ss->next; > delete_inferior (ss); > - ss = *ss_link; > + ss = ss_next; Please declare ss_next here instead of at the top. You can omit the redundant "struct" while at it: inferior *next = ss->next; delete_inferior (ss); ss = next; OK with that change. Thanks! Pedro Alves
On Sun, 1 Dec 2019 16:52:20 +0100 Philippe Waroquiers <philippe.waroquiers@skynet.be> wrote: > Valgrind detects various inferior related leaks, such as: > ==31877== 5,530 (56 direct, 5,474 indirect) bytes in 1 blocks are definitely lost in loss record 7,131 of 7,355 > ==31877== at 0x4C2E18C: calloc (vg_replace_malloc.c:760) > ==31877== by 0x23E580: xcalloc (alloc.c:100) > ==31877== by 0x4794A9: xcnewvec<void*> (poison.h:158) > ==31877== by 0x4794A9: registry_alloc_data(registry_data_registry*, registry_fields*) (registry.c:51) > ==31877== by 0x3A537C: inferior_alloc_data (inferior.c:43) > ==31877== by 0x3A537C: inferior::inferior(int) (inferior.c:92) > ==31877== by 0x3A5426: add_inferior_silent(int) (inferior.c:98) > ==31877== by 0x3A5530: add_inferior(int) (inferior.c:122) > ... > > Origin of the leaks is in prune_inferiors: prune_inferiors is first removing > the inferior to prune from the inferior list, then calls delete_inferior. > But delete_inferior will only really destroy the inferior when it finds > it into the inferior list. > As delete_inferior is removing the inferior to delete from the inferior list, > ensure prune_inferiors only calls delete_inferior, without touching the > inferior list. > > gdb/ChangeLog > YYYY-MM-DD Philippe Waroquiers <philippe.waroquiers@skynet.be> > * inferior.c (prune_inferiors): Only call delete_inferior, > Do not modify the inferior list. Okay. (Though I'd rather see a semicolon or a period in place of the comma in the ChangeLog entry.) Kevin
On Thu, 2019-12-05 at 16:42 +0000, Pedro Alves wrote: > Please declare ss_next here instead of at the top. > You can omit the redundant "struct" while at it: > > inferior *next = ss->next; > delete_inferior (ss); > ss = next; > > OK with that change. Thanks! Pushed after fixing the above, and the ChangeLog message as suggested by Kevin. Thanks for the review. Philippe
diff --git a/gdb/inferior.c b/gdb/inferior.c index 84e4d24e73..55c8409cd9 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -370,24 +370,22 @@ have_live_inferiors (void) void prune_inferiors (void) { - struct inferior *ss, **ss_link; + struct inferior *ss, *ss_next; ss = inferior_list; - ss_link = &inferior_list; while (ss) { if (!ss->deletable () || !ss->removable || ss->pid != 0) { - ss_link = &ss->next; - ss = *ss_link; + ss = ss->next; continue; } - *ss_link = ss->next; + ss_next = ss->next; delete_inferior (ss); - ss = *ss_link; + ss = ss_next; } }