From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20408 invoked by alias); 12 Sep 2011 21:27:32 -0000 Received: (qmail 20392 invoked by uid 22791); 12 Sep 2011 21:27:29 -0000 X-SWARE-Spam-Status: No, hits=-1.3 required=5.0 tests=AWL,BAYES_00,FROM_12LTRDOM 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; Mon, 12 Sep 2011 21:27:13 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1R3E24-0002H6-Lc from pedro_alves@mentor.com ; Mon, 12 Sep 2011 14:27:13 -0700 Received: from scottsdale.localnet ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Mon, 12 Sep 2011 22:27:09 +0100 From: Pedro Alves To: Matt Rice Subject: PR gdb/13175 (Re: Eliminate tui_command_loop) Date: Tue, 13 Sep 2011 08:29: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-15" Content-Transfer-Encoding: 7bit Message-Id: <201109122227.08033.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/msg00206.txt.bz2 On Sunday 11 September 2011 14:24:22, Matt Rice wrote: > On Sat, Sep 10, 2011 at 11:47 AM, Matt Rice 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 Matt Rice PR gdb/13175 * interps.c (struct interp) : 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) : 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) : 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 Pedro Alves 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 . +*/ + +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