Re: [PATCH] [7.8 regression] PR cli/17828: -batch -ex r breaks terminal

[Pedro Alves <palves at redhat dot com>]

On 01/12/2015 06:39 PM, Doug Evans wrote:
> 
> IWBN if the subtleties going on here were explained in the code.
> 
> Maybe something like
> 
>   /* Don't call gdb_rl_callback_handler_reinstall if !interpreter_async:
>      It will prep the terminal, readline-style (raw, noecho), which is not
>      what we want if we're not using readline (e.g., --batch).  */
> 
> I'm sure that can be improved on, but the point is this stuff is
> obviously subtle and readers should at least be given a bit of
> assistance (I'm all for more full explanations residing in the commit log).

Below's what I pushed, to master, 7.9 and 7.8.

>From 6c400b59d574782afdb26eb6c021f28c31bb9f1c Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 14 Jan 2015 11:51:06 +0000
Subject: [PATCH 1/2] PR cli/17828: -batch -ex r breaks terminal

Commit d3d4baed (PR python/17372 - Python hangs when displaying
help()) had the side effect of causing 'gdb -batch' to leave the
terminal in the wrong state if the program was run.  E.g,.

 $ echo 'main(){*(int*)0=0;}' | gcc -x c -; ./gdb/gdb -batch -ex r ./a.out
 Program received signal SIGSEGV, Segmentation fault.
 0x00000000004004ff in main ()
 $

If you start typing the next command, seemingly nothing happens - GDB
left the terminal with echo disabled.

The issue is that that "r" ends up in fetch_inferior_event, which
calls reinstall_readline_callback_handler_cleanup, which causes
readline to prep the terminal (raw, echo disabled).  But "-batch"
causes GDB to exit before the top level event loop is first started,
and then nothing de-preps the terminal.

The reinstall_readline_callback_handler_cleanup function's intro
comment mentions:

 "Need to do this as we go back to the event loop, ready to process
 further input."

but the implementation forgets the case of when the interpreter is
sync, which indicates we won't return to the event loop yet, or as in
the case of -batch, we have not started it yet.

The fix is to not install the readline callback in that case.

For the test, in this case, checking that command echo still works is
sufficient.  Comparing stty output before/after running GDB is even
better.  Because stty may not be available, the test tries both ways.
In any case, since expect's spawn (what we use to start gdb) creates a
new pseudo tty, another expect spawn or tcl exec after GDB exits would
not see the wrong terminal settings.  So instead, the test spawns a
shell and runs stty and GDB in it.

Tested on x86_64 Fedora 20.

gdb/
2015-01-14  Pedro Alves  <palves@redhat.com>

	PR cli/17828
	* infrun.c (reinstall_readline_callback_handler_cleanup): Don't
	reinstall if the interpreter is sync.

gdb/testsuite/
2015-01-14  Pedro Alves  <palves@redhat.com>

	PR cli/17828
	* gdb.base/batch-preserve-term-settings.c: New file.
	* gdb.base/batch-preserve-term-settings.exp: New file.
---
 gdb/ChangeLog                                      |   6 +
 gdb/testsuite/ChangeLog                            |   6 +
 gdb/infrun.c                                       |  10 ++
 .../gdb.base/batch-preserve-term-settings.c        |  22 +++
 .../gdb.base/batch-preserve-term-settings.exp      | 186 +++++++++++++++++++++
 5 files changed, 230 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/batch-preserve-term-settings.c
 create mode 100644 gdb/testsuite/gdb.base/batch-preserve-term-settings.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3431c2c..b84ffc8 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2015-01-14  Pedro Alves  <palves@redhat.com>
+
+	PR cli/17828
+	* infrun.c (reinstall_readline_callback_handler_cleanup): Don't
+	reinstall if the interpreter is sync.
+
 2015-01-13  Doug Evans  <dje@google.com>
 
 	* objfiles.c (objfile_filename): New function.
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index fa7ffa9..7e74501 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2015-01-14  Pedro Alves  <palves@redhat.com>
+
+	PR cli/17828
+	* gdb.base/batch-preserve-term-settings.c: New file.
+	* gdb.base/batch-preserve-term-settings.exp: New file.
+
 2015-01-13  Doug Evans  <dje@google.com>
 
 	* gdb.python/py-objfile.exp: Test gdb.lookup_objfile on symlinked
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 105862a..11dcc0e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3178,6 +3178,16 @@ wait_for_inferior (void)
 static void
 reinstall_readline_callback_handler_cleanup (void *arg)
 {
+  if (!interpreter_async)
+    {
+      /* We're not going back to the top level event loop yet.  Don't
+	 install the readline callback, as it'd prep the terminal,
+	 readline-style (raw, noecho) (e.g., --batch).  We'll install
+	 it the next time the prompt is displayed, when we're ready
+	 for input.  */
+      return;
+    }
+
   if (async_command_editing_p && !sync_execution)
     gdb_rl_callback_handler_reinstall ();
 }
diff --git a/gdb/testsuite/gdb.base/batch-preserve-term-settings.c b/gdb/testsuite/gdb.base/batch-preserve-term-settings.c
new file mode 100644
index 0000000..7e362b2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/batch-preserve-term-settings.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 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/>.  */
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/batch-preserve-term-settings.exp b/gdb/testsuite/gdb.base/batch-preserve-term-settings.exp
new file mode 100644
index 0000000..834e7a9
--- /dev/null
+++ b/gdb/testsuite/gdb.base/batch-preserve-term-settings.exp
@@ -0,0 +1,186 @@
+# Copyright (C) 2015 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/>.
+
+# Check that "gdb -batch -ex run" does not leave the terminal in the
+# wrong state.
+
+standard_testfile
+
+if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
+    return -1
+}
+
+set file_arg $binfile
+if [is_remote host] {
+  set file_arg [remote_download host $file_arg]
+}
+
+global GDBFLAGS
+set saved_gdbflags $GDBFLAGS
+
+# The shell's prompt.
+set shell_prompt "$ "
+set shell_prompt_re [string_to_regexp $shell_prompt]
+
+# Spawn shell.  Returns true on success, false otherwise.
+
+proc spawn_shell {} {
+    global shell_prompt_re
+
+    set res [remote_spawn host "/bin/sh"]
+    if { $res < 0 || $res == "" } {
+	unsupported "Spawning shell failed."
+	return 0
+    }
+
+    set gotit 0
+    set test "spawn shell"
+    gdb_expect {
+	-re "$shell_prompt_re$" {
+	    pass $test
+	    set gotit 1
+	}
+	timeout {
+	    fail "$test (timeout)"
+	}
+	eof {
+	    fail "$test (eof)"
+	}
+    }
+
+    return $gotit
+}
+
+# Exit the shell.
+
+proc exit_shell {} {
+    global shell_prompt_re
+
+    set test "exit shell"
+    send_gdb "exit\n"
+    gdb_expect {
+	timeout {
+	    fail "$test (timeout)"
+	    return 0
+	}
+	eof {
+	    pass "$test"
+	}
+    }
+    if ![is_remote host] {
+	remote_close host
+    }
+}
+
+# Run "stty" and store the output in $result.  Returns true on
+# success, false otherwise.
+
+proc run_stty {message result} {
+    global shell_prompt_re
+
+    upvar $result output
+
+    send_gdb "stty || echo \"not found\"\n"
+    set gotit 0
+    gdb_expect {
+	-re "not found.*not found.*$shell_prompt_re$" {
+	    pass "$message (not found)"
+	}
+	-re "(.*)$shell_prompt_re$" {
+	    set output $expect_out(1,string)
+	    set gotit 1
+	    pass $message
+	}
+	timeout {
+	    fail "$message (timeout)"
+	}
+	eof {
+	    fail "$message (eof)"
+	}
+    }
+    return $gotit
+}
+
+# Check that "gdb -batch -ex run" does not leave the terminal in the
+# wrong state.
+
+proc test_terminal_settings_preserved {} {
+    global file_arg
+    global GDB INTERNAL_GDBFLAGS GDBFLAGS
+    global gdb_prompt pagination_prompt
+    global saved_gdbflags
+    global shell_prompt_re
+
+    if ![spawn_shell] {
+	return
+    }
+
+    set stty_supported [run_stty "stty before" stty_before]
+
+    set test "gdb -batch -ex run"
+    set GDBFLAGS $saved_gdbflags
+    append GDBFLAGS " -batch"
+    append GDBFLAGS " -ex \"set height unlimited\""
+    append GDBFLAGS " -ex \"start\""
+    append GDBFLAGS " --args \"$file_arg\""
+    send_gdb "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts]\n"
+    gdb_expect {
+	-re "Don't know how to run.*$shell_prompt_re$" {
+	    unsupported $test
+	}
+	-re "$gdb_prompt $" {
+	    # -batch implies no GDB prompt.
+	    fail $test
+	}
+	-re "Temporary breakpoint .*$shell_prompt_re$" {
+	    pass $test
+	}
+	timeout {
+	    fail "$test (timeout)"
+	}
+	eof {
+	    fail "$test (eof)"
+	}
+    }
+
+    set test "echo test_echo"
+    send_gdb "echo test_echo\n"
+    gdb_expect {
+	-re "^echo test_echo\r\ntest_echo\r\n.*$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
+}
+
+test_terminal_settings_preserved
+
+set GDBFLAGS $saved_gdbflags
-- 
1.9.3