From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25117 invoked by alias); 29 Jan 2015 15:59:24 -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 25093 invoked by uid 89); 29 Jan 2015 15:59:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: usevmg20.ericsson.net Received: from usevmg20.ericsson.net (HELO usevmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 29 Jan 2015 15:59:18 +0000 Received: from EUSAAHC003.ericsson.se (Unknown_Domain [147.117.188.81]) by usevmg20.ericsson.net (Symantec Mail Security) with SMTP id 2F.87.03307.FD50AC45; Thu, 29 Jan 2015 11:05:19 +0100 (CET) Received: from [142.133.110.232] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.83) with Microsoft SMTP Server id 14.3.195.1; Thu, 29 Jan 2015 10:59:15 -0500 Message-ID: <54CA58D3.2000201@ericsson.com> Date: Thu, 29 Jan 2015 16:28:00 -0000 From: Simon Marchi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Joel Brobecker CC: Subject: Re: [PATCH] Free results of varobj_get_type and type_to_string References: <1422383618-8215-1-git-send-email-simon.marchi@ericsson.com> <20150129051126.GD5193@adacore.com> In-Reply-To: <20150129051126.GD5193@adacore.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-01/txt/msg00761.txt.bz2 On 15-01-29 12:11 AM, Joel Brobecker wrote: > 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 *"... Done. >> @@ -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. Done and done. >> /* 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) > { Done. Thanks, here's v2. >From a1f4ce1e15a1ce7806c331a2600b57353331807b Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Tue, 27 Jan 2015 11:43:35 -0500 Subject: [PATCH] Free results of varobj_get_type and type_to_string varobj_get_type and type_to_string return an allocated string, which is not freed at a couple of places. New in v2: * Rename char * type to type_name. * Free in all cases in update_type_if_necessary. 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. --- gdb/mi/mi-cmd-var.c | 13 +++++++++++-- gdb/varobj.c | 10 ++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/gdb/mi/mi-cmd-var.c b/gdb/mi/mi-cmd-var.c index 01838b1..d873a17 100644 --- a/gdb/mi/mi-cmd-var.c +++ b/gdb/mi/mi-cmd-var.c @@ -447,14 +447,18 @@ mi_cmd_var_info_type (char *command, char **argv, int argc) { struct ui_out *uiout = current_uiout; struct varobj *var; + char *type_name; if (argc != 1) error (_("-var-info-type: Usage: NAME.")); /* Get varobj handle, if a valid var obj name was specified. */ var = varobj_get_handle (argv[0]); + type_name = varobj_get_type (var); - ui_out_field_string (uiout, "type", varobj_get_type (var)); + ui_out_field_string (uiout, "type", type_name); + + xfree (type_name); } void @@ -765,7 +769,12 @@ 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_name = varobj_get_type (r->varobj); + + ui_out_field_string (uiout, "new_type", type_name); + xfree (type_name); + } if (r->type_changed || r->children_changed) ui_out_field_int (uiout, "new_num_children", diff --git a/gdb/varobj.c b/gdb/varobj.c index a10560f..dad284d 100644 --- a/gdb/varobj.c +++ b/gdb/varobj.c @@ -972,7 +972,8 @@ varobj_add_child (struct varobj *var, struct varobj_item *item) } /* 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. + */ char * varobj_get_type (struct varobj *var) @@ -1289,11 +1290,16 @@ update_type_if_necessary (struct varobj *var, struct value *new_value) { struct type *new_type; char *curr_type_str, *new_type_str; + int type_name_changed; new_type = value_actual_type (new_value, 0, 0); new_type_str = type_to_string (new_type); curr_type_str = varobj_get_type (var); - if (strcmp (curr_type_str, new_type_str) != 0) + type_name_changed = strcmp (curr_type_str, new_type_str) != 0; + xfree (curr_type_str); + xfree (new_type_str); + + if (type_name_changed) { var->type = new_type; -- 1.9.1