public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v3] gdb, python: selectively omit enabling stdin in gdb.execute exception
@ 2023-03-31  8:11 Tankut Baris Aktemur
  2023-06-20 13:04 ` Aktemur, Tankut Baris
  2023-11-14 11:06 ` [PATCH v4] gdb, python: selectively omit enabling stdin in gdb.execute Tankut Baris Aktemur
  0 siblings, 2 replies; 8+ messages in thread
From: Tankut Baris Aktemur @ 2023-03-31  8:11 UTC (permalink / raw)
  To: gdb-patches; +Cc: blarsen, tom, aburgess

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>
Reviewed-By: Bruno Larsen <blarsen@redhat.com>
---
 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 ++++++++++++++
 4 files changed, 123 insertions(+), 4 deletions(-)
 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

diff --git a/gdb/python/python.c b/gdb/python/python.c
index b295ff88743..5b34d142d1d 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;
@@ -698,10 +703,26 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
     {
       /* 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 ();
+	 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:
+
+	 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
+	 --> 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.c b/gdb/testsuite/gdb.python/py-cmd-exception.c
new file mode 100644
index 00000000000..f151960ce3a
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-cmd-exception.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
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..3b81131ee08
--- /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
+
+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()
-- 
2.25.1

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v3] gdb, python: selectively omit enabling stdin in gdb.execute exception
  2023-03-31  8:11 [PATCH v3] gdb, python: selectively omit enabling stdin in gdb.execute exception Tankut Baris Aktemur
@ 2023-06-20 13:04 ` Aktemur, Tankut Baris
  2023-11-14 11:06 ` [PATCH v4] gdb, python: selectively omit enabling stdin in gdb.execute Tankut Baris Aktemur
  1 sibling, 0 replies; 8+ messages in thread
From: Aktemur, Tankut Baris @ 2023-06-20 13:04 UTC (permalink / raw)
  To: gdb-patches; +Cc: blarsen, tom, aburgess

Kindly pinging.

Regards
-Baris

On Friday, March 31, 2023 10:11 AM, Aktemur, Tankut Baris 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>
> Reviewed-By: Bruno Larsen <blarsen@redhat.com>
> ---
>  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 ++++++++++++++
>  4 files changed, 123 insertions(+), 4 deletions(-)
>  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
> 
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index b295ff88743..5b34d142d1d 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;
> @@ -698,10 +703,26 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
>      {
>        /* 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 ();
> +	 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:
> +
> +	 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
> +	 --> 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.c b/gdb/testsuite/gdb.python/py-cmd-
> exception.c
> new file mode 100644
> index 00000000000..f151960ce3a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-cmd-exception.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 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/>.  */
> +
> +int
> +main ()
> +{
> +  return 0;
> +}
> 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..3b81131ee08
> --- /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
> +
> +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()
> --
> 2.25.1

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v4] gdb, python: selectively omit enabling stdin in gdb.execute
  2023-03-31  8:11 [PATCH v3] gdb, python: selectively omit enabling stdin in gdb.execute exception Tankut Baris Aktemur
  2023-06-20 13:04 ` Aktemur, Tankut Baris
@ 2023-11-14 11:06 ` Tankut Baris Aktemur
  2023-11-16 11:32   ` Andrew Burgess
  1 sibling, 1 reply; 8+ messages in thread
From: Tankut Baris Aktemur @ 2023-11-14 11:06 UTC (permalink / raw)
  To: gdb-patches; +Cc: blarsen, tom, aburgess

=========

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.

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.
+
+     For these reasons, we keep the prompt blocked, if it already is.  */
+  bool prompt_was_blocked = (current_ui->prompt_state == PROMPT_BLOCKED);
+  scoped_restore save_prompt_state
+    = make_scoped_restore (&current_ui->keep_prompt_blocked,
+			   prompt_was_blocked);
+
   try
     {
       gdbpy_allow_threads allow_threads;
diff --git a/gdb/testsuite/gdb.python/py-cmd-exception.c b/gdb/testsuite/gdb.python/py-cmd-exception.c
new file mode 100644
index 00000000000..f151960ce3a
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-cmd-exception.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
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..3b81131ee08
--- /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
+
+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()
diff --git a/gdb/testsuite/gdb.python/py-cmd-prompt.c b/gdb/testsuite/gdb.python/py-cmd-prompt.c
new file mode 100644
index 00000000000..6956ba4858d
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-cmd-prompt.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+int
+main ()
+{
+  return 0; /* break-here */
+}
diff --git a/gdb/testsuite/gdb.python/py-cmd-prompt.exp b/gdb/testsuite/gdb.python/py-cmd-prompt.exp
new file mode 100644
index 00000000000..968f5603fc1
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-cmd-prompt.exp
@@ -0,0 +1,55 @@
+# 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 enables the stdin while running
+# inside a synchronous command, causing the GDB prompt to be displayed
+# prematurely.
+
+load_lib gdb-python.exp
+load_lib gdbserver-support.exp
+
+# We use the start command.
+require !use_gdb_stub
+require allow_python_tests allow_gdbserver_tests
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set bp_line [gdb_get_line_number "break-here"]
+gdb_breakpoint $bp_line
+
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+gdb_test_no_output "source $pyfile" "source the script"
+
+set gdbserver [gdbserver_start "" [standard_output_file $binfile]]
+set gdbserver_gdbport [lindex $gdbserver 1]
+gdb_test_no_output "python the_listener.port = '${gdbserver_gdbport}'"
+
+gdb_run_cmd
+
+gdb_test_multiple "" "prompt is positioned correctly" {
+    -re -wrap "break-here \[^\r\n\]+" {
+	pass $gdb_test_name
+    }
+}
+
+# Clean up the gdbserver.
+gdb_test "inferior 2" "Switching to inferior 2.*" \
+    "switch to gdbserver for clean up"
+gdbserver_exit 0
diff --git a/gdb/testsuite/gdb.python/py-cmd-prompt.py b/gdb/testsuite/gdb.python/py-cmd-prompt.py
new file mode 100644
index 00000000000..a920309d39e
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-cmd-prompt.py
@@ -0,0 +1,36 @@
+# 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
+        self.port = "uninitialized"
+
+    def handle_new_objfile_event(self, event):
+        if self.processed_objfile:
+            return
+
+        print('loading ' + event.new_objfile.filename)
+        self.processed_objfile = True
+
+        gdb.execute('add-inferior -no-connection')
+        gdb.execute('inferior 2')
+        gdb.execute('target remote ' + self.port)
+        gdb.execute('inferior 1')
+
+the_listener = MyListener()
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;
+
   /* The fields below that start with "m_" are "private".  They're
      meant to be accessed through wrapper macros that make them look
      like globals.  */
-- 
2.34.1

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v4] gdb, python: selectively omit enabling stdin in gdb.execute
  2023-11-14 11:06 ` [PATCH v4] gdb, python: selectively omit enabling stdin in gdb.execute Tankut Baris Aktemur
@ 2023-11-16 11:32   ` Andrew Burgess
  2023-11-20 20:21     ` [PATCH v5] " Tankut Baris Aktemur
  2023-11-20 20:24     ` [PATCH v4] " Aktemur, Tankut Baris
  0 siblings, 2 replies; 8+ messages in thread
From: Andrew Burgess @ 2023-11-16 11:32 UTC (permalink / raw)
  To: Tankut Baris Aktemur, gdb-patches; +Cc: blarsen, tom

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.

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

> +
> +     For these reasons, we keep the prompt blocked, if it already is.  */
> +  bool prompt_was_blocked = (current_ui->prompt_state == PROMPT_BLOCKED);
> +  scoped_restore save_prompt_state
> +    = make_scoped_restore (&current_ui->keep_prompt_blocked,
> +			   prompt_was_blocked);
> +
>    try
>      {
>        gdbpy_allow_threads allow_threads;
> diff --git a/gdb/testsuite/gdb.python/py-cmd-exception.c b/gdb/testsuite/gdb.python/py-cmd-exception.c
> new file mode 100644
> index 00000000000..f151960ce3a
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-cmd-exception.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 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/>.  */
> +
> +int
> +main ()
> +{
> +  return 0;
> +}
> 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..3b81131ee08
> --- /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
> +
> +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()
> diff --git a/gdb/testsuite/gdb.python/py-cmd-prompt.c b/gdb/testsuite/gdb.python/py-cmd-prompt.c
> new file mode 100644
> index 00000000000..6956ba4858d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-cmd-prompt.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 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/>.  */
> +
> +int
> +main ()
> +{
> +  return 0; /* break-here */
> +}
> diff --git a/gdb/testsuite/gdb.python/py-cmd-prompt.exp b/gdb/testsuite/gdb.python/py-cmd-prompt.exp
> new file mode 100644
> index 00000000000..968f5603fc1
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-cmd-prompt.exp
> @@ -0,0 +1,55 @@
> +# 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 enables the stdin while running
> +# inside a synchronous command, causing the GDB prompt to be displayed
> +# prematurely.
> +
> +load_lib gdb-python.exp
> +load_lib gdbserver-support.exp
> +
> +# We use the start command.
> +require !use_gdb_stub
> +require allow_python_tests allow_gdbserver_tests
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +set bp_line [gdb_get_line_number "break-here"]
> +gdb_breakpoint $bp_line
> +
> +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> +gdb_test_no_output "source $pyfile" "source the script"
> +
> +set gdbserver [gdbserver_start "" [standard_output_file $binfile]]
> +set gdbserver_gdbport [lindex $gdbserver 1]
> +gdb_test_no_output "python the_listener.port = '${gdbserver_gdbport}'"
> +
> +gdb_run_cmd
> +
> +gdb_test_multiple "" "prompt is positioned correctly" {
> +    -re -wrap "break-here \[^\r\n\]+" {
> +	pass $gdb_test_name
> +    }
> +}
> +
> +# Clean up the gdbserver.
> +gdb_test "inferior 2" "Switching to inferior 2.*" \
> +    "switch to gdbserver for clean up"
> +gdbserver_exit 0
> diff --git a/gdb/testsuite/gdb.python/py-cmd-prompt.py b/gdb/testsuite/gdb.python/py-cmd-prompt.py
> new file mode 100644
> index 00000000000..a920309d39e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-cmd-prompt.py
> @@ -0,0 +1,36 @@
> +# 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
> +        self.port = "uninitialized"
> +
> +    def handle_new_objfile_event(self, event):
> +        if self.processed_objfile:
> +            return
> +
> +        print('loading ' + event.new_objfile.filename)
> +        self.processed_objfile = True
> +
> +        gdb.execute('add-inferior -no-connection')
> +        gdb.execute('inferior 2')
> +        gdb.execute('target remote ' + self.port)
> +        gdb.execute('inferior 1')
> +
> +the_listener = MyListener()
> 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.

Thanks,
Andrew

> +
>    /* The fields below that start with "m_" are "private".  They're
>       meant to be accessed through wrapper macros that make them look
>       like globals.  */
> -- 
> 2.34.1
>
> 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v5] gdb, python: selectively omit enabling stdin in gdb.execute
  2023-11-16 11:32   ` Andrew Burgess
@ 2023-11-20 20:21     ` Tankut Baris Aktemur
  2024-02-06 17:22       ` Tom Tromey
  2023-11-20 20:24     ` [PATCH v4] " Aktemur, Tankut Baris
  1 sibling, 1 reply; 8+ messages in thread
From: Tankut Baris Aktemur @ 2023-11-20 20:21 UTC (permalink / raw)
  To: gdb-patches; +Cc: blarsen, tom, aburgess

===========

Hi,

The previous revision (v4) of this patch can be found at

  https://sourceware.org/pipermail/gdb-patches/2023-November/204107.html

Changes in this version:

* Revised the commit message to address Andrew Burgess' concerns in
  https://sourceware.org/pipermail/gdb-patches/2023-November/204215.html

* Made a minor change to a code comment.

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

with the following code:

  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);
    }

In this patch, we explain what happens when we run a GDB command in
the context of a synchronous command, e.g.  via Python observer
notifications.

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('add-inferior -no-connection')
        gdb.execute('inferior 2')
        gdb.execute('target remote | gdbserver - /tmp/a.out')
        gdb.execute('inferior 1')

the_listener = MyListener()
~~~

Using this Python file, we see the behavior below:

  $ gdb -q -ex "source file.py" -ex "run" --args a.out
  Reading symbols from a.out...
  Starting program: /tmp/a.out
  loading /lib64/ld-linux-x86-64.so.2
  [New inferior 2]
  Added inferior 2
  [Switching to inferior 2 [<null>] (<noexec>)]
  stdin/stdout redirected
  Process /tmp/a.out created; pid = 3075406
  Remote debugging using stdio
  Reading /tmp/a.out from remote target...
  ...
  [Switching to inferior 1 [process 3075400] (/tmp/a.out)]
  [Switching to thread 1.1 (process 3075400)]
  #0  0x00007ffff7fe3290 in ?? () from /lib64/ld-linux-x86-64.so.2
  (gdb) [Thread debugging using libthread_db enabled]
  Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
  [Inferior 1 (process 3075400) exited normally]

Note how the GDB prompt comes in-between the debugger output.  We have this
obscure behavior, because the executed command, "target remote", triggers
an invocation of `normal_stop` that enables stdin.  After that, however,
the Python notification context completes and GDB continues with its normal
flow of executing the 'run' command.  This can be seen in the call stack
below:

  (top-gdb) bt
  #0  async_enable_stdin () at src/gdb/event-top.c:523
  #1  0x00005555561c3acd in normal_stop () at src/gdb/infrun.c:9432
  #2  0x00005555561b328e in start_remote (from_tty=0) at src/gdb/infrun.c:3801
  #3  0x0000555556441224 in remote_target::start_remote_1 (this=0x5555587882e0, from_tty=0, extended_p=0) at src/gdb/remote.c:5225
  #4  0x000055555644166c in remote_target::start_remote (this=0x5555587882e0, from_tty=0, extended_p=0) at src/gdb/remote.c:5316
  #5  0x00005555564430cf in remote_target::open_1 (name=0x55555878525e "| gdbserver - /tmp/a.out", from_tty=0, extended_p=0) at src/gdb/remote.c:6175
  #6  0x0000555556441707 in remote_target::open (name=0x55555878525e "| gdbserver - /tmp/a.out", from_tty=0) at src/gdb/remote.c:5338
  #7  0x00005555565ea63f in open_target (args=0x55555878525e "| gdbserver - /tmp/a.out", from_tty=0, command=0x555558589280)  at src/gdb/target.c:824
  #8  0x0000555555f0d89a in cmd_func (cmd=0x555558589280, args=0x55555878525e "| gdbserver - /tmp/a.out", from_tty=0) at src/gdb/cli/cli-decode.c:2735
  #9  0x000055555661fb42 in execute_command (p=0x55555878529e "t", from_tty=0) at src/gdb/top.c:575
  #10 0x0000555555f1a506 in execute_control_command_1 (cmd=0x555558756f00, from_tty=0) at src/gdb/cli/cli-script.c:529
  #11 0x0000555555f1abea in execute_control_command (cmd=0x555558756f00, from_tty=0) at src/gdb/cli/cli-script.c:701
  #12 0x0000555555f19fc7 in execute_control_commands (cmdlines=0x555558756f00, from_tty=0) at src/gdb/cli/cli-script.c:411
  #13 0x0000555556400d91 in execute_gdb_command (self=0x7ffff43b5d00, args=0x7ffff440ab60, kw=0x0) at src/gdb/python/python.c:700
  #14 0x00007ffff7a96023 in ?? () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #15 0x00007ffff7a4dadc in _PyObject_MakeTpCall () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #16 0x00007ffff79e9a1c in _PyEval_EvalFrameDefault () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #17 0x00007ffff7b303af in ?? () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #18 0x00007ffff7a50358 in ?? () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #19 0x00007ffff7a4f3f4 in ?? () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #20 0x00007ffff7a4f883 in PyObject_CallFunctionObjArgs () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #21 0x00005555563a9758 in evpy_emit_event (event=0x7ffff42b5430, registry=0x7ffff42b4690) at src/gdb/python/py-event.c:104
  #22 0x00005555563cb874 in emit_new_objfile_event (objfile=0x555558761700) at src/gdb/python/py-newobjfileevent.c:52
  #23 0x00005555563b53bc in python_new_objfile (objfile=0x555558761700) at src/gdb/python/py-inferior.c:195
  #24 0x0000555555d6dff0 in std::__invoke_impl<void, void (*&)(objfile*), objfile*> (__f=@0x5555585b5860: 0x5555563b5360 <python_new_objfile(objfile*)>) at /usr/include/c++/11/bits/invoke.h:61
  #25 0x0000555555d6be18 in std::__invoke_r<void, void (*&)(objfile*), objfile*> (__fn=@0x5555585b5860: 0x5555563b5360 <python_new_objfile(objfile*)>) at /usr/include/c++/11/bits/invoke.h:111
  #26 0x0000555555d69661 in std::_Function_handler<void (objfile*), void (*)(objfile*)>::_M_invoke(std::_Any_data const&, objfile*&&) (__functor=..., __args#0=@0x7fffffffd080: 0x555558761700) at /usr/include/c++/11/bits/std_function.h:290
  #27 0x0000555556314caf in std::function<void (objfile*)>::operator()(objfile*) const (this=0x5555585b5860, __args#0=0x555558761700) at /usr/include/c++/11/bits/std_function.h:590
  #28 0x000055555631444e in gdb::observers::observable<objfile*>::notify (this=0x55555836eea0 <gdb::observers::new_objfile>, args#0=0x555558761700) at src/gdb/../gdbsupport/observable.h:166
  #29 0x0000555556599b3f in symbol_file_add_with_addrs (abfd=..., name=0x55555875d310 "/lib64/ld-linux-x86-64.so.2", add_flags=..., addrs=0x7fffffffd2f0, flags=..., parent=0x0) at src/gdb/symfile.c:1125
  #30 0x0000555556599ca4 in symbol_file_add_from_bfd (abfd=..., name=0x55555875d310 "/lib64/ld-linux-x86-64.so.2", add_flags=..., addrs=0x7fffffffd2f0, flags=..., parent=0x0) at src/gdb/symfile.c:1160
  #31 0x0000555556546371 in solib_read_symbols (so=..., flags=...) at src/gdb/solib.c:692
  #32 0x0000555556546f0f in solib_add (pattern=0x0, from_tty=0, readsyms=1) at src/gdb/solib.c:1015
  #33 0x0000555556539891 in enable_break (info=0x55555874e180, from_tty=0) at src/gdb/solib-svr4.c:2416
  #34 0x000055555653b305 in svr4_solib_create_inferior_hook (from_tty=0) at src/gdb/solib-svr4.c:3058
  #35 0x0000555556547cee in solib_create_inferior_hook (from_tty=0) at src/gdb/solib.c:1217
  #36 0x0000555556196f6a in post_create_inferior (from_tty=0) at src/gdb/infcmd.c:275
  #37 0x0000555556197670 in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at src/gdb/infcmd.c:486
  #38 0x000055555619783f in run_command (args=0x0, from_tty=1) at src/gdb/infcmd.c:512
  #39 0x0000555555f0798d in do_simple_func (args=0x0, from_tty=1, c=0x555558567510) at src/gdb/cli/cli-decode.c:95
  #40 0x0000555555f0d89a in cmd_func (cmd=0x555558567510, args=0x0, from_tty=1) at src/gdb/cli/cli-decode.c:2735
  #41 0x000055555661fb42 in execute_command (p=0x7fffffffe2c4 "", from_tty=1) at src/gdb/top.c:575
  #42 0x000055555626303b in catch_command_errors (command=0x55555661f4ab <execute_command(char const*, int)>, arg=0x7fffffffe2c1 "run", from_tty=1, do_bp_actions=true) at src/gdb/main.c:513
  #43 0x000055555626328a in execute_cmdargs (cmdarg_vec=0x7fffffffdaf0, file_type=CMDARG_FILE, cmd_type=CMDARG_COMMAND, ret=0x7fffffffda3c) at src/gdb/main.c:612
  #44 0x0000555556264849 in captured_main_1 (context=0x7fffffffdd40) at src/gdb/main.c:1293
  #45 0x0000555556264a7f in captured_main (data=0x7fffffffdd40) at src/gdb/main.c:1314
  #46 0x0000555556264b2e in gdb_main (args=0x7fffffffdd40) at src/gdb/main.c:1343
  #47 0x0000555555ceccab in main (argc=9, argv=0x7fffffffde78) at src/gdb/gdb.c:39
  (top-gdb)

The use of the "target remote" command here is just an example.  In
principle, we would reproduce the problem with any command that
triggers an invocation of `normal_stop`.

To omit enabling the stdin in `normal_stop`, we would have to check the
context we are in.  Since we cannot do that, we add a new field to
`struct ui` to track whether the prompt was already blocked, and set
the tracker flag in the Python context before executing a GDB command.

After applying this patch, the output becomes

  ...
  Reading symbols from a.out...
  Starting program: /tmp/a.out
  loading /lib64/ld-linux-x86-64.so.2
  [New inferior 2]
  Added inferior 2
  [Switching to inferior 2 [<null>] (<noexec>)]
  stdin/stdout redirected
  Process /tmp/a.out created; pid = 3032261
  Remote debugging using stdio
  Reading /tmp/a.out from remote target...
  ...
  [Switching to inferior 1 [process 3032255] (/tmp/a.out)]
  [Switching to thread 1.1 (process 3032255)]
  #0  0x00007ffff7fe3290 in ?? () from /lib64/ld-linux-x86-64.so.2
  [Thread debugging using libthread_db enabled]
  Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
  [Inferior 1 (process 3032255) exited normally]
  (gdb)

Let's now consider a secondary scenario, where the command executed from
the Python raises an error.  As an example, suppose we have the Python
file below:

    def handle_new_objfile_event(self, event):
        ...
        print("loading " + event.new_objfile.filename)
        self.processed_objfile = True
        gdb.execute('print a')

The executed command, "print a", gives an error because "a" is not
defined.  Without this patch, we see the behavior below, where the
prompt is again placed incorrectly:

  ...
  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]

This time, `async_enable_stdin` is called from the 'catch' block in
`execute_gdb_command`:

  (top-gdb) bt
  #0  async_enable_stdin () at src/gdb/event-top.c:523
  #1  0x0000555556400f0a in execute_gdb_command (self=0x7ffff43b5d00, args=0x7ffff440ab60, kw=0x0) at src/gdb/python/python.c:713
  #2  0x00007ffff7a96023 in ?? () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #3  0x00007ffff7a4dadc in _PyObject_MakeTpCall () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #4  0x00007ffff79e9a1c in _PyEval_EvalFrameDefault () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #5  0x00007ffff7b303af in ?? () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #6  0x00007ffff7a50358 in ?? () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #7  0x00007ffff7a4f3f4 in ?? () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #8  0x00007ffff7a4f883 in PyObject_CallFunctionObjArgs () from /lib/x86_64-linux-gnu/libpython3.10.so.1.0
  #9  0x00005555563a9758 in evpy_emit_event (event=0x7ffff42b5430, registry=0x7ffff42b4690) at src/gdb/python/py-event.c:104
  #10 0x00005555563cb874 in emit_new_objfile_event (objfile=0x555558761410) at src/gdb/python/py-newobjfileevent.c:52
  #11 0x00005555563b53bc in python_new_objfile (objfile=0x555558761410) at src/gdb/python/py-inferior.c:195
  #12 0x0000555555d6dff0 in std::__invoke_impl<void, void (*&)(objfile*), objfile*> (__f=@0x5555585b5860: 0x5555563b5360 <python_new_objfile(objfile*)>) at /usr/include/c++/11/bits/invoke.h:61
  #13 0x0000555555d6be18 in std::__invoke_r<void, void (*&)(objfile*), objfile*> (__fn=@0x5555585b5860: 0x5555563b5360 <python_new_objfile(objfile*)>) at /usr/include/c++/11/bits/invoke.h:111
  #14 0x0000555555d69661 in std::_Function_handler<void (objfile*), void (*)(objfile*)>::_M_invoke(std::_Any_data const&, objfile*&&) (__functor=..., __args#0=@0x7fffffffd080: 0x555558761410) at /usr/include/c++/11/bits/std_function.h:290
  #15 0x0000555556314caf in std::function<void (objfile*)>::operator()(objfile*) const (this=0x5555585b5860, __args#0=0x555558761410) at /usr/include/c++/11/bits/std_function.h:590
  #16 0x000055555631444e in gdb::observers::observable<objfile*>::notify (this=0x55555836eea0 <gdb::observers::new_objfile>, args#0=0x555558761410) at src/gdb/../gdbsupport/observable.h:166
  #17 0x0000555556599b3f in symbol_file_add_with_addrs (abfd=..., name=0x55555875d020 "/lib64/ld-linux-x86-64.so.2", add_flags=..., addrs=0x7fffffffd2f0, flags=..., parent=0x0) at src/gdb/symfile.c:1125
  #18 0x0000555556599ca4 in symbol_file_add_from_bfd (abfd=..., name=0x55555875d020 "/lib64/ld-linux-x86-64.so.2", add_flags=..., addrs=0x7fffffffd2f0, flags=..., parent=0x0) at src/gdb/symfile.c:1160
  #19 0x0000555556546371 in solib_read_symbols (so=..., flags=...) at src/gdb/solib.c:692
  #20 0x0000555556546f0f in solib_add (pattern=0x0, from_tty=0, readsyms=1) at src/gdb/solib.c:1015
  #21 0x0000555556539891 in enable_break (info=0x55555874a670, from_tty=0) at src/gdb/solib-svr4.c:2416
  #22 0x000055555653b305 in svr4_solib_create_inferior_hook (from_tty=0) at src/gdb/solib-svr4.c:3058
  #23 0x0000555556547cee in solib_create_inferior_hook (from_tty=0) at src/gdb/solib.c:1217
  #24 0x0000555556196f6a in post_create_inferior (from_tty=0) at src/gdb/infcmd.c:275
  #25 0x0000555556197670 in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at src/gdb/infcmd.c:486
  #26 0x000055555619783f in run_command (args=0x0, from_tty=1) at src/gdb/infcmd.c:512
  #27 0x0000555555f0798d in do_simple_func (args=0x0, from_tty=1, c=0x555558567510) at src/gdb/cli/cli-decode.c:95
  #28 0x0000555555f0d89a in cmd_func (cmd=0x555558567510, args=0x0, from_tty=1) at src/gdb/cli/cli-decode.c:2735
  #29 0x000055555661fb42 in execute_command (p=0x7fffffffe2c4 "", from_tty=1) at src/gdb/top.c:575
  #30 0x000055555626303b in catch_command_errors (command=0x55555661f4ab <execute_command(char const*, int)>, arg=0x7fffffffe2c1 "run", from_tty=1, do_bp_actions=true) at src/gdb/main.c:513
  #31 0x000055555626328a in execute_cmdargs (cmdarg_vec=0x7fffffffdaf0, file_type=CMDARG_FILE, cmd_type=CMDARG_COMMAND, ret=0x7fffffffda3c) at src/gdb/main.c:612
  #32 0x0000555556264849 in captured_main_1 (context=0x7fffffffdd40) at src/gdb/main.c:1293
  #33 0x0000555556264a7f in captured_main (data=0x7fffffffdd40) at src/gdb/main.c:1314
  #34 0x0000555556264b2e in gdb_main (args=0x7fffffffdd40) at src/gdb/main.c:1343
  #35 0x0000555555ceccab in main (argc=9, argv=0x7fffffffde78) at src/gdb/gdb.c:39
  (top-gdb)

Again, after we enable stdin, GDB continues with its normal flow
of the 'run' command and receives the inferior's exit event, where
it would have enabled stdin, if we had not done it prematurely.

  (top-gdb) bt
  #0  async_enable_stdin () at src/gdb/event-top.c:523
  #1  0x00005555561c3acd in normal_stop () at src/gdb/infrun.c:9432
  #2  0x00005555561b5bf1 in fetch_inferior_event () at src/gdb/infrun.c:4700
  #3  0x000055555618d6a7 in inferior_event_handler (event_type=INF_REG_EVENT) at src/gdb/inf-loop.c:42
  #4  0x000055555620ecdb in handle_target_event (error=0, client_data=0x0) at src/gdb/linux-nat.c:4316
  #5  0x0000555556f33035 in handle_file_event (file_ptr=0x5555587024e0, ready_mask=1) at src/gdbsupport/event-loop.cc:573
  #6  0x0000555556f3362f in gdb_wait_for_event (block=0) at src/gdbsupport/event-loop.cc:694
  #7  0x0000555556f322cd in gdb_do_one_event (mstimeout=-1) at src/gdbsupport/event-loop.cc:217
  #8  0x0000555556262df8 in start_event_loop () at src/gdb/main.c:407
  #9  0x0000555556262f85 in captured_command_loop () at src/gdb/main.c:471
  #10 0x0000555556264a84 in captured_main (data=0x7fffffffdd40) at src/gdb/main.c:1324
  #11 0x0000555556264b2e in gdb_main (args=0x7fffffffdd40) at src/gdb/main.c:1343
  #12 0x0000555555ceccab in main (argc=9, argv=0x7fffffffde78) at src/gdb/gdb.c:39
  (top-gdb)

The solution implemented by this patch addresses the problem.  After
applying the patch, the output becomes

  $ 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 6ce53704539..62918edda1b 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -522,7 +522,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 7e48165db21..b40a9439a21 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -662,6 +662,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.
+
+     For these reasons, we keep the prompt blocked, if it already is.  */
+  bool prompt_was_blocked = (current_ui->prompt_state == PROMPT_BLOCKED);
+  scoped_restore save_prompt_state
+    = make_scoped_restore (&current_ui->keep_prompt_blocked,
+			   prompt_was_blocked);
+
   try
     {
       gdbpy_allow_threads allow_threads;
diff --git a/gdb/testsuite/gdb.python/py-cmd-exception.c b/gdb/testsuite/gdb.python/py-cmd-exception.c
new file mode 100644
index 00000000000..f151960ce3a
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-cmd-exception.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
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..3b81131ee08
--- /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
+
+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()
diff --git a/gdb/testsuite/gdb.python/py-cmd-prompt.c b/gdb/testsuite/gdb.python/py-cmd-prompt.c
new file mode 100644
index 00000000000..6956ba4858d
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-cmd-prompt.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.  */
+
+int
+main ()
+{
+  return 0; /* break-here */
+}
diff --git a/gdb/testsuite/gdb.python/py-cmd-prompt.exp b/gdb/testsuite/gdb.python/py-cmd-prompt.exp
new file mode 100644
index 00000000000..968f5603fc1
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-cmd-prompt.exp
@@ -0,0 +1,55 @@
+# 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 enables the stdin while running
+# inside a synchronous command, causing the GDB prompt to be displayed
+# prematurely.
+
+load_lib gdb-python.exp
+load_lib gdbserver-support.exp
+
+# We use the start command.
+require !use_gdb_stub
+require allow_python_tests allow_gdbserver_tests
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+set bp_line [gdb_get_line_number "break-here"]
+gdb_breakpoint $bp_line
+
+set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+gdb_test_no_output "source $pyfile" "source the script"
+
+set gdbserver [gdbserver_start "" [standard_output_file $binfile]]
+set gdbserver_gdbport [lindex $gdbserver 1]
+gdb_test_no_output "python the_listener.port = '${gdbserver_gdbport}'"
+
+gdb_run_cmd
+
+gdb_test_multiple "" "prompt is positioned correctly" {
+    -re -wrap "break-here \[^\r\n\]+" {
+	pass $gdb_test_name
+    }
+}
+
+# Clean up the gdbserver.
+gdb_test "inferior 2" "Switching to inferior 2.*" \
+    "switch to gdbserver for clean up"
+gdbserver_exit 0
diff --git a/gdb/testsuite/gdb.python/py-cmd-prompt.py b/gdb/testsuite/gdb.python/py-cmd-prompt.py
new file mode 100644
index 00000000000..a920309d39e
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-cmd-prompt.py
@@ -0,0 +1,36 @@
+# 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
+        self.port = "uninitialized"
+
+    def handle_new_objfile_event(self, event):
+        if self.processed_objfile:
+            return
+
+        print('loading ' + event.new_objfile.filename)
+        self.processed_objfile = True
+
+        gdb.execute('add-inferior -no-connection')
+        gdb.execute('inferior 2')
+        gdb.execute('target remote ' + self.port)
+        gdb.execute('inferior 1')
+
+the_listener = MyListener()
diff --git a/gdb/ui.h b/gdb/ui.h
index ed75e041e5f..1b6bf5b466e 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
+     unblock the prompt too early in the context of nested command
+     execution.  */
+  bool keep_prompt_blocked = false;
+
   /* The fields below that start with "m_" are "private".  They're
      meant to be accessed through wrapper macros that make them look
      like globals.  */
-- 
2.34.1

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v4] gdb, python: selectively omit enabling stdin in gdb.execute
  2023-11-16 11:32   ` Andrew Burgess
  2023-11-20 20:21     ` [PATCH v5] " Tankut Baris Aktemur
@ 2023-11-20 20:24     ` Aktemur, Tankut Baris
  1 sibling, 0 replies; 8+ messages in thread
From: Aktemur, Tankut Baris @ 2023-11-20 20:24 UTC (permalink / raw)
  To: Andrew Burgess, gdb-patches; +Cc: blarsen, tom

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5] gdb, python: selectively omit enabling stdin in gdb.execute
  2023-11-20 20:21     ` [PATCH v5] " Tankut Baris Aktemur
@ 2024-02-06 17:22       ` Tom Tromey
  2024-02-19 10:00         ` Aktemur, Tankut Baris
  0 siblings, 1 reply; 8+ messages in thread
From: Tom Tromey @ 2024-02-06 17:22 UTC (permalink / raw)
  To: Tankut Baris Aktemur; +Cc: gdb-patches, blarsen, tom, aburgess

>>>>> Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:

> The previous revision (v4) of this patch can be found at

>   https://sourceware.org/pipermail/gdb-patches/2023-November/204107.html

> Changes in this version:

> * Revised the commit message to address Andrew Burgess' concerns in
>   https://sourceware.org/pipermail/gdb-patches/2023-November/204215.html

> * Made a minor change to a code comment.

I didn't see a response to this.

I think this whole area is pretty ugly, but at the same time, I don't
think that's your fault.

In short I think this is OK.
Approved-By: Tom Tromey <tom@tromey.com>

thanks,
Tom

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH v5] gdb, python: selectively omit enabling stdin in gdb.execute
  2024-02-06 17:22       ` Tom Tromey
@ 2024-02-19 10:00         ` Aktemur, Tankut Baris
  0 siblings, 0 replies; 8+ messages in thread
From: Aktemur, Tankut Baris @ 2024-02-19 10:00 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches; +Cc: blarsen, aburgess

On February 6, 2024 6:23 PM, Tom Tromey wrote:
> >>>>> Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:
> 
> > The previous revision (v4) of this patch can be found at
> 
> >   https://sourceware.org/pipermail/gdb-patches/2023-November/204107.html
> 
> > Changes in this version:
> 
> > * Revised the commit message to address Andrew Burgess' concerns in
> >   https://sourceware.org/pipermail/gdb-patches/2023-November/204215.html
> 
> > * Made a minor change to a code comment.
> 
> I didn't see a response to this.
> 
> I think this whole area is pretty ugly, but at the same time, I don't
> think that's your fault.
> 
> In short I think this is OK.
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> thanks,
> Tom

Thank you.  I updated the copyright years of the newly-introduced files from
2023 to 2023-2024 and pushed the patch.

-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


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-02-19 10:00 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-31  8:11 [PATCH v3] gdb, python: selectively omit enabling stdin in gdb.execute exception Tankut Baris Aktemur
2023-06-20 13:04 ` Aktemur, Tankut Baris
2023-11-14 11:06 ` [PATCH v4] gdb, python: selectively omit enabling stdin in gdb.execute Tankut Baris Aktemur
2023-11-16 11:32   ` Andrew Burgess
2023-11-20 20:21     ` [PATCH v5] " Tankut Baris Aktemur
2024-02-06 17:22       ` Tom Tromey
2024-02-19 10:00         ` Aktemur, Tankut Baris
2023-11-20 20:24     ` [PATCH v4] " Aktemur, Tankut Baris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).