This is the mail archive of the
gdb-cvs@sourceware.org
mailing list for the GDB project.
[binutils-gdb] Fix PR20494 - User input stops being echoed in CLI
- From: Pedro Alves <palves at sourceware dot org>
- To: gdb-cvs at sourceware dot org
- Date: 23 Aug 2016 15:06:59 -0000
- Subject: [binutils-gdb] Fix PR20494 - User input stops being echoed in CLI
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=d9de1fe3d5607f96491e8f16f474b9441cbec849
commit d9de1fe3d5607f96491e8f16f474b9441cbec849
Author: Pedro Alves <palves@redhat.com>
Date: Tue Aug 23 16:03:28 2016 +0100
Fix PR20494 - User input stops being echoed in CLI
This patch fixes a problem that problem triggers if you start an
inferior, e.g., with the "start" command, in a UI created with the
new-ui command, and then run a foreground execution command in the
main UI. Once the program stops for the latter command, typing in the
main UI no longer echoes back to the user.
The problem revolves around this:
- gdb_has_a_terminal computes its result lazily, on first call.
that is what saves gdb's initial main UI terminal state (the UI
associated with stdin):
our_terminal_info.ttystate = serial_get_tty_state (stdin_serial);
This is the state that target_terminal_ours() restores.
- In this scenario, the gdb_has_a_terminal function happens to be
first ever called from within the target_terminal_init call in
startup_inferior:
(top-gdb) bt
#0 gdb_has_a_terminal () at src/gdb/inflow.c:157
#1 0x000000000079db22 in child_terminal_init_with_pgrp () at src/gdb/inflow.c:217
[...]
#4 0x000000000065bacb in target_terminal_init () at src/gdb/target.c:456
#5 0x00000000004676d2 in startup_inferior () at src/gdb/fork-child.c:531
[...]
#7 0x000000000046b168 in linux_nat_create_inferior () at src/gdb/linux-nat.c:1112
[...]
#9 0x00000000005f20c9 in start_command (args=0x0, from_tty=1) at src/gdb/infcmd.c:657
If the command to start the inferior is issued on the main UI, then
readline will have deprepped the terminal when we reach the above, and
the problem doesn't appear.
If however the command is issued on a non-main UI, then when we reach
that gdb_has_a_terminal call, the main UI's terminal state is still
set to whatever readline has sets it to in rl_prep_terminal, which
happens to have echo disabled. Later, when the following synchronous
execution command finishes, we'll call target_terminal_ours to restore
gdb's the main UI's terminal settings, and that restores the terminal
state with echo disabled...
Conceptually, the fix is to move the gdb_has_a_terminal call earlier,
to someplace during GDB initialization, before readline/ncurses have
had a chance to change terminal settings. Turns out that
"set_initial_gdb_ttystate" is exactly such a place.
I say conceptually, because the fix actually inlines the
gdb_has_a_terminal part that saves the terminal state in
set_initial_gdb_ttystate and then simplifies gdb_has_a_terminal, since
there's no point in making gdb_has_a_terminal do lazy computation.
gdb/ChangeLog:
2016-08-23 Pedro Alves <palves@redhat.com>
PR gdb/20494
* inflow.c (our_terminal_info, initial_gdb_ttystate): Update
comments.
(enum gdb_has_a_terminal_flag_enum, gdb_has_a_terminal_flag):
Delete.
(set_initial_gdb_ttystate): Record our_terminal_info here too,
instead of ...
(gdb_has_a_terminal): ... here. Reimplement in terms of
initial_gdb_ttystate. Make static.
* terminal.h (gdb_has_a_terminal): Delete declaration.
(set_initial_gdb_ttystate): Add comment.
* top.c (show_interactive_mode): Use input_interactive_p instead
of gdb_has_a_terminal.
gdb/testsuite/ChangeLog:
2016-08-23 Pedro Alves <palves@redhat.com>
PR gdb/20494
* gdb.base/new-ui-echo.c: New file.
* gdb.base/new-ui-echo.exp: New file.
Diff:
---
gdb/ChangeLog | 16 +++++
gdb/inflow.c | 68 +++++++-----------
gdb/terminal.h | 4 +-
gdb/testsuite/ChangeLog | 6 ++
gdb/testsuite/gdb.base/new-ui-echo.c | 30 ++++++++
gdb/testsuite/gdb.base/new-ui-echo.exp | 127 +++++++++++++++++++++++++++++++++
gdb/top.c | 2 +-
7 files changed, 206 insertions(+), 47 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8de59d0..08a9bb5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@
+2016-08-23 Pedro Alves <palves@redhat.com>
+
+ PR gdb/20494
+ * inflow.c (our_terminal_info, initial_gdb_ttystate): Update
+ comments.
+ (enum gdb_has_a_terminal_flag_enum, gdb_has_a_terminal_flag):
+ Delete.
+ (set_initial_gdb_ttystate): Record our_terminal_info here too,
+ instead of ...
+ (gdb_has_a_terminal): ... here. Reimplement in terms of
+ initial_gdb_ttystate. Make static.
+ * terminal.h (gdb_has_a_terminal): Delete declaration.
+ (set_initial_gdb_ttystate): Add comment.
+ * top.c (show_interactive_mode): Use input_interactive_p instead
+ of gdb_has_a_terminal.
+
2016-08-22 Pedro Alves <palves@redhat.com>
PR gdb/20505
diff --git a/gdb/inflow.c b/gdb/inflow.c
index 27ce0b0..5b55cec 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -74,13 +74,17 @@ struct terminal_info
};
/* Our own tty state, which we restore every time we need to deal with
- the terminal. This is only set once, when GDB first starts. The
- settings of flags which readline saves and restores and
+ the terminal. This is set once, when GDB first starts, and then
+ whenever we enter/leave TUI mode (gdb_save_tty_state). The
+ settings of flags which readline saves and restores are
unimportant. */
static struct terminal_info our_terminal_info;
-/* Snapshot of our own tty state taken during initialization of GDB.
- This is used as the initial tty state given to each new inferior. */
+/* Snapshot of the initial tty state taken during initialization of
+ GDB, before readline/ncurses have had a chance to change it. This
+ is used as the initial tty state given to each new spawned
+ inferior. Unlike our_terminal_info, this is only ever set
+ once. */
static serial_ttystate initial_gdb_ttystate;
static struct terminal_info *get_inflow_inferior_data (struct inferior *);
@@ -136,61 +140,37 @@ gdb_getpgrp (void)
}
#endif
-enum gdb_has_a_terminal_flag_enum
- {
- yes, no, have_not_checked
- }
-gdb_has_a_terminal_flag = have_not_checked;
-
-/* Set the initial tty state that is to be inherited by new inferiors. */
+/* See terminal.h. */
void
set_initial_gdb_ttystate (void)
{
+ /* Note we can't do any of this in _initialize_inflow because at
+ that point stdin_serial has not been created yet. */
+
initial_gdb_ttystate = serial_get_tty_state (stdin_serial);
-}
-/* Does GDB have a terminal (on stdin)? */
-int
-gdb_has_a_terminal (void)
-{
- switch (gdb_has_a_terminal_flag)
+ if (initial_gdb_ttystate != NULL)
{
- case yes:
- return 1;
- case no:
- return 0;
- case have_not_checked:
- /* Get all the current tty settings (including whether we have a
- tty at all!). Can't do this in _initialize_inflow because
- serial_fdopen() won't work until the serial_ops_list is
- initialized. */
-
+ our_terminal_info.ttystate
+ = serial_copy_tty_state (stdin_serial, initial_gdb_ttystate);
#ifdef F_GETFL
our_terminal_info.tflags = fcntl (0, F_GETFL, 0);
#endif
-
- gdb_has_a_terminal_flag = no;
- if (stdin_serial != NULL)
- {
- our_terminal_info.ttystate = serial_get_tty_state (stdin_serial);
-
- if (our_terminal_info.ttystate != NULL)
- {
- gdb_has_a_terminal_flag = yes;
#ifdef PROCESS_GROUP_TYPE
- our_terminal_info.process_group = gdb_getpgrp ();
+ our_terminal_info.process_group = gdb_getpgrp ();
#endif
- }
- }
-
- return gdb_has_a_terminal_flag == yes;
- default:
- /* "Can't happen". */
- return 0;
}
}
+/* Does GDB have a terminal (on stdin)? */
+
+static int
+gdb_has_a_terminal (void)
+{
+ return initial_gdb_ttystate != NULL;
+}
+
/* Macro for printing errors from ioctl operations */
#define OOPSY(what) \
diff --git a/gdb/terminal.h b/gdb/terminal.h
index 4a00716..0deced4 100644
--- a/gdb/terminal.h
+++ b/gdb/terminal.h
@@ -99,10 +99,10 @@ extern int gdb_setpgid (void);
/* Set up a serial structure describing standard input. In inflow.c. */
extern void initialize_stdin_serial (void);
-extern int gdb_has_a_terminal (void);
-
extern void gdb_save_tty_state (void);
+/* Take a snapshot of our initial tty state before readline/ncurses
+ have had a chance to alter it. */
extern void set_initial_gdb_ttystate (void);
/* Set the process group of the caller to its own pid, or do nothing
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 4fd5344..26ce978 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2016-08-23 Pedro Alves <palves@redhat.com>
+
+ PR gdb/20494
+ * gdb.base/new-ui-echo.c: New file.
+ * gdb.base/new-ui-echo.exp: New file.
+
2016-08-23 Yao Qi <yao.qi@linaro.org>
* gdb.server/connect-stopped-target.exp (do_test): Pass "" to
diff --git a/gdb/testsuite/gdb.base/new-ui-echo.c b/gdb/testsuite/gdb.base/new-ui-echo.c
new file mode 100644
index 0000000..5bd140e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/new-ui-echo.c
@@ -0,0 +1,30 @@
+/* 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/>.
+
+*/
+
+volatile int global = 0;
+
+int
+main (void)
+{
+ global = 1; /* set break main console here */
+ global = 1;
+ global = 1; /* set break extra console here */
+ global = 1;
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.base/new-ui-echo.exp b/gdb/testsuite/gdb.base/new-ui-echo.exp
new file mode 100644
index 0000000..c182445
--- /dev/null
+++ b/gdb/testsuite/gdb.base/new-ui-echo.exp
@@ -0,0 +1,127 @@
+# 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/>.
+
+# Regression test for PR 20494 (User input stops being echoed in CLI).
+# Before that bug was fixed, starting an inferior in a non-main UI
+# would result in GDB saving readline's prepped terminal state as
+# gdb's "own" terminal state (i.e., target_terminal_ours state),
+# resulting in subsequent synchronous execution commands in the main
+# UI disabling input echo.
+
+standard_testfile
+
+set compile_options "debug"
+if {[build_executable $testfile.exp $testfile ${srcfile} ${compile_options}] == -1} {
+ untested "failed to compile $testfile"
+ return -1
+}
+
+# Start gdb and create an extra console UI. Start the inferior in the
+# DRIVER console (either "main" or "extra"), and then enter a
+# synchronous execution command in the extra console. Before PR 20494
+# was fixed, if DRIVER was a secondary UI, GDB would lose input echo
+# on the main UI after the synchronous execution command. We test
+# with both main and extra UIs as driver consoles for completeness.
+
+proc echo_test {driver} {
+ global srcfile testfile
+ global gdb_prompt
+ global gdb_spawn_id
+ global gdb_main_spawn_id extra_spawn_id
+ global decimal
+
+ clean_restart $testfile
+
+ # Save the main UI's spawn ID.
+ set gdb_main_spawn_id $gdb_spawn_id
+
+ # Create the new PTY for the secondary console UI.
+ spawn -pty
+ set extra_spawn_id $spawn_id
+ set extra_tty_name $spawn_out(slave,name)
+ gdb_test_multiple "new-ui console $extra_tty_name" "new-ui" {
+ -re "New UI allocated\r\n$gdb_prompt $" {
+ }
+ }
+
+ with_spawn_id $extra_spawn_id {
+ set test "initial prompt on extra console"
+ gdb_test_multiple "" $test {
+ -re "$gdb_prompt $" {
+ pass $test
+ }
+ }
+ }
+
+ set main_console [list $gdb_main_spawn_id "main console"]
+ set extra_console [list $extra_spawn_id "extra console"]
+
+ if {$driver == "main"} {
+ set con1 $main_console
+ set con2 $extra_console
+ } else {
+ set con1 $extra_console
+ set con2 $main_console
+ }
+
+ set con1_spawn_id [lindex $con1 0]
+ set con2_spawn_id [lindex $con2 0]
+ set con1_name [lindex $con1 1]
+ set con2_name [lindex $con2 1]
+
+ set bp_lineno [gdb_get_line_number "set break $con1_name here"]
+
+ with_spawn_id $con1_spawn_id {
+ gdb_test "break $srcfile:$bp_lineno" \
+ "Breakpoint $decimal .*$srcfile, line $bp_lineno\\." \
+ "set breakpoint using $con1_name"
+ gdb_run_cmd
+ gdb_test "" "set break $con1_name here .*" "run to breakpoint on $con1_name"
+ }
+
+ with_spawn_id $con2_spawn_id {
+ set test "breakpoint hit reported on $con2_name too"
+ gdb_test_multiple "" $test {
+ -re "Breakpoint $decimal, .* set break $con1_name here " {
+ pass $test
+ }
+ }
+ gdb_test "next" "global = 1;" "next on $con2_name"
+ }
+
+ # Ensure echo remains enabled in both consoles.
+ with_spawn_id $con1_spawn_id {
+ gdb_test "print 1" "^print 1\r\n\\\$1 = 1" "print on $con1_name echoes"
+ }
+ with_spawn_id $con2_spawn_id {
+ gdb_test "print 2" "^print 2\r\n\\\$2 = 2" "print on $con2_name echoes"
+ }
+}
+
+# The test driver.
+
+proc test_driver {} {
+
+ with_test_prefix "extra console as driver" {
+ echo_test "extra"
+ }
+
+ with_test_prefix "main console as driver" {
+ echo_test "main"
+ }
+
+}
+
+test_driver
diff --git a/gdb/top.c b/gdb/top.c
index 36c300e..bc44192 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -1719,7 +1719,7 @@ show_interactive_mode (struct ui_file *file, int from_tty,
if (interactive_mode == AUTO_BOOLEAN_AUTO)
fprintf_filtered (file, "Debugger's interactive mode "
"is %s (currently %s).\n",
- value, gdb_has_a_terminal () ? "on" : "off");
+ value, input_interactive_p (current_ui) ? "on" : "off");
else
fprintf_filtered (file, "Debugger's interactive mode is %s.\n", value);
}