[review] gdb: Enable stdin on exception in execute_gdb_command

[review] gdb: Enable stdin on exception in execute_gdb_command

[Andrew Burgess (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/701
......................................................................

gdb: Enable stdin on exception in execute_gdb_command

This is an update of this patch:

  https://sourceware.org/ml/gdb-patches/2018-09/msg00884.html

This patch attempts to address PR gdb/23718 by re-enabling stdin
whenever an exception is caught during gdb.execute().

When Python gdb.execute() is called, an exception could occur (e.g. the
target disappearing), which is then converted into a Python exception.  If
stdin was disabled before the exception is caught, it is not re-enabled,
because the exception doesn't propagate to the top level of the event loop,
whose catch block would otherwise enable it.

The result is that when execution of a Python script completes, GDB does
not prompt or accept input, and is effectively hung.

This change rectifies the issue by re-enabling stdin in the catch block of
execute_gdb_command, prior to converting the exception to a Python
exception.

Since this patch was originally posted I've added a test, and also I
converted the code to re-enable stdin from this:

  SWITCH_THRU_ALL_UIS ()
    {
      async_enable_stdin ();
    }

to simply this:

  async_enable_stdin ();

My reasoning is that we only need the SWITCH_THRU_ALL_UIS if, at the time
the exception is caught, the current_ui might be different than at the time
we called async_disable_stdin.  Within python's execute_gdb_command I think
it should be impossible to switch current_ui, so the SWITCH_THRU_ALL_UIS
isn't needed.

gdb/ChangeLog:

	PR gdb/23718
	* gdb/python/python.c (execute_gdb_command): Call
	async_enable_stdin in catch block.

gdb/testsuite/ChangeLog:

        PR gdb/23718
	* gdb.server/server-kill-python.exp: New file.

Change-Id: I1cfc36ee9f8484cc1ed82be9be338353db6bc080
---
M gdb/ChangeLog
M gdb/python/python.c
M gdb/testsuite/ChangeLog
A gdb/testsuite/gdb.server/server-kill-python.exp
4 files changed, 98 insertions(+), 0 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index aa7e5db..3675088 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-11-22  Graham Markall  <graham.markall@embecosm.com>
+
+	PR gdb/23718
+	* gdb/python/python.c (execute_gdb_command): Call
+	async_enable_stdin in catch block.
+
 2019-11-22  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* event-loop.c (start_event_loop): Wrap async_enable_stdin with
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 7b561a1..324b031 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -621,6 +621,12 @@
     }
   catch (const gdb_exception &except)
     {
+      /* If an exception occurred then we won't hit normal_stop (), or have
+         an exception reach the top level of the event loop, which are the
+         two usual places in which stdin would be re-enabled. So, before we
+         convert the exception and continue back in Python, we should
+         re-enable stdin here.  */
+      async_enable_stdin ();
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 3d2c4c4..692eecf 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@
 2019-11-22  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	PR gdb/23718
+	* gdb.server/server-kill-python.exp: New file.
+
+2019-11-22  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* gdb.server/multi-ui-errors.c: New file.
 	* gdb.server/multi-ui-errors.exp: New file.
 
diff --git a/gdb/testsuite/gdb.server/server-kill-python.exp b/gdb/testsuite/gdb.server/server-kill-python.exp
new file mode 100644
index 0000000..0130459
--- /dev/null
+++ b/gdb/testsuite/gdb.server/server-kill-python.exp
@@ -0,0 +1,81 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2019 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/>.
+
+# This test script exposes a bug where, if gdbserver dies while GDB is
+# sourcing a python command like 'gdb.execute ("continue")', then GDB
+# will deadlock.
+
+load_lib gdbserver-support.exp
+
+standard_testfile multi-ui-errors.c
+
+if {[skip_gdbserver_tests]} {
+    return 0
+}
+
+if {[build_executable "failed to prepare" ${testfile} \
+	 ${srcfile}] == -1} {
+    return -1
+}
+
+# Start gdbserver.
+set res [gdbserver_spawn "${binfile}"]
+set gdbserver_protocol [lindex $res 0]
+set gdbserver_gdbport [lindex $res 1]
+
+# Generate a python script we will later source.
+set file1 [standard_output_file file1.py]
+set fd [open "$file1" w]
+puts $fd \
+"import gdb
+
+def do_gdb_stuff ():
+    gdb.execute ('target $gdbserver_protocol $gdbserver_gdbport')
+    gdb.execute ('continue')
+
+do_gdb_stuff()"
+close $fd
+
+# Figure out the pid for the gdbserver, and arrange to kill it in a
+# short while.
+set gdbserver_pid [exp_pid -i $server_spawn_id]
+after 1000 { remote_exec target "kill -9 $gdbserver_pid" }
+
+# Now start GDB, sourcing the python command file we generated above.
+# Set the height and width so we don't end up at a paging prompt.
+if {[gdb_spawn_with_cmdline_opts \
+	 "-quiet -iex \"set height 0\" -iex \"set width 0\" -ex \"source $file1\""] != 0} {
+    fail "spawn"
+    return
+}
+
+# Expect that we get back to a GDB prompt.  We can't use
+# gdb_test_multiple here as we don't need to send a command to GDB;
+# the script we source at startup issues a command for us.  Here we
+# really are waiting for GDB to give us back a prompt.
+set testname "landed at prompt after gdbserver dies"
+gdb_expect 10 {
+    -re "$gdb_prompt $" {
+	pass $testname
+    }
+    timeout {
+	fail "$testname (timeout)"
+    }
+}
+
+# Run a simple command to ensure we can interact with GDB.
+gdb_test "echo hello\\n" "hello" "can we interact with gdb"

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I1cfc36ee9f8484cc1ed82be9be338353db6bc080
Gerrit-Change-Number: 701
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-MessageType: newchange

[review v2] gdb: Enable stdin on exception in execute_gdb_command

[Andrew Burgess (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/701
......................................................................

gdb: Enable stdin on exception in execute_gdb_command

This is an update of this patch:

  https://sourceware.org/ml/gdb-patches/2018-09/msg00884.html

This patch attempts to address PR gdb/23718 by re-enabling stdin
whenever an exception is caught during gdb.execute().

When Python gdb.execute() is called, an exception could occur (e.g. the
target disappearing), which is then converted into a Python exception.  If
stdin was disabled before the exception is caught, it is not re-enabled,
because the exception doesn't propagate to the top level of the event loop,
whose catch block would otherwise enable it.

The result is that when execution of a Python script completes, GDB does
not prompt or accept input, and is effectively hung.

This change rectifies the issue by re-enabling stdin in the catch block of
execute_gdb_command, prior to converting the exception to a Python
exception.

Since this patch was originally posted I've added a test, and also I
converted the code to re-enable stdin from this:

  SWITCH_THRU_ALL_UIS ()
    {
      async_enable_stdin ();
    }

to simply this:

  async_enable_stdin ();

My reasoning is that we only need the SWITCH_THRU_ALL_UIS if, at the time
the exception is caught, the current_ui might be different than at the time
we called async_disable_stdin.  Within python's execute_gdb_command I think
it should be impossible to switch current_ui, so the SWITCH_THRU_ALL_UIS
isn't needed.

gdb/ChangeLog:

	PR gdb/23718
	* gdb/python/python.c (execute_gdb_command): Call
	async_enable_stdin in catch block.

gdb/testsuite/ChangeLog:

        PR gdb/23718
	* gdb.server/server-kill-python.exp: New file.

Change-Id: I1cfc36ee9f8484cc1ed82be9be338353db6bc080
---
M gdb/ChangeLog
M gdb/python/python.c
M gdb/testsuite/ChangeLog
A gdb/testsuite/gdb.server/server-kill-python.exp
4 files changed, 98 insertions(+), 0 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3b4afc8..e68591b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-12-06  Graham Markall  <graham.markall@embecosm.com>
+
+	PR gdb/23718
+	* gdb/python/python.c (execute_gdb_command): Call
+	async_enable_stdin in catch block.
+
 2019-12-06  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* event-loop.c (start_event_loop): Wrap async_enable_stdin with
diff --git a/gdb/python/python.c b/gdb/python/python.c
index fad54e9..65ccc40 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -620,6 +620,12 @@
     }
   catch (const gdb_exception &except)
     {
+      /* If an exception occurred then we won't hit normal_stop (), or have
+	 an exception reach the top level of the event loop, which are the
+	 two usual places in which stdin would be re-enabled. So, before we
+	 convert the exception and continue back in Python, we should
+	 re-enable stdin here.  */
+      async_enable_stdin ();
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index d3666d9..5b19ddd 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@
 2019-12-06  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	PR gdb/23718
+	* gdb.server/server-kill-python.exp: New file.
+
+2019-12-06  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* gdb.server/multi-ui-errors.c: New file.
 	* gdb.server/multi-ui-errors.exp: New file.
 
diff --git a/gdb/testsuite/gdb.server/server-kill-python.exp b/gdb/testsuite/gdb.server/server-kill-python.exp
new file mode 100644
index 0000000..0130459
--- /dev/null
+++ b/gdb/testsuite/gdb.server/server-kill-python.exp
@@ -0,0 +1,81 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2019 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/>.
+
+# This test script exposes a bug where, if gdbserver dies while GDB is
+# sourcing a python command like 'gdb.execute ("continue")', then GDB
+# will deadlock.
+
+load_lib gdbserver-support.exp
+
+standard_testfile multi-ui-errors.c
+
+if {[skip_gdbserver_tests]} {
+    return 0
+}
+
+if {[build_executable "failed to prepare" ${testfile} \
+	 ${srcfile}] == -1} {
+    return -1
+}
+
+# Start gdbserver.
+set res [gdbserver_spawn "${binfile}"]
+set gdbserver_protocol [lindex $res 0]
+set gdbserver_gdbport [lindex $res 1]
+
+# Generate a python script we will later source.
+set file1 [standard_output_file file1.py]
+set fd [open "$file1" w]
+puts $fd \
+"import gdb
+
+def do_gdb_stuff ():
+    gdb.execute ('target $gdbserver_protocol $gdbserver_gdbport')
+    gdb.execute ('continue')
+
+do_gdb_stuff()"
+close $fd
+
+# Figure out the pid for the gdbserver, and arrange to kill it in a
+# short while.
+set gdbserver_pid [exp_pid -i $server_spawn_id]
+after 1000 { remote_exec target "kill -9 $gdbserver_pid" }
+
+# Now start GDB, sourcing the python command file we generated above.
+# Set the height and width so we don't end up at a paging prompt.
+if {[gdb_spawn_with_cmdline_opts \
+	 "-quiet -iex \"set height 0\" -iex \"set width 0\" -ex \"source $file1\""] != 0} {
+    fail "spawn"
+    return
+}
+
+# Expect that we get back to a GDB prompt.  We can't use
+# gdb_test_multiple here as we don't need to send a command to GDB;
+# the script we source at startup issues a command for us.  Here we
+# really are waiting for GDB to give us back a prompt.
+set testname "landed at prompt after gdbserver dies"
+gdb_expect 10 {
+    -re "$gdb_prompt $" {
+	pass $testname
+    }
+    timeout {
+	fail "$testname (timeout)"
+    }
+}
+
+# Run a simple command to ensure we can interact with GDB.
+gdb_test "echo hello\\n" "hello" "can we interact with gdb"

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I1cfc36ee9f8484cc1ed82be9be338353db6bc080
Gerrit-Change-Number: 701
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-MessageType: newpatchset

[review v2] gdb: Enable stdin on exception in execute_gdb_command

[Andrew Burgess (Code Review)]

Andrew Burgess has abandoned this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/701 )

Change subject: gdb: Enable stdin on exception in execute_gdb_command
......................................................................


Abandoned

I've now posted this to the mailing list.
-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I1cfc36ee9f8484cc1ed82be9be338353db6bc080
Gerrit-Change-Number: 701
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-MessageType: abandon