[PATCH 0/2] Remove some use of xfree from MI

[PATCH 0/2] Remove some use of xfree from MI

[Tom Tromey]

This series removes some uses of xfree from the MI code.  No visible
changes are expected.  Regression tested on x86-64 Fedora 36.

---
Tom Tromey (2):
      Use std::string for MI token
      Use unique_xmalloc_ptr for mi_parse::command

 gdb/mi/mi-cmds.c      |  4 ++--
 gdb/mi/mi-main.c      | 28 ++++++++++++++--------------
 gdb/mi/mi-main.h      |  2 +-
 gdb/mi/mi-parse.c     | 30 ++++++++++++------------------
 gdb/mi/mi-parse.h     | 15 ++++++++-------
 gdb/python/py-micmd.c |  2 +-
 6 files changed, 38 insertions(+), 43 deletions(-)
---
base-commit: 05d63bafad71b534c041275831489f3e0dedcf3f
change-id: 20230609-mi-xfree-47c5b7daed76

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/2] Use std::string for MI token

[Tom Tromey]

This changes the MI "token" to be a std::string, removing some manual
memory management.  It also makes current_token 'const' in order to
support this change.
---
 gdb/mi/mi-main.c  | 21 ++++++++++-----------
 gdb/mi/mi-main.h  |  2 +-
 gdb/mi/mi-parse.c |  9 +++------
 gdb/mi/mi-parse.h | 11 +++++------
 4 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 7503ffd2f18..7d671657a44 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -78,7 +78,7 @@ static struct mi_timestamp *current_command_ts;
 
 static int do_timings = 0;
 
-char *current_token;
+const char *current_token;
 /* Few commands would like to know if options like --thread-group were
    explicitly specified.  This variable keeps the current parsed
    command including all option, and make it possible.  */
@@ -1810,7 +1810,7 @@ captured_mi_execute_command (struct ui_out *uiout, struct mi_parse *context)
     current_command_ts = context->cmd_start;
 
   scoped_restore save_token = make_scoped_restore (&current_token,
-						   context->token);
+						   context->token.c_str ());
 
   mi->running_result_record_printed = 0;
   mi->mi_proceeded = 0;
@@ -1821,7 +1821,7 @@ captured_mi_execute_command (struct ui_out *uiout, struct mi_parse *context)
       if (mi_debug_p)
 	gdb_printf (gdb_stdlog,
 		    " token=`%s' command=`%s' args=`%s'\n",
-		    context->token, context->command, context->args ());
+		    context->token.c_str (), context->command, context->args ());
 
       mi_cmd_execute (context);
 
@@ -1833,7 +1833,7 @@ captured_mi_execute_command (struct ui_out *uiout, struct mi_parse *context)
 	 uiout will most likely crash in the mi_out_* routines.  */
       if (!mi->running_result_record_printed)
 	{
-	  gdb_puts (context->token, mi->raw_stdout);
+	  gdb_puts (context->token.c_str (), mi->raw_stdout);
 	  /* There's no particularly good reason why target-connect results
 	     in not ^done.  Should kill ^connected for MI3.  */
 	  gdb_puts (strcmp (context->command, "target-select") == 0
@@ -1872,7 +1872,7 @@ captured_mi_execute_command (struct ui_out *uiout, struct mi_parse *context)
 	  {
 	    if (!mi->running_result_record_printed)
 	      {
-		gdb_puts (context->token, mi->raw_stdout);
+		gdb_puts (context->token.c_str (), mi->raw_stdout);
 		gdb_puts ("^done", mi->raw_stdout);
 		mi_out_put (uiout, mi->raw_stdout);
 		mi_out_rewind (uiout);
@@ -1915,7 +1915,7 @@ mi_print_exception (const char *token, const struct gdb_exception &exception)
 void
 mi_execute_command (const char *cmd, int from_tty)
 {
-  char *token;
+  std::string token;
   std::unique_ptr<struct mi_parse> command;
 
   /* This is to handle EOF (^D). We just quit gdb.  */
@@ -1931,13 +1931,12 @@ mi_execute_command (const char *cmd, int from_tty)
     }
   catch (const gdb_exception &exception)
     {
-      mi_print_exception (token, exception);
-      xfree (token);
+      mi_print_exception (token.c_str (), exception);
     }
 
   if (command != NULL)
     {
-      command->token = token;
+      command->token = std::move (token);
 
       if (do_timings)
 	{
@@ -1960,7 +1959,7 @@ mi_execute_command (const char *cmd, int from_tty)
 
 	  /* The command execution failed and error() was called
 	     somewhere.  */
-	  mi_print_exception (command->token, result);
+	  mi_print_exception (command->token.c_str (), result);
 	  mi_out_rewind (current_uiout);
 
 	  /* Throw to a higher level catch for SIGTERM sent to GDB.  */
@@ -1982,7 +1981,7 @@ mi_execute_command (mi_parse *context)
     error (_("Command is not an MI command"));
 
   scoped_restore save_token = make_scoped_restore (&current_token,
-						   context->token);
+						   context->token.c_str ());
   scoped_restore save_debug = make_scoped_restore (&mi_debug_p, 0);
 
   mi_cmd_execute (context);
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index 1741f48b3ee..cb17921c150 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -34,7 +34,7 @@ extern void mi_print_timing_maybe (struct ui_file *file);
 
 extern int mi_async_p (void);
 
-extern char *current_token;
+extern const char *current_token;
 
 struct mi_suppress_notification
 {
diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c
index a113d4d48da..aceecad6db6 100644
--- a/gdb/mi/mi-parse.c
+++ b/gdb/mi/mi-parse.c
@@ -216,7 +216,6 @@ mi_parse::parse_argv ()
 mi_parse::~mi_parse ()
 {
   xfree (command);
-  xfree (token);
   freeargv (argv);
 }
 
@@ -290,7 +289,7 @@ mi_parse::set_language (const char *arg, const char **endp)
 }
 
 std::unique_ptr<struct mi_parse>
-mi_parse::make (const char *cmd, char **token)
+mi_parse::make (const char *cmd, std::string *token)
 {
   const char *chp;
 
@@ -302,9 +301,7 @@ mi_parse::make (const char *cmd, char **token)
   /* Find/skip any token and then extract it.  */
   for (chp = cmd; *chp >= '0' && *chp <= '9'; chp++)
     ;
-  *token = (char *) xmalloc (chp - cmd + 1);
-  memcpy (*token, cmd, (chp - cmd));
-  (*token)[chp - cmd] = '\0';
+  *token = std::string (cmd, chp - cmd);
 
   /* This wasn't a real MI command.  Return it as a CLI_COMMAND.  */
   if (*chp != '-')
@@ -422,7 +419,7 @@ mi_parse::make (gdb::unique_xmalloc_ptr<char> command,
   std::unique_ptr<struct mi_parse> parse (new struct mi_parse);
 
   parse->command = command.release ();
-  parse->token = xstrdup ("");
+  parse->token = "";
 
   if (parse->command[0] != '-')
     throw_error (UNDEFINED_COMMAND_ERROR,
diff --git a/gdb/mi/mi-parse.h b/gdb/mi/mi-parse.h
index 6373543529b..78fb414b4f0 100644
--- a/gdb/mi/mi-parse.h
+++ b/gdb/mi/mi-parse.h
@@ -44,13 +44,12 @@ struct mi_parse
     /* Attempts to parse CMD returning a ``struct mi_parse''.  If CMD is
        invalid, an exception is thrown.  For an MI_COMMAND COMMAND, ARGS
        and OP are initialized.  Un-initialized fields are zero.  *TOKEN is
-       set to the token, even if an exception is thrown.  It is allocated
-       with xmalloc; it must either be freed with xfree, or assigned to
-       the TOKEN field of the resultant mi_parse object, to be freed by
-       mi_parse_free.  */
+       set to the token, even if an exception is thrown.  It can be
+       assigned to the TOKEN field of the resultant mi_parse object,
+       to be freed by mi_parse_free.  */
 
     static std::unique_ptr<struct mi_parse> make (const char *cmd,
-						  char **token);
+						  std::string *token);
 
     /* Create an mi_parse object given the command name and a vector
        of arguments.  Unlike with the other constructor, here the
@@ -74,7 +73,7 @@ struct mi_parse
 
     enum mi_command_type op = MI_COMMAND;
     char *command = nullptr;
-    char *token = nullptr;
+    std::string token;
     const struct mi_command *cmd = nullptr;
     struct mi_timestamp *cmd_start = nullptr;
     char **argv = nullptr;

-- 
2.40.1

[PATCH 2/2] Use unique_xmalloc_ptr for mi_parse::command

[Tom Tromey]

This changes mi_parse::command to be a unique_xmalloc_ptr and fixes up
all the uses.  This avoids some manual memory management.  std::string
is not used here due to how the Python API works -- this approach
avoids an extra copy there.
---
 gdb/mi/mi-cmds.c      |  4 ++--
 gdb/mi/mi-main.c      |  9 +++++----
 gdb/mi/mi-parse.c     | 21 +++++++++------------
 gdb/mi/mi-parse.h     |  4 +++-
 gdb/python/py-micmd.c |  2 +-
 5 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index f8cae4131d8..5ea31fc98d1 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -52,10 +52,10 @@ struct mi_command_mi : public mi_command
     parse->parse_argv ();
 
     if (parse->argv == nullptr)
-      error (_("Problem parsing arguments: %s %s"), parse->command,
+      error (_("Problem parsing arguments: %s %s"), parse->command.get (),
 	     parse->args ());
 
-    this->m_argv_function (parse->command, parse->argv, parse->argc);
+    this->m_argv_function (parse->command.get (), parse->argv, parse->argc);
   }
 
 private:
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 7d671657a44..9108cf505c7 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1821,7 +1821,8 @@ captured_mi_execute_command (struct ui_out *uiout, struct mi_parse *context)
       if (mi_debug_p)
 	gdb_printf (gdb_stdlog,
 		    " token=`%s' command=`%s' args=`%s'\n",
-		    context->token.c_str (), context->command, context->args ());
+		    context->token.c_str (), context->command.get (),
+		    context->args ());
 
       mi_cmd_execute (context);
 
@@ -1836,7 +1837,7 @@ captured_mi_execute_command (struct ui_out *uiout, struct mi_parse *context)
 	  gdb_puts (context->token.c_str (), mi->raw_stdout);
 	  /* There's no particularly good reason why target-connect results
 	     in not ^done.  Should kill ^connected for MI3.  */
-	  gdb_puts (strcmp (context->command, "target-select") == 0
+	  gdb_puts (strcmp (context->command.get (), "target-select") == 0
 		    ? "^connected" : "^done", mi->raw_stdout);
 	  mi_out_put (uiout, mi->raw_stdout);
 	  mi_out_rewind (uiout);
@@ -1858,10 +1859,10 @@ captured_mi_execute_command (struct ui_out *uiout, struct mi_parse *context)
 	/* This "feature" will be removed as soon as we have a
 	   complete set of mi commands.  */
 	/* Echo the command on the console.  */
-	gdb_printf (gdb_stdlog, "%s\n", context->command);
+	gdb_printf (gdb_stdlog, "%s\n", context->command.get ());
 	/* Call the "console" interpreter.  */
 	argv[0] = INTERP_CONSOLE;
-	argv[1] = context->command;
+	argv[1] = context->command.get ();
 	mi_cmd_interpreter_exec ("-interpreter-exec", argv, 2);
 
 	/* If we changed interpreters, DON'T print out anything.  */
diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c
index aceecad6db6..a9b9cdaf88f 100644
--- a/gdb/mi/mi-parse.c
+++ b/gdb/mi/mi-parse.c
@@ -215,7 +215,6 @@ mi_parse::parse_argv ()
 
 mi_parse::~mi_parse ()
 {
-  xfree (command);
   freeargv (argv);
 }
 
@@ -307,7 +306,7 @@ mi_parse::make (const char *cmd, std::string *token)
   if (*chp != '-')
     {
       chp = skip_spaces (chp);
-      parse->command = xstrdup (chp);
+      parse->command = make_unique_xstrdup (chp);
       parse->op = CLI_COMMAND;
 
       return parse;
@@ -319,16 +318,14 @@ mi_parse::make (const char *cmd, std::string *token)
 
     for (; *chp && !isspace (*chp); chp++)
       ;
-    parse->command = (char *) xmalloc (chp - tmp + 1);
-    memcpy (parse->command, tmp, chp - tmp);
-    parse->command[chp - tmp] = '\0';
+    parse->command = make_unique_xstrndup (tmp, chp - tmp);
   }
 
   /* Find the command in the MI table.  */
-  parse->cmd = mi_cmd_lookup (parse->command);
+  parse->cmd = mi_cmd_lookup (parse->command.get ());
   if (parse->cmd == NULL)
     throw_error (UNDEFINED_COMMAND_ERROR,
-		 _("Undefined MI command: %s"), parse->command);
+		 _("Undefined MI command: %s"), parse->command.get ());
 
   /* Skip white space following the command.  */
   chp = skip_spaces (chp);
@@ -418,19 +415,19 @@ mi_parse::make (gdb::unique_xmalloc_ptr<char> command,
 {
   std::unique_ptr<struct mi_parse> parse (new struct mi_parse);
 
-  parse->command = command.release ();
+  parse->command = std::move (command);
   parse->token = "";
 
-  if (parse->command[0] != '-')
+  if (parse->command.get ()[0] != '-')
     throw_error (UNDEFINED_COMMAND_ERROR,
 		 _("MI command '%s' does not start with '-'"),
-		 parse->command);
+		 parse->command.get ());
 
   /* Find the command in the MI table.  */
-  parse->cmd = mi_cmd_lookup (parse->command + 1);
+  parse->cmd = mi_cmd_lookup (parse->command.get () + 1);
   if (parse->cmd == NULL)
     throw_error (UNDEFINED_COMMAND_ERROR,
-		 _("Undefined MI command: %s"), parse->command);
+		 _("Undefined MI command: %s"), parse->command.get ());
 
   /* This over-allocates slightly, but it seems unimportant.  */
   parse->argv = XCNEWVEC (char *, args.size () + 1);
diff --git a/gdb/mi/mi-parse.h b/gdb/mi/mi-parse.h
index 78fb414b4f0..c729e94c1f0 100644
--- a/gdb/mi/mi-parse.h
+++ b/gdb/mi/mi-parse.h
@@ -72,7 +72,9 @@ struct mi_parse
     const char *args ();
 
     enum mi_command_type op = MI_COMMAND;
-    char *command = nullptr;
+    /* This is not std::string because it avoids a copy in the Python
+       API case.  */
+    gdb::unique_xmalloc_ptr<char> command;
     std::string token;
     const struct mi_command *cmd = nullptr;
     struct mi_timestamp *cmd_start = nullptr;
diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
index 7027210d0d8..01fc6060ece 100644
--- a/gdb/python/py-micmd.c
+++ b/gdb/python/py-micmd.c
@@ -358,7 +358,7 @@ mi_command_py::invoke (struct mi_parse *parse) const
   parse->parse_argv ();
 
   if (parse->argv == nullptr)
-    error (_("Problem parsing arguments: %s %s"), parse->command,
+    error (_("Problem parsing arguments: %s %s"), parse->command.get (),
 	   parse->args ());
 
 

-- 
2.40.1

Re: [PATCH 0/2] Remove some use of xfree from MI

[Keith Seitz]

On 6/9/23 09:46, Tom Tromey via Gdb-patches wrote:
> This series removes some uses of xfree from the MI code.  No visible
> changes are expected.  Regression tested on x86-64 Fedora 36.

Ha, I am also using F36 (and regtested there, too).

The changes are pretty mechanical and LGTM.

Reviewed-by: Keith Seitz <keiths@redhat.com>

Keith