Make sure terminal settings are restored before exiting

Message ID 1438053504-21507-1-git-send-email-patrick@parcs.ath.cx
State New, archived
Headers

Commit Message

Patrick Palka July 28, 2015, 3:18 a.m. UTC
  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(+)
  

Comments

Pedro Alves July 29, 2015, 12:09 a.m. UTC | #1
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
  
Patrick Palka July 29, 2015, 11:37 a.m. UTC | #2
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.
  
Pedro Alves July 29, 2015, 12:15 p.m. UTC | #3
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
  

Patch

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)