From d9de1fe3d5607f96491e8f16f474b9441cbec849 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 23 Aug 2016 16:03:28 +0100 Subject: [PATCH] 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 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 PR gdb/20494 * gdb.base/new-ui-echo.c: New file. * gdb.base/new-ui-echo.exp: New file. --- 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(-) create mode 100644 gdb/testsuite/gdb.base/new-ui-echo.c create mode 100644 gdb/testsuite/gdb.base/new-ui-echo.exp diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 8de59d0969c..08a9bb56cf4 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,19 @@ +2016-08-23 Pedro Alves + + 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 PR gdb/20505 diff --git a/gdb/inflow.c b/gdb/inflow.c index 27ce0b04358..5b55ceca5ff 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 4a00716f150..0deced4535a 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 4fd53449d78..26ce978b290 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,9 @@ +2016-08-23 Pedro Alves + + PR gdb/20494 + * gdb.base/new-ui-echo.c: New file. + * gdb.base/new-ui-echo.exp: New file. + 2016-08-23 Yao Qi * 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 00000000000..5bd140ef84b --- /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 . + +*/ + +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 00000000000..c18244554b7 --- /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 . + +# 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 36c300e8ce1..bc44192b8ad 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); } -- 2.30.2