Eliminate tui_command_loop

Eliminate tui_command_loop

[Pedro Alves]

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:

 - change both start_event_loop and tui_command_loop
   to use TRY_CATCH instead of catch_errors.
 - removes that tui_command_loop code quoted above
 - since tui_command_loop is now just a copy of 
   cli_event_loop, we simply delete it

In practice it just does the equivalent:

 - changes start_event_loop to use TRY_CATCH.
 - removes tui_command_loop

The original patch that added the duplication 
justifies it exactly on the fact that some function
that later ended up as the current catch_errors had been
changed to preserve uiout:
 <http://sourceware.org/ml/gdb-patches/2002-08/msg00866.html>

I found the patch that explains the "catch_errors" change at
 <http://sourceware.org/ml/gdb-patches/2001-08/msg00144.html>.

which predates TRY_CATCH by a few years:
 <http://sourceware.org/ml/gdb/2005-01/msg00064.html>

It seems TRY_CATCH ended up save/restoring uiout
due to the nature of incremental improvements only,
not because it was thought it was required.  Good.

BTW, I tried removing just the

-      /* 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;

bit to see what breaks.  "bt" broke immediately.  The whole screen messes up.

-- 
Pedro Alves

2011-08-04  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* event-loop.c (gdb_do_one_event): Remove `data' parameter.
	(start_event_loop): Use TRY_CATCH instead of catch_errors.
	* event-loop.h (gdb_do_one_event): Remove `data' parameter.
	* top.c (gdb_readline_wrapper): Adjust.
	* tui/tui-interp.c (tui_command_loop):
	(_initialize_tui_interp): Don't install it.

---
 gdb/event-loop.c     |   39 ++++++++++++-------------
 gdb/event-loop.h     |    2 -
 gdb/top.c            |    3 -
 gdb/tui/tui-interp.c |   79 ---------------------------------------------------
 4 files changed, 22 insertions(+), 101 deletions(-)

Index: src/gdb/event-loop.c
===================================================================
--- src.orig/gdb/event-loop.c	2011-08-04 20:20:50.326650779 +0100
+++ src/gdb/event-loop.c	2011-08-04 20:30:29.386650881 +0100
@@ -410,11 +410,10 @@ process_event (void)
 /* Process one high level event.  If nothing is ready at this time,
    wait for something to happen (via gdb_wait_for_event), then process
    it.  Returns >0 if something was done otherwise returns <0 (this
-   can happen if there are no event sources to wait for).  If an error
-   occurs catch_errors() which calls this function returns zero.  */
+   can happen if there are no event sources to wait for).  */
 
 int
-gdb_do_one_event (void *data)
+gdb_do_one_event (void)
 {
   static int event_source_head = 0;
   const int number_of_sources = 3;
@@ -478,30 +477,30 @@ gdb_do_one_event (void *data)
 void
 start_event_loop (void)
 {
-  /* Loop until there is nothing to do.  This is the entry point to the
-     event loop engine.  gdb_do_one_event, called via catch_errors()
-     will process one event for each invocation.  It blocks waits for
-     an event and then processes it.  >0 when an event is processed, 0
-     when catch_errors() caught an error and <0 when there are no
-     longer any event sources registered.  */
+  /* Loop until there is nothing to do.  This is the entry point to
+     the event loop engine.  gdb_do_one_event will process one event
+     for each invocation.  It blocks waiting for an event and then
+     processes it.  */
   while (1)
     {
-      int gdb_result;
+      volatile struct gdb_exception ex;
+      int result = 0;
 
-      gdb_result = catch_errors (gdb_do_one_event, 0, "", RETURN_MASK_ALL);
-      if (gdb_result < 0)
-	break;
-
-      /* If we long-jumped out of do_one_event, we probably
-         didn't get around to resetting the prompt, which leaves
-         readline in a messed-up state.  Reset it here.  */
-
-      if (gdb_result == 0)
+      TRY_CATCH (ex, RETURN_MASK_ALL)
 	{
+	  result = gdb_do_one_event ();
+	}
+      if (ex.reason < 0)
+	{
+	  exception_print (gdb_stderr, ex);
+
 	  /* If any exception escaped to here, we better enable
 	     stdin.  Otherwise, any command that calls async_disable_stdin,
 	     and then throws, will leave stdin inoperable.  */
 	  async_enable_stdin ();
+	  /* If we long-jumped out of do_one_event, we probably didn't
+	     get around to resetting the prompt, which leaves readline
+	     in a messed-up state.  Reset it here.  */
 	  /* FIXME: this should really be a call to a hook that is
 	     interface specific, because interfaces can display the
 	     prompt in their own way.  */
@@ -517,6 +516,8 @@ start_event_loop (void)
 	  /* Maybe better to set a flag to be checked somewhere as to
 	     whether display the prompt or not.  */
 	}
+      if (result < 0)
+	break;
     }
 
   /* We are done with the event loop.  There are no more event sources
Index: src/gdb/event-loop.h
===================================================================
--- src.orig/gdb/event-loop.h	2011-08-04 20:20:50.326650779 +0100
+++ src/gdb/event-loop.h	2011-08-04 20:25:03.256650823 +0100
@@ -91,7 +91,7 @@ queue_position;
 /* Exported functions from event-loop.c */
 
 extern void start_event_loop (void);
-extern int gdb_do_one_event (void *data);
+extern int gdb_do_one_event (void);
 extern void delete_file_handler (int fd);
 extern void add_file_handler (int fd, handler_func *proc, 
 			      gdb_client_data client_data);
Index: src/gdb/top.c
===================================================================
--- src.orig/gdb/top.c	2011-08-04 20:20:50.326650779 +0100
+++ src/gdb/top.c	2011-08-04 20:25:03.256650823 +0100
@@ -797,8 +797,7 @@ gdb_readline_wrapper (char *prompt)
     (*after_char_processing_hook) ();
   gdb_assert (after_char_processing_hook == NULL);
 
-  /* gdb_do_one_event argument is unused.  */
-  while (gdb_do_one_event (NULL) >= 0)
+  while (gdb_do_one_event () >= 0)
     if (gdb_readline_wrapper_done)
       break;
 
Index: src/gdb/tui/tui-interp.c
===================================================================
--- src.orig/gdb/tui/tui-interp.c	2011-08-04 20:25:02.046650823 +0100
+++ src/gdb/tui/tui-interp.c	2011-08-04 20:25:03.256650823 +0100
@@ -132,84 +132,6 @@ tui_exec (void *data, const char *comman
   internal_error (__FILE__, __LINE__, _("tui_exec called"));
 }
 
-
-/* Initialize all the necessary variables, start the event loop,
-   register readline, and stdin, start the loop.  */
-
-static void
-tui_command_loop (void *data)
-{
-  /* If we are using readline, set things up and display the first
-     prompt, otherwise just print the prompt.  */
-  if (async_command_editing_p)
-    {
-      int length;
-      char *a_prompt;
-      char *gdb_prompt = get_prompt (0);
-
-      /* Tell readline what the prompt to display is and what function
-         it will need to call after a whole line is read. This also
-         displays the first prompt.  */
-      length = strlen (get_prefix (0))
-	+ strlen (gdb_prompt) + strlen (get_suffix (0)) + 1;
-      a_prompt = (char *) alloca (length);
-      strcpy (a_prompt, get_prefix (0));
-      strcat (a_prompt, gdb_prompt);
-      strcat (a_prompt, get_suffix (0));
-      rl_callback_handler_install (a_prompt, input_handler);
-    }
-  else
-    display_gdb_prompt (0);
-
-  /* Loop until there is nothing to do. This is the entry point to the
-     event loop engine. gdb_do_one_event, called via catch_errors()
-     will process one event for each invocation.  It blocks waits for
-     an event and then processes it.  >0 when an event is processed, 0
-     when catch_errors() caught an error and <0 when there are no
-     longer any event sources registered.  */
-  while (1)
-    {
-      int result = catch_errors (gdb_do_one_event, 0, "", RETURN_MASK_ALL);
-
-      if (result < 0)
-	break;
-
-      /* 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;
-
-      if (result == 0)
-	{
-	  /* If any exception escaped to here, we better enable
-	     stdin.  Otherwise, any command that calls async_disable_stdin,
-	     and then throws, will leave stdin inoperable.  */
-	  async_enable_stdin ();
-	  /* FIXME: this should really be a call to a hook that is
-	     interface specific, because interfaces can display the
-	     prompt in their own way.  */
-	  display_gdb_prompt (0);
-	  /* This call looks bizarre, but it is required.  If the user
-	     entered a command that caused an error,
-	     after_char_processing_hook won't be called from
-	     rl_callback_read_char_wrapper.  Using a cleanup there
-	     won't work, since we want this function to be called
-	     after a new prompt is printed.  */
-	  if (after_char_processing_hook)
-	    (*after_char_processing_hook) ();
-	  /* Maybe better to set a flag to be checked somewhere as to
-	     whether display the prompt or not.  */
-	}
-    }
-
-  /* We are done with the event loop. There are no more event sources
-     to listen to.  So we exit GDB.  */
-  return;
-}
-
 /* Provide a prototype to silence -Wmissing-prototypes.  */
 extern initialize_file_ftype _initialize_tui_interp;
 
@@ -222,7 +144,6 @@ _initialize_tui_interp (void)
     tui_suspend,
     tui_exec,
     tui_display_prompt_p,
-    tui_command_loop,
   };
 
   /* Create a default uiout builder for the TUI.  */

Re: Eliminate tui_command_loop

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

[...]
Pedro> It seems TRY_CATCH ended up save/restoring uiout
Pedro> due to the nature of incremental improvements only,
Pedro> not because it was thought it was required.  Good.

Nice research.

Pedro> -    tui_command_loop,

I think this was the last user of the command_loop_proc field.

Tom

Re: Eliminate tui_command_loop

[Pedro Alves]

On Thursday 04 August 2011 21:17:40, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

> Pedro> -    tui_command_loop,
> 
> I think this was the last user of the command_loop_proc field.

Indeed.  Though,

/* Run the current command interpreter's main loop.  */
void
current_interp_command_loop (void)
{
  /* Somewhat messy.  For the moment prop up all the old ways of
     selecting the command loop.  `deprecated_command_loop_hook'
     should be deprecated.  */
  if (deprecated_command_loop_hook != NULL)
    deprecated_command_loop_hook ();
  else if (current_interpreter != NULL
	   && current_interpreter->procs->command_loop_proc != NULL)
    current_interpreter->procs->command_loop_proc (current_interpreter->data);
  else
    cli_command_loop ();
}

deprecated_command_loop_hook is used by insight.
Sounds like we could get rid of it, and make insight 
install a command_loop_proc callback in its interpreter.
(gdb/gdbtk/gdbtk-interp.c).

-- 
Pedro Alves

Re: Eliminate tui_command_loop

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> deprecated_command_loop_hook is used by insight.
Pedro> Sounds like we could get rid of it, and make insight 
Pedro> install a command_loop_proc callback in its interpreter.
Pedro> (gdb/gdbtk/gdbtk-interp.c).

Yeah, I think that would be an improvement.
The fewer functions with "deprecated" in the name, the better.

Tom

Re: Eliminate tui_command_loop

[Thiago Jung Bauermann]

On 08/04/2011 05:10 PM, Pedro Alves wrote:
> which predates TRY_CATCH by a few years:
>   <http://sourceware.org/ml/gdb/2005-01/msg00064.html>

Since you brought the subject: I noticed a while ago that there are some
places which declare the exception variable used in TRY_CATCH without
the volatile keyword. At the time I changed all such occurrences to volatile
but there was no effect in the testsuite so I didn't bother submitting the
patch upstream. Is this important?
--
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: Eliminate tui_command_loop

[Sergio Durigan Junior]

Thiago Jung Bauermann <bauerman@br.ibm.com> writes:

> On 08/04/2011 05:10 PM, Pedro Alves wrote:
>> which predates TRY_CATCH by a few years:
>>   <http://sourceware.org/ml/gdb/2005-01/msg00064.html>
>
> Since you brought the subject: I noticed a while ago that there are some
> places which declare the exception variable used in TRY_CATCH without
> the volatile keyword. At the time I changed all such occurrences to volatile
> but there was no effect in the testsuite so I didn't bother submitting the
> patch upstream. Is this important?

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2016.html

IMHO (and if you still have this patch in hands) this is important, even
if just for keeping the standard :-).

Re: Eliminate tui_command_loop

[Pedro Alves]

On Thursday 04 August 2011 21:38:48, Thiago Jung Bauermann wrote:
> On 08/04/2011 05:10 PM, Pedro Alves wrote:
> > which predates TRY_CATCH by a few years:
> >   <http://sourceware.org/ml/gdb/2005-01/msg00064.html>
> 
> Since you brought the subject: I noticed a while ago that there are some
> places which declare the exception variable used in TRY_CATCH without
> the volatile keyword. At the time I changed all such occurrences to volatile
> but there was no effect in the testsuite so I didn't bother submitting the
> patch upstream. Is this important?

Yes, because GDB exceptions are implemented on top of setjmp/longjmp.

From N1256 (c99+tc1+tc2+tc3) <www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf>

"7.13.2.1 The longjmp function
All accessible objects have values, and all other components of the abstract machine
have state, as of the time the longjmp function was called, except that the values of
objects of automatic storage duration that are local to the function containing the
invocation of the corresponding setjmp macro that do not have volatile-qualified type
and have been changed between the setjmp invocation and longjmp call are
indeterminate.
"

setjmp/longjmp are implemented by saving/restoring
registers.  Without volatile, a compiler would be free to put the
local exception object in a register or registers instead of on
stack memory.  Since longjmp restores registers to the
state they had at setjmp time, we'd lose changes to the exception
object done between setjmp and longjmp.

-- 
Pedro Alves

Re: Eliminate tui_command_loop

[Matt Rice]

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.  */

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.

Re: Eliminate tui_command_loop

[Matt Rice]

On Sat, Sep 10, 2011 at 5:46 AM, Matt Rice <ratmice@gmail.com> wrote:

> Sorry to report that there is an issue here,

forgot the reproducer,

./gdb -quiet -ex 'interpreter-exec mi2 "-break-insert main"' -ex start
~/tests/test

e.g.

$ ./gdb/gdb -quiet -ex 'interpreter-exec mi2 "-break-insert main"' -ex
start ~/tests/test
Reading symbols from /home/ratmice/tests/test...done.
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000400598",func="main",file="/home/ratmice/tests/test.c",fullname="/home/ratmice/tests/test.c",line="36",times="0",original-location="main"}
(gdb)
Temporary breakpoint 2 at 0x400598: file /home/ratmice/tests/test.c, line 36.
Starting program: /home/ratmice/tests/test

Breakpoint 1, main (argc=1, argv=0x7fffffffe178) at
/home/ratmice/tests/test.c:36
Segmentation fault

Re: Eliminate tui_command_loop

[Pedro Alves]

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));
 }

Re: Eliminate tui_command_loop

[Matt Rice]

On Sat, Sep 10, 2011 at 9:51 AM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Saturday 10 September 2011 13:46:22, Matt Rice wrote:
>
>> 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.)
>

good idea I think,  since sending the previous email, I'm also a
little worried about the inverse scenario though,
that current_uiout can become out of date, when we call tui_enable/tui_disable

it'd seem adding something
  current_uiout = current_interpreter->procs->ui_out_proc (current_interpreter);
in start_event_loop would get us back to where
tui_command_loop was when it set the current_uiout.

or something like:
interp_changed_ui_out(struct interp *interp)
{
  if (interp == current_interpreter)
    current_uiout = current_interpreter->procs->ui_out_proc
(current_interpreter);
}

(and calling that from tui_enable/tui_disable after setting tui_active)

I believe that this sort of current_uiout problem is going to be less apparent,

e.g. if we're using the cli out and we should be using the tui one we
may just get wrong behavior instead of a segfault because
a) tui_out contains cli_out_impl stuff, so the behaviour may even
appear correct.
b) because tui_old_uiout probably won't segfault somewhere random in
ncurses like tui_out does when we use the wrong one.

I haven't figured out a way to cause issues this way, but haven't
tried terribly hard,
testsuite/gdb.base/tui-layout.exp though does appear to get a stale
current_uiout because I added
some a gdb_assert though i'm not sure how long/if it is long enough to
cause problems.

I could easily be missing something though and it's fine.

Re: Eliminate tui_command_loop

[Matt Rice]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

On Sat, Sep 10, 2011 at 11:47 AM, Matt Rice <ratmice@gmail.com> wrote:

>
> I could easily be missing something though and it's fine.
>

^ correct answer... it's handled in tui_setup_io().

the attached patch based on Pedro's WIP, fixes a small case that he missed.
and makes interp.exp hit all of these failures

this is PR gdb/13175
(though Nick's reproducer hits the case you missed).

I modified the existing prompt checks in this file, because the
gdb_expect was causing the test to take 30 seconds to complete, which
is now down to 0 seconds... hopefully that change is ok...

with this Pedro's patch runs through the testsuite no new failures...

feel free to merge in with your patch or have me commit the tests
seperately, whatever is easiest for you really...

2011-09-11  Matt Rice  <ratmice@gmail.com>

        * mi/mi-interp.c (mi_interpreter_init): Initialize default MI version
        to 2.
        (_initialize_mi_interp): Move comment to mi_interpreter_init.

2011-09-11  Matt Rice  <ratmice@gmail.com>

        PR gdb/13175
        * gdb.base/interp.exp: New tests.
        * gdb.base/interp.c: New file.

[-- Attachment #2: foo.diff --]
[-- Type: text/x-patch, Size: 5124 bytes --]

From cbaebffb6492e1fe5b033c5695731b0b59f698f5 Mon Sep 17 00:00:00 2001
From: matt rice <ratmice@gmail.com>
Date: Sun, 11 Sep 2011 04:40:51 -0700
Subject: [PATCH] Default INTER_MI to version 2, add tests.

---
 gdb/mi/mi-interp.c                |    7 ++--
 gdb/testsuite/gdb.base/interp.c   |   23 ++++++++++++++
 gdb/testsuite/gdb.base/interp.exp |   60 ++++++++++++++++++++++++++++++++-----
 3 files changed, 78 insertions(+), 12 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/interp.c

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index bd49ebc..f561cd6 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -93,9 +93,11 @@ mi_interpreter_init (struct interp *interp, int top_level)
   mi->event_channel = mi_console_file_new (raw_stdout, "=", 0);
 
   name = interp_name (interp);
+  /* INTERP_MI selects the most recent released version.  "mi2" was
+     released as part of GDB 6.0.  */
   if (strcmp (name, INTERP_MI1) == 0)
     mi_version = 1;
-  else if (strcmp (name, INTERP_MI2) == 0)
+  else if (strcmp (name, INTERP_MI2) == 0 || strcmp (name, INTERP_MI) == 0)
     mi_version = 2;
   else if (strcmp (name, INTERP_MI3) == 0)
     mi_version = 3;
@@ -742,8 +744,5 @@ _initialize_mi_interp (void)
   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, &procs));
 }
diff --git a/gdb/testsuite/gdb.base/interp.c b/gdb/testsuite/gdb.base/interp.c
new file mode 100644
index 0000000..2f52d3e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/interp.c
@@ -0,0 +1,23 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   Copyright 2011
+   Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+int main(int argc, const char **argv)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/interp.exp b/gdb/testsuite/gdb.base/interp.exp
index bb11f04..ca09cb3 100644
--- a/gdb/testsuite/gdb.base/interp.exp
+++ b/gdb/testsuite/gdb.base/interp.exp
@@ -20,16 +20,29 @@ if $tracelevel then {
     strace $tracelevel
 }
 
+set testfile "interp"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested "Couldn't compile $testfile.exp test program"
+    return -1
+}
+
+gdb_exit
 gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load $binfile
 
+# Force these to off for two prompt matching.
+gdb_test "maint time 0" "" ""
+gdb_test "maint space 0" "" ""
+
+set twoprompts "\r\n$gdb_prompt \r\n$gdb_prompt $"
 # Do not use gdb_test for this test, since it has two prompts.
 set cmd "interpreter-exec mi \"-var-update *\""
 gdb_test_multiple $cmd $cmd {
-    -re "\\^done,changelist=\\\[\\\]\r\n$gdb_prompt " {
+    -re "\\^done,changelist=\\\[\\\]$twoprompts" {
 	pass "$cmd"
-	gdb_expect 1 {
-	    -re "\r\n$gdb_prompt $" { }
-	}
     }
 }
 gdb_test "interpreter-exec console \"show version\"" "GNU gdb .*"
@@ -37,12 +50,43 @@ gdb_test "interpreter-exec console \"show version\"" "GNU gdb .*"
 # Regression test for crash when an exception occurs in mi_parse.
 gdb_test_multiple "interpreter-exec mi \"-break-insert --thread a\"" \
     "regression test for mi_parse crash" {
-	-re ".error,msg=.Invalid value for the '--thread' option.\r\n$gdb_prompt " {
+	-re ".error,msg=.Invalid value for the '--thread' option.$twoprompts" {
 	    pass "$cmd"
-	    gdb_expect 1 {
-		-re "\r\n$gdb_prompt $" { }
-	    }
 	}
     }
 
+set cmd "interpreter-exec mi \"-stack-info-frame\""
+gdb_test_multiple $cmd $cmd {
+    -re ".error,msg=.No registers\..$twoprompts" {
+        pass "$cmd"
+    }
+}
+
+set cmd "interpreter-exec mi1 \"-break-insert main\""
+gdb_test_multiple $cmd $cmd {
+    -re ".done.bkpt=.number=.\[0-9\].*$twoprompts" {
+        pass "$cmd"
+    }
+}
+
+set cmd "interpreter-exec mi2 \"-break-insert main\""
+gdb_test_multiple $cmd $cmd {
+    -re ".done.bkpt=.number=.\[0-9\].*$twoprompts" {
+        pass "$cmd"
+    }
+}
+
+set cmd "interpreter-exec mi3 \"-break-insert main\""
+gdb_test_multiple $cmd $cmd {
+    -re ".done.bkpt=.number=.\[0-9\].*$twoprompts" {
+        pass "$cmd"
+    }
+}
+
+if ![runto_main] then {
+  fail "${testfile}.exp can run to main."
+  return -1;
+}
+
+gdb_test "list" ".*\[0-9\].*int main.*" "${testfile}.exp can list sources"
 gdb_exit
-- 
1.7.4.4

PR gdb/13175 (Re: Eliminate tui_command_loop)

[Pedro Alves]

On Sunday 11 September 2011 14:24:22, Matt Rice wrote:
> On Sat, Sep 10, 2011 at 11:47 AM, Matt Rice <ratmice@gmail.com> wrote:
> 
> >
> > I could easily be missing something though and it's fine.
> >
> 
> ^ correct answer... it's handled in tui_setup_io().

Yep.

> the attached patch based on Pedro's WIP, fixes a small case that he missed.
> and makes interp.exp hit all of these failures

Thanks!

> this is PR gdb/13175
> (though Nick's reproducer hits the case you missed).

Okay.

> I modified the existing prompt checks in this file, because the
> gdb_expect was causing the test to take 30 seconds to complete, which
> is now down to 0 seconds... hopefully that change is ok...

Sorry, it isn't.  You got the long delay because your test has a
racy `.*'.  If you had written it like the other tests, like:

set cmd "interpreter-exec mi1 \"-break-insert main\""
gdb_test_multiple $cmd $cmd {
    -re ".done.bkpt=.number=.\[0-9\].*\r\n$gdb_prompt " {
                                    ^^
        pass "$cmd"
        gdb_expect 1 {
            -re "\r\n$gdb_prompt $" { }
        }

    }
}

then that .* could (and would often) eat one prompt, while
the $gdb_prompt afterward would consume the other.  By the
time you got to the "gdb_expect 1", there was no input left
in the buffer, so you'd hang there, until that timed out.
Since there's no fail in that gdb_expect, the problem was
sort of silent.

Your change isn't correct, because gdb_test_multiple
has:

	 -re "\r\n$gdb_prompt $" {
	    if ![string match "" $message] then {
		fail "$message"
	    }
	    set result 1
	}

And that could match the first prompt if just the right amount of
characters end up in the buffer ($ anchors on the end of current
contents on the expect buffer), before the second prompt is output.
If you run your test with the race reproducer from PR12649, you'll see
the test failing consistently due to this issue (it forces expect to
add one new character at a time to the buffer).

The easiest fix is to keep using the old mechanism, getting rid of
all .*'s in the expected regex.  See new version of the patch below.

> with this Pedro's patch runs through the testsuite no new failures...

I see the same.

> feel free to merge in with your patch or have me commit the tests
> seperately, whatever is easiest for you really...

It was easier to merge it in.  I did a couple more cosmetic tweaks
in the code patch, and made the test use prepare_for_testing, along
with removing a few redundant ${testfile}.exp's in gdb.sum output.

Tested on x86_64-linux and applied.

Thanks again for identifying the problem and for the tests!

-- 
Pedro Alves

gdb/
2011-09-12  Pedro Alves  <pedro@codesourcery.com>
	    Matt Rice  <ratmice@gmail.com>

	PR gdb/13175

	* interps.c (struct interp) <interpreter_out>: Delete field.
	(interp_new): Remove the data and uiout parameters and adjust.
	(interp_set): Only set the current_uiout from the interpreter's
	uiout after initializing the interpreter.  Adjust call to
	init_proc.
	(interp_ui_out): Adjust to call procs->ui_out_proc.
	(interp_data, interp_name): New.
	* interps.h (interp_init_ftype): Add `self' parameter.
	(interp_ui_out_ftype): New typedef.
	(struct interp_procs) <ui_out_proc>: New method pointer.
	(interp_new): Remove the data and uiout parameters.
	(interp_data, interp_name): Declare.
	* tui/tui-interp.c (tui_init): Adjust prototype.
	(tui_ui_out): New.
	(_initialize_tui_interp): Install tui_ui_out.  Don't instanciate
	tui_out here.  Adjust call to interp_new.
	* tui/tui-io.c (tui_initialize_io): Don't set current_uiout here.
	* cli/cli-interp.c (cli_interpreter_init): Adjust prototype.
	(cli_ui_out): New.
	(_initialize_cli_interp): Install it.  Adjust call to interp_new.
	* mi/mi-common.h (struct mi_interp) <uiout>: New field.
	* mi/mi-interp.c (mi_interpreter_init): Adjust prototype.
	Initialize mi->uiout depending on the mi_version as extracted from
	the interpreter's name.
	(mi_ui_out): New.
	(_initialize_mi_interp): Install mi_ui_out.  Adjust calls to
	interp_new.  Don't allocate the ui_out's of the interpreters here.

gdb/testsuite/
2011-09-12  Matt Rice  <ratmice@gmail.com>
	    Pedro Alves  <pedro@codesourcery.com>

	PR gdb/13175

	* gdb.base/interp.exp: New tests.
	* gdb.base/interp.c: New file.

---
 gdb/cli/cli-interp.c              |   12 ++++++--
 gdb/interps.c                     |   38 +++++++++++++++++----------
 gdb/interps.h                     |   16 ++++++++---
 gdb/mi/mi-common.h                |    3 ++
 gdb/mi/mi-interp.c                |   42 ++++++++++++++++++++++++------
 gdb/testsuite/gdb.base/interp.c   |   24 +++++++++++++++++
 gdb/testsuite/gdb.base/interp.exp |   52 +++++++++++++++++++++++++++++++++++++-
 gdb/tui/tui-interp.c              |   17 ++++++++++--
 gdb/tui/tui-io.c                  |    2 -
 9 files changed, 171 insertions(+), 35 deletions(-)

Index: src/gdb/interps.c
===================================================================
--- src.orig/gdb/interps.c	2011-09-12 13:28:27.000000000 +0100
+++ src/gdb/interps.c	2011-09-12 20:29:07.216237941 +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,25 @@ 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);
+}
+
+/* Returns the interpreter's cookie.  */
 
-  return current_interpreter->interpreter_out;
+void *
+interp_data (struct interp *interp)
+{
+  return interp->data;
+}
+
+/* Returns the interpreter's name.  */
+
+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-12 13:28:26.000000000 +0100
+++ src/gdb/interps.h	2011-09-12 13:28:28.416229191 +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-12 13:28:26.000000000 +0100
+++ src/gdb/tui/tui-interp.c	2011-09-12 13:28:28.416229191 +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-12 13:28:26.000000000 +0100
+++ src/gdb/tui/tui-io.c	2011-09-12 20:32:42.866238015 +0100
@@ -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-12 13:28:27.000000000 +0100
+++ src/gdb/cli/cli-interp.c	2011-09-12 13:28:28.416229191 +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-12 13:28:26.000000000 +0100
+++ src/gdb/mi/mi-common.h	2011-09-12 20:32:06.506238003 +0100
@@ -52,6 +52,9 @@ struct mi_interp
   struct ui_file *targ;
   struct ui_file *event_channel;
 
+  /* MI's 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-12 13:28:27.000000000 +0100
+++ src/gdb/mi/mi-interp.c	2011-09-12 13:40:23.000000000 +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,22 @@ 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);
+  /* INTERP_MI selects the most recent released version.  "mi2" was
+     released as part of GDB 6.0.  */
+  if (strcmp (name, INTERP_MI) == 0)
+    mi_version = 2;
+  else 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 +719,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 +738,13 @@ _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));
-
-  /* "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_MI1, &procs));
+  interp_add (interp_new (INTERP_MI2, &procs));
+  interp_add (interp_new (INTERP_MI3, &procs));
+  interp_add (interp_new (INTERP_MI, &procs));
 }
Index: src/gdb/testsuite/gdb.base/interp.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/interp.c	2011-09-12 20:34:18.000000000 +0100
@@ -0,0 +1,24 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   Copyright 2011
+   Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+int
+main (int argc, const char **argv)
+{
+  return 0;
+}
Index: src/gdb/testsuite/gdb.base/interp.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.base/interp.exp	2011-09-12 20:34:00.000000000 +0100
+++ src/gdb/testsuite/gdb.base/interp.exp	2011-09-12 20:34:18.000000000 +0100
@@ -20,7 +20,11 @@ if $tracelevel then {
     strace $tracelevel
 }
 
-gdb_start
+set testfile "interp"
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${testfile}.c {debug}] } {
+    return -1
+}
 
 # Do not use gdb_test for this test, since it has two prompts.
 set cmd "interpreter-exec mi \"-var-update *\""
@@ -45,4 +49,50 @@ gdb_test_multiple "interpreter-exec mi \
 	}
     }
 
+set cmd "interpreter-exec mi \"-stack-info-frame\""
+gdb_test_multiple $cmd $cmd {
+    -re ".error,msg=.No registers\..\r\n$gdb_prompt " {
+	pass "$cmd"
+	gdb_expect 1 {
+	    -re "\r\n$gdb_prompt $" { }
+	}
+    }
+}
+
+set cmd "interpreter-exec mi1 \"-break-insert main\""
+gdb_test_multiple $cmd $cmd {
+    -re ".done.bkpt=.number=.\[0-9\]\[^\n\]+\r\n$gdb_prompt " {
+	pass "$cmd"
+	gdb_expect 1 {
+	    -re "\r\n$gdb_prompt $" { }
+	}
+    }
+}
+
+set cmd "interpreter-exec mi2 \"-break-insert main\""
+gdb_test_multiple $cmd $cmd {
+    -re ".done.bkpt=.number=.\[0-9\]\[^\n\]+\r\n$gdb_prompt " {
+	pass "$cmd"
+	gdb_expect 1 {
+	    -re "\r\n$gdb_prompt $" { }
+	}
+    }
+}
+
+set cmd "interpreter-exec mi3 \"-break-insert main\""
+gdb_test_multiple $cmd $cmd {
+    -re ".done.bkpt=.number=.\[0-9\]\[^\n\]+\r\n$gdb_prompt " {
+	pass "$cmd"
+	gdb_expect 1 {
+	    -re "\r\n$gdb_prompt $" { }
+	}
+    }
+}
+
+if ![runto_main] then {
+  fail "run to main"
+  return -1;
+}
+
+gdb_test "list" ".*\[0-9\].*main \\(int argc.*" "can list sources"
 gdb_exit