* [PATCH 0/2] Fix a couple MI/new-ui bugs (for master and 7.12)
@ 2016-08-02 23:21 Pedro Alves
2016-08-02 23:21 ` [PATCH 1/2] Fix PR mi/20431 - Missing MI prompts after sync execution MI command (-exec-continue, etc.) errors Pedro Alves
2016-08-02 23:21 ` [PATCH 2/2] Fix PR gdb/20418 - Problems with synchronous commands and new-ui Pedro Alves
0 siblings, 2 replies; 5+ messages in thread
From: Pedro Alves @ 2016-08-02 23:21 UTC (permalink / raw)
To: gdb-patches
This series fixes a bug exposed by the new console integration in
Eclipse and a bug discovered in discussions of the former. I'd like
to put these in 7.12.
Pedro Alves (2):
Fix PR mi/20431 - Missing MI prompts after sync execution MI command
(-exec-continue, etc.) errors
Fix PR gdb/20418 - Problems with synchronous commands and new-ui
gdb/event-top.c | 20 +++++-
gdb/infrun.c | 1 +
gdb/mi/mi-main.c | 7 ++
gdb/target.c | 11 ---
gdb/target.h | 14 ++--
gdb/testsuite/gdb.mi/mi-cmd-error.exp | 80 ++++++++++++++++++++++
gdb/testsuite/gdb.mi/new-ui-mi-sync.c | 25 +++++++
gdb/testsuite/gdb.mi/new-ui-mi-sync.exp | 114 ++++++++++++++++++++++++++++++++
gdb/testsuite/lib/mi-support.exp | 4 +-
gdb/top.h | 6 ++
gdb/utils.c | 43 +++++++++---
11 files changed, 295 insertions(+), 30 deletions(-)
create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-error.exp
create mode 100644 gdb/testsuite/gdb.mi/new-ui-mi-sync.c
create mode 100644 gdb/testsuite/gdb.mi/new-ui-mi-sync.exp
--
2.5.5
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/2] Fix PR mi/20431 - Missing MI prompts after sync execution MI command (-exec-continue, etc.) errors 2016-08-02 23:21 [PATCH 0/2] Fix a couple MI/new-ui bugs (for master and 7.12) Pedro Alves @ 2016-08-02 23:21 ` Pedro Alves 2016-08-02 23:21 ` [PATCH 2/2] Fix PR gdb/20418 - Problems with synchronous commands and new-ui Pedro Alves 1 sibling, 0 replies; 5+ messages in thread From: Pedro Alves @ 2016-08-02 23:21 UTC (permalink / raw) To: gdb-patches gdb 7.11 introduced an MI regression: a failing MI sync execution command misses printing the MI prompt, and then all subsequent command miss it too: $ gdb-7.11.1 -i=mi [...] p 1 &"p 1\n" ~"$1 = 1" ~"\n" ^done (gdb) <<< prompted ok -exec-continue ^error,msg="The program is not being run." <<< missing prompt after this print 1 &"print 1\n" ~"$2 = 1" ~"\n" ^done <<< missing prompt after this gdb 7.10.1 behaved correctly, even with "set mi-async on": -exec-continue ^error,msg="The program is not being run." (gdb) <<< prompted ok etc. Bisecting points at: commit 0b333c5e7d6c Author: Pedro Alves <palves@redhat.com> Date: Wed Sep 9 18:23:23 2015 +0100 Merge async and sync code paths some more [...] The problem is that when an exception is thrown, we leave the prompt state set to PROMPT_BLOCKED, and then mi_execute_command_input_handler doesn't print the prompt. It used to work because before that patch, we happened to skip disabling stdin if the current target didn't do async (which it never does before execution). I was surprised to find that this bug isn't caught by the testsuite, so I made a thorough test that tests all combinations of pairs of: - a failing synchronous execution command - a failing non-execution command - a non-failing command gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> PR mi/20431 * mi/mi-main.c (mi_execute_command): Enable input and set prompt state to PROMPT_NEEDED. gdb/testsuite/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> PR mi/20431 * gdb.mi/mi-cmd-error.exp: New file. --- gdb/mi/mi-main.c | 7 +++ gdb/testsuite/gdb.mi/mi-cmd-error.exp | 80 +++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-error.exp diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index b1cbd8b..1913157 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -2139,6 +2139,13 @@ mi_execute_command (const char *cmd, int from_tty) } CATCH (result, RETURN_MASK_ALL) { + /* Like in start_event_loop, enable input and force display + of the prompt. Otherwise, any command that calls + async_disable_stdin, and then throws, will leave input + disabled. */ + async_enable_stdin (); + current_ui->prompt_state = PROMPT_NEEDED; + /* The command execution failed and error() was called somewhere. */ mi_print_exception (command->token, result); diff --git a/gdb/testsuite/gdb.mi/mi-cmd-error.exp b/gdb/testsuite/gdb.mi/mi-cmd-error.exp new file mode 100644 index 0000000..a6d4791 --- /dev/null +++ b/gdb/testsuite/gdb.mi/mi-cmd-error.exp @@ -0,0 +1,80 @@ +# Copyright 2016 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/>. + +# Test that after: +# +# - a failing synchronous execution command, or, +# - a failing non-execution command, or, +# - a non-failing command, +# +# ... MI continues processing input. We actually test all +# combinations of pairs of the above. See PR mi/20431. + +load_lib mi-support.exp +set MIFLAGS "-i=mi" + +gdb_exit +if [mi_gdb_start] { + continue +} + +standard_testfile + +# A synchronous execution execution command that errors out. + +proc failing_sync_execution_command {} { + mi_gdb_test "-exec-continue" \ + {\^error,msg=\"The program is not being run.\"} \ + "failing sync execution command" +} + +# A non-execution command that errors out. + +proc failing_non_execution_command {} { + mi_gdb_test "-invalid-command" \ + {\^error,msg=\"Undefined MI command: invalid-command\",code=\"undefined-command\"} \ + "failing non-execution command" +} + +# A command that doesn't error out. + +proc non_failing_command {} { + mi_gdb_test "-gdb-show version" \ + ".*Free Software Foundation.*\\^done" \ + "non-failing command" +} + +# A list of procedures to try. +set procs { + failing_sync_execution_command + failing_non_execution_command + non_failing_command +} + +# User-friendly names for procedures above, in the same order. +set cmdnames { + "failing sync execution command" + "failing non-execution command" + "non-failing command" +} + +for {set i 0} {$i < [llength $procs]} {incr i} { + for {set j 0} {$j < [llength $procs]} {incr j} { + with_test_prefix "[lindex $cmdnames $i] first ($i x $j)" { + with_test_prefix "1st" [lindex $procs $i] + with_test_prefix "2nd" [lindex $procs $j] + } + } +} -- 2.5.5 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] Fix PR gdb/20418 - Problems with synchronous commands and new-ui 2016-08-02 23:21 [PATCH 0/2] Fix a couple MI/new-ui bugs (for master and 7.12) Pedro Alves 2016-08-02 23:21 ` [PATCH 1/2] Fix PR mi/20431 - Missing MI prompts after sync execution MI command (-exec-continue, etc.) errors Pedro Alves @ 2016-08-02 23:21 ` Pedro Alves 2016-08-03 16:09 ` Simon Marchi 1 sibling, 1 reply; 5+ messages in thread From: Pedro Alves @ 2016-08-02 23:21 UTC (permalink / raw) To: gdb-patches When executing commands on a secondary UI running the MI interpreter, some commands that should be synchronous are not. MI incorrectly continues processing input right after the synchronous command is sent, before the target stops. The problem happens when we emit MI async events (=library-loaded, etc.), and we go about restoring the previous terminal state, we end up calling target_terminal_ours, which incorrectly always installs the current UI's input_fd in the event loop... That is, code like this: old_chain = make_cleanup_restore_target_terminal (); target_terminal_ours_for_output (); fprintf_unfiltered (mi->event_channel, "library-loaded"); ... do_cleanups (old_chain); The fix is to move the add_file_handler/delete_file_handler calls out of target_terminal_$foo, making these completely no-ops unless called with the main UI as current UI. gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> PR gdb/20418 * event-top.c (ui_register_input_event_handler) (ui_unregister_input_event_handler): New functions. (async_enable_stdin): Register input in the event loop. (async_disable_stdin): Unregister input from the event loop. (gdb_setup_readline): Register input in the event loop. * infrun.c (check_curr_ui_sync_execution_done): Register input in the event loop. * target.c (target_terminal_inferior): Don't unregister input from the event loop. (target_terminal_ours): Don't register input in the event loop. * target.h (target_terminal_inferior) (target_terminal_ours_for_output, target_terminal_ours): Update comments. * top.h (ui_register_input_event_handler) (ui_unregister_input_event_handler): New declarations. * utils.c (ui_unregister_input_event_handler_cleanup) (prepare_to_handle_input): New functions. (defaulted_query, prompt_for_continue): Use prepare_to_handle_input. gdb/testsuite/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> Simon Marchi <simon.marchi@ericsson.com> PR gdb/20418 * gdb.mi/new-ui-mi-sync.c, gdb.mi/new-ui-mi-sync.exp: New files. * lib/mi-support.exp (mi_expect_interrupt): Remove anchors. --- gdb/event-top.c | 20 +++++- gdb/infrun.c | 1 + gdb/target.c | 11 --- gdb/target.h | 14 ++-- gdb/testsuite/gdb.mi/new-ui-mi-sync.c | 25 +++++++ gdb/testsuite/gdb.mi/new-ui-mi-sync.exp | 114 ++++++++++++++++++++++++++++++++ gdb/testsuite/lib/mi-support.exp | 4 +- gdb/top.h | 6 ++ gdb/utils.c | 43 +++++++++--- 9 files changed, 208 insertions(+), 30 deletions(-) create mode 100644 gdb/testsuite/gdb.mi/new-ui-mi-sync.c create mode 100644 gdb/testsuite/gdb.mi/new-ui-mi-sync.exp diff --git a/gdb/event-top.c b/gdb/event-top.c index 072ad2a..91b06e6 100644 --- a/gdb/event-top.c +++ b/gdb/event-top.c @@ -550,6 +550,22 @@ stdin_event_handler (int error, gdb_client_data client_data) } } +/* See top.h. */ + +void +ui_register_input_event_handler (struct ui *ui) +{ + add_file_handler (ui->input_fd, stdin_event_handler, ui); +} + +/* See top.h. */ + +void +ui_unregister_input_event_handler (struct ui *ui) +{ + delete_file_handler (ui->input_fd); +} + /* Re-enable stdin after the end of an execution command in synchronous mode, or after an error from the target, and we aborted the exec operation. */ @@ -562,6 +578,7 @@ async_enable_stdin (void) if (ui->prompt_state == PROMPT_BLOCKED) { target_terminal_ours (); + ui_register_input_event_handler (ui); ui->prompt_state = PROMPT_NEEDED; } } @@ -575,6 +592,7 @@ async_disable_stdin (void) struct ui *ui = current_ui; ui->prompt_state = PROMPT_BLOCKED; + delete_file_handler (ui->input_fd); } \f @@ -1284,7 +1302,7 @@ gdb_setup_readline (int editing) Another source is going to be the target program (inferior), but that must be registered only when it actually exists (I.e. after we say 'run' or after we connect to a remote target. */ - add_file_handler (ui->input_fd, stdin_event_handler, ui); + ui_register_input_event_handler (ui); } /* Disable command input through the standard CLI channels. Used in diff --git a/gdb/infrun.c b/gdb/infrun.c index 257ac8a..90841f4 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3848,6 +3848,7 @@ check_curr_ui_sync_execution_done (void) { target_terminal_ours (); observer_notify_sync_execution_done (); + ui_register_input_event_handler (ui); } } diff --git a/gdb/target.c b/gdb/target.c index 6228361..5603d01 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -486,15 +486,9 @@ target_terminal_inferior (void) if (ui->prompt_state != PROMPT_BLOCKED) return; - /* Always delete the current UI's input file handler, regardless of - terminal_state, because terminal_state is only valid for the main - UI. */ - delete_file_handler (ui->input_fd); - /* Since we always run the inferior in the main console (unless "set inferior-tty" is in effect), when some UI other than the main one calls target_terminal_inferior/target_terminal_inferior, then we - only register/unregister the UI's input from the event loop, but leave the main UI's terminal settings as is. */ if (ui != main_ui) return; @@ -520,11 +514,6 @@ target_terminal_ours (void) { struct ui *ui = current_ui; - /* Always add the current UI's input file handler, regardless of - terminal_state, because terminal_state is only valid for the main - UI. */ - add_file_handler (ui->input_fd, stdin_event_handler, ui); - /* See target_terminal_inferior. */ if (ui != main_ui) return; diff --git a/gdb/target.h b/gdb/target.h index 15fd806..7091360 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -1523,21 +1523,23 @@ extern int target_terminal_is_ours (void); extern void target_terminal_init (void); -/* Put the inferior's terminal settings into effect. - This is preparation for starting or resuming the inferior. */ +/* Put the inferior's terminal settings into effect. This is + preparation for starting or resuming the inferior. This is a no-op + unless called with the main UI as current UI. */ extern void target_terminal_inferior (void); /* Put some of our terminal settings into effect, enough to get proper results from our output, but do not change into or out of RAW mode so that no input is discarded. This is a no-op if terminal_ours - was most recently called. */ + was most recently called. This is a no-op unless called with the main + UI as current UI. */ extern void target_terminal_ours_for_output (void); -/* Put our terminal settings into effect. - First record the inferior's terminal settings - so they can be restored properly later. */ +/* Put our terminal settings into effect. First record the inferior's + terminal settings so they can be restored properly later. This is + a no-op unless called with the main UI as current UI. */ extern void target_terminal_ours (void); diff --git a/gdb/testsuite/gdb.mi/new-ui-mi-sync.c b/gdb/testsuite/gdb.mi/new-ui-mi-sync.c new file mode 100644 index 0000000..9cc8c90 --- /dev/null +++ b/gdb/testsuite/gdb.mi/new-ui-mi-sync.c @@ -0,0 +1,25 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2016 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/>. */ + +#include <unistd.h> + +int +main (void) +{ + sleep (180); + return 0; +} diff --git a/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp b/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp new file mode 100644 index 0000000..5165b9e --- /dev/null +++ b/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp @@ -0,0 +1,114 @@ +# Copyright 2016 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/>. + +# Test that on a separate MI UI (new-ui mi <tty>), the printing of an +# asynchronous event (e.g. =library-loaded) during the synchronous +# execution of a command (e.g. -exec-run or -exec-continue) does not +# prematurely re-enable MI input. After executing synchronous +# commands, MI should not process further commands until the inferior +# stops again. See PR gdb/20418. + +load_lib mi-support.exp + +standard_testfile + +if {[build_executable $testfile.exp $testfile ${srcfile} "debug"] == -1} { + untested "failed to compile $testfile" + return -1 +} + +# The test driver. SYNC_COMMAND specifies which command is used to +# synchronously start the program running. + +proc do_test {sync_command} { + global srcdir subdir binfile srcfile + global gdb_spawn_id gdb_main_spawn_id mi_spawn_id inferior_spawn_id + global gdb_prompt mi_gdb_prompt + + mi_gdb_exit + + if {[mi_gdb_start "separate-mi-tty"] != 0} { + fail "Could not start gdb" + return + } + + mi_delete_breakpoints + mi_gdb_reinitialize_dir $srcdir/$subdir + mi_gdb_reinitialize_dir $srcdir/$subdir + mi_gdb_load $binfile + + # Start a synchronous run/continue on the MI UI. + set test "send synchronous execution command" + if {$sync_command == "run"} { + if {[mi_run_cmd] >= 0} { + pass $test + } else { + return + } + } else { + if {[mi_runto main] < 0} { + return + } + if {[mi_send_resuming_command_raw "123-exec-continue" $test] >= 0} { + pass $test + } else { + return + } + } + + # Send -thread-info immediately after. If everything works + # correctly, this is only serviced by GDB when the execution + # stops. + send_gdb "456-thread-info\n" + pass "send -thread-info" + + # Make sure we trigger an asynchronous event (=thread-group-added) + # in the separate MI UI. Note the "run" variant usually triggers + # =thread-group-started/=thread-created/=library-loaded as well. + with_spawn_id $gdb_main_spawn_id { + gdb_test "add-inferior" "Added inferior 2" + } + + # Interrupt the program. + with_spawn_id $gdb_main_spawn_id { + set test "interrupt on the CLI" + gdb_test_multiple "$test" "$test" { + -re "$gdb_prompt " { + gdb_test_multiple "" "$test" { + -re "received signal SIGINT" { + pass $test + } + } + } + } + } + + # On the MI channel, we should see the interrupt output _before_ + # the -thread-info output. + with_spawn_id $mi_spawn_id { + mi_expect_interrupt "got MI interrupt output" + } + + # Look for the result of our -thread-info. If input were + # re-enabled too soon, the thread would still be running. + with_spawn_id $mi_spawn_id { + mi_gdb_test "" "456\\^.*state=\"stopped\".*" \ + "got -thread-info output and thread is stopped" + } +} + +foreach_with_prefix sync-command {"run" "continue"} { + do_test ${sync-command} +} diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp index 28af70a..18664c4 100644 --- a/gdb/testsuite/lib/mi-support.exp +++ b/gdb/testsuite/lib/mi-support.exp @@ -1272,7 +1272,7 @@ proc mi_expect_interrupt { test } { if {$async} { set prompt_re "" } else { - set prompt_re "$mi_gdb_prompt$" + set prompt_re "$mi_gdb_prompt" } set r_nonstop "reason=\"signal-received\",signal-name=\"0\",signal-meaning=\"Signal 0\"" @@ -1287,7 +1287,7 @@ proc mi_expect_interrupt { test } { pass "$test" return 0 } - -re ".*\r\n$mi_gdb_prompt$" { + -re ".*\r\n$mi_gdb_prompt" { verbose -log "got $expect_out(buffer)" fail "$test (unknown output after running)" return -1 diff --git a/gdb/top.h b/gdb/top.h index bdc3529..c5f6bc7 100644 --- a/gdb/top.h +++ b/gdb/top.h @@ -191,6 +191,12 @@ extern struct cleanup *make_delete_ui_cleanup (struct ui *ui); /* Cleanup that restores the current UI. */ extern void restore_ui_cleanup (void *data); +/* Register the UI's input file descriptor in the event loop. */ +extern void ui_register_input_event_handler (struct ui *ui); + +/* Unregister the UI's input file descriptor from the event loop. */ +extern void ui_unregister_input_event_handler (struct ui *ui); + /* From top.c. */ extern char *saved_command_line; extern int in_user_command; diff --git a/gdb/utils.c b/gdb/utils.c index c70e99c..5188828 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -1209,6 +1209,33 @@ compile_rx_or_error (regex_t *pattern, const char *rx, const char *message) return make_regfree_cleanup (pattern); } +/* A cleanup that simply calls ui_unregister_input_event_handler. */ + +static void +ui_unregister_input_event_handler_cleanup (void *ui) +{ + ui_unregister_input_event_handler ((struct ui *) ui); +} + +/* Set up to handle input. */ + +static struct cleanup * +prepare_to_handle_input (void) +{ + struct cleanup *old_chain; + + old_chain = make_cleanup_restore_target_terminal (); + target_terminal_ours (); + + ui_register_input_event_handler (current_ui); + if (current_ui->prompt_state == PROMPT_BLOCKED) + make_cleanup (ui_unregister_input_event_handler_cleanup, current_ui); + + make_cleanup_override_quit_handler (default_quit_handler); + + return old_chain; +} + \f /* This function supports the query, nquery, and yquery functions. @@ -1265,8 +1292,6 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) if (!confirm || server_command) return def_value; - old_chain = make_cleanup_restore_target_terminal (); - /* If input isn't coming from the user directly, just say what question we're asking, and then answer the default automatically. This way, important error messages don't get lost when talking to GDB @@ -1274,6 +1299,8 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) if (current_ui->instream != current_ui->stdin_stream || !input_interactive_p (current_ui)) { + old_chain = make_cleanup_restore_target_terminal (); + target_terminal_ours_for_output (); wrap_here (""); vfprintf_filtered (gdb_stdout, ctlstr, args); @@ -1291,6 +1318,7 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) { int res; + old_chain = make_cleanup_restore_target_terminal (); res = deprecated_query_hook (ctlstr, args); do_cleanups (old_chain); return res; @@ -1298,7 +1326,7 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) /* Format the question outside of the loop, to avoid reusing args. */ question = xstrvprintf (ctlstr, args); - make_cleanup (xfree, question); + old_chain = make_cleanup (xfree, question); prompt = xstrprintf (_("%s%s(%s or %s) %s"), annotation_level > 1 ? "\n\032\032pre-query\n" : "", question, y_string, n_string, @@ -1308,9 +1336,7 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) /* Used for calculating time spend waiting for user. */ gettimeofday (&prompt_started, NULL); - /* We'll need to handle input. */ - target_terminal_ours (); - make_cleanup_override_quit_handler (default_quit_handler); + prepare_to_handle_input (); while (1) { @@ -1882,10 +1908,7 @@ prompt_for_continue (void) beyond the end of the screen. */ reinitialize_more_filter (); - /* We'll need to handle input. */ - make_cleanup_restore_target_terminal (); - target_terminal_ours (); - make_cleanup_override_quit_handler (default_quit_handler); + prepare_to_handle_input (); /* Call gdb_readline_wrapper, not readline, in order to keep an event loop running. */ -- 2.5.5 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] Fix PR gdb/20418 - Problems with synchronous commands and new-ui 2016-08-02 23:21 ` [PATCH 2/2] Fix PR gdb/20418 - Problems with synchronous commands and new-ui Pedro Alves @ 2016-08-03 16:09 ` Simon Marchi 2016-08-09 22:00 ` Pedro Alves 0 siblings, 1 reply; 5+ messages in thread From: Simon Marchi @ 2016-08-03 16:09 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 16-08-02 07:21 PM, Pedro Alves wrote: > +proc do_test {sync_command} { > + global srcdir subdir binfile srcfile > + global gdb_spawn_id gdb_main_spawn_id mi_spawn_id inferior_spawn_id > + global gdb_prompt mi_gdb_prompt > + > + mi_gdb_exit > + > + if {[mi_gdb_start "separate-mi-tty"] != 0} { > + fail "Could not start gdb" > + return > + } > + > + mi_delete_breakpoints > + mi_gdb_reinitialize_dir $srcdir/$subdir > + mi_gdb_reinitialize_dir $srcdir/$subdir This line is there twice. > + mi_gdb_load $binfile > + > + # Start a synchronous run/continue on the MI UI. > + set test "send synchronous execution command" > + if {$sync_command == "run"} { > + if {[mi_run_cmd] >= 0} { > + pass $test > + } else { > + return > + } > + } else { > + if {[mi_runto main] < 0} { > + return > + } > + if {[mi_send_resuming_command_raw "123-exec-continue" $test] >= 0} { > + pass $test > + } else { > + return > + } > + } > + > + # Send -thread-info immediately after. If everything works > + # correctly, this is only serviced by GDB when the execution > + # stops. > + send_gdb "456-thread-info\n" > + pass "send -thread-info" > + > + # Make sure we trigger an asynchronous event (=thread-group-added) > + # in the separate MI UI. Note the "run" variant usually triggers > + # =thread-group-started/=thread-created/=library-loaded as well. > + with_spawn_id $gdb_main_spawn_id { > + gdb_test "add-inferior" "Added inferior 2" > + } > + > + # Interrupt the program. > + with_spawn_id $gdb_main_spawn_id { > + set test "interrupt on the CLI" > + gdb_test_multiple "$test" "$test" { This uses "interrupt on the CLI" for both the command and the message. I guess it works because interrupt doesn't complain about arguments it doesn't know about. > + -re "$gdb_prompt " { > + gdb_test_multiple "" "$test" { > + -re "received signal SIGINT" { > + pass $test > + } > + } > + } > + } > + } > + > + # On the MI channel, we should see the interrupt output _before_ > + # the -thread-info output. > + with_spawn_id $mi_spawn_id { > + mi_expect_interrupt "got MI interrupt output" > + } > + > + # Look for the result of our -thread-info. If input were > + # re-enabled too soon, the thread would still be running. Maybe say "appear to be running in the -thread-info output" or something like that. Otherwise it implies that the thread would still be running at this point in time in the test. But in general it looks good to me, even though the fine details escape my comprehension. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] Fix PR gdb/20418 - Problems with synchronous commands and new-ui 2016-08-03 16:09 ` Simon Marchi @ 2016-08-09 22:00 ` Pedro Alves 0 siblings, 0 replies; 5+ messages in thread From: Pedro Alves @ 2016-08-09 22:00 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 08/03/2016 05:02 PM, Simon Marchi wrote: > On 16-08-02 07:21 PM, Pedro Alves wrote: > > This line is there twice. > Fixed. >> + # Interrupt the program. >> + with_spawn_id $gdb_main_spawn_id { >> + set test "interrupt on the CLI" >> + gdb_test_multiple "$test" "$test" { > > This uses "interrupt on the CLI" for both the command and the message. I guess it works > because interrupt doesn't complain about arguments it doesn't know about. > Good catch. Fixed. >> + >> + # Look for the result of our -thread-info. If input were >> + # re-enabled too soon, the thread would still be running. > > Maybe say "appear to be running in the -thread-info output" or something like > that. Otherwise it implies that the thread would still be running at this point in > time in the test. I did this: # Look for the result of our -thread-info. If input were - # re-enabled too soon, the thread would still be running. + # re-enabled too soon, the thread would incorrectly show up with + # state="running". > > But in general it looks good to me, even though the fine details escape my comprehension. > If you have specific questions, I can try to answer them. Below is the full patch, as pushed. From b24bdfa398beba87b48fffeb3b1f9bcfe7bf924d Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Tue, 9 Aug 2016 22:52:46 +0100 Subject: [PATCH] Fix PR gdb/20418 - Problems with synchronous commands and new-ui When executing commands on a secondary UI running the MI interpreter, some commands that should be synchronous are not. MI incorrectly continues processing input right after the synchronous command is sent, before the target stops. The problem happens when we emit MI async events (=library-loaded, etc.), and we go about restoring the previous terminal state, we end up calling target_terminal_ours, which incorrectly always installs the current UI's input_fd in the event loop... That is, code like this: old_chain = make_cleanup_restore_target_terminal (); target_terminal_ours_for_output (); fprintf_unfiltered (mi->event_channel, "library-loaded"); ... do_cleanups (old_chain); The fix is to move the add_file_handler/delete_file_handler calls out of target_terminal_$foo, making these completely no-ops unless called with the main UI as current UI. gdb/ChangeLog: 2016-08-09 Pedro Alves <palves@redhat.com> PR gdb/20418 * event-top.c (ui_register_input_event_handler) (ui_unregister_input_event_handler): New functions. (async_enable_stdin): Register input in the event loop. (async_disable_stdin): Unregister input from the event loop. (gdb_setup_readline): Register input in the event loop. * infrun.c (check_curr_ui_sync_execution_done): Register input in the event loop. * target.c (target_terminal_inferior): Don't unregister input from the event loop. (target_terminal_ours): Don't register input in the event loop. * target.h (target_terminal_inferior) (target_terminal_ours_for_output, target_terminal_ours): Update comments. * top.h (ui_register_input_event_handler) (ui_unregister_input_event_handler): New declarations. * utils.c (ui_unregister_input_event_handler_cleanup) (prepare_to_handle_input): New functions. (defaulted_query, prompt_for_continue): Use prepare_to_handle_input. gdb/testsuite/ChangeLog: 2016-08-09 Pedro Alves <palves@redhat.com> Simon Marchi <simon.marchi@ericsson.com> PR gdb/20418 * gdb.mi/new-ui-mi-sync.c, gdb.mi/new-ui-mi-sync.exp: New files. * lib/mi-support.exp (mi_expect_interrupt): Remove anchors. --- gdb/ChangeLog | 23 +++++++ gdb/testsuite/ChangeLog | 7 ++ gdb/event-top.c | 20 +++++- gdb/infrun.c | 1 + gdb/target.c | 11 --- gdb/target.h | 14 ++-- gdb/testsuite/gdb.mi/new-ui-mi-sync.c | 25 +++++++ gdb/testsuite/gdb.mi/new-ui-mi-sync.exp | 114 ++++++++++++++++++++++++++++++++ gdb/testsuite/lib/mi-support.exp | 4 +- gdb/top.h | 6 ++ gdb/utils.c | 43 +++++++++--- 11 files changed, 238 insertions(+), 30 deletions(-) create mode 100644 gdb/testsuite/gdb.mi/new-ui-mi-sync.c create mode 100644 gdb/testsuite/gdb.mi/new-ui-mi-sync.exp diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 53283b5..eca96db 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,28 @@ 2016-08-09 Pedro Alves <palves@redhat.com> + PR gdb/20418 + * event-top.c (ui_register_input_event_handler) + (ui_unregister_input_event_handler): New functions. + (async_enable_stdin): Register input in the event loop. + (async_disable_stdin): Unregister input from the event loop. + (gdb_setup_readline): Register input in the event loop. + * infrun.c (check_curr_ui_sync_execution_done): Register input in + the event loop. + * target.c (target_terminal_inferior): Don't unregister input from + the event loop. + (target_terminal_ours): Don't register input in the event loop. + * target.h (target_terminal_inferior) + (target_terminal_ours_for_output, target_terminal_ours): Update + comments. + * top.h (ui_register_input_event_handler) + (ui_unregister_input_event_handler): New declarations. + * utils.c (ui_unregister_input_event_handler_cleanup) + (prepare_to_handle_input): New functions. + (defaulted_query, prompt_for_continue): Use + prepare_to_handle_input. + +2016-08-09 Pedro Alves <palves@redhat.com> + PR mi/20431 * mi/mi-main.c (mi_execute_command): Enable input and set prompt state to PROMPT_NEEDED. diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 605802f..1b9ffed 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,4 +1,11 @@ 2016-08-09 Pedro Alves <palves@redhat.com> + Simon Marchi <simon.marchi@ericsson.com> + + PR gdb/20418 + * gdb.mi/new-ui-mi-sync.c, gdb.mi/new-ui-mi-sync.exp: New files. + * lib/mi-support.exp (mi_expect_interrupt): Remove anchors. + +2016-08-09 Pedro Alves <palves@redhat.com> PR mi/20431 * gdb.mi/mi-cmd-error.exp: New file. diff --git a/gdb/event-top.c b/gdb/event-top.c index 072ad2a..91b06e6 100644 --- a/gdb/event-top.c +++ b/gdb/event-top.c @@ -550,6 +550,22 @@ stdin_event_handler (int error, gdb_client_data client_data) } } +/* See top.h. */ + +void +ui_register_input_event_handler (struct ui *ui) +{ + add_file_handler (ui->input_fd, stdin_event_handler, ui); +} + +/* See top.h. */ + +void +ui_unregister_input_event_handler (struct ui *ui) +{ + delete_file_handler (ui->input_fd); +} + /* Re-enable stdin after the end of an execution command in synchronous mode, or after an error from the target, and we aborted the exec operation. */ @@ -562,6 +578,7 @@ async_enable_stdin (void) if (ui->prompt_state == PROMPT_BLOCKED) { target_terminal_ours (); + ui_register_input_event_handler (ui); ui->prompt_state = PROMPT_NEEDED; } } @@ -575,6 +592,7 @@ async_disable_stdin (void) struct ui *ui = current_ui; ui->prompt_state = PROMPT_BLOCKED; + delete_file_handler (ui->input_fd); } \f @@ -1284,7 +1302,7 @@ gdb_setup_readline (int editing) Another source is going to be the target program (inferior), but that must be registered only when it actually exists (I.e. after we say 'run' or after we connect to a remote target. */ - add_file_handler (ui->input_fd, stdin_event_handler, ui); + ui_register_input_event_handler (ui); } /* Disable command input through the standard CLI channels. Used in diff --git a/gdb/infrun.c b/gdb/infrun.c index 257ac8a..90841f4 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3848,6 +3848,7 @@ check_curr_ui_sync_execution_done (void) { target_terminal_ours (); observer_notify_sync_execution_done (); + ui_register_input_event_handler (ui); } } diff --git a/gdb/target.c b/gdb/target.c index 6228361..5603d01 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -486,15 +486,9 @@ target_terminal_inferior (void) if (ui->prompt_state != PROMPT_BLOCKED) return; - /* Always delete the current UI's input file handler, regardless of - terminal_state, because terminal_state is only valid for the main - UI. */ - delete_file_handler (ui->input_fd); - /* Since we always run the inferior in the main console (unless "set inferior-tty" is in effect), when some UI other than the main one calls target_terminal_inferior/target_terminal_inferior, then we - only register/unregister the UI's input from the event loop, but leave the main UI's terminal settings as is. */ if (ui != main_ui) return; @@ -520,11 +514,6 @@ target_terminal_ours (void) { struct ui *ui = current_ui; - /* Always add the current UI's input file handler, regardless of - terminal_state, because terminal_state is only valid for the main - UI. */ - add_file_handler (ui->input_fd, stdin_event_handler, ui); - /* See target_terminal_inferior. */ if (ui != main_ui) return; diff --git a/gdb/target.h b/gdb/target.h index 15fd806..7091360 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -1523,21 +1523,23 @@ extern int target_terminal_is_ours (void); extern void target_terminal_init (void); -/* Put the inferior's terminal settings into effect. - This is preparation for starting or resuming the inferior. */ +/* Put the inferior's terminal settings into effect. This is + preparation for starting or resuming the inferior. This is a no-op + unless called with the main UI as current UI. */ extern void target_terminal_inferior (void); /* Put some of our terminal settings into effect, enough to get proper results from our output, but do not change into or out of RAW mode so that no input is discarded. This is a no-op if terminal_ours - was most recently called. */ + was most recently called. This is a no-op unless called with the main + UI as current UI. */ extern void target_terminal_ours_for_output (void); -/* Put our terminal settings into effect. - First record the inferior's terminal settings - so they can be restored properly later. */ +/* Put our terminal settings into effect. First record the inferior's + terminal settings so they can be restored properly later. This is + a no-op unless called with the main UI as current UI. */ extern void target_terminal_ours (void); diff --git a/gdb/testsuite/gdb.mi/new-ui-mi-sync.c b/gdb/testsuite/gdb.mi/new-ui-mi-sync.c new file mode 100644 index 0000000..9cc8c90 --- /dev/null +++ b/gdb/testsuite/gdb.mi/new-ui-mi-sync.c @@ -0,0 +1,25 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2016 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/>. */ + +#include <unistd.h> + +int +main (void) +{ + sleep (180); + return 0; +} diff --git a/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp b/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp new file mode 100644 index 0000000..55c66a5 --- /dev/null +++ b/gdb/testsuite/gdb.mi/new-ui-mi-sync.exp @@ -0,0 +1,114 @@ +# Copyright 2016 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/>. + +# Test that on a separate MI UI (new-ui mi <tty>), the printing of an +# asynchronous event (e.g. =library-loaded) during the synchronous +# execution of a command (e.g. -exec-run or -exec-continue) does not +# prematurely re-enable MI input. After executing synchronous +# commands, MI should not process further commands until the inferior +# stops again. See PR gdb/20418. + +load_lib mi-support.exp + +standard_testfile + +if {[build_executable $testfile.exp $testfile ${srcfile} "debug"] == -1} { + untested "failed to compile $testfile" + return -1 +} + +# The test driver. SYNC_COMMAND specifies which command is used to +# synchronously start the program running. + +proc do_test {sync_command} { + global srcdir subdir binfile srcfile + global gdb_spawn_id gdb_main_spawn_id mi_spawn_id inferior_spawn_id + global gdb_prompt mi_gdb_prompt + + mi_gdb_exit + + if {[mi_gdb_start "separate-mi-tty"] != 0} { + fail "Could not start gdb" + return + } + + mi_delete_breakpoints + mi_gdb_reinitialize_dir $srcdir/$subdir + mi_gdb_load $binfile + + # Start a synchronous run/continue on the MI UI. + set test "send synchronous execution command" + if {$sync_command == "run"} { + if {[mi_run_cmd] >= 0} { + pass $test + } else { + return + } + } else { + if {[mi_runto main] < 0} { + return + } + if {[mi_send_resuming_command_raw "123-exec-continue" $test] >= 0} { + pass $test + } else { + return + } + } + + # Send -thread-info immediately after. If everything works + # correctly, this is only serviced by GDB when the execution + # stops. + send_gdb "456-thread-info\n" + pass "send -thread-info" + + # Make sure we trigger an asynchronous event (=thread-group-added) + # in the separate MI UI. Note the "run" variant usually triggers + # =thread-group-started/=thread-created/=library-loaded as well. + with_spawn_id $gdb_main_spawn_id { + gdb_test "add-inferior" "Added inferior 2" + } + + # Interrupt the program. + with_spawn_id $gdb_main_spawn_id { + set message "interrupt on the CLI" + gdb_test_multiple "interrupt" "$message" { + -re "$gdb_prompt " { + gdb_test_multiple "" "$message" { + -re "received signal SIGINT" { + pass $message + } + } + } + } + } + + # On the MI channel, we should see the interrupt output _before_ + # the -thread-info output. + with_spawn_id $mi_spawn_id { + mi_expect_interrupt "got MI interrupt output" + } + + # Look for the result of our -thread-info. If input were + # re-enabled too soon, the thread would incorrectly show up with + # state="running". + with_spawn_id $mi_spawn_id { + mi_gdb_test "" "456\\^.*state=\"stopped\".*" \ + "got -thread-info output and thread is stopped" + } +} + +foreach_with_prefix sync-command {"run" "continue"} { + do_test ${sync-command} +} diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp index 28af70a..18664c4 100644 --- a/gdb/testsuite/lib/mi-support.exp +++ b/gdb/testsuite/lib/mi-support.exp @@ -1272,7 +1272,7 @@ proc mi_expect_interrupt { test } { if {$async} { set prompt_re "" } else { - set prompt_re "$mi_gdb_prompt$" + set prompt_re "$mi_gdb_prompt" } set r_nonstop "reason=\"signal-received\",signal-name=\"0\",signal-meaning=\"Signal 0\"" @@ -1287,7 +1287,7 @@ proc mi_expect_interrupt { test } { pass "$test" return 0 } - -re ".*\r\n$mi_gdb_prompt$" { + -re ".*\r\n$mi_gdb_prompt" { verbose -log "got $expect_out(buffer)" fail "$test (unknown output after running)" return -1 diff --git a/gdb/top.h b/gdb/top.h index bdc3529..c5f6bc7 100644 --- a/gdb/top.h +++ b/gdb/top.h @@ -191,6 +191,12 @@ extern struct cleanup *make_delete_ui_cleanup (struct ui *ui); /* Cleanup that restores the current UI. */ extern void restore_ui_cleanup (void *data); +/* Register the UI's input file descriptor in the event loop. */ +extern void ui_register_input_event_handler (struct ui *ui); + +/* Unregister the UI's input file descriptor from the event loop. */ +extern void ui_unregister_input_event_handler (struct ui *ui); + /* From top.c. */ extern char *saved_command_line; extern int in_user_command; diff --git a/gdb/utils.c b/gdb/utils.c index c70e99c..5188828 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -1209,6 +1209,33 @@ compile_rx_or_error (regex_t *pattern, const char *rx, const char *message) return make_regfree_cleanup (pattern); } +/* A cleanup that simply calls ui_unregister_input_event_handler. */ + +static void +ui_unregister_input_event_handler_cleanup (void *ui) +{ + ui_unregister_input_event_handler ((struct ui *) ui); +} + +/* Set up to handle input. */ + +static struct cleanup * +prepare_to_handle_input (void) +{ + struct cleanup *old_chain; + + old_chain = make_cleanup_restore_target_terminal (); + target_terminal_ours (); + + ui_register_input_event_handler (current_ui); + if (current_ui->prompt_state == PROMPT_BLOCKED) + make_cleanup (ui_unregister_input_event_handler_cleanup, current_ui); + + make_cleanup_override_quit_handler (default_quit_handler); + + return old_chain; +} + \f /* This function supports the query, nquery, and yquery functions. @@ -1265,8 +1292,6 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) if (!confirm || server_command) return def_value; - old_chain = make_cleanup_restore_target_terminal (); - /* If input isn't coming from the user directly, just say what question we're asking, and then answer the default automatically. This way, important error messages don't get lost when talking to GDB @@ -1274,6 +1299,8 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) if (current_ui->instream != current_ui->stdin_stream || !input_interactive_p (current_ui)) { + old_chain = make_cleanup_restore_target_terminal (); + target_terminal_ours_for_output (); wrap_here (""); vfprintf_filtered (gdb_stdout, ctlstr, args); @@ -1291,6 +1318,7 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) { int res; + old_chain = make_cleanup_restore_target_terminal (); res = deprecated_query_hook (ctlstr, args); do_cleanups (old_chain); return res; @@ -1298,7 +1326,7 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) /* Format the question outside of the loop, to avoid reusing args. */ question = xstrvprintf (ctlstr, args); - make_cleanup (xfree, question); + old_chain = make_cleanup (xfree, question); prompt = xstrprintf (_("%s%s(%s or %s) %s"), annotation_level > 1 ? "\n\032\032pre-query\n" : "", question, y_string, n_string, @@ -1308,9 +1336,7 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) /* Used for calculating time spend waiting for user. */ gettimeofday (&prompt_started, NULL); - /* We'll need to handle input. */ - target_terminal_ours (); - make_cleanup_override_quit_handler (default_quit_handler); + prepare_to_handle_input (); while (1) { @@ -1882,10 +1908,7 @@ prompt_for_continue (void) beyond the end of the screen. */ reinitialize_more_filter (); - /* We'll need to handle input. */ - make_cleanup_restore_target_terminal (); - target_terminal_ours (); - make_cleanup_override_quit_handler (default_quit_handler); + prepare_to_handle_input (); /* Call gdb_readline_wrapper, not readline, in order to keep an event loop running. */ -- 2.5.5 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-08-09 22:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-02 23:21 [PATCH 0/2] Fix a couple MI/new-ui bugs (for master and 7.12) Pedro Alves 2016-08-02 23:21 ` [PATCH 1/2] Fix PR mi/20431 - Missing MI prompts after sync execution MI command (-exec-continue, etc.) errors Pedro Alves 2016-08-02 23:21 ` [PATCH 2/2] Fix PR gdb/20418 - Problems with synchronous commands and new-ui Pedro Alves 2016-08-03 16:09 ` Simon Marchi 2016-08-09 22:00 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).