[2/2] 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
---
 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
  

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