[PATCH 1/2] gdb, python: do minor modernization in execute_gdb_command

[PATCH 1/2] gdb, python: do minor modernization in execute_gdb_command

[Tankut Baris Aktemur]

Use nullptr instead of NULL and boolify two local variables in
execute_gdb_command.
---
 gdb/python/python.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/gdb/python/python.c b/gdb/python/python.c
index ed466cc4511..5719f351528 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -621,31 +621,32 @@ static PyObject *
 execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
 {
   const char *arg;
-  PyObject *from_tty_obj = NULL, *to_string_obj = NULL;
-  int from_tty, to_string;
-  static const char *keywords[] = { "command", "from_tty", "to_string", NULL };
+  PyObject *from_tty_obj = nullptr;
+  PyObject *to_string_obj = nullptr;
+  static const char *keywords[] = { "command", "from_tty", "to_string",
+				    nullptr };
 
   if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|O!O!", keywords, &arg,
 					&PyBool_Type, &from_tty_obj,
 					&PyBool_Type, &to_string_obj))
-    return NULL;
+    return nullptr;
 
-  from_tty = 0;
-  if (from_tty_obj)
+  bool from_tty = false;
+  if (from_tty_obj != nullptr)
     {
       int cmp = PyObject_IsTrue (from_tty_obj);
       if (cmp < 0)
-	return NULL;
-      from_tty = cmp;
+	return nullptr;
+      from_tty = ((cmp == 0) ? false : true);
     }
 
-  to_string = 0;
-  if (to_string_obj)
+  bool to_string = false;
+  if (to_string_obj != nullptr)
     {
       int cmp = PyObject_IsTrue (to_string_obj);
       if (cmp < 0)
-	return NULL;
-      to_string = cmp;
+	return nullptr;
+      to_string = ((cmp == 0) ? false : true);
     }
 
   std::string to_string_res;
-- 
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

[PATCH 2/2] gdb, python: selectively omit enabling stdin in gdb.execute exception

[Tankut Baris Aktemur]

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 emitted a Python event.  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 we are inside emitting a Python event.

With this patch, we see

  $ gdb -q -ex "source file.py" -ex "run" --args a.out
  Reading symbols from /tmp/a.out...
  Starting program: /tmp/a.out
  loading /lib64/ld-linux-x86-64.so.2
  Python Exception <class 'gdb.error'>: No symbol "a" in current context.
  [Inferior 1 (process 3984511) exited normally]
  (gdb)

Regression-tested on X86_64 Linux using the default board file (i.e.  unix).

Co-Authored-By: Oguzhan Karakaya <oguzhan.karakaya@intel.com>
---
 gdb/python/py-event.c                         |  6 +++
 gdb/python/py-event.h                         |  5 +++
 gdb/python/python.c                           |  9 +++-
 gdb/testsuite/gdb.python/py-cmd-exception.exp | 43 +++++++++++++++++++
 gdb/testsuite/gdb.python/py-cmd-exception.py  | 32 ++++++++++++++
 5 files changed, 93 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.python/py-cmd-exception.exp
 create mode 100644 gdb/testsuite/gdb.python/py-cmd-exception.py

diff --git a/gdb/python/py-event.c b/gdb/python/py-event.c
index 3ff31acf85b..6daf504398b 100644
--- a/gdb/python/py-event.c
+++ b/gdb/python/py-event.c
@@ -75,6 +75,9 @@ gdbpy_initialize_event_generic (PyTypeObject *type,
   return gdb_pymodule_addobject (gdb_module, name, (PyObject *) type);
 }
 
+/* See py-event.h.  */
+
+bool in_evpy_emit_event = false;
 
 /* Notify the list of listens that the given EVENT has occurred.
    returns 0 if emit is successful -1 otherwise.  */
@@ -85,6 +88,9 @@ evpy_emit_event (PyObject *event,
 {
   Py_ssize_t i;
 
+  scoped_restore save_flag
+    = make_scoped_restore (&in_evpy_emit_event, true);
+
   /* Create a copy of call back list and use that for
      notifying listeners to avoid skipping callbacks
      in the case of a callback being disconnected during
diff --git a/gdb/python/py-event.h b/gdb/python/py-event.h
index 0a7d31d02e9..8e42bcd00dd 100644
--- a/gdb/python/py-event.h
+++ b/gdb/python/py-event.h
@@ -58,6 +58,11 @@ extern int emit_inferior_call_event (inferior_call_kind kind,
 extern int emit_register_changed_event (frame_info_ptr frame,
 					int regnum);
 extern int emit_memory_changed_event (CORE_ADDR addr, ssize_t len);
+
+/* Flag to indicate whether we are currently handling emitted Python
+   events.  */
+extern bool in_evpy_emit_event;
+
 extern int evpy_emit_event (PyObject *event,
 			    eventregistry_object *registry);
 
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 5719f351528..074b92c4d02 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -700,8 +700,13 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
 	 an exception reach the top level of the event loop, which are the
 	 two usual places in which stdin would be re-enabled. So, before we
 	 convert the exception and continue back in Python, we should
-	 re-enable stdin here.  */
-      async_enable_stdin ();
+	 re-enable stdin here, unless we are currently handling emitted
+	 Python events.  In that case, GDB would continue its normal course
+	 of execution and enable stdin itself in the way it normally
+	 does.  */
+      if (!in_evpy_emit_event)
+	async_enable_stdin ();
+
       GDB_PY_HANDLE_EXCEPTION (except);
     }
 
diff --git a/gdb/testsuite/gdb.python/py-cmd-exception.exp b/gdb/testsuite/gdb.python/py-cmd-exception.exp
new file mode 100644
index 00000000000..9cb4fd6b233
--- /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 causes
+# the GDB prompt to be displayed prematurely.
+
+load_lib gdb-python.exp
+
+require !use_gdb_stub allow_python_tests
+
+standard_testfile py-cmd.c
+
+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..846c41e07de
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-cmd-exception.py
@@ -0,0 +1,32 @@
+# 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
+
+        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

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

[Tom Tromey]

>>>>> "Tankut" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:

Tankut> +/* See py-event.h.  */
Tankut> +
Tankut> +bool in_evpy_emit_event = false;
 
It seems like there has to be a better way than introducing this global.

For one thing, are we sure this is the only possible context where this
bug can occur?

I wonder if execute_gdb_command could instead determine if stdin is
enabled before the 'try', and only re-enabled it if it was.

Tom

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

[Aktemur, Tankut Baris]

On Friday, February 24, 2023 8:43 PM, Tom Tromey wrote:
> >>>>> "Tankut" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tankut> +/* See py-event.h.  */
> Tankut> +
> Tankut> +bool in_evpy_emit_event = false;
> 
> It seems like there has to be a better way than introducing this global.
> 
> For one thing, are we sure this is the only possible context where this
> bug can occur?
> 
> I wonder if execute_gdb_command could instead determine if stdin is
> enabled before the 'try', and only re-enabled it if it was.

This would work, I think.  We have the following state:

  User runs a synchronous execution command such as "continue", "run", etc.
  --> command blocks the prompt
  --> Python API is invoked, e.g.  via events
  --> gdb.execute invoked inside Python
  --> command raises an exception
  --> we're inside the 'catch' block.

Before running the command that was passed to us with gdb.execute,
current_ui's prompt_state is already PROMPT_BLOCKED.  It seems we can
use this to omit enabling stdin.

I'll send the change as v2.

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

Re: [PATCH 1/2] gdb, python: do minor modernization in execute_gdb_command

[Tom Tromey]

>>>>> "Tankut" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:

Tankut> Use nullptr instead of NULL and boolify two local variables in
Tankut> execute_gdb_command.

Seems fine.
Approved-By: Tom Tromey <tom@tromey.com>

Tankut> +      from_tty = ((cmp == 0) ? false : true);

I'd probably write '= cmp != 0' but this is alright.

Tom

Re: [PATCH 1/2] gdb, python: do minor modernization in execute_gdb_command

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Tankut" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Tankut> Use nullptr instead of NULL and boolify two local variables in
> Tankut> execute_gdb_command.
>
> Seems fine.
> Approved-By: Tom Tromey <tom@tromey.com>
>
> Tankut> +      from_tty = ((cmp == 0) ? false : true);
>
> I'd probably write '= cmp != 0' but this is alright.

+1 from me.  Please don't use: 'COND ? false : true'.

Thanks,
Andrew

RE: [PATCH 1/2] gdb, python: do minor modernization in execute_gdb_command

[Aktemur, Tankut Baris]

On Saturday, February 25, 2023 12:08 PM, Andrew Burgess wrote:
> Tom Tromey <tom@tromey.com> writes:
> 
> >>>>>> "Tankut" == Tankut Baris Aktemur via Gdb-patches <gdb-patches@sourceware.org> writes:
> >
> > Tankut> Use nullptr instead of NULL and boolify two local variables in
> > Tankut> execute_gdb_command.
> >
> > Seems fine.
> > Approved-By: Tom Tromey <tom@tromey.com>
> >
> > Tankut> +      from_tty = ((cmp == 0) ? false : true);
> >
> > I'd probably write '= cmp != 0' but this is alright.
> 
> +1 from me.  Please don't use: 'COND ? false : true'.

I made this change (in two places) and pushed the patch.

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