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

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

On Wednesday, March 29, 2023 4:03 PM, Bruno Larsen wrote:
> 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 (...)"

I have updated the comment as follows:

+        two usual places in which stdin would be re-enabled. So, we check
+        here if stdin should be re-enabled, and do so if it is the case.
+        Stdin should not be re-enabled if it is already blocked because,
+        for example, we are running a command in the context of a
+        synchronous execution command ("run", "continue", etc.).  Like
+        this:
...
 
> > +	 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?

I've added a separate test file.

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

Thanks for the review.

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928