From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10881 invoked by alias); 29 Jan 2015 05:11:37 -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 10843 invoked by uid 89); 29 Jan 2015 05:11:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 29 Jan 2015 05:11:32 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id A07CD116690; Thu, 29 Jan 2015 00:11:30 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id jFhqJgQj1T5a; Thu, 29 Jan 2015 00:11:30 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 41B6C11668F; Thu, 29 Jan 2015 00:11:30 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 0D144491CD; Thu, 29 Jan 2015 09:11:26 +0400 (RET) Date: Thu, 29 Jan 2015 07:38:00 -0000 From: Joel Brobecker To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Free results of varobj_get_type and type_to_string Message-ID: <20150129051126.GD5193@adacore.com> References: <1422383618-8215-1-git-send-email-simon.marchi@ericsson.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1422383618-8215-1-git-send-email-simon.marchi@ericsson.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2015-01/txt/msg00749.txt.bz2 Hi Simon, On Tue, Jan 27, 2015 at 01:33:38PM -0500, Simon Marchi wrote: > varobj_get_type and type_to_string return an allocated string, which is > not freed at a couple of places. > > gdb/ChangeLog: > > * mi/mi-cmd-var.c (mi_cmd_var_info_type): Free result of > varobj_get_type. > (varobj_update_one): Same. > * varobj.c (update_type_if_necessary): Free curr_type_str and > new_type_str. > (varobj_get_type): Specify in comment that the result needs to be > freed by the caller. Thanks looking into this. Comments below. > struct ui_out *uiout = current_uiout; > struct varobj *var; > + char *type; Would you mind renaming this variable "type_name" instead of type. I don't know about the others, but "type" is now wired into my brain as to be a "struct type *"... > @@ -765,7 +769,11 @@ varobj_update_one (struct varobj *var, enum print_values print_values, > } > > if (r->type_changed) > - ui_out_field_string (uiout, "new_type", varobj_get_type (r->varobj)); > + { > + char *type = varobj_get_type (r->varobj); > + ui_out_field_string (uiout, "new_type", type); > + xfree (type); > + } Same here, please. Also, can you add an empty line after the local declarations? This is part of GDB's Coding Style. > /* Obtain the type of an object Variable as a string similar to the one gdb > - prints on the console. */ > + prints on the console. The caller is responsible for freeing the string. > + */ Thanks for updating functions' documentation. I really appreciate that. > char * > varobj_get_type (struct varobj *var) > @@ -1303,6 +1304,8 @@ update_type_if_necessary (struct varobj *var, struct value *new_value) > var->num_children = -1; > return 1; > } > + xfree (curr_type_str); > + xfree (new_type_str); In this case, you're still missing the case where the function returns, I believe. One way to handle the situation, I think in a way that makes the allocation + deallocation localized would be to introduce a variable containing the result of the strcmp? For instance: new_type_str = type_to_string (new_type); curr_type_str = varobj_get_type (var); type_name_has_changed = strcmp (curr_type_str, new_type_str) != 0; xfree (new_type_str); xfree (curr_type_str); if (type_name_has_changed) { -- Joel