Re: [PATCH v2] gdb, python: selectively omit enabling stdin in gdb.execute exception

[Bruno Larsen <blarsen@redhat.com>]

On 27/02/2023 10:56, Tankut Baris Aktemur via Gdb-patches wrote:
>  From the Python API, we can execute GDB commands via gdb.execute.  If
> the command gives an exception, however, we need to recover the GDB
> prompt and enable stdin, because the exception does not reach
> top-level GDB or normal_stop.  This was done in commit
>
>    commit 1ba1ac88011703abcd0271e4f5d00927dc69a09a
>    Author: Andrew Burgess <andrew.burgess@embecosm.com>
>    Date:   Tue Nov 19 11:17:20 2019 +0000
>
>      gdb: Enable stdin on exception in execute_gdb_command
>
> However, we face a glitch if the Python side executes the command in a
> context where GDB had already disabled stdin, because it was running a
> synchronous execution command such as "continue" or "run".  As an
> example, suppose we have the following objfile event listener,
> specified in a file named file.py:
>
> ~~~
> import gdb
>
> class MyListener:
>      def __init__(self):
>          gdb.events.new_objfile.connect(self.handle_new_objfile_event)
>          self.processed_objfile = False
>
>      def handle_new_objfile_event(self, event):
>          if self.processed_objfile:
>              return
>
>          print("loading " + event.new_objfile.filename)
>          self.processed_objfile = True
>          gdb.execute("print a")
>
> the_listener = MyListener()
> ~~~
>
> The executed command "print a", gives an error because "a" is not
> defined.  We use the listener as follows:
>
>    $ gdb -q -ex "source file.py" -ex "run" --args a.out
>    Reading symbols from /tmp/a.out...
>    Starting program: /tmp/a.out
>    loading /lib64/ld-linux-x86-64.so.2
>    Python Exception <class 'gdb.error'>: No symbol "a" in current context.
>    (gdb) [Inferior 1 (process 3980401) exited normally]
>
> Note how the GDB prompt comes inbetween the exception message and the
> inferior's exit message.  We have this obscure behavior, because GDB
> continues to execute its flow after emitting the Python event.  In
> this case, GDB would enable stdin in the normal way.  Hence, we do not
> need to explicitly enable stdin in execute_gdb_command when an
> exception occurs.
>
> As a solution, we track whether the prompt was already blocked.  If so,
> we leave enabling stdin to GDB.
>
> With this patch, we see
>
>    $ gdb -q -ex "source file.py" -ex "run" --args a.out
>    Reading symbols from /tmp/a.out...
>    Starting program: /tmp/a.out
>    loading /lib64/ld-linux-x86-64.so.2
>    Python Exception <class 'gdb.error'>: No symbol "a" in current context.
>    [Inferior 1 (process 3984511) exited normally]
>    (gdb)
>
> Regression-tested on X86_64 Linux using the default board file (i.e.  unix).
>
> Co-Authored-By: Oguzhan Karakaya <oguzhan.karakaya@intel.com>
> ---
>   gdb/python/python.c                           | 26 ++++++++++-
>   gdb/testsuite/gdb.python/py-cmd-exception.exp | 43 +++++++++++++++++++
>   gdb/testsuite/gdb.python/py-cmd-exception.py  | 33 ++++++++++++++
>   3 files changed, 100 insertions(+), 2 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.python/py-cmd-exception.exp
>   create mode 100644 gdb/testsuite/gdb.python/py-cmd-exception.py
>
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 1ed13f2789b..9a0cc1e0d4e 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -653,6 +653,11 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
>   
>     scoped_restore preventer = prevent_dont_repeat ();
>   
> +  /* If the executed command raises an exception, we may have to
> +     enable stdin and recover the GDB prompt.  Check the current
> +     state.  */
> +  bool prompt_was_blocked = (current_ui->prompt_state == PROMPT_BLOCKED);
> +
>     try
>       {
>         gdbpy_allow_threads allow_threads;
> @@ -700,8 +705,25 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
>   	 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 ();
> +	 re-enable stdin here, unless the prompt was already blocked before
> +	 we started executing the command.  This could be the case, for

I feel like this explanation is a little backwards. "we will do A before 
B, unless C" is a bit confusing in my opinion. I think the comment could 
be reworded to

"two usual places in which stdin would be re-enabled. So we check here 
if stdin should be re-enabled, and do it if it is the case. An example 
of when stdin should not be re-enabled is if we are currently (...)"

> +	 instance, if we are currently handling emitted Python events inside
> +	 a synchronous execution command ("run", "continue", etc.).
> +	 Like this:
> +
> +	 User runs "continue"
> +	 --> command blocks the prompt
> +	 --> Python API is invoked, e.g.  via events
> +	 --> gdb.execute invoked inside Python
> +	 --> command raises an exception
> +	 --> this location
> +
> +	 In this case case, GDB would go back to the top "continue" command
> +	 and move on with its normal course of execution.  That is, it
> +	 would enable stdin in the way it normally does.  */
> +      if (!prompt_was_blocked)
> +	async_enable_stdin ();
> +
>         GDB_PY_HANDLE_EXCEPTION (except);
>       }
>   
> diff --git a/gdb/testsuite/gdb.python/py-cmd-exception.exp b/gdb/testsuite/gdb.python/py-cmd-exception.exp
> new file mode 100644
> index 00000000000..6ab1970b26b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-cmd-exception.exp
> @@ -0,0 +1,43 @@
> +# Copyright (C) 2023 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 file is part of the GDB testsuite.  It tests a corner case where
> +# the executed GDB command gives an exception and enabling the stdin would
> +# cause the GDB prompt to be displayed prematurely.
> +
> +load_lib gdb-python.exp
> +
> +require !use_gdb_stub allow_python_tests
> +
> +standard_testfile py-cmd.c

I think we like to avoid re-using test files, because it could lead to 
issues with hard-to-notice dependencies and such. I think making a 
minimal py-cmd-exception.c with a 2-line main function would probably be 
enough, right?

With these changes, you can add my rb tag:
Reviewed-By: Bruno Larsen <blarsen@redhat.com>

-- 
Cheers,
Bruno

> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> +gdb_test_no_output "source $pyfile" "source the script"
> +
> +gdb_start_cmd
> +
> +gdb_test_multiple "" "check the prompt" {
> +    -re "breakpoint $decimal, main .*\r\n$gdb_prompt $" {
> +	# The prompt is positioned correctly.
> +	pass $gdb_test_name
> +    }
> +    -re "No symbol \"a\" in current context.\r\n$gdb_prompt " {
> +	fail $gdb_test_name
> +    }
> +}
> diff --git a/gdb/testsuite/gdb.python/py-cmd-exception.py b/gdb/testsuite/gdb.python/py-cmd-exception.py
> new file mode 100644
> index 00000000000..51199bd3fe3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-cmd-exception.py
> @@ -0,0 +1,33 @@
> +# Copyright (C) 2023 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/>.
> +
> +import gdb
> +
> +class MyListener:
> +    def __init__(self):
> +        gdb.events.new_objfile.connect(self.handle_new_objfile_event)
> +        self.processed_objfile = False
> +
> +    def handle_new_objfile_event(self, event):
> +        if self.processed_objfile:
> +            return
> +
> +        print('loading ' + event.new_objfile.filename)
> +        self.processed_objfile = True
> +
> +        # There is no variable 'a'.  The command raises an exception.
> +        gdb.execute('print a')
> +
> +the_listener = MyListener()