[PATCH] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[PATCH] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Simon Marchi]

This avoids some manual memory management.

The current code leaks (on purpose) the command name if adding the
command was successful.  This is now explicitly done with a release.

gdb/ChangeLog:

	* python/python-internal.h (gdbpy_parse_command_name): Return
	gdb::unique_xmalloc_ptr.
	* python/py-cmd.c (gdbpy_parse_command_name): Likewise.
	(cmdpy_init): Adjust.
	* python/py-param.c (parmpy_init): Adjust.

Change-Id: Iae5bc21fe2b22f12d5f954057b0aca7ca4cd3f0d
---
 gdb/python/py-cmd.c          | 35 +++++++++++++++++------------------
 gdb/python/py-param.c        | 22 +++++++++++-----------
 gdb/python/python-internal.h |  6 +++---
 3 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index f4d3dcc31218..56ca4764929b 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -348,7 +348,7 @@ cmdpy_completer (struct cmd_list_element *command,
    This function returns the xmalloc()d name of the new command.  On
    error sets the Python error and returns NULL.  */
 
-char *
+gdb::unique_xmalloc_ptr<char>
 gdbpy_parse_command_name (const char *name,
 			  struct cmd_list_element ***base_list,
 			  struct cmd_list_element **start_list)
@@ -357,7 +357,6 @@ gdbpy_parse_command_name (const char *name,
   int len = strlen (name);
   int i, lastchar;
   const char *prefix_text2;
-  char *result;
 
   /* Skip trailing whitespace.  */
   for (i = len - 1; i >= 0 && (name[i] == ' ' || name[i] == '\t'); --i)
@@ -372,9 +371,10 @@ gdbpy_parse_command_name (const char *name,
   /* Find first character of the final word.  */
   for (; i > 0 && valid_cmd_char_p (name[i - 1]); --i)
     ;
-  result = (char *) xmalloc (lastchar - i + 2);
-  memcpy (result, &name[i], lastchar - i + 1);
-  result[lastchar - i + 1] = '\0';
+
+  gdb::unique_xmalloc_ptr<char> result ((char *) xmalloc (lastchar - i + 2));
+  memcpy (result.get (), &name[i], lastchar - i + 1);
+  result.get ()[lastchar - i + 1] = '\0';
 
   /* Skip whitespace again.  */
   for (--i; i >= 0 && (name[i] == ' ' || name[i] == '\t'); --i)
@@ -393,7 +393,6 @@ gdbpy_parse_command_name (const char *name,
     {
       PyErr_Format (PyExc_RuntimeError, _("Could not find command prefix %s."),
 		    prefix_text.c_str ());
-      xfree (result);
       return NULL;
     }
 
@@ -405,7 +404,6 @@ gdbpy_parse_command_name (const char *name,
 
   PyErr_Format (PyExc_RuntimeError, _("'%s' is not a prefix command."),
 		prefix_text.c_str ());
-  xfree (result);
   return NULL;
 }
 
@@ -438,7 +436,7 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
   int completetype = -1;
   char *docstring = NULL;
   struct cmd_list_element **cmd_list;
-  char *cmd_name, *pfx_name;
+  char *pfx_name;
   static const char *keywords[] = { "name", "command_class", "completer_class",
 				    "prefix", NULL };
   PyObject *is_prefix = NULL;
@@ -477,8 +475,9 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
       return -1;
     }
 
-  cmd_name = gdbpy_parse_command_name (name, &cmd_list, &cmdlist);
-  if (! cmd_name)
+  gdb::unique_xmalloc_ptr<char> cmd_name
+    = gdbpy_parse_command_name (name, &cmd_list, &cmdlist);
+  if (cmd_name == nullptr)
     return -1;
 
   pfx_name = NULL;
@@ -509,10 +508,7 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
 	  pfx_name[out] = '\0';
 	}
       else if (cmp < 0)
-	{
-	  xfree (cmd_name);
-	  return -1;
-	}
+	return -1;
     }
   if (PyObject_HasAttr (self, gdbpy_doc_cst))
     {
@@ -523,7 +519,6 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
 	  docstring = python_string_to_host_string (ds_obj.get ()).release ();
 	  if (docstring == NULL)
 	    {
-	      xfree (cmd_name);
 	      xfree (pfx_name);
 	      return -1;
 	    }
@@ -545,14 +540,19 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
 	  /* If we have our own "invoke" method, then allow unknown
 	     sub-commands.  */
 	  allow_unknown = PyObject_HasAttr (self, invoke_cst);
-	  cmd = add_prefix_cmd (cmd_name, (enum command_class) cmdtype,
+	  cmd = add_prefix_cmd (cmd_name.get (),
+				(enum command_class) cmdtype,
 				NULL, docstring, &obj->sub_list,
 				pfx_name, allow_unknown, cmd_list);
 	}
       else
-	cmd = add_cmd (cmd_name, (enum command_class) cmdtype,
+	cmd = add_cmd (cmd_name.get (), (enum command_class) cmdtype,
 		       docstring, cmd_list);
 
+      /* The above doesn't copy nor take ownership of the name... so we just
+         leak it.  */
+      cmd_name.release ();
+
       /* There appears to be no API to set this.  */
       cmd->func = cmdpy_function;
       cmd->destroyer = cmdpy_destroyer;
@@ -569,7 +569,6 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
     }
   catch (const gdb_exception &except)
     {
-      xfree (cmd_name);
       xfree (docstring);
       xfree (pfx_name);
       gdbpy_convert_exception (except);
diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c
index ab9e883dc2d6..954b820c41ef 100644
--- a/gdb/python/py-param.c
+++ b/gdb/python/py-param.c
@@ -657,7 +657,6 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds)
   parmpy_object *obj = (parmpy_object *) self;
   const char *name;
   gdb::unique_xmalloc_ptr<char> set_doc, show_doc, doc;
-  char *cmd_name;
   int parmclass, cmdtype;
   PyObject *enum_values = NULL;
   struct cmd_list_element **set_list, **show_list;
@@ -706,15 +705,13 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds)
   obj->type = (enum var_types) parmclass;
   memset (&obj->value, 0, sizeof (obj->value));
 
-  cmd_name = gdbpy_parse_command_name (name, &set_list,
-				       &setlist);
-
-  if (! cmd_name)
+  gdb::unique_xmalloc_ptr<char> cmd_name
+    = gdbpy_parse_command_name (name, &set_list, &setlist);
+  if (cmd_name == nullptr)
     return -1;
-  xfree (cmd_name);
-  cmd_name = gdbpy_parse_command_name (name, &show_list,
-				       &showlist);
-  if (! cmd_name)
+
+  cmd_name = gdbpy_parse_command_name (name, &show_list, &showlist);
+  if (cmd_name == nullptr)
     return -1;
 
   set_doc = get_doc_string (self, set_doc_cst);
@@ -726,13 +723,16 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds)
   try
     {
       add_setshow_generic (parmclass, (enum command_class) cmdtype,
-			   cmd_name, obj,
+			   cmd_name.get (), obj,
 			   set_doc.get (), show_doc.get (),
 			   doc.get (), set_list, show_list);
+
+      /* The above doesn't copy nor take ownership of the name... so we just
+	 leak it.  */
+      cmd_name.release ();
     }
   catch (const gdb_exception &except)
     {
-      xfree (cmd_name);
       Py_DECREF (self);
       gdbpy_convert_exception (except);
       return -1;
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 56702cad53a0..690d2fb43c06 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -439,9 +439,9 @@ PyObject *gdbpy_selected_thread (PyObject *self, PyObject *args);
 PyObject *gdbpy_selected_inferior (PyObject *self, PyObject *args);
 PyObject *gdbpy_string_to_argv (PyObject *self, PyObject *args);
 PyObject *gdbpy_parameter_value (enum var_types type, void *var);
-char *gdbpy_parse_command_name (const char *name,
-				struct cmd_list_element ***base_list,
-				struct cmd_list_element **start_list);
+gdb::unique_xmalloc_ptr<char> gdbpy_parse_command_name
+  (const char *name, struct cmd_list_element ***base_list,
+   struct cmd_list_element **start_list);
 PyObject *gdbpy_register_tui_window (PyObject *self, PyObject *args,
 				     PyObject *kw);
 
-- 
2.30.1

Re: [PATCH] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Andreas Schwab]

On Mär 23 2021, Simon Marchi via Gdb-patches wrote:

> @@ -545,14 +540,19 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
>  	  /* If we have our own "invoke" method, then allow unknown
>  	     sub-commands.  */
>  	  allow_unknown = PyObject_HasAttr (self, invoke_cst);
> -	  cmd = add_prefix_cmd (cmd_name, (enum command_class) cmdtype,
> +	  cmd = add_prefix_cmd (cmd_name.get (),
> +				(enum command_class) cmdtype,
>  				NULL, docstring, &obj->sub_list,
>  				pfx_name, allow_unknown, cmd_list);
>  	}
>        else
> -	cmd = add_cmd (cmd_name, (enum command_class) cmdtype,
> +	cmd = add_cmd (cmd_name.get (), (enum command_class) cmdtype,
>  		       docstring, cmd_list);
>  
> +      /* The above doesn't copy nor take ownership of the name... so we just
> +         leak it.  */

s/leak/release/?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Simon Marchi]

On 2021-03-23 2:07 p.m., Andreas Schwab wrote:> On Mär 23 2021, Simon Marchi via Gdb-patches wrote:
> 
>> @@ -545,14 +540,19 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
>>  	  /* If we have our own "invoke" method, then allow unknown
>>  	     sub-commands.  */
>>  	  allow_unknown = PyObject_HasAttr (self, invoke_cst);
>> -	  cmd = add_prefix_cmd (cmd_name, (enum command_class) cmdtype,
>> +	  cmd = add_prefix_cmd (cmd_name.get (),
>> +				(enum command_class) cmdtype,
>>  				NULL, docstring, &obj->sub_list,
>>  				pfx_name, allow_unknown, cmd_list);
>>  	}
>>        else
>> -	cmd = add_cmd (cmd_name, (enum command_class) cmdtype,
>> +	cmd = add_cmd (cmd_name.get (), (enum command_class) cmdtype,
>>  		       docstring, cmd_list);
>>  
>> +      /* The above doesn't copy nor take ownership of the name... so we just
>> +         leak it.  */
> 
> s/leak/release/?

I really wanted to say "leak", as we release it without an owner and
accept it will never be freed.  Sometimes we release in order to
transfer ownership to the callee (who hasn't been updated to use unique
pointers yet), but that's not the case here.  I think the comment would
be clear with either word.

Simon

Re: [PATCH] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Simon Marchi]

On 2021-03-23 2:11 p.m., Simon Marchi via Gdb-patches wrote:
> On 2021-03-23 2:07 p.m., Andreas Schwab wrote:> On Mär 23 2021, Simon Marchi via Gdb-patches wrote:
>>
>>> @@ -545,14 +540,19 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
>>>  	  /* If we have our own "invoke" method, then allow unknown
>>>  	     sub-commands.  */
>>>  	  allow_unknown = PyObject_HasAttr (self, invoke_cst);
>>> -	  cmd = add_prefix_cmd (cmd_name, (enum command_class) cmdtype,
>>> +	  cmd = add_prefix_cmd (cmd_name.get (),
>>> +				(enum command_class) cmdtype,
>>>  				NULL, docstring, &obj->sub_list,
>>>  				pfx_name, allow_unknown, cmd_list);
>>>  	}
>>>        else
>>> -	cmd = add_cmd (cmd_name, (enum command_class) cmdtype,
>>> +	cmd = add_cmd (cmd_name.get (), (enum command_class) cmdtype,
>>>  		       docstring, cmd_list);
>>>  
>>> +      /* The above doesn't copy nor take ownership of the name... so we just
>>> +         leak it.  */
>>
>> s/leak/release/?
> 
> I really wanted to say "leak", as we release it without an owner and
> accept it will never be freed.  Sometimes we release in order to
> transfer ownership to the callee (who hasn't been updated to use unique
> pointers yet), but that's not the case here.  I think the comment would
> be clear with either word.

Ah, I think I'm mistaken, the Python code uses:

    cmd->name_allocated = 1;

Which means the cmd_list_element would free the name if it ever gets
deleted.

I'll change the comment to:

      /* If successful, the above takes ownership of the name, since we set
         name_allocated, so release it.  */
      cmd_name.release ();

However, name_allocated isn't set in py-param.c, I guess it should...

Simon

[PATCH v2] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Simon Marchi]

This avoids some manual memory management.

cmdpy_init correctly transfers ownership of the name to the
cmd_list_element, as it sets the name_allocated flag.  However,
cmdpy_init (and add_setshow_generic) doesn't, it looks like the name is
just leaked.  This is a bit tricky, because it actually creates two
commands (one set and one show), it would take a bit of refactoring of
the command code to give each their own allocated copy.  For now, just
keep doing what the current code does but in a more explicit fashion,
with an explicit release.

gdb/ChangeLog:

	* python/python-internal.h (gdbpy_parse_command_name): Return
	gdb::unique_xmalloc_ptr.
	* python/py-cmd.c (gdbpy_parse_command_name): Likewise.
	(cmdpy_init): Adjust.
	* python/py-param.c (parmpy_init): Adjust.
	(add_setshow_generic): Take gdb::unique_xmalloc_ptr, release it
	when done.

Change-Id: Iae5bc21fe2b22f12d5f954057b0aca7ca4cd3f0d
---
 gdb/python/py-cmd.c          | 35 ++++++++++++------------
 gdb/python/py-param.c        | 52 ++++++++++++++++++------------------
 gdb/python/python-internal.h |  6 ++---
 3 files changed, 46 insertions(+), 47 deletions(-)

diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index f4d3dcc31218..eaaa5ea84a9b 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -348,7 +348,7 @@ cmdpy_completer (struct cmd_list_element *command,
    This function returns the xmalloc()d name of the new command.  On
    error sets the Python error and returns NULL.  */
 
-char *
+gdb::unique_xmalloc_ptr<char>
 gdbpy_parse_command_name (const char *name,
 			  struct cmd_list_element ***base_list,
 			  struct cmd_list_element **start_list)
@@ -357,7 +357,6 @@ gdbpy_parse_command_name (const char *name,
   int len = strlen (name);
   int i, lastchar;
   const char *prefix_text2;
-  char *result;
 
   /* Skip trailing whitespace.  */
   for (i = len - 1; i >= 0 && (name[i] == ' ' || name[i] == '\t'); --i)
@@ -372,9 +371,10 @@ gdbpy_parse_command_name (const char *name,
   /* Find first character of the final word.  */
   for (; i > 0 && valid_cmd_char_p (name[i - 1]); --i)
     ;
-  result = (char *) xmalloc (lastchar - i + 2);
-  memcpy (result, &name[i], lastchar - i + 1);
-  result[lastchar - i + 1] = '\0';
+
+  gdb::unique_xmalloc_ptr<char> result ((char *) xmalloc (lastchar - i + 2));
+  memcpy (result.get (), &name[i], lastchar - i + 1);
+  result.get ()[lastchar - i + 1] = '\0';
 
   /* Skip whitespace again.  */
   for (--i; i >= 0 && (name[i] == ' ' || name[i] == '\t'); --i)
@@ -393,7 +393,6 @@ gdbpy_parse_command_name (const char *name,
     {
       PyErr_Format (PyExc_RuntimeError, _("Could not find command prefix %s."),
 		    prefix_text.c_str ());
-      xfree (result);
       return NULL;
     }
 
@@ -405,7 +404,6 @@ gdbpy_parse_command_name (const char *name,
 
   PyErr_Format (PyExc_RuntimeError, _("'%s' is not a prefix command."),
 		prefix_text.c_str ());
-  xfree (result);
   return NULL;
 }
 
@@ -438,7 +436,7 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
   int completetype = -1;
   char *docstring = NULL;
   struct cmd_list_element **cmd_list;
-  char *cmd_name, *pfx_name;
+  char *pfx_name;
   static const char *keywords[] = { "name", "command_class", "completer_class",
 				    "prefix", NULL };
   PyObject *is_prefix = NULL;
@@ -477,8 +475,9 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
       return -1;
     }
 
-  cmd_name = gdbpy_parse_command_name (name, &cmd_list, &cmdlist);
-  if (! cmd_name)
+  gdb::unique_xmalloc_ptr<char> cmd_name
+    = gdbpy_parse_command_name (name, &cmd_list, &cmdlist);
+  if (cmd_name == nullptr)
     return -1;
 
   pfx_name = NULL;
@@ -509,10 +508,7 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
 	  pfx_name[out] = '\0';
 	}
       else if (cmp < 0)
-	{
-	  xfree (cmd_name);
-	  return -1;
-	}
+	return -1;
     }
   if (PyObject_HasAttr (self, gdbpy_doc_cst))
     {
@@ -523,7 +519,6 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
 	  docstring = python_string_to_host_string (ds_obj.get ()).release ();
 	  if (docstring == NULL)
 	    {
-	      xfree (cmd_name);
 	      xfree (pfx_name);
 	      return -1;
 	    }
@@ -545,14 +540,19 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
 	  /* If we have our own "invoke" method, then allow unknown
 	     sub-commands.  */
 	  allow_unknown = PyObject_HasAttr (self, invoke_cst);
-	  cmd = add_prefix_cmd (cmd_name, (enum command_class) cmdtype,
+	  cmd = add_prefix_cmd (cmd_name.get (),
+				(enum command_class) cmdtype,
 				NULL, docstring, &obj->sub_list,
 				pfx_name, allow_unknown, cmd_list);
 	}
       else
-	cmd = add_cmd (cmd_name, (enum command_class) cmdtype,
+	cmd = add_cmd (cmd_name.get (), (enum command_class) cmdtype,
 		       docstring, cmd_list);
 
+      /* If successful, the above takes ownership of the name, since we set
+         name_allocated, so release it.  */
+      cmd_name.release ();
+
       /* There appears to be no API to set this.  */
       cmd->func = cmdpy_function;
       cmd->destroyer = cmdpy_destroyer;
@@ -569,7 +569,6 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
     }
   catch (const gdb_exception &except)
     {
-      xfree (cmd_name);
       xfree (docstring);
       xfree (pfx_name);
       gdbpy_convert_exception (except);
diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c
index ab9e883dc2d6..71fb9a53595a 100644
--- a/gdb/python/py-param.c
+++ b/gdb/python/py-param.c
@@ -458,7 +458,8 @@ get_show_value (struct ui_file *file, int from_tty,
    function.  */
 static void
 add_setshow_generic (int parmclass, enum command_class cmdclass,
-		     const char *cmd_name, parmpy_object *self,
+		     gdb::unique_xmalloc_ptr<char> &&cmd_name,
+		     parmpy_object *self,
 		     const char *set_doc, const char *show_doc,
 		     const char *help_doc,
 		     struct cmd_list_element **set_list,
@@ -471,7 +472,7 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
     {
     case var_boolean:
 
-      add_setshow_boolean_cmd (cmd_name, cmdclass,
+      add_setshow_boolean_cmd (cmd_name.get (), cmdclass,
 			       &self->value.boolval, set_doc, show_doc,
 			       help_doc, get_set_value, get_show_value,
 			       set_list, show_list);
@@ -479,7 +480,7 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
       break;
 
     case var_auto_boolean:
-      add_setshow_auto_boolean_cmd (cmd_name, cmdclass,
+      add_setshow_auto_boolean_cmd (cmd_name.get (), cmdclass,
 				    &self->value.autoboolval,
 				    set_doc, show_doc, help_doc,
 				    get_set_value, get_show_value,
@@ -487,26 +488,26 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
       break;
 
     case var_uinteger:
-      add_setshow_uinteger_cmd (cmd_name, cmdclass,
+      add_setshow_uinteger_cmd (cmd_name.get (), cmdclass,
 				&self->value.uintval, set_doc, show_doc,
 				help_doc, get_set_value, get_show_value,
 				set_list, show_list);
       break;
 
     case var_integer:
-      add_setshow_integer_cmd (cmd_name, cmdclass,
+      add_setshow_integer_cmd (cmd_name.get (), cmdclass,
 			       &self->value.intval, set_doc, show_doc,
 			       help_doc, get_set_value, get_show_value,
 			       set_list, show_list); break;
 
     case var_string:
-      add_setshow_string_cmd (cmd_name, cmdclass,
+      add_setshow_string_cmd (cmd_name.get (), cmdclass,
 			      &self->value.stringval, set_doc, show_doc,
 			      help_doc, get_set_value, get_show_value,
 			      set_list, show_list); break;
 
     case var_string_noescape:
-      add_setshow_string_noescape_cmd (cmd_name, cmdclass,
+      add_setshow_string_noescape_cmd (cmd_name.get (), cmdclass,
 				       &self->value.stringval,
 				       set_doc, show_doc, help_doc,
 				       get_set_value, get_show_value,
@@ -515,7 +516,7 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
       break;
 
     case var_optional_filename:
-      add_setshow_optional_filename_cmd (cmd_name, cmdclass,
+      add_setshow_optional_filename_cmd (cmd_name.get (), cmdclass,
 					 &self->value.stringval, set_doc,
 					 show_doc, help_doc, get_set_value,
 					 get_show_value, set_list,
@@ -523,27 +524,27 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
       break;
 
     case var_filename:
-      add_setshow_filename_cmd (cmd_name, cmdclass,
+      add_setshow_filename_cmd (cmd_name.get (), cmdclass,
 				&self->value.stringval, set_doc, show_doc,
 				help_doc, get_set_value, get_show_value,
 				set_list, show_list); break;
 
     case var_zinteger:
-      add_setshow_zinteger_cmd (cmd_name, cmdclass,
+      add_setshow_zinteger_cmd (cmd_name.get (), cmdclass,
 				&self->value.intval, set_doc, show_doc,
 				help_doc, get_set_value, get_show_value,
 				set_list, show_list);
       break;
 
     case var_zuinteger:
-      add_setshow_zuinteger_cmd (cmd_name, cmdclass,
+      add_setshow_zuinteger_cmd (cmd_name.get (), cmdclass,
 				&self->value.uintval, set_doc, show_doc,
 				help_doc, get_set_value, get_show_value,
 				set_list, show_list);
       break;
 
     case var_zuinteger_unlimited:
-      add_setshow_zuinteger_unlimited_cmd (cmd_name, cmdclass,
+      add_setshow_zuinteger_unlimited_cmd (cmd_name.get (), cmdclass,
 					   &self->value.intval, set_doc,
 					   show_doc, help_doc, get_set_value,
 					   get_show_value,
@@ -551,7 +552,7 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
       break;
 
     case var_enum:
-      add_setshow_enum_cmd (cmd_name, cmdclass, self->enumeration,
+      add_setshow_enum_cmd (cmd_name.get (), cmdclass, self->enumeration,
 			    &self->value.cstringval, set_doc, show_doc,
 			    help_doc, get_set_value, get_show_value,
 			    set_list, show_list);
@@ -562,15 +563,18 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
 
   /* Lookup created parameter, and register Python object against the
      parameter context.  Perform this task against both lists.  */
-  tmp_name = cmd_name;
+  tmp_name = cmd_name.get ();
   param = lookup_cmd (&tmp_name, *show_list, "", NULL, 0, 1);
   if (param)
     set_cmd_context (param, self);
 
-  tmp_name = cmd_name;
+  tmp_name = cmd_name.get ();
   param = lookup_cmd (&tmp_name, *set_list, "", NULL, 0, 1);
   if (param)
     set_cmd_context (param, self);
+
+  /* We (unfortunately) currently leak the command name.  */
+  cmd_name.release ();
 }
 
 /* A helper which computes enum values.  Returns 1 on success.  Returns 0 on
@@ -657,7 +661,6 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds)
   parmpy_object *obj = (parmpy_object *) self;
   const char *name;
   gdb::unique_xmalloc_ptr<char> set_doc, show_doc, doc;
-  char *cmd_name;
   int parmclass, cmdtype;
   PyObject *enum_values = NULL;
   struct cmd_list_element **set_list, **show_list;
@@ -706,15 +709,13 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds)
   obj->type = (enum var_types) parmclass;
   memset (&obj->value, 0, sizeof (obj->value));
 
-  cmd_name = gdbpy_parse_command_name (name, &set_list,
-				       &setlist);
-
-  if (! cmd_name)
+  gdb::unique_xmalloc_ptr<char> cmd_name
+    = gdbpy_parse_command_name (name, &set_list, &setlist);
+  if (cmd_name == nullptr)
     return -1;
-  xfree (cmd_name);
-  cmd_name = gdbpy_parse_command_name (name, &show_list,
-				       &showlist);
-  if (! cmd_name)
+
+  cmd_name = gdbpy_parse_command_name (name, &show_list, &showlist);
+  if (cmd_name == nullptr)
     return -1;
 
   set_doc = get_doc_string (self, set_doc_cst);
@@ -726,13 +727,12 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds)
   try
     {
       add_setshow_generic (parmclass, (enum command_class) cmdtype,
-			   cmd_name, obj,
+			   std::move (cmd_name), obj,
 			   set_doc.get (), show_doc.get (),
 			   doc.get (), set_list, show_list);
     }
   catch (const gdb_exception &except)
     {
-      xfree (cmd_name);
       Py_DECREF (self);
       gdbpy_convert_exception (except);
       return -1;
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 56702cad53a0..690d2fb43c06 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -439,9 +439,9 @@ PyObject *gdbpy_selected_thread (PyObject *self, PyObject *args);
 PyObject *gdbpy_selected_inferior (PyObject *self, PyObject *args);
 PyObject *gdbpy_string_to_argv (PyObject *self, PyObject *args);
 PyObject *gdbpy_parameter_value (enum var_types type, void *var);
-char *gdbpy_parse_command_name (const char *name,
-				struct cmd_list_element ***base_list,
-				struct cmd_list_element **start_list);
+gdb::unique_xmalloc_ptr<char> gdbpy_parse_command_name
+  (const char *name, struct cmd_list_element ***base_list,
+   struct cmd_list_element **start_list);
 PyObject *gdbpy_register_tui_window (PyObject *self, PyObject *args,
 				     PyObject *kw);
 
-- 
2.30.1

Re: [PATCH] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> I'll change the comment to:

Simon>       /* If successful, the above takes ownership of the name, since we set
Simon>          name_allocated, so release it.  */
Simon>       cmd_name.release ();

Sounds reasonable.

Tom

Re: [PATCH v2] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> 	* python/python-internal.h (gdbpy_parse_command_name): Return
Simon> 	gdb::unique_xmalloc_ptr.
Simon> 	* python/py-cmd.c (gdbpy_parse_command_name): Likewise.
Simon> 	(cmdpy_init): Adjust.
Simon> 	* python/py-param.c (parmpy_init): Adjust.
Simon> 	(add_setshow_generic): Take gdb::unique_xmalloc_ptr, release it
Simon> 	when done.

Looks good.

Simon> +  /* We (unfortunately) currently leak the command name.  */
Simon> +  cmd_name.release ();

I wonder if the issue here is that two copies would need to be made,
because add_setshow_* makes two commands.

Tom

Re: [PATCH v2] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Marco Barisione]

On Tue, 23 Mar 2021 at 18:38, Simon Marchi via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
> This avoids some manual memory management.
>
> cmdpy_init correctly transfers ownership of the name to the
> cmd_list_element, as it sets the name_allocated flag.  However,
> cmdpy_init (and add_setshow_generic) doesn't, it looks like the name is
> just leaked.  This is a bit tricky, because it actually creates two
> commands (one set and one show), it would take a bit of refactoring of
> the command code to give each their own allocated copy.  For now, just
> keep doing what the current code does but in a more explicit fashion,
> with an explicit release.
>
> gdb/ChangeLog:
>
>         * python/python-internal.h (gdbpy_parse_command_name): Return
>         gdb::unique_xmalloc_ptr.
>         * python/py-cmd.c (gdbpy_parse_command_name): Likewise.
>         (cmdpy_init): Adjust.
>         * python/py-param.c (parmpy_init): Adjust.
>         (add_setshow_generic): Take gdb::unique_xmalloc_ptr, release it
>         when done.

Any news on this patch (which looks good to me)?
You wrote this in response to my command renaming work[1]. Sorry for
only looking at it now.
It would be helpful to merge it, so I can rebase my command renaming
patches on this. If it's not ready and needs more work, I'm happy to help.

[1] https://sourceware.org/pipermail/gdb-patches/2021-March/177165.html

Re: [PATCH v2] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Simon Marchi]

On 2021-05-12 10:12 a.m., Marco Barisione wrote:> On Tue, 23 Mar 2021 at 18:38, Simon Marchi via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
>>
>> This avoids some manual memory management.
>>
>> cmdpy_init correctly transfers ownership of the name to the
>> cmd_list_element, as it sets the name_allocated flag.  However,
>> cmdpy_init (and add_setshow_generic) doesn't, it looks like the name is
>> just leaked.  This is a bit tricky, because it actually creates two
>> commands (one set and one show), it would take a bit of refactoring of
>> the command code to give each their own allocated copy.  For now, just
>> keep doing what the current code does but in a more explicit fashion,
>> with an explicit release.
>>
>> gdb/ChangeLog:
>>
>>         * python/python-internal.h (gdbpy_parse_command_name): Return
>>         gdb::unique_xmalloc_ptr.
>>         * python/py-cmd.c (gdbpy_parse_command_name): Likewise.
>>         (cmdpy_init): Adjust.
>>         * python/py-param.c (parmpy_init): Adjust.
>>         (add_setshow_generic): Take gdb::unique_xmalloc_ptr, release it
>>         when done.
> 
> Any news on this patch (which looks good to me)?
> You wrote this in response to my command renaming work[1]. Sorry for
> only looking at it now.
> It would be helpful to merge it, so I can rebase my command renaming
> patches on this. If it's not ready and needs more work, I'm happy to help.
> 
> [1] https://sourceware.org/pipermail/gdb-patches/2021-March/177165.html
> 

I had forgotten about it!  I see that Tom sent some comments.  I'll
check that and merge or send a new version accordingly.

Simon

Re: [PATCH v2] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Simon Marchi]

On 2021-04-01 1:56 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> 	* python/python-internal.h (gdbpy_parse_command_name): Return
> Simon> 	gdb::unique_xmalloc_ptr.
> Simon> 	* python/py-cmd.c (gdbpy_parse_command_name): Likewise.
> Simon> 	(cmdpy_init): Adjust.
> Simon> 	* python/py-param.c (parmpy_init): Adjust.
> Simon> 	(add_setshow_generic): Take gdb::unique_xmalloc_ptr, release it
> Simon> 	when done.
> 
> Looks good.
> 
> Simon> +  /* We (unfortunately) currently leak the command name.  */
> Simon> +  cmd_name.release ();
> 
> I wonder if the issue here is that two copies would need to be made,
> because add_setshow_* makes two commands.

Exactly.  It's the same as before my patch, the comment only documents
what is happening.  This is really low prio, so I do not intend to fix
it.

Simon

[pushed] gdb: make gdbpy_parse_command_name return a unique_xmalloc_ptr

[Simon Marchi]

This is the version I pushed, after rebasing.

This avoids some manual memory management.

cmdpy_init correctly transfers ownership of the name to the
cmd_list_element, as it sets the name_allocated flag.  However,
cmdpy_init (and add_setshow_generic) doesn't, it looks like the name is
just leaked.  This is a bit tricky, because it actually creates two
commands (one set and one show), it would take a bit of refactoring of
the command code to give each their own allocated copy.  For now, just
keep doing what the current code does but in a more explicit fashion,
with an explicit release.

gdb/ChangeLog:

	* python/python-internal.h (gdbpy_parse_command_name): Return
	gdb::unique_xmalloc_ptr.
	* python/py-cmd.c (gdbpy_parse_command_name): Likewise.
	(cmdpy_init): Adjust.
	* python/py-param.c (parmpy_init): Adjust.
	(add_setshow_generic): Take gdb::unique_xmalloc_ptr, release it
	when done.

Change-Id: Iae5bc21fe2b22f12d5f954057b0aca7ca4cd3f0d
---
 gdb/ChangeLog                | 10 +++++++
 gdb/python/py-cmd.c          | 47 +++++++++++++++-----------------
 gdb/python/py-param.c        | 52 ++++++++++++++++++------------------
 gdb/python/python-internal.h |  6 ++---
 4 files changed, 61 insertions(+), 54 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d564621f4c64..fe75cee4b5e7 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,13 @@
+2021-05-12  Simon Marchi  <simon.marchi@polymtl.ca>
+
+	* python/python-internal.h (gdbpy_parse_command_name): Return
+	gdb::unique_xmalloc_ptr.
+	* python/py-cmd.c (gdbpy_parse_command_name): Likewise.
+	(cmdpy_init): Adjust.
+	* python/py-param.c (parmpy_init): Adjust.
+	(add_setshow_generic): Take gdb::unique_xmalloc_ptr, release it
+	when done.
+
 2021-05-12  George Barrett  <bob@bob131.so>
 
 	* NEWS (Guile API): Note the addition of the new procedure.
diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index 9833bf863b09..6f7794cdf53d 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -342,10 +342,10 @@ cmdpy_completer (struct cmd_list_element *command,
 
    START_LIST is the list in which the search starts.
 
-   This function returns the xmalloc()d name of the new command.  On
-   error sets the Python error and returns NULL.  */
+   This function returns the name of the new command.  On error sets the Python
+   error and returns NULL.  */
 
-char *
+gdb::unique_xmalloc_ptr<char>
 gdbpy_parse_command_name (const char *name,
 			  struct cmd_list_element ***base_list,
 			  struct cmd_list_element **start_list)
@@ -354,7 +354,6 @@ gdbpy_parse_command_name (const char *name,
   int len = strlen (name);
   int i, lastchar;
   const char *prefix_text2;
-  char *result;
 
   /* Skip trailing whitespace.  */
   for (i = len - 1; i >= 0 && (name[i] == ' ' || name[i] == '\t'); --i)
@@ -369,9 +368,10 @@ gdbpy_parse_command_name (const char *name,
   /* Find first character of the final word.  */
   for (; i > 0 && valid_cmd_char_p (name[i - 1]); --i)
     ;
-  result = (char *) xmalloc (lastchar - i + 2);
-  memcpy (result, &name[i], lastchar - i + 1);
-  result[lastchar - i + 1] = '\0';
+
+  gdb::unique_xmalloc_ptr<char> result ((char *) xmalloc (lastchar - i + 2));
+  memcpy (result.get (), &name[i], lastchar - i + 1);
+  result.get ()[lastchar - i + 1] = '\0';
 
   /* Skip whitespace again.  */
   for (--i; i >= 0 && (name[i] == ' ' || name[i] == '\t'); --i)
@@ -390,7 +390,6 @@ gdbpy_parse_command_name (const char *name,
     {
       PyErr_Format (PyExc_RuntimeError, _("Could not find command prefix %s."),
 		    prefix_text.c_str ());
-      xfree (result);
       return NULL;
     }
 
@@ -402,7 +401,6 @@ gdbpy_parse_command_name (const char *name,
 
   PyErr_Format (PyExc_RuntimeError, _("'%s' is not a prefix command."),
 		prefix_text.c_str ());
-  xfree (result);
   return NULL;
 }
 
@@ -435,7 +433,6 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
   int completetype = -1;
   char *docstring = NULL;
   struct cmd_list_element **cmd_list;
-  char *cmd_name;
   static const char *keywords[] = { "name", "command_class", "completer_class",
 				    "prefix", NULL };
   PyObject *is_prefix_obj = NULL;
@@ -474,19 +471,18 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
       return -1;
     }
 
-  cmd_name = gdbpy_parse_command_name (name, &cmd_list, &cmdlist);
-  if (! cmd_name)
+  gdb::unique_xmalloc_ptr<char> cmd_name
+    = gdbpy_parse_command_name (name, &cmd_list, &cmdlist);
+  if (cmd_name == nullptr)
     return -1;
 
   if (is_prefix_obj != NULL)
     {
       int cmp = PyObject_IsTrue (is_prefix_obj);
-       if (cmp < 0)
-	{
-	  xfree (cmd_name);
-	  return -1;
-	}
-       is_prefix = cmp > 0;
+      if (cmp < 0)
+	return -1;
+
+      is_prefix = cmp > 0;
     }
 
   if (PyObject_HasAttr (self, gdbpy_doc_cst))
@@ -497,10 +493,7 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
 	{
 	  docstring = python_string_to_host_string (ds_obj.get ()).release ();
 	  if (docstring == NULL)
-	    {
-	      xfree (cmd_name);
-	      return -1;
-	    }
+	    return -1;
 	}
     }
   if (! docstring)
@@ -519,14 +512,19 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
 	  /* If we have our own "invoke" method, then allow unknown
 	     sub-commands.  */
 	  allow_unknown = PyObject_HasAttr (self, invoke_cst);
-	  cmd = add_prefix_cmd (cmd_name, (enum command_class) cmdtype,
+	  cmd = add_prefix_cmd (cmd_name.get (),
+				(enum command_class) cmdtype,
 				NULL, docstring, &obj->sub_list,
 				allow_unknown, cmd_list);
 	}
       else
-	cmd = add_cmd (cmd_name, (enum command_class) cmdtype,
+	cmd = add_cmd (cmd_name.get (), (enum command_class) cmdtype,
 		       docstring, cmd_list);
 
+      /* If successful, the above takes ownership of the name, since we set
+         name_allocated, so release it.  */
+      cmd_name.release ();
+
       /* There appears to be no API to set this.  */
       cmd->func = cmdpy_function;
       cmd->destroyer = cmdpy_destroyer;
@@ -543,7 +541,6 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
     }
   catch (const gdb_exception &except)
     {
-      xfree (cmd_name);
       xfree (docstring);
       gdbpy_convert_exception (except);
       return -1;
diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c
index ab9e883dc2d6..99a57960c059 100644
--- a/gdb/python/py-param.c
+++ b/gdb/python/py-param.c
@@ -458,7 +458,8 @@ get_show_value (struct ui_file *file, int from_tty,
    function.  */
 static void
 add_setshow_generic (int parmclass, enum command_class cmdclass,
-		     const char *cmd_name, parmpy_object *self,
+		     gdb::unique_xmalloc_ptr<char> cmd_name,
+		     parmpy_object *self,
 		     const char *set_doc, const char *show_doc,
 		     const char *help_doc,
 		     struct cmd_list_element **set_list,
@@ -471,7 +472,7 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
     {
     case var_boolean:
 
-      add_setshow_boolean_cmd (cmd_name, cmdclass,
+      add_setshow_boolean_cmd (cmd_name.get (), cmdclass,
 			       &self->value.boolval, set_doc, show_doc,
 			       help_doc, get_set_value, get_show_value,
 			       set_list, show_list);
@@ -479,7 +480,7 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
       break;
 
     case var_auto_boolean:
-      add_setshow_auto_boolean_cmd (cmd_name, cmdclass,
+      add_setshow_auto_boolean_cmd (cmd_name.get (), cmdclass,
 				    &self->value.autoboolval,
 				    set_doc, show_doc, help_doc,
 				    get_set_value, get_show_value,
@@ -487,26 +488,26 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
       break;
 
     case var_uinteger:
-      add_setshow_uinteger_cmd (cmd_name, cmdclass,
+      add_setshow_uinteger_cmd (cmd_name.get (), cmdclass,
 				&self->value.uintval, set_doc, show_doc,
 				help_doc, get_set_value, get_show_value,
 				set_list, show_list);
       break;
 
     case var_integer:
-      add_setshow_integer_cmd (cmd_name, cmdclass,
+      add_setshow_integer_cmd (cmd_name.get (), cmdclass,
 			       &self->value.intval, set_doc, show_doc,
 			       help_doc, get_set_value, get_show_value,
 			       set_list, show_list); break;
 
     case var_string:
-      add_setshow_string_cmd (cmd_name, cmdclass,
+      add_setshow_string_cmd (cmd_name.get (), cmdclass,
 			      &self->value.stringval, set_doc, show_doc,
 			      help_doc, get_set_value, get_show_value,
 			      set_list, show_list); break;
 
     case var_string_noescape:
-      add_setshow_string_noescape_cmd (cmd_name, cmdclass,
+      add_setshow_string_noescape_cmd (cmd_name.get (), cmdclass,
 				       &self->value.stringval,
 				       set_doc, show_doc, help_doc,
 				       get_set_value, get_show_value,
@@ -515,7 +516,7 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
       break;
 
     case var_optional_filename:
-      add_setshow_optional_filename_cmd (cmd_name, cmdclass,
+      add_setshow_optional_filename_cmd (cmd_name.get (), cmdclass,
 					 &self->value.stringval, set_doc,
 					 show_doc, help_doc, get_set_value,
 					 get_show_value, set_list,
@@ -523,27 +524,27 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
       break;
 
     case var_filename:
-      add_setshow_filename_cmd (cmd_name, cmdclass,
+      add_setshow_filename_cmd (cmd_name.get (), cmdclass,
 				&self->value.stringval, set_doc, show_doc,
 				help_doc, get_set_value, get_show_value,
 				set_list, show_list); break;
 
     case var_zinteger:
-      add_setshow_zinteger_cmd (cmd_name, cmdclass,
+      add_setshow_zinteger_cmd (cmd_name.get (), cmdclass,
 				&self->value.intval, set_doc, show_doc,
 				help_doc, get_set_value, get_show_value,
 				set_list, show_list);
       break;
 
     case var_zuinteger:
-      add_setshow_zuinteger_cmd (cmd_name, cmdclass,
+      add_setshow_zuinteger_cmd (cmd_name.get (), cmdclass,
 				&self->value.uintval, set_doc, show_doc,
 				help_doc, get_set_value, get_show_value,
 				set_list, show_list);
       break;
 
     case var_zuinteger_unlimited:
-      add_setshow_zuinteger_unlimited_cmd (cmd_name, cmdclass,
+      add_setshow_zuinteger_unlimited_cmd (cmd_name.get (), cmdclass,
 					   &self->value.intval, set_doc,
 					   show_doc, help_doc, get_set_value,
 					   get_show_value,
@@ -551,7 +552,7 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
       break;
 
     case var_enum:
-      add_setshow_enum_cmd (cmd_name, cmdclass, self->enumeration,
+      add_setshow_enum_cmd (cmd_name.get (), cmdclass, self->enumeration,
 			    &self->value.cstringval, set_doc, show_doc,
 			    help_doc, get_set_value, get_show_value,
 			    set_list, show_list);
@@ -562,15 +563,18 @@ add_setshow_generic (int parmclass, enum command_class cmdclass,
 
   /* Lookup created parameter, and register Python object against the
      parameter context.  Perform this task against both lists.  */
-  tmp_name = cmd_name;
+  tmp_name = cmd_name.get ();
   param = lookup_cmd (&tmp_name, *show_list, "", NULL, 0, 1);
   if (param)
     set_cmd_context (param, self);
 
-  tmp_name = cmd_name;
+  tmp_name = cmd_name.get ();
   param = lookup_cmd (&tmp_name, *set_list, "", NULL, 0, 1);
   if (param)
     set_cmd_context (param, self);
+
+  /* We (unfortunately) currently leak the command name.  */
+  cmd_name.release ();
 }
 
 /* A helper which computes enum values.  Returns 1 on success.  Returns 0 on
@@ -657,7 +661,6 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds)
   parmpy_object *obj = (parmpy_object *) self;
   const char *name;
   gdb::unique_xmalloc_ptr<char> set_doc, show_doc, doc;
-  char *cmd_name;
   int parmclass, cmdtype;
   PyObject *enum_values = NULL;
   struct cmd_list_element **set_list, **show_list;
@@ -706,15 +709,13 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds)
   obj->type = (enum var_types) parmclass;
   memset (&obj->value, 0, sizeof (obj->value));
 
-  cmd_name = gdbpy_parse_command_name (name, &set_list,
-				       &setlist);
-
-  if (! cmd_name)
+  gdb::unique_xmalloc_ptr<char> cmd_name
+    = gdbpy_parse_command_name (name, &set_list, &setlist);
+  if (cmd_name == nullptr)
     return -1;
-  xfree (cmd_name);
-  cmd_name = gdbpy_parse_command_name (name, &show_list,
-				       &showlist);
-  if (! cmd_name)
+
+  cmd_name = gdbpy_parse_command_name (name, &show_list, &showlist);
+  if (cmd_name == nullptr)
     return -1;
 
   set_doc = get_doc_string (self, set_doc_cst);
@@ -726,13 +727,12 @@ parmpy_init (PyObject *self, PyObject *args, PyObject *kwds)
   try
     {
       add_setshow_generic (parmclass, (enum command_class) cmdtype,
-			   cmd_name, obj,
+			   std::move (cmd_name), obj,
 			   set_doc.get (), show_doc.get (),
 			   doc.get (), set_list, show_list);
     }
   catch (const gdb_exception &except)
     {
-      xfree (cmd_name);
       Py_DECREF (self);
       gdbpy_convert_exception (except);
       return -1;
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 56702cad53a0..690d2fb43c06 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -439,9 +439,9 @@ PyObject *gdbpy_selected_thread (PyObject *self, PyObject *args);
 PyObject *gdbpy_selected_inferior (PyObject *self, PyObject *args);
 PyObject *gdbpy_string_to_argv (PyObject *self, PyObject *args);
 PyObject *gdbpy_parameter_value (enum var_types type, void *var);
-char *gdbpy_parse_command_name (const char *name,
-				struct cmd_list_element ***base_list,
-				struct cmd_list_element **start_list);
+gdb::unique_xmalloc_ptr<char> gdbpy_parse_command_name
+  (const char *name, struct cmd_list_element ***base_list,
+   struct cmd_list_element **start_list);
 PyObject *gdbpy_register_tui_window (PyObject *self, PyObject *args,
 				     PyObject *kw);
 
-- 
2.30.1