Fix PR gdb/20418 - Problems with synchronous commands and new-ui
authorPedro Alves <palves@redhat.com>
Tue, 9 Aug 2016 21:45:40 +0000 (22:45 +0100)
committerPedro Alves <palves@redhat.com>
Tue, 9 Aug 2016 21:50:45 +0000 (22:50 +0100)
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
gdb/event-top.c
gdb/infrun.c
gdb/target.c
gdb/target.h
gdb/testsuite/ChangeLog
gdb/testsuite/gdb.mi/new-ui-mi-sync.c [new file with mode: 0644]
gdb/testsuite/gdb.mi/new-ui-mi-sync.exp [new file with mode: 0644]
gdb/testsuite/lib/mi-support.exp
gdb/top.h
gdb/utils.c

index b2b518aa0865b46793d3e14adc6014167e997fd8..17c799eefc7ed7ab9dc531a3d6a4ea92f032e550 100644 (file)
@@ -1,3 +1,26 @@
+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
index 072ad2aceee65fbabbe265ae65cce040ef2e4c3e..91b06e6e1f6078b10eb899f517af1aeaa9e0f6be 100644 (file)
@@ -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
index 257ac8ae32ffa9b9883e2f33b764aa0093f462f8..90841f4e55f4ebb4f1ed9112d726d223abce8013 100644 (file)
@@ -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);
     }
 }
 
index 622836135f255c552a1d7761aa1a1c840b39e61e..5603d0193d7e8d33e21fab8f77fbbe5fd873c018 100644 (file)
@@ -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;
index 15fd806ad94acbedcc67e0a04092964f94043b17..70913602603b1b8cc42a451d0285a26b5df134ba 100644 (file)
@@ -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);
 
index 605802f7178912bc241b80ce422d89a63678ac0a..1b9ffed384e5f0979de147a10ebfe76fe94349fc 100644 (file)
@@ -1,3 +1,10 @@
+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
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 (file)
index 0000000..9cc8c90
--- /dev/null
@@ -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 (file)
index 0000000..55c66a5
--- /dev/null
@@ -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}
+}
index 28af70a8f4078fc416af3621de21b77647a83c46..18664c4d56f478f5c4d6d96d07e4b46ddb11718e 100644 (file)
@@ -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
index bdc3529ab136d0a90ed79eaef76809cb3a73d7ea..c5f6bc7b45b947a1b7afa67a8818689f59e78b52 100644 (file)
--- 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;
index c70e99cea4924a3dc5093b3d340db29a03b85784..5188828ef486af76b3ef500045615d8a987f0f3a 100644 (file)
@@ -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.  */