From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 88258 invoked by alias); 12 Feb 2019 21:55:36 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 88246 invoked by uid 89); 12 Feb 2019 21:55:36 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-27.6 required=5.0 tests=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.2 spammy= X-HELO: mailsec105.isp.belgacom.be Received: from mailsec105.isp.belgacom.be (HELO mailsec105.isp.belgacom.be) (195.238.20.101) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Feb 2019 21:55:33 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1550008533; x=1581544533; h=message-id:subject:from:to:cc:date:in-reply-to: references:mime-version:content-transfer-encoding; bh=GAHXq09Gyk2Q7nYjg8WoIS0ZA69BCYfiCQXyH/2sllA=; b=OB7jLPhfKcXMscE0m/N48WW+FL2Jz5/ITDdVj0l4D9orl3Eqdr6xPj9p gBzFSCX+0J3A0qkWdX76joSMLFz0UQ==; Received: from 147.122-130-109.adsl-dyn.isp.belgacom.be (HELO md) ([109.130.122.147]) by relay.skynet.be with ESMTP/TLS/AES256-GCM-SHA384; 12 Feb 2019 22:55:30 +0100 Message-ID: <1550008530.1438.10.camel@skynet.be> Subject: Re: [RFA] Fix leaks in ada catch command From: Philippe Waroquiers To: Tom Tromey Cc: gdb-patches@sourceware.org Date: Tue, 12 Feb 2019 21:55:00 -0000 In-Reply-To: <87bm3g7krk.fsf@tromey.com> References: <20190211053728.7668-1-philippe.waroquiers@skynet.be> <87bm3g7krk.fsf@tromey.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-02/txt/msg00166.txt.bz2 On Tue, 2019-02-12 at 14:35 -0700, Tom Tromey wrote: > > > > > > "Philippe" == Philippe Waroquiers writes: > > Philippe> Fix the first leak by xfree-ing the addr_string allocated by ada_exception_sal. > Philippe> Fix the second leak by calling the base bp_location dtor. > > Philippe> Tested on debian/amd64, natively and under valgrind. > > Thanks. > > Philippe> - const char *addr_string = NULL; > Philippe> + char *addr_string = NULL; > Philippe> const struct breakpoint_ops *ops = NULL; > Philippe> - struct symtab_and_line sal = ada_exception_sal (ex_kind, &addr_string, &ops); > Philippe> + struct symtab_and_line sal = ada_exception_sal (ex_kind, > Philippe> + (const char **) &addr_string, > Philippe> + &ops); > > This cast seems questionable to me -- and since ada_exception_sal is > only called from this one spot, it seems like it would be just as > simple, but also more clear and more obviously correct to simply change > the type of "addr_string" to "std::string *". This would also avoid any > potential leak if find_function_start_sal can throw (I didn't check). > > Philippe> static void > Philippe> -bp_location_dtor (struct bp_location *self) > Philippe> +base_bp_location_dtor (struct bp_location *self) > Philippe> { > Philippe> xfree (self->function_name); > Philippe> } > > For this part, I think it's easy in this case to just further C++-ify > bp_location. I've done this in the appended -- let me know what you > think. > > Tom Yes, your suggestions to use std::string * and the below patch seem better to me. So, I assume you will push fixes for both Thanks Philippe > > commit 65a60067205f97696a91dd7f134013ffa112dc21 > Author: Tom Tromey > Date: Tue Feb 12 14:28:07 2019 -0700 > > C++-ify bp_location > > Philippe noticed a memory leak coming from ada_catchpoint_location -- > it was not freeing the "function_name" member from its base class. > > This patch fixes the problem by further C++-ifying bp_location. In > particular, bp_location_ops is now removed, and the "dtor" function > pointer is replaced with an ordinary destructor. > > gdb/ChangeLog > 2019-02-12 Tom Tromey > > * breakpoint.c (~bp_location): Rename from bp_location_dtor. > (bp_location_ops): Remove. > (base_breakpoint_allocate_location): Update. > (free_bp_location): Update. > * ada-lang.c (class ada_catchpoint_location) > : Remove ops parameter. > (ada_catchpoint_location_dtor): Remove. > (ada_catchpoint_location_ops): Remove. > (allocate_location_exception): Update. > * breakpoint.h (struct bp_location_ops): Remove. > (class bp_location) : Remove bp_location_ops > parameter. > <~bp_location>: Add destructor. > : Remove. > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index 194ae465769..a773943b8e8 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,20 @@ > +2019-02-12 Tom Tromey > + > + * breakpoint.c (~bp_location): Rename from bp_location_dtor. > + (bp_location_ops): Remove. > + (base_breakpoint_allocate_location): Update. > + (free_bp_location): Update. > + * ada-lang.c (class ada_catchpoint_location) > + : Remove ops parameter. > + (ada_catchpoint_location_dtor): Remove. > + (ada_catchpoint_location_ops): Remove. > + (allocate_location_exception): Update. > + * breakpoint.h (struct bp_location_ops): Remove. > + (class bp_location) : Remove bp_location_ops > + parameter. > + <~bp_location>: Add destructor. > + : Remove. > + > 2019-02-12 Philippe Waroquiers > > * symtab.h (struct minimal_symbol data_p): New const method. > diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c > index a878d4d1af2..602facb35e7 100644 > --- a/gdb/ada-lang.c > +++ b/gdb/ada-lang.c > @@ -12396,8 +12396,8 @@ static std::string ada_exception_catchpoint_cond_string > class ada_catchpoint_location : public bp_location > { > public: > - ada_catchpoint_location (const bp_location_ops *ops, breakpoint *owner) > - : bp_location (ops, owner) > + ada_catchpoint_location (breakpoint *owner) > + : bp_location (owner) > {} > > /* The condition that checks whether the exception that was raised > @@ -12406,24 +12406,6 @@ public: > expression_up excep_cond_expr; > }; > > -/* Implement the DTOR method in the bp_location_ops structure for all > - Ada exception catchpoint kinds. */ > - > -static void > -ada_catchpoint_location_dtor (struct bp_location *bl) > -{ > - struct ada_catchpoint_location *al = (struct ada_catchpoint_location *) bl; > - > - al->excep_cond_expr.reset (); > -} > - > -/* The vtable to be used in Ada catchpoint locations. */ > - > -static const struct bp_location_ops ada_catchpoint_location_ops = > -{ > - ada_catchpoint_location_dtor > -}; > - > /* An instance of this type is used to represent an Ada catchpoint. */ > > struct ada_catchpoint : public breakpoint > @@ -12493,7 +12475,7 @@ static struct bp_location * > allocate_location_exception (enum ada_exception_catchpoint_kind ex, > struct breakpoint *self) > { > - return new ada_catchpoint_location (&ada_catchpoint_location_ops, self); > + return new ada_catchpoint_location (self); > } > > /* Implement the RE_SET method in the breakpoint_ops structure for all > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 67d83e6f465..9be99ff4efa 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -6958,13 +6958,10 @@ adjust_breakpoint_address (struct gdbarch *gdbarch, > } > } > > -bp_location::bp_location (const bp_location_ops *ops, breakpoint *owner) > +bp_location::bp_location (breakpoint *owner) > { > bp_location *loc = this; > > - gdb_assert (ops != NULL); > - > - loc->ops = ops; > loc->owner = owner; > loc->cond_bytecode = NULL; > loc->shlib_disabled = 0; > @@ -7033,7 +7030,6 @@ allocate_bp_location (struct breakpoint *bpt) > static void > free_bp_location (struct bp_location *loc) > { > - loc->ops->dtor (loc); > delete loc; > } > > @@ -12166,19 +12162,11 @@ say_where (struct breakpoint *b) > } > } > > -/* Default bp_location_ops methods. */ > - > -static void > -bp_location_dtor (struct bp_location *self) > +bp_location::~bp_location () > { > - xfree (self->function_name); > + xfree (function_name); > } > > -static const struct bp_location_ops bp_location_ops = > -{ > - bp_location_dtor > -}; > - > /* Destructor for the breakpoint base class. */ > > breakpoint::~breakpoint () > @@ -12191,7 +12179,7 @@ breakpoint::~breakpoint () > static struct bp_location * > base_breakpoint_allocate_location (struct breakpoint *self) > { > - return new bp_location (&bp_location_ops, self); > + return new bp_location (self); > } > > static void > diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h > index 8c8c66a8158..a91e3e334cf 100644 > --- a/gdb/breakpoint.h > +++ b/gdb/breakpoint.h > @@ -301,31 +301,19 @@ enum bp_loc_type > bp_loc_other /* Miscellaneous... */ > }; > > -/* This structure is a collection of function pointers that, if > - available, will be called instead of performing the default action > - for this bp_loc_type. */ > - > -struct bp_location_ops > -{ > - /* Destructor. Releases everything from SELF (but not SELF > - itself). */ > - void (*dtor) (struct bp_location *self); > -}; > - > class bp_location > { > public: > bp_location () = default; > > - bp_location (const bp_location_ops *ops, breakpoint *owner); > + bp_location (breakpoint *owner); > + > + virtual ~bp_location (); > > /* Chain pointer to the next breakpoint location for > the same parent breakpoint. */ > bp_location *next = NULL; > > - /* Methods associated with this location. */ > - const bp_location_ops *ops = NULL; > - > /* The reference count. */ > int refc = 0; >