From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 57723 invoked by alias); 12 Feb 2019 21:35:31 -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 57715 invoked by uid 89); 12 Feb 2019 21:35:31 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=sk:adjust_ X-HELO: gateway36.websitewelcome.com Received: from gateway36.websitewelcome.com (HELO gateway36.websitewelcome.com) (192.185.200.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Feb 2019 21:35:29 +0000 Received: from cm16.websitewelcome.com (cm16.websitewelcome.com [100.42.49.19]) by gateway36.websitewelcome.com (Postfix) with ESMTP id F0E7D400C566B for ; Tue, 12 Feb 2019 14:49:36 -0600 (CST) Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with SMTP id tfiCg4Cjy4FKptfiCgOuBi; Tue, 12 Feb 2019 15:35:28 -0600 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Type:MIME-Version:Message-ID:In-Reply-To:Date: References:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=vCcwh9RAZVDxBmaOFotr0DCaJbvV9ri93dOsqmS/psU=; b=U9/ovc53bP5MDuU6+uA6iWvRH7 loN6aBnrhXuamAHGQZjfpwzVhHZTiWFEsCkPrSWYaokCdnvFCMp4J9WHDfVZ/l4yfbq9xN9FlXoI2 HNDzKlgjYfTlpHW7TDwyZg5Av; Received: from 75-166-72-210.hlrn.qwest.net ([75.166.72.210]:35138 helo=murgatroyd) by box5379.bluehost.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.91) (envelope-from ) id 1gtfiB-000I9E-Qz; Tue, 12 Feb 2019 15:35:27 -0600 From: Tom Tromey To: Philippe Waroquiers Cc: gdb-patches@sourceware.org Subject: Re: [RFA] Fix leaks in ada catch command References: <20190211053728.7668-1-philippe.waroquiers@skynet.be> Date: Tue, 12 Feb 2019 21:35:00 -0000 In-Reply-To: <20190211053728.7668-1-philippe.waroquiers@skynet.be> (Philippe Waroquiers's message of "Mon, 11 Feb 2019 06:37:28 +0100") Message-ID: <87bm3g7krk.fsf@tromey.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2019-02/txt/msg00164.txt.bz2 >>>>> "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 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;