[PATCH] Make sure terminal settings are restored before exiting

[PATCH] Make sure terminal settings are restored before exiting

[Patrick Palka]

When exiting GDB -- whether it's via the "quit" command, via a SIGTERM,
or otherwise -- we should leave the terminal in the state we acquired
it.  To that end, we have to undo any modifications that may have been
made by the TUI (ncurses) or by the CLI (readline).

[ Note that we already take a snapshot of the original tty state and save
  it to inflow.c:initial_gdb_ttystate.  Using this variable we can
  define a new function restore_initial_gdb_ttystate and use it here.
  We can replace the call to rl_deprep_terminal with such a function,
  though it wouldn't hurt to have both around either.  Is this a good
  idea?

  As far as testing goes, I am having trouble figuring out how to
  retrieve the pid of the GDB subprocess in order to kill it via SIGTERM
  with the subshell/stty approach used in
  batch-preserve-term-settings.exp.  Seems non-trivial.  Any ideas?  ]

gdb/ChangeLog:

	* top.c (quit_force): Undo any modifications that may have been
	made the terminal by calling target_terminal_ours, then
	tui_disable, then rl_deprep_terminal.
---
 gdb/top.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdb/top.c b/gdb/top.c
index 1e30b1c..55f82ae 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -66,6 +66,7 @@
 #include "cli-out.h"
 #include "tracepoint.h"
 #include "inf-loop.h"
+#include "tui/tui.h"
 
 extern void initialize_all_files (void);
 
@@ -1494,6 +1495,14 @@ quit_force (char *args, int from_tty)
   int exit_code = 0;
   struct qt_args qt;
 
+  /* Make sure that the terminal is left in the state we acquired it.  */
+  target_terminal_ours ();
+#if defined(TUI)
+  tui_disable ();
+#endif
+  if (async_command_editing_p)
+    rl_deprep_terminal ();
+
   /* An optional expression may be used to cause gdb to terminate with the 
      value of that expression.  */
   if (args)
-- 
2.5.0.rc2.22.g69b1679.dirty

[PATCH] Make sure terminal settings are restored before exiting

[Patrick Palka]

[ Here is a new version of the patch that uses gdb_disable_readline
  instead of rl_deprep_terminal or gdb_rl_callback_handler_remove.

  A test is added to batch-preserve-term-settings.exp.  The test
  depends on the cleanup patch I posted earlier.  ]

Tested on x86_64 Stretch.

gdb/ChangeLog:

	* top.c (quit_force): Undo any modifications that may have been
	made to the terminal.

gdb/testsuite/ChangeLog:

	* gdb.base/batch-preserve-term-settings.exp
	(test_terminal_settings_preserved_after_cli_exit): New test.
---
 .../gdb.base/batch-preserve-term-settings.exp      | 94 +++++++++++++++++++++-
 gdb/top.c                                          |  9 +++
 2 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/batch-preserve-term-settings.exp b/gdb/testsuite/gdb.base/batch-preserve-term-settings.exp
index 97ffaa4..ca6f173 100644
--- a/gdb/testsuite/gdb.base/batch-preserve-term-settings.exp
+++ b/gdb/testsuite/gdb.base/batch-preserve-term-settings.exp
@@ -176,4 +176,96 @@ proc test_terminal_settings_preserved {} {
     exit_shell
 }
 
-test_terminal_settings_preserved
+# Check that quitting from the CLI via the "quit" command does not leave the
+# terminal in the wrong state.  The GDB commands CMDS are executed before
+# quitting.
+
+proc test_terminal_settings_preserved_after_cli_exit { cmds } {
+    global file_arg
+    global GDB INTERNAL_GDBFLAGS GDBFLAGS
+    global gdb_prompt
+    global shell_prompt_re
+
+    if ![spawn_shell] {
+	return
+    }
+
+    set saved_gdbflags $GDBFLAGS
+
+    set stty_supported [run_stty "stty before" stty_before]
+
+    set test "start gdb"
+    send_gdb "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts] --args \"$file_arg\"\n"
+    gdb_expect {
+	-re "$gdb_prompt $" {
+	    pass $test
+	}
+	timeout {
+	    fail "$test (timeout)"
+	}
+	eof {
+	    fail "$test (eof)"
+	}
+    }
+
+    foreach cmd $cmds {
+	set test "run command $cmd"
+	send_gdb "$cmd\n"
+	gdb_expect {
+	    -re "$gdb_prompt $" {
+		pass $test
+	    }
+	    timeout {
+		fail "$test (timeout)"
+	    }
+	    eof {
+		fail "$test (eof)"
+	    }
+	}
+    }
+
+    set test "quit gdb"
+    send_gdb "quit\n"
+    gdb_expect {
+	-re "(y or n)" {
+	    send_gdb "y\n"
+	    exp_continue
+	}
+	-re ".*$shell_prompt_re$" {
+	    pass $test
+	}
+	timeout {
+	    fail "$test (timeout)"
+	}
+	eof {
+	    fail "$test (eof)"
+	}
+    }
+
+    set test "terminal settings preserved"
+    if $stty_supported {
+	run_stty "stty after" stty_after
+
+	gdb_assert [string equal $stty_before $stty_after] $test
+    } else {
+	unsupported "$test (no stty)"
+    }
+
+    exit_shell
+}
+
+with_test_prefix "batch run" {
+    test_terminal_settings_preserved
+}
+
+with_test_prefix "cli exit" {
+    test_terminal_settings_preserved_after_cli_exit { }
+}
+
+with_test_prefix "cli exit after start cmd" {
+    test_terminal_settings_preserved_after_cli_exit { "start" }
+}
+
+with_test_prefix "cli exit after run cmd" {
+    test_terminal_settings_preserved_after_cli_exit { "run" }
+}
diff --git a/gdb/top.c b/gdb/top.c
index 1e30b1c..6df987a 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -66,6 +66,7 @@
 #include "cli-out.h"
 #include "tracepoint.h"
 #include "inf-loop.h"
+#include "tui/tui.h"
 
 extern void initialize_all_files (void);
 
@@ -1494,6 +1495,14 @@ quit_force (char *args, int from_tty)
   int exit_code = 0;
   struct qt_args qt;
 
+  /* Make sure that the terminal is left in the state we acquired it.  */
+  target_terminal_ours ();
+#if defined(TUI)
+  tui_disable ();
+#endif
+  if (async_command_editing_p)
+    gdb_disable_readline ();
+
   /* An optional expression may be used to cause gdb to terminate with the 
      value of that expression.  */
   if (args)
-- 
2.5.0.rc2.22.g69b1679.dirty

Re: [PATCH] Make sure terminal settings are restored before exiting

[Pedro Alves]

(Small request: I find it helpful when the proposed commit
log is always present in patch resubmissions, making patches
self-contained.)

> diff --git a/gdb/top.c b/gdb/top.c
> index 1e30b1c..6df987a 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -66,6 +66,7 @@
>  #include "cli-out.h"
>  #include "tracepoint.h"
>  #include "inf-loop.h"
> +#include "tui/tui.h"
>  
>  extern void initialize_all_files (void);
>  
> @@ -1494,6 +1495,14 @@ quit_force (char *args, int from_tty)
>    int exit_code = 0;
>    struct qt_args qt;
>  
> +  /* Make sure that the terminal is left in the state we acquired it.  */
> +  target_terminal_ours ();
> +#if defined(TUI)
> +  tui_disable ();
> +#endif
> +  if (async_command_editing_p)
> +    gdb_disable_readline ();
> +

Can you put these new bits in a separate function, and call it here?
That'll make it easier to reuse, or play with moving the call elsewhere,
if needed.  As bonus, if we do the latter, git blame on the function's contents
will still blame you.  :-)

OK with that change.

Thanks,
Pedro Alves

Re: [PATCH] Make sure terminal settings are restored before exiting

[Patrick Palka]

On Tue, Jul 28, 2015 at 7:53 PM, Pedro Alves <palves@redhat.com> wrote:
> (Small request: I find it helpful when the proposed commit
> log is always present in patch resubmissions, making patches
> self-contained.)

Sure, no problem.

>
>> diff --git a/gdb/top.c b/gdb/top.c
>> index 1e30b1c..6df987a 100644
>> --- a/gdb/top.c
>> +++ b/gdb/top.c
>> @@ -66,6 +66,7 @@
>>  #include "cli-out.h"
>>  #include "tracepoint.h"
>>  #include "inf-loop.h"
>> +#include "tui/tui.h"
>>
>>  extern void initialize_all_files (void);
>>
>> @@ -1494,6 +1495,14 @@ quit_force (char *args, int from_tty)
>>    int exit_code = 0;
>>    struct qt_args qt;
>>
>> +  /* Make sure that the terminal is left in the state we acquired it.  */
>> +  target_terminal_ours ();
>> +#if defined(TUI)
>> +  tui_disable ();
>> +#endif
>> +  if (async_command_editing_p)
>> +    gdb_disable_readline ();
>> +
>
> Can you put these new bits in a separate function, and call it here?
> That'll make it easier to reuse, or play with moving the call elsewhere,
> if needed.  As bonus, if we do the latter, git blame on the function's contents
> will still blame you.  :-)
>
> OK with that change.

Will do.

Re: [PATCH] Make sure terminal settings are restored before exiting

[Pedro Alves]

On 07/28/2015 04:18 AM, Patrick Palka wrote:
> When exiting GDB -- whether it's via the "quit" command, via a SIGTERM,
> or otherwise -- we should leave the terminal in the state we acquired
> it.  To that end, we have to undo any modifications that may have been
> made by the TUI (ncurses) or by the CLI (readline).
> 
> [ Note that we already take a snapshot of the original tty state and save
>   it to inflow.c:initial_gdb_ttystate.  Using this variable we can
>   define a new function restore_initial_gdb_ttystate and use it here.
>   We can replace the call to rl_deprep_terminal with such a function,
>   though it wouldn't hurt to have both around either.  Is this a good
>   idea?
> 
>   As far as testing goes, I am having trouble figuring out how to
>   retrieve the pid of the GDB subprocess in order to kill it via SIGTERM
>   with the subshell/stty approach used in
>   batch-preserve-term-settings.exp.  Seems non-trivial.  Any ideas?  ]

A few ideas:

#0 - We're starting it under a shell we control, so make use of that.
     Start gdb in the background "gdb &" and follow it with "echo $!" to get
     the pid, followed by "fg".  Starting the background might be tricky,
     so alternatively start it as usual and then send a C-z to background it,
     then "echo $!".

#1 - Run "shell ps" in gdb and extract the PID from the first column that has
     a line that matches *gdb*.

#2 - Do like gdb.server/server-kill.c, and the test's program store the parent's
  process id in a global variable with getppid().  You'll need to disable
  "set startup-with-shell", so that the process's direct parent is GDB.
  Read the global variable from gdb.

#3 - add a "maint print pid" command ...

#0 seems preferred.
#1 is hacky, but ... i've seen worse.
#2 only works with native testing.
#3 could be the most reliable...

Thanks,
Pedro Alves

Re: [PATCH] Make sure terminal settings are restored before exiting

[Patrick Palka]

On Tue, Jul 28, 2015 at 8:09 PM, Pedro Alves <palves@redhat.com> wrote:
> On 07/28/2015 04:18 AM, Patrick Palka wrote:
>> When exiting GDB -- whether it's via the "quit" command, via a SIGTERM,
>> or otherwise -- we should leave the terminal in the state we acquired
>> it.  To that end, we have to undo any modifications that may have been
>> made by the TUI (ncurses) or by the CLI (readline).
>>
>> [ Note that we already take a snapshot of the original tty state and save
>>   it to inflow.c:initial_gdb_ttystate.  Using this variable we can
>>   define a new function restore_initial_gdb_ttystate and use it here.
>>   We can replace the call to rl_deprep_terminal with such a function,
>>   though it wouldn't hurt to have both around either.  Is this a good
>>   idea?
>>
>>   As far as testing goes, I am having trouble figuring out how to
>>   retrieve the pid of the GDB subprocess in order to kill it via SIGTERM
>>   with the subshell/stty approach used in
>>   batch-preserve-term-settings.exp.  Seems non-trivial.  Any ideas?  ]
>
> A few ideas:
>
> #0 - We're starting it under a shell we control, so make use of that.
>      Start gdb in the background "gdb &" and follow it with "echo $!" to get
>      the pid, followed by "fg".  Starting the background might be tricky,
>      so alternatively start it as usual and then send a C-z to background it,
>      then "echo $!".
>
> #1 - Run "shell ps" in gdb and extract the PID from the first column that has
>      a line that matches *gdb*.
>
> #2 - Do like gdb.server/server-kill.c, and the test's program store the parent's
>   process id in a global variable with getppid().  You'll need to disable
>   "set startup-with-shell", so that the process's direct parent is GDB.
>   Read the global variable from gdb.
>
> #3 - add a "maint print pid" command ...
>
> #0 seems preferred.
> #1 is hacky, but ... i've seen worse.
> #2 only works with native testing.
> #3 could be the most reliable...

Nice list.

The #0 approach does not work for me.  When I suspend GDB with ^Z, the
shell variable $! remains empty.  It looks like you have to start the
process in the background to populate the $! variable, and starting
GDB in the background does not work too well.

Just very recently I discovered the $PPID environment variable, a
standardized (thus hopefully portable) variable that should contain
the pid of the parent process.  With it we can do "shell echo $PPID"
to print the gdb pid and then capture the output of this command with
expect.  I'll try this approach.

Re: [PATCH] Make sure terminal settings are restored before exiting

[Pedro Alves]

On 07/29/2015 12:37 PM, Patrick Palka wrote:

> Just very recently I discovered the $PPID environment variable, a
> standardized (thus hopefully portable) variable that should contain
> the pid of the parent process.  With it we can do "shell echo $PPID"
> to print the gdb pid and then capture the output of this command with
> expect.  I'll try this approach.

Good idea, sounds good.

Thanks,
Pedro Alves