This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Eliminate tui_command_loop
- From: Pedro Alves <pedro at codesourcery dot com>
- To: Matt Rice <ratmice at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Sat, 10 Sep 2011 17:51:35 +0100
- Subject: Re: Eliminate tui_command_loop
- References: <201108042110.45405.pedro@codesourcery.com> <CACTLOFoWEp9RAjA_mv=iXWAtqw_cert7iQ3Dp=ubQ5_+N434Ow@mail.gmail.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));
}