[PATCH 2/2] gdb: Enable stdin on exception in execute_gdb_command

[PATCH 2/2] gdb: Enable stdin on exception in execute_gdb_command

[Andrew Burgess]

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
---
 gdb/ChangeLog                                   |  6 ++
 gdb/python/python.c                             |  6 ++
 gdb/testsuite/ChangeLog                         |  5 ++
 gdb/testsuite/gdb.server/server-kill-python.exp | 81 +++++++++++++++++++++++++
 4 files changed, 98 insertions(+)
 create mode 100644 gdb/testsuite/gdb.server/server-kill-python.exp

diff --git a/gdb/python/python.c b/gdb/python/python.c
index fad54e9cdb0..65ccc404b15 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -620,6 +620,12 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
     }
   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/gdb.server/server-kill-python.exp b/gdb/testsuite/gdb.server/server-kill-python.exp
new file mode 100644
index 00000000000..0130459fce4
--- /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"
-- 
2.14.5

[PATCH 0/2] Re-enable stdin in various places

[Andrew Burgess]

This mailing list post replaces two patches that were on gerrit:

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

There's no change here except I rebased and retested on current head.

All feedback welcome.

Thanks,
Andrew

---

Andrew Burgess (2):
  gdb: Re-enable stdin for all UIs from start_event_loop
  gdb: Enable stdin on exception in execute_gdb_command

 gdb/ChangeLog                                   |  11 +++
 gdb/event-loop.c                                |   5 +-
 gdb/python/python.c                             |   6 ++
 gdb/testsuite/ChangeLog                         |  10 +++
 gdb/testsuite/gdb.server/multi-ui-errors.c      |  30 +++++++
 gdb/testsuite/gdb.server/multi-ui-errors.exp    | 107 ++++++++++++++++++++++++
 gdb/testsuite/gdb.server/server-kill-python.exp |  81 ++++++++++++++++++
 7 files changed, 249 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.c
 create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.exp
 create mode 100644 gdb/testsuite/gdb.server/server-kill-python.exp

-- 
2.14.5

[PATCH 1/2] gdb: Re-enable stdin for all UIs from start_event_loop

[Andrew Burgess]

If we catch an exception in start_event_loop's call to
gdb_do_one_event, then it is possible that the current_ui has changed
since we called async_disable_stdin.  If that's the case then calling
async_enable_stdin will be called on the wrong UI.

To solve this problem we wrap the call to async_enable_stdin with
SWITCH_THRU_ALL_UIS, this causes us to try and re-enable stdin for all
UIs, which will catch any for which we called async_disable_stdin.

gdb/ChangeLog:

	* event-loop.c (start_event_loop): Wrap async_enable_stdin with
	SWITCH_THRU_ALL_UIS.

gdb/testsuite/ChangeLog:

	* gdb.server/multi-ui-errors.c: New file.
	* gdb.server/multi-ui-errors.exp: New file.

Change-Id: I1e18deff2e6f4e17f7a13adce3553eb001cad93b
---
 gdb/ChangeLog                                |   5 ++
 gdb/event-loop.c                             |   5 +-
 gdb/testsuite/ChangeLog                      |   5 ++
 gdb/testsuite/gdb.server/multi-ui-errors.c   |  30 ++++++++
 gdb/testsuite/gdb.server/multi-ui-errors.exp | 107 +++++++++++++++++++++++++++
 5 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.c
 create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.exp

diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index affa00b4aa9..b9444153ff8 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -376,7 +376,10 @@ start_event_loop (void)
 	  /* If any exception escaped to here, we better enable
 	     stdin.  Otherwise, any command that calls async_disable_stdin,
 	     and then throws, will leave stdin inoperable.  */
-	  async_enable_stdin ();
+	  SWITCH_THRU_ALL_UIS ()
+	    {
+	      async_enable_stdin ();
+	    }
 	  /* If we long-jumped out of do_one_event, we probably didn't
 	     get around to resetting the prompt, which leaves readline
 	     in a messed-up state.  Reset it here.  */
diff --git a/gdb/testsuite/gdb.server/multi-ui-errors.c b/gdb/testsuite/gdb.server/multi-ui-errors.c
new file mode 100644
index 00000000000..0e6c0ea0fde
--- /dev/null
+++ b/gdb/testsuite/gdb.server/multi-ui-errors.c
@@ -0,0 +1,30 @@
+/* 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/>.  */
+
+#include <stdio.h>
+#include <unistd.h>
+
+int
+main (void)
+{
+  printf ("@@XX@@ Inferior Starting @@XX@@\n");
+
+  while (1)
+    sleep (1);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.server/multi-ui-errors.exp b/gdb/testsuite/gdb.server/multi-ui-errors.exp
new file mode 100644
index 00000000000..5162a1a6f9c
--- /dev/null
+++ b/gdb/testsuite/gdb.server/multi-ui-errors.exp
@@ -0,0 +1,107 @@
+# 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/>.
+
+# Test what happens if we have multiple UIs in use, and an error
+# occurs while running a GDB command.  Specifically, do both UIs
+# return to an interactive state, or does one (or both) of them get
+# stuck in a non-interactive state.
+
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+if {[skip_gdbserver_tests]} {
+    return 0
+}
+
+if {[build_executable "failed to prepare" ${testfile} \
+	 ${srcfile}] == -1} {
+    return -1
+}
+
+clean_restart $testfile
+
+# Start gdbserver.
+set res [gdbserver_spawn "${binfile}"]
+set gdbserver_protocol [lindex $res 0]
+set gdbserver_gdbport [lindex $res 1]
+set gdbserver_pid [exp_pid -i $server_spawn_id]
+
+# Save the main UI's spawn ID.
+set gdb_main_spawn_id $gdb_spawn_id
+
+# Create the new PTY for the secondary console UI, issue the 'new-ui'
+# command, and wait for a prompt on the second 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 $" {
+	pass $gdb_test_name
+    }
+}
+with_spawn_id $extra_spawn_id {
+    gdb_test_multiple "" "initial prompt on extra console" {
+	-re "$gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+    }
+}
+
+# Connect to the remote and continue its execution from the other UI.
+with_spawn_id $extra_spawn_id {
+    gdb_test "target $gdbserver_protocol $gdbserver_gdbport" ".*"
+    send_gdb "continue\n"
+}
+
+# We're going to kill the gdbserver, but before we do, lets make sure
+# that the inferior has started executing.
+with_spawn_id $server_spawn_id {
+    gdb_expect 3 {
+	-re "@@XX@@ Inferior Starting @@XX@@" {
+	    verbose -log "APB: Spotted running pattern"
+	}
+	timeout {
+	    verbose -log "APB: Didn't find running message"
+	}
+    }
+}
+
+# Interact with the main UI.
+with_spawn_id $gdb_main_spawn_id {
+    gdb_test "echo hello\\n" "hello" "interact with GDB's main UI"
+}
+
+# Now kill the gdbserver.
+remote_exec target "kill -9 $gdbserver_pid"
+
+# We expect to land back at a GDB prompt in both UIs.  Right now there
+# seems to be some "issues" with the prompt automatically being
+# displayed... Or maybe this is intentional and I'm missunderstanding
+# the expected behaviour.  Either way, getting a prompt isn't the
+# point of this test.  The point is that we should not be able to
+# interact with GDB from either interpreter now.
+
+with_spawn_id $gdb_main_spawn_id {
+    gdb_test "echo" "" \
+	"main UI, prompt after gdbserver dies"
+}
+
+with_spawn_id $extra_spawn_id {
+    gdb_test "echo" "" \
+	"extra UI, prompt after gdbserver dies"
+}
-- 
2.14.5

Re: [PATCH 0/2] Re-enable stdin in various places

[Andrew Burgess]

Ping!

Thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2019-12-23 01:45:37 +0000]:

> This mailing list post replaces two patches that were on gerrit:
> 
>   https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/700
>   https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/701
> 
> There's no change here except I rebased and retested on current head.
> 
> All feedback welcome.
> 
> Thanks,
> Andrew
> 
> ---
> 
> Andrew Burgess (2):
>   gdb: Re-enable stdin for all UIs from start_event_loop
>   gdb: Enable stdin on exception in execute_gdb_command
> 
>  gdb/ChangeLog                                   |  11 +++
>  gdb/event-loop.c                                |   5 +-
>  gdb/python/python.c                             |   6 ++
>  gdb/testsuite/ChangeLog                         |  10 +++
>  gdb/testsuite/gdb.server/multi-ui-errors.c      |  30 +++++++
>  gdb/testsuite/gdb.server/multi-ui-errors.exp    | 107 ++++++++++++++++++++++++
>  gdb/testsuite/gdb.server/server-kill-python.exp |  81 ++++++++++++++++++
>  7 files changed, 249 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.c
>  create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.exp
>  create mode 100644 gdb/testsuite/gdb.server/server-kill-python.exp
> 
> -- 
> 2.14.5
> 

Re: [PATCH 1/2] gdb: Re-enable stdin for all UIs from start_event_loop

[Pedro Alves]

This looks good to me, pending a few comments below.

On 12/23/19 1:45 AM, Andrew Burgess wrote:
> If we catch an exception in start_event_loop's call to
> gdb_do_one_event, then it is possible that the current_ui has changed
> since we called async_disable_stdin.  If that's the case then calling
> async_enable_stdin will be called on the wrong UI.
> 
> To solve this problem we wrap the call to async_enable_stdin with
> SWITCH_THRU_ALL_UIS, this causes us to try and re-enable stdin for all
> UIs, which will catch any for which we called async_disable_stdin.
> 
> gdb/ChangeLog:
> 
> 	* event-loop.c (start_event_loop): Wrap async_enable_stdin with
> 	SWITCH_THRU_ALL_UIS.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.server/multi-ui-errors.c: New file.
> 	* gdb.server/multi-ui-errors.exp: New file.
> 

The test fails when run against --target_board=native-extended-gdbserver:

 target extended-remote localhost:2348
 Already connected to a remote target.  Disconnect? (y or n) [answered Y; input not from terminal]
 n
 Remote debugging using localhost:2348
 Reading symbols from /lib64/ld-linux-x86-64.so.2...
 Reading symbols from /usr/lib/debug/usr/lib64/ld-2.26.so.debug...
 0x00007ffff7dd8ed0 in _start () from /lib64/ld-linux-x86-64.so.2
 (gdb) FAIL: gdb.server/multi-ui-errors.exp: target extended-remote localhost:2348 (got interactive prompt)

Looks like it's just missing the standard "disconnect" most
gdb.server/ tests do.

> Change-Id: I1e18deff2e6f4e17f7a13adce3553eb001cad93b
> ---
>  gdb/ChangeLog                                |   5 ++
>  gdb/event-loop.c                             |   5 +-
>  gdb/testsuite/ChangeLog                      |   5 ++
>  gdb/testsuite/gdb.server/multi-ui-errors.c   |  30 ++++++++
>  gdb/testsuite/gdb.server/multi-ui-errors.exp | 107 +++++++++++++++++++++++++++
>  5 files changed, 151 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.c
>  create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.exp
> 
> diff --git a/gdb/event-loop.c b/gdb/event-loop.c
> index affa00b4aa9..b9444153ff8 100644
> --- a/gdb/event-loop.c
> +++ b/gdb/event-loop.c
> @@ -376,7 +376,10 @@ start_event_loop (void)
>  	  /* If any exception escaped to here, we better enable
>  	     stdin.  Otherwise, any command that calls async_disable_stdin,
>  	     and then throws, will leave stdin inoperable.  */
> -	  async_enable_stdin ();
> +	  SWITCH_THRU_ALL_UIS ()
> +	    {
> +	      async_enable_stdin ();
> +	    }
>  	  /* If we long-jumped out of do_one_event, we probably didn't
>  	     get around to resetting the prompt, which leaves readline
>  	     in a messed-up state.  Reset it here.  */
> diff --git a/gdb/testsuite/gdb.server/multi-ui-errors.c b/gdb/testsuite/gdb.server/multi-ui-errors.c
> new file mode 100644
> index 00000000000..0e6c0ea0fde
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/multi-ui-errors.c
> @@ -0,0 +1,30 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2019 Free Software Foundation, Inc.

Remember to update this to "2019-2020".

> +
> +   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/>.  */
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +
> +int
> +main (void)
> +{
> +  printf ("@@XX@@ Inferior Starting @@XX@@\n");
> +
> +  while (1)
> +    sleep (1);

Avoid infinite loops.  For example, replace the "while" with
a "for" loop that counts 60 seconds.

> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.server/multi-ui-errors.exp b/gdb/testsuite/gdb.server/multi-ui-errors.exp
> new file mode 100644
> index 00000000000..5162a1a6f9c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/multi-ui-errors.exp
> @@ -0,0 +1,107 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2019 Free Software Foundation, Inc.

Ditto.

> +#
> +# 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/>.
> +
> +# Test what happens if we have multiple UIs in use, and an error
> +# occurs while running a GDB command.  Specifically, do both UIs
> +# return to an interactive state, or does one (or both) of them get
> +# stuck in a non-interactive state.
> +
> +load_lib gdbserver-support.exp
> +
> +standard_testfile
> +
> +if {[skip_gdbserver_tests]} {
> +    return 0
> +}
> +
> +if {[build_executable "failed to prepare" ${testfile} \
> +	 ${srcfile}] == -1} {
> +    return -1
> +}
> +
> +clean_restart $testfile

Use prepare_for_testing ?

> +
> +# Start gdbserver.
> +set res [gdbserver_spawn "${binfile}"]
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +set gdbserver_pid [exp_pid -i $server_spawn_id]
> +
> +# Save the main UI's spawn ID.
> +set gdb_main_spawn_id $gdb_spawn_id
> +
> +# Create the new PTY for the secondary console UI, issue the 'new-ui'
> +# command, and wait for a prompt on the second 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 $" {
> +	pass $gdb_test_name
> +    }
> +}
> +with_spawn_id $extra_spawn_id {
> +    gdb_test_multiple "" "initial prompt on extra console" {
> +	-re "$gdb_prompt $" {
> +	    pass $gdb_test_name
> +	}
> +    }
> +}
> +
> +# Connect to the remote and continue its execution from the other UI.
> +with_spawn_id $extra_spawn_id {
> +    gdb_test "target $gdbserver_protocol $gdbserver_gdbport" ".*"
> +    send_gdb "continue\n"
> +}
> +
> +# We're going to kill the gdbserver, but before we do, lets make sure
> +# that the inferior has started executing.
> +with_spawn_id $server_spawn_id {
> +    gdb_expect 3 {
> +	-re "@@XX@@ Inferior Starting @@XX@@" {
> +	    verbose -log "APB: Spotted running pattern"
> +	}
> +	timeout {
> +	    verbose -log "APB: Didn't find running message"
> +	}
> +    }
> +}
> +
> +# Interact with the main UI.
> +with_spawn_id $gdb_main_spawn_id {
> +    gdb_test "echo hello\\n" "hello" "interact with GDB's main UI"
> +}
> +
> +# Now kill the gdbserver.
> +remote_exec target "kill -9 $gdbserver_pid"
> +
> +# We expect to land back at a GDB prompt in both UIs.  Right now there
> +# seems to be some "issues" with the prompt automatically being
> +# displayed... Or maybe this is intentional and I'm missunderstanding
> +# the expected behaviour.  

Please avoid "I" in comments.  Can you clarify what you mean by "issues"?

> +# Either way, getting a prompt isn't the
> +# point of this test.  The point is that we should not be able to
> +# interact with GDB from either interpreter now.

I guess you meant "should" instead of "should not"?

> +
> +with_spawn_id $gdb_main_spawn_id {
> +    gdb_test "echo" "" \
> +	"main UI, prompt after gdbserver dies"
> +}
> +
> +with_spawn_id $extra_spawn_id {
> +    gdb_test "echo" "" \
> +	"extra UI, prompt after gdbserver dies"
> +}
> 

Thanks,
Pedro Alves

Re: [PATCH 2/2] gdb: Enable stdin on exception in execute_gdb_command

[Pedro Alves]

Hi,

This is largely OK, but see comments below.

On 12/23/19 1:45 AM, Andrew Burgess wrote:
> 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
> ---
>  gdb/ChangeLog                                   |  6 ++
>  gdb/python/python.c                             |  6 ++
>  gdb/testsuite/ChangeLog                         |  5 ++
>  gdb/testsuite/gdb.server/server-kill-python.exp | 81 +++++++++++++++++++++++++
>  4 files changed, 98 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.server/server-kill-python.exp
> 
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index fad54e9cdb0..65ccc404b15 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -620,6 +620,12 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
>      }
>    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);

I suppose this is OK-ish and better than the current state, so I'm
OK with going with it.

But, I'm really not sure about it.  The whole "exception thrown from inside
target_wait" business is kind of not very robust.  E.g., should a target close
error really be converted to a new TARGET_WAITKIND_CLOSED, and handled as another
event with its own set of transitions?  Like, in all-stop, stop all threads of
all targets, before presenting the event to the user.  It is kind of
a process exit event.  Anyway, something for some other time.  It's definitely
an improvement to avoid GDB getting stuck.  Plus we have a test now, which
adds good value.

>      }
>  
> 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 00000000000..0130459fce4
> --- /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.

Remember dates here too.

> +#
> +# 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" }

This 1000ms here looks racy.  I don't think it's going to be that
rare for gdb to take that long to start, especially on slower systems.

Can't that kill be done with "shell kill" from inside the python script?

> +
> +# 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.

I'm not sure I follow why we can't use gdb_test_multiple here.
gdb_test_multiple with an empty GDB command is frequently used
whenever you don't want to send a command to GDB.

> +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"

Thanks,
Pedro Alves