From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 441 invoked by alias); 10 Sep 2011 16:52:49 -0000 Received: (qmail 430 invoked by uid 22791); 10 Sep 2011 16:52:46 -0000 X-SWARE-Spam-Status: No, hits=-1.6 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 10 Sep 2011 16:52:30 +0000 Received: from nat-dem.mentorg.com ([195.212.93.2] helo=eu2-mail.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1R2QmT-0006yu-Gj from pedro_alves@mentor.com ; Sat, 10 Sep 2011 09:51:49 -0700 Received: from scottsdale.localnet ([172.16.63.104]) by eu2-mail.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Sat, 10 Sep 2011 18:51:37 +0200 From: Pedro Alves To: Matt Rice Subject: Re: Eliminate tui_command_loop Date: Sat, 10 Sep 2011 18:47:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-11-generic; KDE/4.7.0; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <201108042110.45405.pedro@codesourcery.com> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201109101751.35845.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-09/txt/msg00177.txt.bz2 On Saturday 10 September 2011 13:46:22, Matt Rice wrote: > On Thu, Aug 4, 2011 at 1:10 PM, Pedro Alves 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 #include #include - +#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)); }