[PATCH] Fix FIXME: xstrdup should not be here

[PATCH] Fix FIXME: xstrdup should not be here

[Yao Qi]

Hi,

This FIXME goes into my eyes, when I am about to modify something here,

  /* Name is allocated by name_of_child.  */
  /* FIXME: xstrdup should not be here.  */

This FIXME was introduced in the python pretty-pretter patches.

  Python pretty-printing [6/6]
  https://sourceware.org/ml/gdb-patches/2009-05/msg00467.html

create_child_with_value is called in two paths,

 1. varobj_list_children -> create_child -> create_child_with_value,
 2. install_dynamic_child -> install_dynamic_child -> varobj_add_child
    -> create_child_with_value

In path #1, 'name' is allocated by name_of_child, as the original
comment said, we don't have to duplicate NAME in
create_child_with_value.  In path #2, 'name' is got from
PyArg_ParseTuple, and we have to duplicate NAME.

This patch removes the call to xstrdup in create_child_with_value
and call xstrudp in update_dynamic_varobj_children (path #2).

Regression tested on x86_64-linux.  It depends on patch

  Convert 'name' of 'struct varobj' to 'const char *'.
  https://sourceware.org/ml/gdb-patches/2013-08/msg00895.html

gdb:

2013-08-29  Yao Qi  <yao@codesourcery.com>

	* varobj.c (update_dynamic_varobj_children): Duplicate 'name'
	and pass it to install_dynamic_child.
	(create_child_with_value): Update comments.  Don't dpulicate
	'name'.
---
 gdb/varobj.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/varobj.c b/gdb/varobj.c
index 55cd83e..e76ea61 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -1225,7 +1225,8 @@ update_dynamic_varobj_children (struct varobj *var,
 				 can_mention ? type_changed : NULL,
 				 can_mention ? new : NULL,
 				 can_mention ? unchanged : NULL,
-				 can_mention ? cchanged : NULL, i, name, v);
+				 can_mention ? cchanged : NULL, i,
+				 xstrdup (name), v);
 	  do_cleanups (inner);
 	}
       else
@@ -2439,9 +2440,8 @@ create_child_with_value (struct varobj *parent, int index, const char *name,
 
   child = new_variable ();
 
-  /* Name is allocated by name_of_child.  */
-  /* FIXME: xstrdup should not be here.  */
-  child->name = xstrdup (name);
+  /* NAME is allocated by caller.  */
+  child->name = name;
   child->index = index;
   child->parent = parent;
   child->root = parent->root;
-- 
1.7.7.6

Re: [PATCH] Fix FIXME: xstrdup should not be here

[Yao Qi]

On 08/30/2013 09:27 AM, Yao Qi wrote:
> Regression tested on x86_64-linux.  It depends on patch
>
>    Convert 'name' of 'struct varobj' to 'const char *'.
>    https://sourceware.org/ml/gdb-patches/2013-08/msg00895.html
>

I find this patch can be applied cleanly without patch "Convert 'name'
of 'struct varobj' to 'const char *'" and the memory leak is reported
without this patch.

==4512== 8 bytes in 1 blocks are definitely lost in loss record 1,021 of 
6,873^M
==4512==    at 0x4007D89: malloc (vg_replace_malloc.c:236)^M
==4512==    by 0x82CFF89: xmalloc (common-utils.c:51)^M
==4512==    by 0x8376501: xstrdup (xstrdup.c:34)^M
==4512==    by 0x8278F63: c_describe_child (varobj.c:3337)^M
==4512==    by 0x82790AA: c_name_of_child (varobj.c:3400)^M
==4512==    by 0x82788D5: varobj_list_children (varobj.c:2737)^M
==4512==    by 0x8107520: mi_cmd_var_list_children (mi-cmd-var.c:405)^M
==4512==    by 0x810FFE6: mi_execute_command (mi-main.c:2147)^M
==4512==    by 0x810AA43: mi_execute_command_input_handler 
(mi-interp.c:296)^M
==4512==    by 0x81DE883: handle_file_event (event-loop.c:768)^M
==4512==    by 0x81DEF3A: process_event (event-loop.c:342)^M
==4512==    by 0x81DF354: gdb_do_one_event (event-loop.c:406)^M

This leak is fixed by this patch.
> gdb:
>
> 2013-08-29  Yao Qi<yao@codesourcery.com>
>
> 	* varobj.c (update_dynamic_varobj_children): Duplicate 'name'
> 	and pass it to install_dynamic_child.
> 	(create_child_with_value): Update comments.  Don't dpulicate
                                                            ^^^^^^^ typo

> 	'name'.

gdb:

2013-09-10  Yao Qi  <yao@codesourcery.com>

	* varobj.c (update_dynamic_varobj_children): Duplicate 'name'
	and pass it to install_dynamic_child.
	(create_child_with_value): Update comments.  Don't duplicate
	'name'.

Ping.

-- 
Yao (齐尧)

Re: [PATCH] Fix FIXME: xstrdup should not be here

[Yao Qi]

On 09/10/2013 10:32 AM, Yao Qi wrote:
> gdb:
> 
> 2013-09-10  Yao Qi<yao@codesourcery.com>
> 
> 	* varobj.c (update_dynamic_varobj_children): Duplicate 'name'
> 	and pass it to install_dynamic_child.
> 	(create_child_with_value): Update comments.  Don't duplicate
> 	'name'.

Here is the updated version, with a cast NAME to 'char *'.

-- 
Yao (齐尧)

gdb:

2013-09-13  Yao Qi  <yao@codesourcery.com>

	* varobj.c (update_dynamic_varobj_children): Duplicate 'name'
	and pass it to install_dynamic_child.
	(create_child_with_value): Update comments.  Don't duplicate
	'name'.
---
 gdb/varobj.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/varobj.c b/gdb/varobj.c
index ced3e2d..7bdac62 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -1225,7 +1225,8 @@ update_dynamic_varobj_children (struct varobj *var,
 				 can_mention ? type_changed : NULL,
 				 can_mention ? new : NULL,
 				 can_mention ? unchanged : NULL,
-				 can_mention ? cchanged : NULL, i, name, v);
+				 can_mention ? cchanged : NULL, i,
+				 xstrdup (name), v);
 	  do_cleanups (inner);
 	}
       else
@@ -2439,9 +2440,8 @@ create_child_with_value (struct varobj *parent, int index, const char *name,
 
   child = new_variable ();
 
-  /* Name is allocated by name_of_child.  */
-  /* FIXME: xstrdup should not be here.  */
-  child->name = xstrdup (name);
+  /* NAME is allocated by caller.  */
+  child->name = (char *) name;
   child->index = index;
   child->parent = parent;
   child->root = parent->root;
-- 
1.7.7.6

Re: [PATCH] Fix FIXME: xstrdup should not be here

[Mark Kettenis]

> Date: Fri, 13 Sep 2013 14:49:11 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> On 09/10/2013 10:32 AM, Yao Qi wrote:
> > gdb:
> > 
> > 2013-09-10  Yao Qi<yao@codesourcery.com>
> > 
> > 	* varobj.c (update_dynamic_varobj_children): Duplicate 'name'
> > 	and pass it to install_dynamic_child.
> > 	(create_child_with_value): Update comments.  Don't duplicate
> > 	'name'.
> 
> Here is the updated version, with a cast NAME to 'char *'.

If you do this, you should drop the const from the
install_dynamic_child() prototype instead of adding the (char *) cast.

Re: [PATCH] Fix FIXME: xstrdup should not be here

[Yao Qi]

On 09/13/2013 03:52 PM, Mark Kettenis wrote:
> If you do this, you should drop the const from the
> install_dynamic_child() prototype instead of adding the (char *) cast.

Looks I've been confused by the usage of const, :).  Here is an updated
one.

-- 
Yao (齐尧)

gdb:

2013-09-16  Yao Qi  <yao@codesourcery.com>

	* varobj.c (create_child_with_value): Remove 'const' from the
	type of parameter 'name'.
	(varobj_add_child): Likewise.
	(install_dynamic_child): Remove 'const' from the type of
	parameter 'name'.
	(varobj_add_child): Likewise.
	(create_child_with_value): Likewise.  Update comments.  Don't
	duplicate 'name'.
	(update_dynamic_varobj_children): Duplicate 'name'
	and pass it to install_dynamic_child.
---
 gdb/varobj.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/gdb/varobj.c b/gdb/varobj.c
index ced3e2d..cd75a32 100644
--- a/gdb/varobj.c
+++ b/gdb/varobj.c
@@ -245,7 +245,7 @@ static void uninstall_variable (struct varobj *);
 static struct varobj *create_child (struct varobj *, int, char *);
 
 static struct varobj *
-create_child_with_value (struct varobj *parent, int index, const char *name,
+create_child_with_value (struct varobj *parent, int index, char *name,
 			 struct value *value);
 
 /* Utility routines */
@@ -304,7 +304,7 @@ static int is_root_p (struct varobj *var);
 #if HAVE_PYTHON
 
 static struct varobj *varobj_add_child (struct varobj *var,
-					const char *name,
+					char *name,
 					struct value *value);
 
 #endif /* HAVE_PYTHON */
@@ -1031,7 +1031,7 @@ install_dynamic_child (struct varobj *var,
 		       VEC (varobj_p) **unchanged,
 		       int *cchanged,
 		       int index,
-		       const char *name,
+		       char *name,
 		       struct value *value)
 {
   if (VEC_length (varobj_p, var->children) < index + 1)
@@ -1225,7 +1225,8 @@ update_dynamic_varobj_children (struct varobj *var,
 				 can_mention ? type_changed : NULL,
 				 can_mention ? new : NULL,
 				 can_mention ? unchanged : NULL,
-				 can_mention ? cchanged : NULL, i, name, v);
+				 can_mention ? cchanged : NULL, i,
+				 xstrdup (name), v);
 	  do_cleanups (inner);
 	}
       else
@@ -1344,7 +1345,7 @@ varobj_list_children (struct varobj *var, int *from, int *to)
 #if HAVE_PYTHON
 
 static struct varobj *
-varobj_add_child (struct varobj *var, const char *name, struct value *value)
+varobj_add_child (struct varobj *var, char *name, struct value *value)
 {
   varobj_p v = create_child_with_value (var, 
 					VEC_length (varobj_p, var->children), 
@@ -2431,7 +2432,7 @@ is_anonymous_child (struct varobj *child)
 }
 
 static struct varobj *
-create_child_with_value (struct varobj *parent, int index, const char *name,
+create_child_with_value (struct varobj *parent, int index, char *name,
 			 struct value *value)
 {
   struct varobj *child;
@@ -2439,9 +2440,8 @@ create_child_with_value (struct varobj *parent, int index, const char *name,
 
   child = new_variable ();
 
-  /* Name is allocated by name_of_child.  */
-  /* FIXME: xstrdup should not be here.  */
-  child->name = xstrdup (name);
+  /* NAME is allocated by caller.  */
+  child->name = name;
   child->index = index;
   child->parent = parent;
   child->root = parent->root;
-- 
1.7.7.6

Re: [PATCH] Fix FIXME: xstrdup should not be here

[Yao Qi]

On 09/16/2013 09:13 PM, Yao Qi wrote:
> 2013-09-16  Yao Qi<yao@codesourcery.com>
>
> 	* varobj.c (create_child_with_value): Remove 'const' from the
> 	type of parameter 'name'.
> 	(varobj_add_child): Likewise.
> 	(install_dynamic_child): Remove 'const' from the type of
> 	parameter 'name'.
> 	(varobj_add_child): Likewise.
> 	(create_child_with_value): Likewise.  Update comments.  Don't
> 	duplicate 'name'.
> 	(update_dynamic_varobj_children): Duplicate 'name'
> 	and pass it to install_dynamic_child.

Ping.  https://sourceware.org/ml/gdb-patches/2013-09/msg00442.html

-- 
Yao (齐尧)

Re: [PATCH] Fix FIXME: xstrdup should not be here

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 2013-09-16  Yao Qi  <yao@codesourcery.com>
Yao> 	* varobj.c (create_child_with_value): Remove 'const' from the
Yao> 	type of parameter 'name'.
Yao> 	(varobj_add_child): Likewise.
Yao> 	(install_dynamic_child): Remove 'const' from the type of
Yao> 	parameter 'name'.
Yao> 	(varobj_add_child): Likewise.
Yao> 	(create_child_with_value): Likewise.  Update comments.  Don't
Yao> 	duplicate 'name'.
Yao> 	(update_dynamic_varobj_children): Duplicate 'name'
Yao> 	and pass it to install_dynamic_child.

Thanks Yao.  This is ok.

Tom

Re: [PATCH] Fix FIXME: xstrdup should not be here

[Yao Qi]

On 10/03/2013 04:11 AM, Tom Tromey wrote:
>>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
>
> Yao> 2013-09-16  Yao Qi  <yao@codesourcery.com>
> Yao> 	* varobj.c (create_child_with_value): Remove 'const' from the
> Yao> 	type of parameter 'name'.
> Yao> 	(varobj_add_child): Likewise.
> Yao> 	(install_dynamic_child): Remove 'const' from the type of
> Yao> 	parameter 'name'.
> Yao> 	(varobj_add_child): Likewise.
> Yao> 	(create_child_with_value): Likewise.  Update comments.  Don't
> Yao> 	duplicate 'name'.
> Yao> 	(update_dynamic_varobj_children): Duplicate 'name'
> Yao> 	and pass it to install_dynamic_child.
>
> Thanks Yao.  This is ok.
>

Patch is committed.

-- 
Yao (齐尧)