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

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

On Thursday, November 16, 2023 12:33 PM, Andrew Burgess wrote:
> Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:
> 
> Hi Baris,
> 
> I took a look through this patch.  I have a few thoughts.
> 
> > =========
> >
> > Hi,
> >
> > The previous revision (v3) of this patch can be found at
> >
> >   https://sourceware.org/pipermail/gdb-patches/2023-March/198508.html
> >
> > Changes in this version:
> >
> > * Rebased on the current master.
> > * Added a new test scenario in gdb.python/py-cmd-prompt.exp.
> > * The new scenario made me add a new boolean field to `struct ui` in ui.h.
> >
> > Regards,
> > Baris
> >
> > =========
> >
> > 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.
> >
> > A similar problem occurs also when the command completes without
> > exception, but if it enables stdin upon completion.  The "target
> > remote" command is an example for such case.  For more details of the
> > scenario, see the test case added by this patch.
> 
> First, thanks for digging into this problem more since v3 and finding
> the additional case that justifies the approach taken here.
> 
> However, I think that the commit message is showing the legacy of the
> original problem.  When I first read through this, my first thought was
> still exactly what Tom suggested (I hadn't looked at v3 at this point);
> that we should record if the prompt is blocked inside
> execute_gdb_command instead of adding the new ui member variable.
> 
> You say "A similar problem occurs also ... " and yes, the outcome is the
> same (early prompt), but really I think this is a totally different
> problem which defines _why_ we need this (or something like this) as the
> solution.
> 
> Also, why you say "For more details of the scenario, see the test case
> added by this patch.", this isn't super helpful.  Yes, the test gives an
> _example_, and there is an explanation similar to the above .. but I'd
> really like to see far more detail, e.g. a discussion of the call stack
> including start_remote and normal_stop, and why this is a problem.
> 
> I suspect we can grep GDB for calls to normal_stop, and if we can
> trigger any of these from Python as a result of an event, then this is
> another broken case.  For example, stepping into an inline frame calls
> normal_stop, so I suspect that using 'step' from a Python event could
> break -- not suggesting that more tests are needed, but maybe we should
> mention that there are multiple ways this can break.

Thanks for your comments, Andrew.  You're right.  I should've explained
the reasoning better.  I've updated the commit message in v5.  I hope
it's clearer now.
 
> >
> > 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>
> > Reviewed-By: Guinevere Larsen <blarsen@redhat.com>
> > ---
> >  gdb/event-top.c                               |  3 +-
> >  gdb/python/python.c                           | 29 ++++++++++
> >  gdb/testsuite/gdb.python/py-cmd-exception.c   | 22 ++++++++
> >  gdb/testsuite/gdb.python/py-cmd-exception.exp | 43 +++++++++++++++
> >  gdb/testsuite/gdb.python/py-cmd-exception.py  | 33 +++++++++++
> >  gdb/testsuite/gdb.python/py-cmd-prompt.c      | 22 ++++++++
> >  gdb/testsuite/gdb.python/py-cmd-prompt.exp    | 55 +++++++++++++++++++
> >  gdb/testsuite/gdb.python/py-cmd-prompt.py     | 36 ++++++++++++
> >  gdb/ui.h                                      |  5 ++
> >  9 files changed, 247 insertions(+), 1 deletion(-)
> >  create mode 100644 gdb/testsuite/gdb.python/py-cmd-exception.c
> >  create mode 100644 gdb/testsuite/gdb.python/py-cmd-exception.exp
> >  create mode 100644 gdb/testsuite/gdb.python/py-cmd-exception.py
> >  create mode 100644 gdb/testsuite/gdb.python/py-cmd-prompt.c
> >  create mode 100644 gdb/testsuite/gdb.python/py-cmd-prompt.exp
> >  create mode 100644 gdb/testsuite/gdb.python/py-cmd-prompt.py
> >
> > diff --git a/gdb/event-top.c b/gdb/event-top.c
> > index 9886ca46e7b..c24717eb2f0 100644
> > --- a/gdb/event-top.c
> > +++ b/gdb/event-top.c
> > @@ -508,7 +508,8 @@ async_enable_stdin (void)
> >  {
> >    struct ui *ui = current_ui;
> >
> > -  if (ui->prompt_state == PROMPT_BLOCKED)
> > +  if (ui->prompt_state == PROMPT_BLOCKED
> > +      && !ui->keep_prompt_blocked)
> >      {
> >        target_terminal::ours ();
> >        ui->register_file_handler ();
> > diff --git a/gdb/python/python.c b/gdb/python/python.c
> > index d569fb5a3e4..eef0017e407 100644
> > --- a/gdb/python/python.c
> > +++ b/gdb/python/python.c
> > @@ -658,6 +658,35 @@ 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.
> > +
> > +     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:
> > +
> > +     User runs "continue"
> > +     --> command blocks the prompt
> > +     --> Python API is invoked, e.g.  via events
> > +     --> gdb.execute(C) invoked inside Python
> > +     --> command C raises an exception
> > +
> > +     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.
> > +
> > +     Similarly, if the command we are about to execute enables the
> > +     stdin while we are still in the context of a synchronous
> > +     execution command, we would be displaying the prompt too early,
> > +     before the surrounding command completes.
> 
> As with the commit message, this comment seems to focus on the wrong
> case.  We should stress the non-local case more, as that justifies
> _this_ solution over a solution that is handled entirely within this
> function.

In v5, I changed the order of the examples and gave more details.

...
> > diff --git a/gdb/ui.h b/gdb/ui.h
> > index ed75e041e5f..4303d11c58c 100644
> > --- a/gdb/ui.h
> > +++ b/gdb/ui.h
> > @@ -135,6 +135,11 @@ struct ui
> >    /* See enum prompt_state's description.  */
> >    enum prompt_state prompt_state = PROMPT_NEEDED;
> >
> > +  /* Whether the prompt should be kept blocked.  This is useful to not
> > +     recover the prompt too early in the context of nested command
> > +     execution.  */
> > +  bool keep_prompt_blocked = false;
> 
> In the comment: s/recover/unblock/ makes more sense I think.

Fixed in v5.

Regards
-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