[PATCH 0/3] Minor improvements to 'interp'

[PATCH 0/3] Minor improvements to 'interp'

[Tom Tromey]

This series removes some manual memory management from 'interp', and
also makes some recompilations a bit faster by reducing dependencies
on interps.h.

Regression tested on x86-64 Fedora 34.

Tom

[PATCH 1/3] Use unique_xmalloc_ptr in interp

[Tom Tromey]

This changes interp::m_name to be a unique_xmalloc_ptr, removing some
manual memory management.  It also cleans up the initialization of the
'inited' member, and moves the 'private:' and 'public:' keywords to
their proper spots.
---
 gdb/interps.c |  4 +---
 gdb/interps.h | 10 +++++-----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index 44002ff2cb5..0c440e78685 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -79,14 +79,12 @@ static struct interp *interp_lookup_existing (struct ui *ui,
 					      const char *name);
 
 interp::interp (const char *name)
-  : m_name (xstrdup (name))
+  : m_name (make_unique_xstrdup (name))
 {
-  this->inited = false;
 }
 
 interp::~interp ()
 {
-  xfree (m_name);
 }
 
 /* An interpreter factory.  Maps an interpreter name to the factory
diff --git a/gdb/interps.h b/gdb/interps.h
index 330c1ba6615..e393b08c962 100644
--- a/gdb/interps.h
+++ b/gdb/interps.h
@@ -78,20 +78,20 @@ class interp
 
   const char *name () const
   {
-    return m_name;
+    return m_name.get ();
   }
 
-  /* This is the name in "-i=" and "set interpreter".  */
 private:
-  char *m_name;
+  /* This is the name in "-i=" and "set interpreter".  */
+  gdb::unique_xmalloc_ptr<char> m_name;
 
+public:
   /* Interpreters are stored in a linked list, this is the next
      one...  */
-public:
   struct interp *next;
 
   /* Has the init method been run?  */
-  bool inited;
+  bool inited = false;
 };
 
 /* Look up the interpreter for NAME, creating one if none exists yet.
-- 
2.34.1

[PATCH 2/3] Move mi_interpreter to mi-interp.h

[Tom Tromey]

I noticed that touching interps.h caused a lot of recompilation.  I
tracked this down to mi-common.h including this file.  This patch
moves the MI interpreter to mi-interp.h, which cuts down on
recompilation when modifying interps.h.
---
 gdb/mi/mi-common.h | 50 ----------------------------------------------
 gdb/mi/mi-interp.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/mi/mi-main.c   |  2 +-
 3 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/gdb/mi/mi-common.h b/gdb/mi/mi-common.h
index 0c254baf8b4..6970d41ae08 100644
--- a/gdb/mi/mi-common.h
+++ b/gdb/mi/mi-common.h
@@ -19,10 +19,6 @@
 #ifndef MI_MI_COMMON_H
 #define MI_MI_COMMON_H
 
-#include "interps.h"
-
-struct mi_console_file;
-
 /* Represents the reason why GDB is sending an asynchronous command to
    the front end.  NOTE: When modifing this, don't forget to update
    gdb.texinfo!  */
@@ -52,50 +48,4 @@ enum async_reply_reason
 
 const char *async_reason_lookup (enum async_reply_reason reason);
 
-/* An MI interpreter.  */
-
-class mi_interp final : public interp
-{
-public:
-  mi_interp (const char *name)
-    : interp (name)
-  {}
-
-  void init (bool top_level) override;
-  void resume () override;
-  void suspend () override;
-  gdb_exception exec (const char *command_str) override;
-  ui_out *interp_ui_out () override;
-  void set_logging (ui_file_up logfile, bool logging_redirect,
-		    bool debug_redirect) override;
-  void pre_command_loop () override;
-
-  /* MI's output channels */
-  mi_console_file *out;
-  mi_console_file *err;
-  mi_console_file *log;
-  mi_console_file *targ;
-  mi_console_file *event_channel;
-
-  /* Raw console output.  */
-  struct ui_file *raw_stdout;
-
-  /* Raw logfile output.  */
-  struct ui_file *raw_stdlog;
-
-  /* Save the original value of raw_stdout and raw_stdlog here when logging, and
-     the file which we need to delete, so we can restore correctly when
-     done.  */
-  struct ui_file *saved_raw_stdout;
-  struct ui_file *saved_raw_stdlog;
-  struct ui_file *saved_raw_file_to_delete;
-
-
-  /* MI's builder.  */
-  struct ui_out *mi_uiout;
-
-  /* MI's CLI builder (wraps OUT).  */
-  struct ui_out *cli_uiout;
-};
-
 #endif /* MI_MI_COMMON_H */
diff --git a/gdb/mi/mi-interp.h b/gdb/mi/mi-interp.h
index 53369f2eea7..adf6eaffef4 100644
--- a/gdb/mi/mi-interp.h
+++ b/gdb/mi/mi-interp.h
@@ -20,6 +20,56 @@
 #ifndef MI_MI_INTERP_H
 #define MI_MI_INTERP_H
 
+#include "interps.h"
+
+struct mi_console_file;
+
+/* An MI interpreter.  */
+
+class mi_interp final : public interp
+{
+public:
+  mi_interp (const char *name)
+    : interp (name)
+  {}
+
+  void init (bool top_level) override;
+  void resume () override;
+  void suspend () override;
+  gdb_exception exec (const char *command_str) override;
+  ui_out *interp_ui_out () override;
+  void set_logging (ui_file_up logfile, bool logging_redirect,
+		    bool debug_redirect) override;
+  void pre_command_loop () override;
+
+  /* MI's output channels */
+  mi_console_file *out;
+  mi_console_file *err;
+  mi_console_file *log;
+  mi_console_file *targ;
+  mi_console_file *event_channel;
+
+  /* Raw console output.  */
+  struct ui_file *raw_stdout;
+
+  /* Raw logfile output.  */
+  struct ui_file *raw_stdlog;
+
+  /* Save the original value of raw_stdout and raw_stdlog here when logging, and
+     the file which we need to delete, so we can restore correctly when
+     done.  */
+  struct ui_file *saved_raw_stdout;
+  struct ui_file *saved_raw_stdlog;
+  struct ui_file *saved_raw_file_to_delete;
+
+
+  /* MI's builder.  */
+  struct ui_out *mi_uiout;
+
+  /* MI's CLI builder (wraps OUT).  */
+  struct ui_out *cli_uiout;
+};
+
 /* Output the shared object attributes to UIOUT.  */
 
 void mi_output_solib_attribs (ui_out *uiout, struct so_list *solib);
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 18707bf62e7..68868e49e99 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -40,7 +40,7 @@
 #include "regcache.h"
 #include "frame.h"
 #include "mi-main.h"
-#include "mi-common.h"
+#include "mi-interp.h"
 #include "language.h"
 #include "valprint.h"
 #include "osdata.h"
-- 
2.34.1

[PATCH 3/3] Use std::string for interpreter_p

[Tom Tromey]

The global interpreter_p is a manually-managed 'char *'.  This patch
changes it to be a std::string instead, and removes some erroneous
comments.
---
 gdb/interps.c        | 11 ++---------
 gdb/main.c           | 24 +++++++++---------------
 gdb/main.h           |  2 +-
 gdb/tui/tui-interp.c |  9 +++------
 4 files changed, 15 insertions(+), 31 deletions(-)

diff --git a/gdb/interps.c b/gdb/interps.c
index 0c440e78685..3a9c590b8c8 100644
--- a/gdb/interps.c
+++ b/gdb/interps.c
@@ -168,15 +168,8 @@ interp_set (struct interp *interp, bool top_level)
   if (top_level)
     ui_interp->top_level_interpreter = interp;
 
-  /* We use interpreter_p for the "set interpreter" variable, so we need
-     to make sure we have a malloc'ed copy for the set command to free.  */
-  if (interpreter_p != NULL
-      && strcmp (interp->name (), interpreter_p) != 0)
-    {
-      xfree (interpreter_p);
-
-      interpreter_p = xstrdup (interp->name ());
-    }
+  if (interpreter_p != interp->name ())
+    interpreter_p = interp->name ();
 
   /* Run the init proc.  */
   if (!interp->inited)
diff --git a/gdb/main.c b/gdb/main.c
index ec2b7b01752..8c97987956b 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -56,10 +56,8 @@
 #include "observable.h"
 #include "serial.h"
 
-/* The selected interpreter.  This will be used as a set command
-   variable, so it should always be malloc'ed - since
-   do_setshow_command will free it.  */
-char *interpreter_p;
+/* The selected interpreter.  */
+std::string interpreter_p;
 
 /* System root path, used to find libraries etc.  */
 std::string gdb_sysroot;
@@ -729,7 +727,7 @@ captured_main_1 (struct captured_main_args *context)
      this captured main, or one specified by the user at start up, or
      the console.  Initialize the interpreter to the one requested by 
      the application.  */
-  interpreter_p = xstrdup (context->interpreter_p);
+  interpreter_p = context->interpreter_p;
 
   /* Parse arguments and options.  */
   {
@@ -866,8 +864,7 @@ captured_main_1 (struct captured_main_args *context)
 	  case OPT_TUI:
 	    /* --tui is equivalent to -i=tui.  */
 #ifdef TUI
-	    xfree (interpreter_p);
-	    interpreter_p = xstrdup (INTERP_TUI);
+	    interpreter_p = INTERP_TUI;
 #else
 	    error (_("%s: TUI mode is not supported"), gdb_program_name);
 #endif
@@ -877,14 +874,12 @@ captured_main_1 (struct captured_main_args *context)
 	       actually useful, and if it is, what it should do.  */
 #ifdef GDBTK
 	    /* --windows is equivalent to -i=insight.  */
-	    xfree (interpreter_p);
-	    interpreter_p = xstrdup (INTERP_INSIGHT);
+	    interpreter_p = INTERP_INSIGHT;
 #endif
 	    break;
 	  case OPT_NOWINDOWS:
 	    /* -nw is equivalent to -i=console.  */
-	    xfree (interpreter_p);
-	    interpreter_p = xstrdup (INTERP_CONSOLE);
+	    interpreter_p = INTERP_CONSOLE;
 	    break;
 	  case 'f':
 	    annotation_level = 1;
@@ -950,8 +945,7 @@ captured_main_1 (struct captured_main_args *context)
 	    }
 #endif /* GDBTK */
 	  case 'i':
-	    xfree (interpreter_p);
-	    interpreter_p = xstrdup (optarg);
+	    interpreter_p = optarg;
 	    break;
 	  case 'd':
 	    dirarg.push_back (optarg);
@@ -1133,7 +1127,7 @@ captured_main_1 (struct captured_main_args *context)
      GDB retain the old MI1 interpreter startup behavior.  Output the
      copyright message before the interpreter is installed.  That way
      it isn't encapsulated in MI output.  */
-  if (!quiet && strcmp (interpreter_p, INTERP_MI1) == 0)
+  if (!quiet && interpreter_p == INTERP_MI1)
     {
       /* Print all the junk at the top, with trailing "..." if we are
 	 about to read a symbol file (possibly slowly).  */
@@ -1147,7 +1141,7 @@ captured_main_1 (struct captured_main_args *context)
 
   /* Install the default UI.  All the interpreters should have had a
      look at things by now.  Initialize the default interpreter.  */
-  set_top_level_interpreter (interpreter_p);
+  set_top_level_interpreter (interpreter_p.c_str ());
 
   /* FIXME: cagney/2003-02-03: The big hack (part 2 of 2) that lets
      GDB retain the old MI1 interpreter startup behavior.  Output the
diff --git a/gdb/main.h b/gdb/main.h
index 60b3569438e..7cdd93f3420 100644
--- a/gdb/main.h
+++ b/gdb/main.h
@@ -36,7 +36,7 @@ extern int batch_silent;
 extern int batch_flag;
 
 /* * The name of the interpreter if specified on the command line.  */
-extern char *interpreter_p;
+extern std::string interpreter_p;
 
 /* From mingw-hdep.c, used by main.c.  */
 
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index e867441afb0..b4faede8ce6 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -343,14 +343,11 @@ _initialize_tui_interp ()
 {
   interp_factory_register (INTERP_TUI, tui_interp_factory);
 
-  if (interpreter_p && strcmp (interpreter_p, INTERP_TUI) == 0)
+  if (interpreter_p == INTERP_TUI)
     tui_start_enabled = true;
 
-  if (interpreter_p && strcmp (interpreter_p, INTERP_CONSOLE) == 0)
-    {
-      xfree (interpreter_p);
-      interpreter_p = xstrdup (INTERP_TUI);
-    }
+  if (interpreter_p == INTERP_CONSOLE)
+    interpreter_p = INTERP_TUI;
 
   /* If changing this, remember to update cli-interp.c as well.  */
   gdb::observers::normal_stop.attach (tui_on_normal_stop, "tui-interp");
-- 
2.34.1

Re: [PATCH 3/3] Use std::string for interpreter_p

[Andrew Burgess]

Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

> The global interpreter_p is a manually-managed 'char *'.  This patch
> changes it to be a std::string instead, and removes some erroneous
> comments.

I took a look through this series, and it all looks good to me.

I don't like the name interpreter_p - the _p says "predicate" to me, but
I that's not your fault, so can be fixed some other day.

Thanks,
Andrew


> ---
>  gdb/interps.c        | 11 ++---------
>  gdb/main.c           | 24 +++++++++---------------
>  gdb/main.h           |  2 +-
>  gdb/tui/tui-interp.c |  9 +++------
>  4 files changed, 15 insertions(+), 31 deletions(-)
>
> diff --git a/gdb/interps.c b/gdb/interps.c
> index 0c440e78685..3a9c590b8c8 100644
> --- a/gdb/interps.c
> +++ b/gdb/interps.c
> @@ -168,15 +168,8 @@ interp_set (struct interp *interp, bool top_level)
>    if (top_level)
>      ui_interp->top_level_interpreter = interp;
>  
> -  /* We use interpreter_p for the "set interpreter" variable, so we need
> -     to make sure we have a malloc'ed copy for the set command to free.  */
> -  if (interpreter_p != NULL
> -      && strcmp (interp->name (), interpreter_p) != 0)
> -    {
> -      xfree (interpreter_p);
> -
> -      interpreter_p = xstrdup (interp->name ());
> -    }
> +  if (interpreter_p != interp->name ())
> +    interpreter_p = interp->name ();
>  
>    /* Run the init proc.  */
>    if (!interp->inited)
> diff --git a/gdb/main.c b/gdb/main.c
> index ec2b7b01752..8c97987956b 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -56,10 +56,8 @@
>  #include "observable.h"
>  #include "serial.h"
>  
> -/* The selected interpreter.  This will be used as a set command
> -   variable, so it should always be malloc'ed - since
> -   do_setshow_command will free it.  */
> -char *interpreter_p;
> +/* The selected interpreter.  */
> +std::string interpreter_p;
>  
>  /* System root path, used to find libraries etc.  */
>  std::string gdb_sysroot;
> @@ -729,7 +727,7 @@ captured_main_1 (struct captured_main_args *context)
>       this captured main, or one specified by the user at start up, or
>       the console.  Initialize the interpreter to the one requested by 
>       the application.  */
> -  interpreter_p = xstrdup (context->interpreter_p);
> +  interpreter_p = context->interpreter_p;
>  
>    /* Parse arguments and options.  */
>    {
> @@ -866,8 +864,7 @@ captured_main_1 (struct captured_main_args *context)
>  	  case OPT_TUI:
>  	    /* --tui is equivalent to -i=tui.  */
>  #ifdef TUI
> -	    xfree (interpreter_p);
> -	    interpreter_p = xstrdup (INTERP_TUI);
> +	    interpreter_p = INTERP_TUI;
>  #else
>  	    error (_("%s: TUI mode is not supported"), gdb_program_name);
>  #endif
> @@ -877,14 +874,12 @@ captured_main_1 (struct captured_main_args *context)
>  	       actually useful, and if it is, what it should do.  */
>  #ifdef GDBTK
>  	    /* --windows is equivalent to -i=insight.  */
> -	    xfree (interpreter_p);
> -	    interpreter_p = xstrdup (INTERP_INSIGHT);
> +	    interpreter_p = INTERP_INSIGHT;
>  #endif
>  	    break;
>  	  case OPT_NOWINDOWS:
>  	    /* -nw is equivalent to -i=console.  */
> -	    xfree (interpreter_p);
> -	    interpreter_p = xstrdup (INTERP_CONSOLE);
> +	    interpreter_p = INTERP_CONSOLE;
>  	    break;
>  	  case 'f':
>  	    annotation_level = 1;
> @@ -950,8 +945,7 @@ captured_main_1 (struct captured_main_args *context)
>  	    }
>  #endif /* GDBTK */
>  	  case 'i':
> -	    xfree (interpreter_p);
> -	    interpreter_p = xstrdup (optarg);
> +	    interpreter_p = optarg;
>  	    break;
>  	  case 'd':
>  	    dirarg.push_back (optarg);
> @@ -1133,7 +1127,7 @@ captured_main_1 (struct captured_main_args *context)
>       GDB retain the old MI1 interpreter startup behavior.  Output the
>       copyright message before the interpreter is installed.  That way
>       it isn't encapsulated in MI output.  */
> -  if (!quiet && strcmp (interpreter_p, INTERP_MI1) == 0)
> +  if (!quiet && interpreter_p == INTERP_MI1)
>      {
>        /* Print all the junk at the top, with trailing "..." if we are
>  	 about to read a symbol file (possibly slowly).  */
> @@ -1147,7 +1141,7 @@ captured_main_1 (struct captured_main_args *context)
>  
>    /* Install the default UI.  All the interpreters should have had a
>       look at things by now.  Initialize the default interpreter.  */
> -  set_top_level_interpreter (interpreter_p);
> +  set_top_level_interpreter (interpreter_p.c_str ());
>  
>    /* FIXME: cagney/2003-02-03: The big hack (part 2 of 2) that lets
>       GDB retain the old MI1 interpreter startup behavior.  Output the
> diff --git a/gdb/main.h b/gdb/main.h
> index 60b3569438e..7cdd93f3420 100644
> --- a/gdb/main.h
> +++ b/gdb/main.h
> @@ -36,7 +36,7 @@ extern int batch_silent;
>  extern int batch_flag;
>  
>  /* * The name of the interpreter if specified on the command line.  */
> -extern char *interpreter_p;
> +extern std::string interpreter_p;
>  
>  /* From mingw-hdep.c, used by main.c.  */
>  
> diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
> index e867441afb0..b4faede8ce6 100644
> --- a/gdb/tui/tui-interp.c
> +++ b/gdb/tui/tui-interp.c
> @@ -343,14 +343,11 @@ _initialize_tui_interp ()
>  {
>    interp_factory_register (INTERP_TUI, tui_interp_factory);
>  
> -  if (interpreter_p && strcmp (interpreter_p, INTERP_TUI) == 0)
> +  if (interpreter_p == INTERP_TUI)
>      tui_start_enabled = true;
>  
> -  if (interpreter_p && strcmp (interpreter_p, INTERP_CONSOLE) == 0)
> -    {
> -      xfree (interpreter_p);
> -      interpreter_p = xstrdup (INTERP_TUI);
> -    }
> +  if (interpreter_p == INTERP_CONSOLE)
> +    interpreter_p = INTERP_TUI;
>  
>    /* If changing this, remember to update cli-interp.c as well.  */
>    gdb::observers::normal_stop.attach (tui_on_normal_stop, "tui-interp");
> -- 
> 2.34.1

Re: [PATCH 3/3] Use std::string for interpreter_p

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:
>> The global interpreter_p is a manually-managed 'char *'.  This patch
>> changes it to be a std::string instead, and removes some erroneous
>> comments.

Andrew> I took a look through this series, and it all looks good to me.

Thanks.

Andrew> I don't like the name interpreter_p - the _p says "predicate" to me, but
Andrew> I that's not your fault, so can be fixed some other day.

Yeah.  I think maybe it means "pointer"?  Not sure.

I looked at removing this entirely, and it's almost possible except for
the hacks in tui-interp.c:

  interp_factory_register (INTERP_TUI, tui_interp_factory);

  if (interpreter_p == INTERP_TUI)
    tui_start_enabled = true;

  if (interpreter_p == INTERP_CONSOLE)
    interpreter_p = INTERP_TUI;

I don't really understand why the TUI is considered a distinct
interpreter at all.  That seems weird to me.  For one thing, I found it
doesn't work properly with "new-ui" (I filed a bug about this).

Tom