Re: Eliminate tui_command_loop

[Pedro Alves <pedro at codesourcery dot com>]

On Saturday 10 September 2011 13:46:22, Matt Rice wrote:
> On Thu, Aug 4, 2011 at 1:10 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> > On Wednesday 03 August 2011 19:10:41, Pedro Alves wrote:
> >> Looking at the differences between tui/tui-interp.c:tui_command_loop
> >> and event-top.c:cli_event_loop,
> >> thinking of getting rid of tui_command_loop, the only difference is
> >> that tui_command_loop inlines event-loop.c:start_event_loop, and
> >> adds this bit:
> >>
> >> +      /* Update gdb output according to TUI mode.  Since catch_errors
> >> +         preserves the uiout from changing, this must be done at top
> >> +         level of event loop.  */
> >> +      if (tui_active)
> >> +        current_uiout = tui_out;
> >> +      else
> >> +        current_uiout = tui_old_uiout;
> >>
> >> I thought, "why don't we just use TRY_CATCH instead of
> >> catch_errors then?".  Turns out we can't as is, because it is
> >> TRY_CATCH itself that preserves the uiout from changing...
> >
> > TRY_CATCH no longer does that.  I've applied the
> > patch below that conceptually does:
> 
> Sorry to report that there is an issue here,
> Unforunately the story with this current_uiout frobbing doesn't really
> end at catch_errors/TRY_CATCH.
> 
> because interps.c:interp_set goes:
>   current_uiout = interp->interpreter_out;
> and tui/tui-io.c:tui_initialize_io goes:
>   tui_old_uiout = current_uiout = cli_out_new (gdb_stdout);
> 
> so after tui_initialize_io,
> we have a situation where interp->interpreter_out != current_uiout
> 
> then, in 'interpreter_exec_cmd'
> old_interp = current_interpreter;
> ....
> interp_set (old_interp, 0);
> and now current_uiout has now reverted interpreter->interpreter_out.
> this then leads to some issue in the future when we try to output something.
> 
> saving and restoring current_uiout in interpreter_exec_cmd
> seems to work, but I fear its just a piece of bubble gum holding
> things together (and further breaks the already broken abstraction),
> 
> this comment from captured_mi_execute_command leads me to think that
> if the command does actually want to modify the uiout restoring it to
> whatever was the current_uiout could be incorrect..
> 
>       /* Print the result if there were no errors.
> 
>          Remember that on the way out of executing a command, you have
>          to directly use the mi_interp's uiout, since the command could
>          have reset the interpreter, in which case the current uiout
>          will most likely crash in the mi_out_* routines.  */
> 

Thanks for the analysis!

> I suppose the right thing to do is add interp_set_uiout(interp, uiout)
> so tui can control whatever its current_interpreter->interpreter_out field is.

Yeah.  Though I think making current_interpreter->interpreter_out a callback
that returns the correct uiout instead is a bit better.  One place less
to store the uiout to use that way.  (I wonder if we can make the tui return
cli_out and friends when the TUI is off.)

Here's a WIP patch (don't have right time now to finish it).

Fixes your reproducer for me.

-- 
Pedro Alves
---
 gdb/cli/cli-interp.c |   12 +++++++++---
 gdb/interps.c        |   34 ++++++++++++++++++++--------------
 gdb/interps.h        |   16 ++++++++++++----
 gdb/mi/mi-common.h   |    3 +++
 gdb/mi/mi-interp.c   |   35 +++++++++++++++++++++++++++++------
 gdb/tui/tui-interp.c |   17 ++++++++++++++---
 gdb/tui/tui-io.c     |    4 ++--
 7 files changed, 89 insertions(+), 32 deletions(-)

Index: src/gdb/interps.c
===================================================================
--- src.orig/gdb/interps.c	2011-09-10 17:48:38.030304225 +0100
+++ src/gdb/interps.c	2011-09-10 17:50:16.410304243 +0100
@@ -67,11 +67,6 @@ struct interp
   /* Has the init_proc been run?  */
   int inited;
 
-  /* This is the ui_out used to collect results for this interpreter.
-     It can be a formatter for stdout, as is the case for the console
-     & mi outputs, or it might be a result formatter.  */
-  struct ui_out *interpreter_out;
-
   const struct interp_procs *procs;
   int quiet_p;
 };
@@ -97,16 +92,14 @@ static int interpreter_initialized = 0;
    fills the fields from the inputs, and returns a pointer to the
    interpreter.  */
 struct interp *
-interp_new (const char *name, void *data, struct ui_out *uiout,
-	    const struct interp_procs *procs)
+interp_new (const char *name, const struct interp_procs *procs)
 {
   struct interp *new_interp;
 
   new_interp = XMALLOC (struct interp);
 
   new_interp->name = xstrdup (name);
-  new_interp->data = data;
-  new_interp->interpreter_out = uiout;
+  new_interp->data = NULL;
   new_interp->quiet_p = 0;
   new_interp->procs = procs;
   new_interp->inited = 0;
@@ -184,19 +177,20 @@ interp_set (struct interp *interp, int t
       interpreter_p = xstrdup (current_interpreter->name);
     }
 
-  current_uiout = interp->interpreter_out;
-
   /* Run the init proc.  If it fails, try to restore the old interp.  */
 
   if (!interp->inited)
     {
       if (interp->procs->init_proc != NULL)
 	{
-	  interp->data = interp->procs->init_proc (top_level);
+	  interp->data = interp->procs->init_proc (interp, top_level);
 	}
       interp->inited = 1;
     }
 
+  /* Do this only after the interpreter is initialized.  */
+  current_uiout = interp->procs->ui_out_proc (interp);
+
   /* Clear out any installed interpreter hooks/event handlers.  */
   clear_interpreter_hooks ();
 
@@ -254,9 +248,21 @@ struct ui_out *
 interp_ui_out (struct interp *interp)
 {
   if (interp != NULL)
-    return interp->interpreter_out;
+    return interp->procs->ui_out_proc (interp);
+
+  return current_interpreter->procs->ui_out_proc (current_interpreter);
+}
 
-  return current_interpreter->interpreter_out;
+void *
+interp_data (struct interp *interp)
+{
+  return interp->data;
+}
+
+const char *
+interp_name (struct interp *interp)
+{
+  return interp->name;
 }
 
 /* Returns true if the current interp is the passed in name.  */
Index: src/gdb/interps.h
===================================================================
--- src.orig/gdb/interps.h	2011-09-10 17:48:38.030304225 +0100
+++ src/gdb/interps.h	2011-09-10 17:48:47.410304227 +0100
@@ -36,13 +36,14 @@ extern struct gdb_exception interp_exec
 					 const char *command);
 extern int interp_quiet_p (struct interp *interp);
 
-typedef void *(interp_init_ftype) (int top_level);
+typedef void *(interp_init_ftype) (struct interp *self, int top_level);
 typedef int (interp_resume_ftype) (void *data);
 typedef int (interp_suspend_ftype) (void *data);
 typedef int (interp_prompt_p_ftype) (void *data);
 typedef struct gdb_exception (interp_exec_ftype) (void *data,
 						  const char *command);
 typedef void (interp_command_loop_ftype) (void *data);
+typedef struct ui_out *(interp_ui_out_ftype) (struct interp *self);
 
 struct interp_procs
 {
@@ -51,16 +52,23 @@ struct interp_procs
   interp_suspend_ftype *suspend_proc;
   interp_exec_ftype *exec_proc;
   interp_prompt_p_ftype *prompt_proc_p;
+
+  /* Returns the ui_out currently used to collect results for this
+     interpreter.  It can be a formatter for stdout, as is the case
+     for the console & mi outputs, or it might be a result
+     formatter.  */
+  interp_ui_out_ftype *ui_out_proc;
+
   interp_command_loop_ftype *command_loop_proc;
 };
 
-extern struct interp *interp_new (const char *name, void *data,
-				  struct ui_out *uiout,
-				  const struct interp_procs *procs);
+extern struct interp *interp_new (const char *name, const struct interp_procs *procs);
 extern void interp_add (struct interp *interp);
 extern int interp_set (struct interp *interp, int top_level);
 extern struct interp *interp_lookup (const char *name);
 extern struct ui_out *interp_ui_out (struct interp *interp);
+extern void *interp_data (struct interp *interp);
+extern const char *interp_name (struct interp *interp);
 
 extern int current_interp_named_p (const char *name);
 extern int current_interp_display_prompt_p (void);
Index: src/gdb/tui/tui-interp.c
===================================================================
--- src.orig/gdb/tui/tui-interp.c	2011-09-10 17:48:38.030304225 +0100
+++ src/gdb/tui/tui-interp.c	2011-09-10 17:48:47.410304227 +0100
@@ -52,7 +52,7 @@ static int tui_is_toplevel = 0;
 /* These implement the TUI interpreter.  */
 
 static void *
-tui_init (int top_level)
+tui_init (struct interp *self, int top_level)
 {
   tui_is_toplevel = top_level;
 
@@ -126,6 +126,15 @@ tui_display_prompt_p (void *data)
     return 1;
 }
 
+static struct ui_out *
+tui_ui_out (struct interp *self)
+{
+  if (tui_active)
+    return tui_out;
+  else
+    return tui_old_uiout;
+}
+
 static struct gdb_exception
 tui_exec (void *data, const char *command_str)
 {
@@ -144,11 +153,13 @@ _initialize_tui_interp (void)
     tui_suspend,
     tui_exec,
     tui_display_prompt_p,
+    tui_ui_out,
   };
+  struct interp *tui_interp;
 
   /* Create a default uiout builder for the TUI.  */
-  tui_out = tui_out_new (gdb_stdout);
-  interp_add (interp_new (INTERP_TUI, NULL, tui_out, &procs));
+  tui_interp = interp_new (INTERP_TUI, &procs);
+  interp_add (tui_interp);
   if (interpreter_p && strcmp (interpreter_p, INTERP_TUI) == 0)
     tui_start_enabled = 1;
 
Index: src/gdb/tui/tui-io.c
===================================================================
--- src.orig/gdb/tui/tui-io.c	2011-09-10 17:48:38.030304225 +0100
+++ src/gdb/tui/tui-io.c	2011-09-10 17:48:47.410304227 +0100
@@ -38,7 +38,7 @@
 #include <fcntl.h>
 #include <signal.h>
 #include <stdio.h>
-
+#include "interps.h"
 #include "gdb_curses.h"
 
 /* This redefines CTRL if it is not already defined, so it must come
@@ -607,7 +607,7 @@ tui_initialize_io (void)
 
   /* Create the default UI.  It is not created because we installed a
      deprecated_init_ui_hook.  */
-  tui_old_uiout = current_uiout = cli_out_new (gdb_stdout);
+  tui_old_uiout = cli_out_new (gdb_stdout);
 
 #ifdef TUI_USE_PIPE_FOR_READLINE
   /* Temporary solution for readline writing to stdout: redirect
Index: src/gdb/cli/cli-interp.c
===================================================================
--- src.orig/gdb/cli/cli-interp.c	2011-09-10 17:48:37.970304225 +0100
+++ src/gdb/cli/cli-interp.c	2011-09-10 17:48:47.410304227 +0100
@@ -40,7 +40,7 @@ static struct gdb_exception safe_execute
 /* These implement the cli out interpreter: */
 
 static void *
-cli_interpreter_init (int top_level)
+cli_interpreter_init (struct interp *self, int top_level)
 {
   return NULL;
 }
@@ -135,6 +135,11 @@ safe_execute_command (struct ui_out *com
   return e;
 }
 
+static struct ui_out *
+cli_ui_out (struct interp *self)
+{
+  return cli_uiout;
+}
 
 /* Standard gdb initialization hook.  */
 extern initialize_file_ftype _initialize_cli_interp; /* -Wmissing-prototypes */
@@ -147,13 +152,14 @@ _initialize_cli_interp (void)
     cli_interpreter_resume,	/* resume_proc */
     cli_interpreter_suspend,	/* suspend_proc */
     cli_interpreter_exec,	/* exec_proc */
-    cli_interpreter_display_prompt_p	/* prompt_proc_p */
+    cli_interpreter_display_prompt_p,	/* prompt_proc_p */
+    cli_ui_out			/* ui_out_proc */
   };
   struct interp *cli_interp;
 
   /* Create a default uiout builder for the CLI.  */
   cli_uiout = cli_out_new (gdb_stdout);
-  cli_interp = interp_new (INTERP_CONSOLE, NULL, cli_uiout, &procs);
+  cli_interp = interp_new (INTERP_CONSOLE, &procs);
 
   interp_add (cli_interp);
 }
Index: src/gdb/mi/mi-common.h
===================================================================
--- src.orig/gdb/mi/mi-common.h	2011-09-10 17:48:37.970304225 +0100
+++ src/gdb/mi/mi-common.h	2011-09-10 17:48:47.410304227 +0100
@@ -52,6 +52,9 @@ struct mi_interp
   struct ui_file *targ;
   struct ui_file *event_channel;
 
+  /* The builder.  */
+  struct ui_out *uiout;
+
   /* This is the interpreter for the mi... */
   struct interp *mi2_interp;
   struct interp *mi1_interp;
Index: src/gdb/mi/mi-interp.c
===================================================================
--- src.orig/gdb/mi/mi-interp.c	2011-09-10 17:48:37.970304225 +0100
+++ src/gdb/mi/mi-interp.c	2011-09-10 17:48:47.410304227 +0100
@@ -72,9 +72,11 @@ static void mi_breakpoint_modified (stru
 static int report_initial_inferior (struct inferior *inf, void *closure);
 
 static void *
-mi_interpreter_init (int top_level)
+mi_interpreter_init (struct interp *interp, int top_level)
 {
   struct mi_interp *mi = XMALLOC (struct mi_interp);
+  const char *name;
+  int mi_version;
 
   /* HACK: We need to force stdout/stderr to point at the console.  This avoids
      any potential side effects caused by legacy code that is still
@@ -90,6 +92,18 @@ mi_interpreter_init (int top_level)
   mi->targ = mi_console_file_new (raw_stdout, "@", '"');
   mi->event_channel = mi_console_file_new (raw_stdout, "=", 0);
 
+  name = interp_name (interp);
+  if (strcmp (name, INTERP_MI1) == 0)
+    mi_version = 1;
+  else if (strcmp (name, INTERP_MI2) == 0)
+    mi_version = 2;
+  else if (strcmp (name, INTERP_MI3) == 0)
+    mi_version = 3;
+  else
+    gdb_assert_not_reached ("unhandled MI version");
+
+  mi->uiout = mi_out_new (mi_version);
+
   if (top_level)
     {
       observer_attach_new_thread (mi_new_thread);
@@ -701,6 +715,14 @@ report_initial_inferior (struct inferior
   return 0;
 }
 
+static struct ui_out *
+mi_ui_out (struct interp *interp)
+{
+  struct mi_interp *mi = interp_data (interp);
+
+  return mi->uiout;
+}
+
 extern initialize_file_ftype _initialize_mi_interp; /* -Wmissing-prototypes */
 
 void
@@ -712,15 +734,16 @@ _initialize_mi_interp (void)
     mi_interpreter_resume,	/* resume_proc */
     mi_interpreter_suspend,	/* suspend_proc */
     mi_interpreter_exec,	/* exec_proc */
-    mi_interpreter_prompt_p	/* prompt_proc_p */
+    mi_interpreter_prompt_p,	/* prompt_proc_p */
+    mi_ui_out 			/* ui_out_proc */
   };
 
   /* The various interpreter levels.  */
-  interp_add (interp_new (INTERP_MI1, NULL, mi_out_new (1), &procs));
-  interp_add (interp_new (INTERP_MI2, NULL, mi_out_new (2), &procs));
-  interp_add (interp_new (INTERP_MI3, NULL, mi_out_new (3), &procs));
+  interp_add (interp_new (INTERP_MI1, &procs));
+  interp_add (interp_new (INTERP_MI2, &procs));
+  interp_add (interp_new (INTERP_MI3, &procs));
 
   /* "mi" selects the most recent released version.  "mi2" was
      released as part of GDB 6.0.  */
-  interp_add (interp_new (INTERP_MI, NULL, mi_out_new (2), &procs));
+  interp_add (interp_new (INTERP_MI, &procs));
 }