[pushed master+7.12 0/2] new-ui command: gdb internal errors if input is already pending

[pushed master+7.12 0/2] new-ui command: gdb internal errors if input is already pending

[Pedro Alves]

This series fixes a bug I tripped on while playing with the new-ui
command.  The real fix is in patch #2.  Patch #1 is just a small
cleanup.

I've pushed them both to master and the 7.12 branch.

Pedro Alves (2):
  Introduce make_cleanup_restore_current_ui
  new-ui command: gdb internal errors if input is already pending

 gdb/ChangeLog                                   |  17 ++++
 gdb/testsuite/ChangeLog                         |   9 ++
 gdb/event-top.c                                 |  14 ++-
 gdb/infcall.c                                   |   2 +-
 gdb/infrun.c                                    |   2 +-
 gdb/testsuite/gdb.base/new-ui-pending-input.c   |  26 +++++
 gdb/testsuite/gdb.base/new-ui-pending-input.exp | 123 ++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                       |  22 ++++-
 gdb/top.c                                       |  13 ++-
 gdb/top.h                                       |   4 +-
 10 files changed, 221 insertions(+), 11 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/new-ui-pending-input.c
 create mode 100644 gdb/testsuite/gdb.base/new-ui-pending-input.exp

-- 
2.5.5

[pushed master+7.12 1/2] Introduce make_cleanup_restore_current_ui

[Pedro Alves]

Just a tidy, no functional changes.

gdb/ChangeLog:
2016-09-06  Pedro Alves  <palves@redhat.com>

	* event-top.c (restore_ui_cleanup): Now static.
	(make_cleanup_restore_current_ui): New function.
	(switch_thru_all_uis_init): Use it.
	* infcall.c (call_thread_fsm_should_stop): Use it.
	* infrun.c (fetch_inferior_event): Use it.
	* top.c (new_ui_command): Use it.
	* top.h (restore_ui_cleanup): Delete declaration.
	(make_cleanup_restore_current_ui): New declaration.
---
 gdb/ChangeLog   | 11 +++++++++++
 gdb/event-top.c | 14 +++++++++++---
 gdb/infcall.c   |  2 +-
 gdb/infrun.c    |  2 +-
 gdb/top.c       |  2 +-
 gdb/top.h       |  4 ++--
 6 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0df1486..e5e0cb5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2016-09-06  Pedro Alves  <palves@redhat.com>
+
+	* event-top.c (restore_ui_cleanup): Now static.
+	(make_cleanup_restore_current_ui): New function.
+	(switch_thru_all_uis_init): Use it.
+	* infcall.c (call_thread_fsm_should_stop): Use it.
+	* infrun.c (fetch_inferior_event): Use it.
+	* top.c (new_ui_command): Use it.
+	* top.h (restore_ui_cleanup): Delete declaration.
+	(make_cleanup_restore_current_ui): New declaration.
+
 2016-09-05  Ulrich Weigand  <uweigand@de.ibm.com>
 
 	* i386-tdep.c (i386_floatformat_for_type): New function.
diff --git a/gdb/event-top.c b/gdb/event-top.c
index 91b06e6..576eded 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -447,9 +447,9 @@ struct ui *main_ui;
 struct ui *current_ui;
 struct ui *ui_list;
 
-/* See top.h.  */
+/* A cleanup handler that restores the current UI.  */
 
-void
+static void
 restore_ui_cleanup (void *data)
 {
   current_ui = (struct ui *) data;
@@ -457,11 +457,19 @@ restore_ui_cleanup (void *data)
 
 /* See top.h.  */
 
+struct cleanup *
+make_cleanup_restore_current_ui (void)
+{
+  return make_cleanup (restore_ui_cleanup, current_ui);
+}
+
+/* See top.h.  */
+
 void
 switch_thru_all_uis_init (struct switch_thru_all_uis *state)
 {
   state->iter = ui_list;
-  state->old_chain = make_cleanup (restore_ui_cleanup, current_ui);
+  state->old_chain = make_cleanup_restore_current_ui ();
 }
 
 /* See top.h.  */
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 8595d9e..3c33c11 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -530,7 +530,7 @@ call_thread_fsm_should_stop (struct thread_fsm *self,
       f->return_value = get_call_return_value (&f->return_meta_info);
 
       /* Break out of wait_sync_command_done.  */
-      old_chain = make_cleanup (restore_ui_cleanup, current_ui);
+      old_chain = make_cleanup_restore_current_ui ();
       current_ui = f->waiting_ui;
       target_terminal_ours ();
       f->waiting_ui->prompt_state = PROMPT_NEEDED;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 90841f4..70d7a09 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3915,7 +3915,7 @@ fetch_inferior_event (void *client_data)
   /* Events are always processed with the main UI as current UI.  This
      way, warnings, debug output, etc. are always consistently sent to
      the main console.  */
-  make_cleanup (restore_ui_cleanup, current_ui);
+  make_cleanup_restore_current_ui ();
   current_ui = main_ui;
 
   /* End up with readline processing input, if necessary.  */
diff --git a/gdb/top.c b/gdb/top.c
index bc44192..5b385d2 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -386,7 +386,7 @@ new_ui_command (char *args, int from_tty)
   interpreter_name = argv[0];
   tty_name = argv[1];
 
-  make_cleanup (restore_ui_cleanup, current_ui);
+  make_cleanup_restore_current_ui ();
 
   failure_chain = make_cleanup (null_cleanup, NULL);
 
diff --git a/gdb/top.h b/gdb/top.h
index c5f6bc7..ee664c1 100644
--- a/gdb/top.h
+++ b/gdb/top.h
@@ -188,8 +188,8 @@ extern void delete_ui (struct ui *todel);
 /* Cleanup that deletes a UI.  */
 extern struct cleanup *make_delete_ui_cleanup (struct ui *ui);
 
-/* Cleanup that restores the current UI.  */
-extern void restore_ui_cleanup (void *data);
+/* Make a cleanup that restores the current UI.  */
+extern struct cleanup *make_cleanup_restore_current_ui (void);
 
 /* Register the UI's input file descriptor in the event loop.  */
 extern void ui_register_input_event_handler (struct ui *ui);
-- 
2.5.5

[pushed master+7.12 2/2] new-ui command: gdb internal errors if input is already pending

[Pedro Alves]

I noticed that if input is already pending on the new-ui TTY, gdb
internal-errors.

E.g., create /dev/pts/2, and type anything there (even just <return>
is sufficient).

Now start GDB creating a new UI on that TTY, while at the same time,
running a synchronous execution command.  Something like:

$ gdb program -ex "new-ui console /dev/pts/2" -ex "start"

Back on /dev/pts/2, we get:

  (gdb) .../src/gdb/event-top.c:360: internal-error: double prompt
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.

While the main UI was waiting for "start" to finish, gdb kepts pumping
events, including the input fd of the extra console.  The problem is
that stdin_event_handler doesn't restore the current UI back to what
it was, assuming that it's only ever called from the top level event
loop.  However, in this case, it's being called from the nested event
loop from within maybe_wait_sync_command_done.

When finally the "start" command is done, we reach the code that
prints the prompt in the main UI, just before starting the main event
loop.  Since now the current UI is pointing at the extra console (by
mistake), we find ourselves printing a double prompt on the extra
console.  This is caught by the assertion that fails, as shown above.

Since other event handlers also don't restore the UI (e.g., signal
event handlers), I think it's better if whatever is pumping events to
take care to restore the UI, if it cares.  That's what this patch
does.  New test included.

gdb/ChangeLog:
2016-09-06  Pedro Alves  <palves@redhat.com>

	* top.c (wait_sync_command_done): Don't assume current_ui doesn't
	change across events.  Restore the current UI before returning.
	(gdb_readline_wrapper): Restore the current UI before returning.

gdb/testsuite/ChangeLog:
2016-09-06  Pedro Alves  <palves@redhat.com>

	* gdb.base/new-ui-pending-input.c: New file.
	* gdb.base/new-ui-pending-input.exp: New file.
	* gdb.exp (clear_gdb_spawn_id): New procedure.
	(with_spawn_id): Check whether gdb_spawn_id exists before
	referencing it.  If gdb_spawn_id didn't exist on entry, clear it
	on exit.
---
 gdb/ChangeLog                                   |   6 ++
 gdb/testsuite/ChangeLog                         |   9 ++
 gdb/testsuite/gdb.base/new-ui-pending-input.c   |  26 +++++
 gdb/testsuite/gdb.base/new-ui-pending-input.exp | 123 ++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                       |  22 ++++-
 gdb/top.c                                       |  11 ++-
 6 files changed, 194 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/new-ui-pending-input.c
 create mode 100644 gdb/testsuite/gdb.base/new-ui-pending-input.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e5e0cb5..108528d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,11 @@
 2016-09-06  Pedro Alves  <palves@redhat.com>
 
+	* top.c (wait_sync_command_done): Don't assume current_ui doesn't
+	change across events.  Restore the current UI before returning.
+	(gdb_readline_wrapper): Restore the current UI before returning.
+
+2016-09-06  Pedro Alves  <palves@redhat.com>
+
 	* event-top.c (restore_ui_cleanup): Now static.
 	(make_cleanup_restore_current_ui): New function.
 	(switch_thru_all_uis_init): Use it.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 16070a3..958ec27 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,12 @@
+2016-09-06  Pedro Alves  <palves@redhat.com>
+
+	* gdb.base/new-ui-pending-input.c: New file.
+	* gdb.base/new-ui-pending-input.exp: New file.
+	* gdb.exp (clear_gdb_spawn_id): New procedure.
+	(with_spawn_id): Check whether gdb_spawn_id exists before
+	referencing it.  If gdb_spawn_id didn't exist on entry, clear it
+	on exit.
+
 2016-09-05  Ulrich Weigand  <uweigand@de.ibm.com>
 
 	* gdb.base/float128.c: New file.
diff --git a/gdb/testsuite/gdb.base/new-ui-pending-input.c b/gdb/testsuite/gdb.base/new-ui-pending-input.c
new file mode 100644
index 0000000..a4bae7d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/new-ui-pending-input.c
@@ -0,0 +1,26 @@
+/* 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 (2);
+  return 0; /* set breakpoint here */
+}
diff --git a/gdb/testsuite/gdb.base/new-ui-pending-input.exp b/gdb/testsuite/gdb.base/new-ui-pending-input.exp
new file mode 100644
index 0000000..325c30f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/new-ui-pending-input.exp
@@ -0,0 +1,123 @@
+# 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 'gdb "-ex new-ui TTY" -ex "start"' (or any other synchronous
+# execution command), when TTY already has input pending.  GDB used to
+# internal error in this situation.
+
+standard_testfile
+
+set compile_options "debug"
+if {[build_executable $testfile.exp $testfile ${srcfile} ${compile_options}] == -1} {
+    untested "failed to compile $testfile"
+    return -1
+}
+
+# See intro.
+
+proc test_command_line_new_ui_pending_input {} {
+    global gdb_prompt
+    global binfile
+
+    # This test requires running a synchronous execution command from
+    # the command line.
+    if {[use_gdb_stub] || [target_info gdb_protocol] == "extended-remote" } {
+	unsupported "can't run from the command line"
+	return 0
+    }
+
+    spawn -pty
+    set extra_spawn_id $spawn_id
+    set extra_tty_name $spawn_out(slave,name)
+
+    # An arbitrary number of prints.
+    set nprints 3
+
+    # Queue a few commands before GDB is started.
+    with_spawn_id $extra_spawn_id {
+	for {set i 1} {$i <= $nprints} {incr i} {
+	    send_gdb "print $i\n"
+	}
+    }
+    pass "commands pending"
+
+    # Now spawn GDB, creating a new-ui and at the same time running a
+    # synchronous command.
+    set test "spawn gdb"
+
+    set bpline [gdb_get_line_number "set breakpoint here"]
+
+    set options ""
+    append options " -iex \"set height 0\""
+    append options " -iex \"set width 0\""
+    append options " -iex \"new-ui console $extra_tty_name\""
+    append options " -ex \"b $bpline\""
+    append options " -ex \"run\""
+
+    if {[gdb_spawn_with_cmdline_opts "$options $binfile"] != 0} {
+	fail $test
+	return
+    } else {
+	pass $test
+    }
+
+    # Consume the initial prompt on the extra console.
+    with_spawn_id $extra_spawn_id {
+	set test "initial prompt on extra console"
+	gdb_test_multiple "" $test {
+	    -re "$gdb_prompt $" {
+		pass $test
+	    }
+	}
+    }
+
+    # Check that we see the result of the print commands in the extra
+    # UI.
+    with_spawn_id $extra_spawn_id {
+	for {set i 1} {$i <= $nprints} {incr i} {
+	    set test "print $i on extra console"
+	    gdb_test_multiple "" $test {
+		-re " = $i\r\n$gdb_prompt " {
+		    pass "$test"
+		}
+	    }
+	}
+    }
+
+    # Now check that we reach the breakpoint successfully.
+    set test "run to breakpoint on main console"
+    gdb_test_multiple "" $test {
+	-re "Breakpoint .* main .*set breakpoint here.*$gdb_prompt $" {
+	    pass $test
+	}
+    }
+
+    # And likewise on the extra console.  No prompt is expected.
+    with_spawn_id $extra_spawn_id {
+	set test "run to breakpoint on extra console"
+	gdb_test_multiple "" $test {
+	    -re "Breakpoint .* main .*set breakpoint here" {
+		pass $test
+	    }
+	}
+    }
+}
+
+# The driver.  Calls all tests.
+proc testcase_driver {} {
+    test_command_line_new_ui_pending_input
+}
+
+testcase_driver
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 5cab774..e538812 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2087,17 +2087,35 @@ proc switch_gdb_spawn_id {spawn_id} {
     set board_info($board,fileid) $spawn_id
 }
 
+# Clear the default spawn id.
+
+proc clear_gdb_spawn_id {} {
+    global gdb_spawn_id
+    global board board_info
+
+    unset -nocomplain gdb_spawn_id
+    set board [host_info name]
+    unset -nocomplain board_info($board,fileid)
+}
+
 # Run BODY with SPAWN_ID as current spawn id.
 
 proc with_spawn_id { spawn_id body } {
     global gdb_spawn_id
 
-    set saved_spawn_id $gdb_spawn_id
+    if [info exists gdb_spawn_id] {
+	set saved_spawn_id $gdb_spawn_id
+    }
+
     switch_gdb_spawn_id $spawn_id
 
     set code [catch {uplevel 1 $body} result]
 
-    switch_gdb_spawn_id $saved_spawn_id
+    if [info exists saved_spawn_id] {
+	switch_gdb_spawn_id $saved_spawn_id
+    } else {
+	clear_gdb_spawn_id
+    }
 
     if {$code == 1} {
 	global errorInfo errorCode
diff --git a/gdb/top.c b/gdb/top.c
index 5b385d2..320c296 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -561,9 +561,15 @@ check_frame_language_change (void)
 void
 wait_sync_command_done (void)
 {
+  /* Processing events may change the current UI.  */
+  struct cleanup *old_chain = make_cleanup_restore_current_ui ();
+  struct ui *ui = current_ui;
+
   while (gdb_do_one_event () >= 0)
-    if (current_ui->prompt_state != PROMPT_BLOCKED)
+    if (ui->prompt_state != PROMPT_BLOCKED)
       break;
+
+  do_cleanups (old_chain);
 }
 
 /* See top.h.  */
@@ -1030,6 +1036,9 @@ gdb_readline_wrapper (const char *prompt)
   ui->secondary_prompt_depth++;
   back_to = make_cleanup (gdb_readline_wrapper_cleanup, cleanup);
 
+  /* Processing events may change the current UI.  */
+  make_cleanup_restore_current_ui ();
+
   if (cleanup->target_is_async_orig)
     target_async (0);
 
-- 
2.5.5