[PATCH 1/2] gdb/python: generalize serialize_mi_result()

[PATCH 1/2] gdb/python: generalize serialize_mi_result()

[Jan Vrany]

This commit renames serialize_mi_result() to serialize_mi_data() and makes
it usable in different contexts than printing result of custom MI command
(hence the rename).

To do so, the check whether passed Python object is a dictionary has been
moved to the caller - at the very least, different usages require different
error messages. Also it seemed to me that py-utils.c is a better place
for its code as it is now a utility function not specific to Python MI
commands.

This is a preparation for implementing Python support for sending custom
MI async events.
---
 gdb/python/py-micmd.c        | 179 ++---------------------------------
 gdb/python/py-utils.c        | 145 ++++++++++++++++++++++++++++
 gdb/python/python-internal.h |  15 +++
 3 files changed, 166 insertions(+), 173 deletions(-)

diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
index 01fc6060ece..26231fc37e8 100644
--- a/gdb/python/py-micmd.c
+++ b/gdb/python/py-micmd.c
@@ -173,178 +173,6 @@ extern PyTypeObject micmdpy_object_type
 
 static PyObject *invoke_cst;
 
-/* Convert KEY_OBJ into a string that can be used as a field name in MI
-   output.  KEY_OBJ must be a Python string object, and must only contain
-   characters suitable for use as an MI field name.
-
-   If KEY_OBJ is not a string, or if KEY_OBJ contains invalid characters,
-   then an error is thrown.  Otherwise, KEY_OBJ is converted to a string
-   and returned.  */
-
-static gdb::unique_xmalloc_ptr<char>
-py_object_to_mi_key (PyObject *key_obj)
-{
-  /* The key must be a string.  */
-  if (!PyUnicode_Check (key_obj))
-    {
-      gdbpy_ref<> key_repr (PyObject_Repr (key_obj));
-      gdb::unique_xmalloc_ptr<char> key_repr_string;
-      if (key_repr != nullptr)
-	key_repr_string = python_string_to_target_string (key_repr.get ());
-      if (key_repr_string == nullptr)
-	gdbpy_handle_exception ();
-
-      gdbpy_error (_("non-string object used as key: %s"),
-		   key_repr_string.get ());
-    }
-
-  gdb::unique_xmalloc_ptr<char> key_string
-    = python_string_to_target_string (key_obj);
-  if (key_string == nullptr)
-    gdbpy_handle_exception ();
-
-  /* Predicate function, returns true if NAME is a valid field name for use
-     in MI result output, otherwise, returns false.  */
-  auto is_valid_key_name = [] (const char *name) -> bool
-  {
-    gdb_assert (name != nullptr);
-
-    if (*name == '\0' || !isalpha (*name))
-      return false;
-
-    for (; *name != '\0'; ++name)
-      if (!isalnum (*name) && *name != '_' && *name != '-')
-	return false;
-
-    return true;
-  };
-
-  if (!is_valid_key_name (key_string.get ()))
-    {
-      if (*key_string.get () == '\0')
-	gdbpy_error (_("Invalid empty key in MI result"));
-      else
-	gdbpy_error (_("Invalid key in MI result: %s"), key_string.get ());
-    }
-
-  return key_string;
-}
-
-/* Serialize RESULT and print it in MI format to the current_uiout.
-   FIELD_NAME is used as the name of this result field.
-
-   RESULT can be a dictionary, a sequence, an iterator, or an object that
-   can be converted to a string, these are converted to the matching MI
-   output format (dictionaries as tuples, sequences and iterators as lists,
-   and strings as named fields).
-
-   If anything goes wrong while formatting the output then an error is
-   thrown.
-
-   This function is the recursive inner core of serialize_mi_result, and
-   should only be called from that function.  */
-
-static void
-serialize_mi_result_1 (PyObject *result, const char *field_name)
-{
-  struct ui_out *uiout = current_uiout;
-
-  if (PyDict_Check (result))
-    {
-      PyObject *key, *value;
-      Py_ssize_t pos = 0;
-      ui_out_emit_tuple tuple_emitter (uiout, field_name);
-      while (PyDict_Next (result, &pos, &key, &value))
-	{
-	  gdb::unique_xmalloc_ptr<char> key_string
-	    (py_object_to_mi_key (key));
-	  serialize_mi_result_1 (value, key_string.get ());
-	}
-    }
-  else if (PySequence_Check (result) && !PyUnicode_Check (result))
-    {
-      ui_out_emit_list list_emitter (uiout, field_name);
-      Py_ssize_t len = PySequence_Size (result);
-      if (len == -1)
-	gdbpy_handle_exception ();
-      for (Py_ssize_t i = 0; i < len; ++i)
-	{
-	  gdbpy_ref<> item (PySequence_ITEM (result, i));
-	  if (item == nullptr)
-	    gdbpy_handle_exception ();
-	  serialize_mi_result_1 (item.get (), nullptr);
-	}
-    }
-  else if (PyIter_Check (result))
-    {
-      gdbpy_ref<> item;
-      ui_out_emit_list list_emitter (uiout, field_name);
-      while (true)
-	{
-	  item.reset (PyIter_Next (result));
-	  if (item == nullptr)
-	    {
-	      if (PyErr_Occurred () != nullptr)
-		gdbpy_handle_exception ();
-	      break;
-	    }
-	  serialize_mi_result_1 (item.get (), nullptr);
-	}
-    }
-  else
-    {
-      if (PyLong_Check (result))
-	{
-	  int overflow = 0;
-	  gdb_py_longest val = gdb_py_long_as_long_and_overflow (result,
-								 &overflow);
-	  if (PyErr_Occurred () != nullptr)
-	    gdbpy_handle_exception ();
-	  if (overflow == 0)
-	    {
-	      uiout->field_signed (field_name, val);
-	      return;
-	    }
-	  /* Fall through to the string case on overflow.  */
-	}
-
-      gdb::unique_xmalloc_ptr<char> string (gdbpy_obj_to_string (result));
-      if (string == nullptr)
-	gdbpy_handle_exception ();
-      uiout->field_string (field_name, string.get ());
-    }
-}
-
-/* Serialize RESULT and print it in MI format to the current_uiout.
-
-   This function handles the top-level result initially returned from the
-   invoke method of the Python command implementation.  At the top-level
-   the result must be a dictionary.  The values within this dictionary can
-   be a wider range of types.  Handling the values of the top-level
-   dictionary is done by serialize_mi_result_1, see that function for more
-   details.
-
-   If anything goes wrong while parsing and printing the MI output then an
-   error is thrown.  */
-
-static void
-serialize_mi_result (PyObject *result)
-{
-  /* At the top-level, the result must be a dictionary.  */
-
-  if (!PyDict_Check (result))
-    gdbpy_error (_("Result from invoke must be a dictionary"));
-
-  PyObject *key, *value;
-  Py_ssize_t pos = 0;
-  while (PyDict_Next (result, &pos, &key, &value))
-    {
-      gdb::unique_xmalloc_ptr<char> key_string
-	(py_object_to_mi_key (key));
-      serialize_mi_result_1 (value, key_string.get ());
-    }
-}
-
 /* Called when the MI command is invoked.  PARSE contains the parsed
    command line arguments from the user.  */
 
@@ -388,7 +216,12 @@ mi_command_py::invoke (struct mi_parse *parse) const
     gdbpy_handle_exception ();
 
   if (result != Py_None)
-    serialize_mi_result (result.get ());
+    {
+      /* At the top-level, the result must be a dictionary.  */
+      if (!PyDict_Check (result.get ()))
+        gdbpy_error (_("Result from invoke must be a dictionary"));
+      serialize_mi_data (result.get ());
+    }
 }
 
 /* See declaration above.  */
diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
index d5b07a80d82..72a232f251d 100644
--- a/gdb/python/py-utils.c
+++ b/gdb/python/py-utils.c
@@ -597,3 +597,148 @@ gdbpy_fix_doc_string_indentation (gdb::unique_xmalloc_ptr<char> doc)
 
   return doc;
 }
+
+/* Convert KEY_OBJ into a string that can be used as a field name in MI
+   output.  KEY_OBJ must be a Python string object, and must only contain
+   characters suitable for use as an MI field name.
+
+   If KEY_OBJ is not a string, or if KEY_OBJ contains invalid characters,
+   then an error is thrown.  Otherwise, KEY_OBJ is converted to a string
+   and returned.  */
+
+static gdb::unique_xmalloc_ptr<char>
+py_object_to_mi_key (PyObject *key_obj)
+{
+  /* The key must be a string.  */
+  if (!PyUnicode_Check (key_obj))
+    {
+      gdbpy_ref<> key_repr (PyObject_Repr (key_obj));
+      gdb::unique_xmalloc_ptr<char> key_repr_string;
+      if (key_repr != nullptr)
+	key_repr_string = python_string_to_target_string (key_repr.get ());
+      if (key_repr_string == nullptr)
+	gdbpy_handle_exception ();
+
+      gdbpy_error (_("non-string object used as key: %s"),
+		   key_repr_string.get ());
+    }
+
+  gdb::unique_xmalloc_ptr<char> key_string
+    = python_string_to_target_string (key_obj);
+  if (key_string == nullptr)
+    gdbpy_handle_exception ();
+
+  /* Predicate function, returns true if NAME is a valid field name for use
+     in MI result output, otherwise, returns false.  */
+  auto is_valid_key_name = [] (const char *name) -> bool
+  {
+    gdb_assert (name != nullptr);
+
+    if (*name == '\0' || !isalpha (*name))
+      return false;
+
+    for (; *name != '\0'; ++name)
+      if (!isalnum (*name) && *name != '_' && *name != '-')
+	return false;
+
+    return true;
+  };
+
+  if (!is_valid_key_name (key_string.get ()))
+    {
+      if (*key_string.get () == '\0')
+	gdbpy_error (_("Invalid empty key in MI result"));
+      else
+	gdbpy_error (_("Invalid key in MI result: %s"), key_string.get ());
+    }
+
+  return key_string;
+}
+
+static void
+serialize_mi_data_1 (PyObject *data, const char *field_name)
+{
+  struct ui_out *uiout = current_uiout;
+
+  if (PyDict_Check (data))
+    {
+      PyObject *key, *value;
+      Py_ssize_t pos = 0;
+      ui_out_emit_tuple tuple_emitter (uiout, field_name);
+      while (PyDict_Next (data, &pos, &key, &value))
+	{
+	  gdb::unique_xmalloc_ptr<char> key_string
+	    (py_object_to_mi_key (key));
+	  serialize_mi_data_1 (value, key_string.get ());
+	}
+    }
+  else if (PySequence_Check (data) && !PyUnicode_Check (data))
+    {
+      ui_out_emit_list list_emitter (uiout, field_name);
+      Py_ssize_t len = PySequence_Size (data);
+      if (len == -1)
+	gdbpy_handle_exception ();
+      for (Py_ssize_t i = 0; i < len; ++i)
+	{
+	  gdbpy_ref<> item (PySequence_ITEM (data, i));
+	  if (item == nullptr)
+	    gdbpy_handle_exception ();
+	  serialize_mi_data_1 (item.get (), nullptr);
+	}
+    }
+  else if (PyIter_Check (data))
+    {
+      gdbpy_ref<> item;
+      ui_out_emit_list list_emitter (uiout, field_name);
+      while (true)
+	{
+	  item.reset (PyIter_Next (data));
+	  if (item == nullptr)
+	    {
+	      if (PyErr_Occurred () != nullptr)
+		gdbpy_handle_exception ();
+	      break;
+	    }
+	  serialize_mi_data_1 (item.get (), nullptr);
+	}
+    }
+  else
+    {
+      if (PyLong_Check (data))
+	{
+	  int overflow = 0;
+	  gdb_py_longest val = gdb_py_long_as_long_and_overflow (data,
+								 &overflow);
+	  if (PyErr_Occurred () != nullptr)
+	    gdbpy_handle_exception ();
+	  if (overflow == 0)
+	    {
+	      uiout->field_signed (field_name, val);
+	      return;
+	    }
+	  /* Fall through to the string case on overflow.  */
+	}
+
+      gdb::unique_xmalloc_ptr<char> string (gdbpy_obj_to_string (data));
+      if (string == nullptr)
+	gdbpy_handle_exception ();
+      uiout->field_string (field_name, string.get ());
+    }
+}
+
+/* See python-internal.h.  */
+
+void
+serialize_mi_data (PyObject *data)
+{
+  gdb_assert (PyDict_Check (data));
+
+  PyObject *key, *value;
+  Py_ssize_t pos = 0;
+  while (PyDict_Next (data, &pos, &key, &value))
+    {
+      gdb::unique_xmalloc_ptr<char> key_string
+	(py_object_to_mi_key (key));
+      serialize_mi_data_1 (value, key_string.get ());
+    }
+}
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 93217375cc5..3f53b0ab6f0 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -486,6 +486,21 @@ struct gdbarch *arch_object_to_gdbarch (PyObject *obj);
 extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
 					   PyObject *kw);
 
+/* Serialize DATA and print it in MI format to the current_uiout.
+
+   This function handles the top-level result initially returned from the
+   invoke method of the Python command implementation.  At the top-level
+   the data must be a dictionary.  Caller of this function is responsible
+   for ensuring that.  The values within this dictionary can
+   be a wider range of types.  Handling the values of the top-level
+   dictionary is done by serialize_mi_data_1, see that function for more
+   details.
+
+   If anything goes wrong while parsing and printing the MI output then an
+   error is thrown.  */
+
+extern void serialize_mi_data (PyObject *result);
+
 /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
    gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
    valid (see gdb.Progspace.is_valid), otherwise return the program_space
-- 
2.40.1

[PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Jan Vrany]

This commit adds a new Python function, gdb.notify_mi, that can be used
to emit custom async notification to MI channel.  This can be used, among
other things, to implement notifications about events MI does not support,
such as remote connection closed or register change.
---
 gdb/NEWS                                  |  3 ++
 gdb/doc/python.texi                       | 38 ++++++++++++++++++
 gdb/python/py-mi.c                        | 49 +++++++++++++++++++++++
 gdb/python/python-internal.h              |  3 ++
 gdb/python/python.c                       |  4 ++
 gdb/testsuite/gdb.python/py-mi-notify.exp | 47 ++++++++++++++++++++++
 6 files changed, 144 insertions(+)
 create mode 100644 gdb/testsuite/gdb.python/py-mi-notify.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 98ff00d5efc..4a1f383a666 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -286,6 +286,9 @@ info main
      might be array- or string-like, even if they do not have the
      corresponding type code.
 
+  ** New function gdb.notify_mi(NAME, DATA), that emits custom
+     GDB/MI async notification.
+
 *** Changes in GDB 13
 
 * MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index e9936991c49..cb2073976ba 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -4704,6 +4704,44 @@ Here is how this works using the commands from the example above:
 @{'string': 'abc, def, ghi'@}
 @end smallexample
 
+@node GDB/MI Notifications In Python
+@subsubsection @sc{gdb/mi} Notifications In Python
+
+@cindex MI notifications in python
+@cindex notifications in python, GDB/MI
+@cindex python notifications, GDB/MI
+
+It is possible to emit @sc{gdb/mi} notifications from
+Python.  This is done with the @code{gdb.notify_mi} function.
+
+@defun gdb.notify_mi (name , data)
+Emit a @sc{gdb/mi} asynchronous notification.  @var{name} is the name of the
+notification, a string.  @var{data} are additional values emitted with the notification, passed
+as Python dictionary. The dictionary is converted to converted
+to a @sc{gdb/mi} @var{result-record} (@pxref{GDB/MI Output Syntax}) the same way
+as result of Python MI command (@pxref{GDB/MI Commands In Python}).
+
+If @var{data} is @code{None} then no additional values are emitted.
+@end defun
+
+Here is how to emit @code{=connection-removed} whenever a connection to remote
+GDB server is closed (see @pxref{Connections In Python}):
+
+@smallexample
+def notify_connection_removed (event):
+    data = @{ 'id'   : event.connection.num,
+              'type' : event.connection.type @}
+    gdb.notify_mi("connection-removed", data)
+
+gdb.events.connection_removed.connect (notify_connection_removed)
+@end smallexample
+
+Then, each time a connection is closed, there will be a notification on MI channel:
+
+@smallexample
+=connection-removed,id="1",type="remote"
+@end smallexample
+
 @node Parameters In Python
 @subsubsection Parameters In Python
 
diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
index 66dc6fb8a32..dc2ec5ddd0f 100644
--- a/gdb/python/py-mi.c
+++ b/gdb/python/py-mi.c
@@ -19,8 +19,14 @@
 
 #include "defs.h"
 #include "python-internal.h"
+#include "utils.h"
+#include "ui.h"
 #include "ui-out.h"
+#include "interps.h"
+#include "target.h"
 #include "mi/mi-parse.h"
+#include "mi/mi-console.h"
+#include "mi/mi-interp.h"
 
 /* A ui_out subclass that creates a Python object based on the data
    that is passed in.  */
@@ -296,3 +302,46 @@ gdbpy_execute_mi_command (PyObject *self, PyObject *args, PyObject *kw)
 
   return uiout.result ();
 }
+
+/* Implementation of the gdb.notify_mi function.  */
+
+PyObject *
+gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs)
+{
+  static const char *keywords[] = { "name", "data", nullptr };
+  const char *name;
+  PyObject *data;
+
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "sO", keywords,
+					&name, &data))
+    return nullptr; // FIXME: is this the correct way to signal error?
+
+  SWITCH_THRU_ALL_UIS ()
+    {
+      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
+
+      if (mi == NULL)
+        continue;
+      
+      gdb_printf (mi->event_channel, "%s", name);
+      if (data != Py_None)      
+        {
+          /* At the top-level, the data must be a dictionary.  */
+          if (!PyDict_Check (data))
+            gdbpy_error (_("Data passed to notify_mi must be either None or a dictionary"));
+
+          target_terminal::scoped_restore_terminal_state term_state;
+          target_terminal::ours_for_output ();
+
+          ui_out *mi_uiout = mi->interp_ui_out ();
+          ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
+          scoped_restore restore_uiout
+            = make_scoped_restore (&current_uiout, mi_uiout);
+
+          serialize_mi_data (data);	  
+        }
+      gdb_flush (mi->event_channel);
+    }
+    
+  Py_RETURN_NONE;
+}
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 3f53b0ab6f0..2a7e8d68179 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -501,6 +501,9 @@ extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
 
 extern void serialize_mi_data (PyObject *result);
 
+extern PyObject *gdbpy_notify_mi (PyObject *self, PyObject *args,
+				  PyObject *kw);
+
 /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
    gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
    valid (see gdb.Progspace.is_valid), otherwise return the program_space
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 6a978d632e9..faa7e0c217d 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -2669,6 +2669,10 @@ Return the name of the currently selected language." },
     "print_options () -> dict\n\
 Return the current print options." },
 
+  { "notify_mi", (PyCFunction) gdbpy_notify_mi,
+    METH_VARARGS | METH_KEYWORDS,
+    "notify_mi (name, data) -> None\n\
+Output async record to MI channels if any." },
   {NULL, NULL, 0, NULL}
 };
 
diff --git a/gdb/testsuite/gdb.python/py-mi-notify.exp b/gdb/testsuite/gdb.python/py-mi-notify.exp
new file mode 100644
index 00000000000..8221794c4f3
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-notify.exp
@@ -0,0 +1,47 @@
+# Copyright (C) 2019-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/>.
+
+# Test custom MI notifications implemented in Python.
+
+load_lib gdb-python.exp
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+gdb_exit
+if {[mi_gdb_start]} {
+    return
+}
+
+if {[lsearch -exact [mi_get_features] python] < 0} {
+    unsupported "python support is disabled"
+    return -1
+}
+
+standard_testfile
+
+mi_gdb_test "set python print-stack full" \
+    ".*\\^done" \
+    "set python print-stack full"
+
+mi_gdb_test "python gdb.notify_mi('test-notification', None)" \
+    ".*=test-notification\r\n\\^done" \
+    "python notification, no additional data"
+
+mi_gdb_test "python gdb.notify_mi('test-notification', \{ 'data1' : 1 , 'data2' : 2 })" \
+    ".*=test-notification,data1=\"1\",data2=\"2\"\r\n\\^done" \
+    "python notification, with additional data"
+
+mi_gdb_test "python gdb.notify_mi('test-notification', 1)" \
+    ".*\\^error,msg=\".*\"" \
+    "python notification, invalid additional data"
-- 
2.40.1

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Eli Zaretskii]

> CC: Jan Vrany <jan.vrany@labware.com>
> Date: Fri,  8 Sep 2023 22:05:04 +0100
> From: Jan Vrany via Gdb-patches <gdb-patches@sourceware.org>
> 
> This commit adds a new Python function, gdb.notify_mi, that can be used
> to emit custom async notification to MI channel.  This can be used, among
> other things, to implement notifications about events MI does not support,
> such as remote connection closed or register change.
> ---
>  gdb/NEWS                                  |  3 ++
>  gdb/doc/python.texi                       | 38 ++++++++++++++++++
>  gdb/python/py-mi.c                        | 49 +++++++++++++++++++++++
>  gdb/python/python-internal.h              |  3 ++
>  gdb/python/python.c                       |  4 ++
>  gdb/testsuite/gdb.python/py-mi-notify.exp | 47 ++++++++++++++++++++++
>  6 files changed, 144 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.python/py-mi-notify.exp

Thanks.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 98ff00d5efc..4a1f383a666 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -286,6 +286,9 @@ info main
>       might be array- or string-like, even if they do not have the
>       corresponding type code.
>  
> +  ** New function gdb.notify_mi(NAME, DATA), that emits custom
> +     GDB/MI async notification.
> +

This part is okay.

> +It is possible to emit @sc{gdb/mi} notifications from
> +Python.  This is done with the @code{gdb.notify_mi} function.

We try to avoid using passive voice when it is not required.  Suggest
to rephrase the last sentence:

  Use the @code{gdb.notify_mi} function to do that.

> +Emit a @sc{gdb/mi} asynchronous notification.  @var{name} is the name of the
> +notification, a string.

This begs the question: is any string allowed?  If any string is
allowed as NAME, I think you should tell which field of the async MI
notification will be set from that string.  If NAME must be one of a
fixed list of known notifications, we should say something about where
to find that list.

> +as Python dictionary. The dictionary is converted to converted
                       ^^
Two spaces there.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Andrew Burgess]

Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:

> This commit adds a new Python function, gdb.notify_mi, that can be used
> to emit custom async notification to MI channel.  This can be used, among
> other things, to implement notifications about events MI does not support,
> such as remote connection closed or register change.

Sounds interesting.  I haven't looked at the code yet, but did have some
docs feedback.

> ---
>  gdb/NEWS                                  |  3 ++
>  gdb/doc/python.texi                       | 38 ++++++++++++++++++
>  gdb/python/py-mi.c                        | 49 +++++++++++++++++++++++
>  gdb/python/python-internal.h              |  3 ++
>  gdb/python/python.c                       |  4 ++
>  gdb/testsuite/gdb.python/py-mi-notify.exp | 47 ++++++++++++++++++++++
>  6 files changed, 144 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.python/py-mi-notify.exp
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 98ff00d5efc..4a1f383a666 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -286,6 +286,9 @@ info main
>       might be array- or string-like, even if they do not have the
>       corresponding type code.
>  
> +  ** New function gdb.notify_mi(NAME, DATA), that emits custom
> +     GDB/MI async notification.
> +
>  *** Changes in GDB 13
>  
>  * MI version 1 is deprecated, and will be removed in GDB 14.
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index e9936991c49..cb2073976ba 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -4704,6 +4704,44 @@ Here is how this works using the commands from the example above:
>  @{'string': 'abc, def, ghi'@}
>  @end smallexample
>  
> +@node GDB/MI Notifications In Python

You need to add 'GDB/MI Notifications In Python' to the @menu section.
As things currently are I get a build error when trying to create the
docs.

> +@subsubsection @sc{gdb/mi} Notifications In Python
> +
> +@cindex MI notifications in python
> +@cindex notifications in python, GDB/MI
> +@cindex python notifications, GDB/MI

I think these @cindex entries are supposed to appear at least before the
@subsubsection heading, that way, when following the link from the index
you'll be taken to the relevant heading rather than the body text -
though I could be wrong about this, Eli might have some thoughts.

> +
> +It is possible to emit @sc{gdb/mi} notifications from
> +Python.  This is done with the @code{gdb.notify_mi} function.
> +
> +@defun gdb.notify_mi (name , data)
> +Emit a @sc{gdb/mi} asynchronous notification.  @var{name} is the name of the
> +notification, a string.  @var{data} are additional values emitted with the notification, passed
> +as Python dictionary. The dictionary is converted to converted
> +to a @sc{gdb/mi} @var{result-record} (@pxref{GDB/MI Output Syntax}) the same way
> +as result of Python MI command (@pxref{GDB/MI Commands In Python}).
> +
> +If @var{data} is @code{None} then no additional values are emitted.
> +@end defun
> +
> +Here is how to emit @code{=connection-removed} whenever a connection to remote
> +GDB server is closed (see @pxref{Connections In Python}):
> +
> +@smallexample
> +def notify_connection_removed (event):
> +    data = @{ 'id'   : event.connection.num,
> +              'type' : event.connection.type @}
> +    gdb.notify_mi("connection-removed", data)

I've been trying to format my Python example code in the same style as
`black` would reformat it.  Though I understand you might feel it is
clearer not to do that.

Still I see two things I think should be fixed here.  First, you are
inconsistent as to whether you use a space before the '('.  In two
places you have a space, and in one case you don't.  I think you should
standardise on no space -- that seems to be the Python way.

Then in the data layout, the spacing is off.  You use extra space so the
':' align, but when the info document is rendered, the '@' are not
present, as a consequence the ':' will be slightly out of alignment.

Thanks,
Andrew

> +
> +gdb.events.connection_removed.connect (notify_connection_removed)
> +@end smallexample
> +
> +Then, each time a connection is closed, there will be a notification on MI channel:
> +
> +@smallexample
> +=connection-removed,id="1",type="remote"
> +@end smallexample
> +
>  @node Parameters In Python
>  @subsubsection Parameters In Python
>  
> diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
> index 66dc6fb8a32..dc2ec5ddd0f 100644
> --- a/gdb/python/py-mi.c
> +++ b/gdb/python/py-mi.c
> @@ -19,8 +19,14 @@
>  
>  #include "defs.h"
>  #include "python-internal.h"
> +#include "utils.h"
> +#include "ui.h"
>  #include "ui-out.h"
> +#include "interps.h"
> +#include "target.h"
>  #include "mi/mi-parse.h"
> +#include "mi/mi-console.h"
> +#include "mi/mi-interp.h"
>  
>  /* A ui_out subclass that creates a Python object based on the data
>     that is passed in.  */
> @@ -296,3 +302,46 @@ gdbpy_execute_mi_command (PyObject *self, PyObject *args, PyObject *kw)
>  
>    return uiout.result ();
>  }
> +
> +/* Implementation of the gdb.notify_mi function.  */
> +
> +PyObject *
> +gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs)
> +{
> +  static const char *keywords[] = { "name", "data", nullptr };
> +  const char *name;
> +  PyObject *data;
> +
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "sO", keywords,
> +					&name, &data))
> +    return nullptr; // FIXME: is this the correct way to signal error?
> +
> +  SWITCH_THRU_ALL_UIS ()
> +    {
> +      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
> +
> +      if (mi == NULL)
> +        continue;
> +      
> +      gdb_printf (mi->event_channel, "%s", name);
> +      if (data != Py_None)      
> +        {
> +          /* At the top-level, the data must be a dictionary.  */
> +          if (!PyDict_Check (data))
> +            gdbpy_error (_("Data passed to notify_mi must be either None or a dictionary"));
> +
> +          target_terminal::scoped_restore_terminal_state term_state;
> +          target_terminal::ours_for_output ();
> +
> +          ui_out *mi_uiout = mi->interp_ui_out ();
> +          ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
> +          scoped_restore restore_uiout
> +            = make_scoped_restore (&current_uiout, mi_uiout);
> +
> +          serialize_mi_data (data);	  
> +        }
> +      gdb_flush (mi->event_channel);
> +    }
> +    
> +  Py_RETURN_NONE;
> +}
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 3f53b0ab6f0..2a7e8d68179 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -501,6 +501,9 @@ extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
>  
>  extern void serialize_mi_data (PyObject *result);
>  
> +extern PyObject *gdbpy_notify_mi (PyObject *self, PyObject *args,
> +				  PyObject *kw);
> +
>  /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
>     gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
>     valid (see gdb.Progspace.is_valid), otherwise return the program_space
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 6a978d632e9..faa7e0c217d 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -2669,6 +2669,10 @@ Return the name of the currently selected language." },
>      "print_options () -> dict\n\
>  Return the current print options." },
>  
> +  { "notify_mi", (PyCFunction) gdbpy_notify_mi,
> +    METH_VARARGS | METH_KEYWORDS,
> +    "notify_mi (name, data) -> None\n\
> +Output async record to MI channels if any." },
>    {NULL, NULL, 0, NULL}
>  };
>  
> diff --git a/gdb/testsuite/gdb.python/py-mi-notify.exp b/gdb/testsuite/gdb.python/py-mi-notify.exp
> new file mode 100644
> index 00000000000..8221794c4f3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-mi-notify.exp
> @@ -0,0 +1,47 @@
> +# Copyright (C) 2019-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/>.
> +
> +# Test custom MI notifications implemented in Python.
> +
> +load_lib gdb-python.exp
> +load_lib mi-support.exp
> +set MIFLAGS "-i=mi"
> +
> +gdb_exit
> +if {[mi_gdb_start]} {
> +    return
> +}
> +
> +if {[lsearch -exact [mi_get_features] python] < 0} {
> +    unsupported "python support is disabled"
> +    return -1
> +}
> +
> +standard_testfile
> +
> +mi_gdb_test "set python print-stack full" \
> +    ".*\\^done" \
> +    "set python print-stack full"
> +
> +mi_gdb_test "python gdb.notify_mi('test-notification', None)" \
> +    ".*=test-notification\r\n\\^done" \
> +    "python notification, no additional data"
> +
> +mi_gdb_test "python gdb.notify_mi('test-notification', \{ 'data1' : 1 , 'data2' : 2 })" \
> +    ".*=test-notification,data1=\"1\",data2=\"2\"\r\n\\^done" \
> +    "python notification, with additional data"
> +
> +mi_gdb_test "python gdb.notify_mi('test-notification', 1)" \
> +    ".*\\^error,msg=\".*\"" \
> +    "python notification, invalid additional data"
> -- 
> 2.40.1

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Jan Vraný]

On Mon, 2023-09-11 at 13:42 +0100, Andrew Burgess wrote:
> Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > This commit adds a new Python function, gdb.notify_mi, that can be used
> > to emit custom async notification to MI channel.  This can be used, among
> > other things, to implement notifications about events MI does not support,
> > such as remote connection closed or register change.
> 
> Sounds interesting.  I haven't looked at the code yet, but did have some
> docs feedback.

I've just discovered weird error in the code and limit / clarify what characters
notification name may contain as Eli suggested so maybe wait with looking at the
code until I send V2 with your and Eli's comments addressed. 

> 
> > ---
> >  gdb/NEWS                                  |  3 ++
> >  gdb/doc/python.texi                       | 38 ++++++++++++++++++
> >  gdb/python/py-mi.c                        | 49 +++++++++++++++++++++++
> >  gdb/python/python-internal.h              |  3 ++
> >  gdb/python/python.c                       |  4 ++
> >  gdb/testsuite/gdb.python/py-mi-notify.exp | 47 ++++++++++++++++++++++
> >  6 files changed, 144 insertions(+)
> >  create mode 100644 gdb/testsuite/gdb.python/py-mi-notify.exp
> > 
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index 98ff00d5efc..4a1f383a666 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -286,6 +286,9 @@ info main
> >       might be array- or string-like, even if they do not have the
> >       corresponding type code.
> >  
> > +  ** New function gdb.notify_mi(NAME, DATA), that emits custom
> > +     GDB/MI async notification.
> > +
> >  *** Changes in GDB 13
> >  
> >  * MI version 1 is deprecated, and will be removed in GDB 14.
> > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> > index e9936991c49..cb2073976ba 100644
> > --- a/gdb/doc/python.texi
> > +++ b/gdb/doc/python.texi
> > @@ -4704,6 +4704,44 @@ Here is how this works using the commands from the example above:
> >  @{'string': 'abc, def, ghi'@}
> >  @end smallexample
> >  
> > +@node GDB/MI Notifications In Python
> 
> You need to add 'GDB/MI Notifications In Python' to the @menu section.
> As things currently are I get a build error when trying to create the
> docs.
> 
> > +@subsubsection @sc{gdb/mi} Notifications In Python
> > +
> > +@cindex MI notifications in python
> > +@cindex notifications in python, GDB/MI
> > +@cindex python notifications, GDB/MI
> 
> I think these @cindex entries are supposed to appear at least before the
> @subsubsection heading, that way, when following the link from the index
> you'll be taken to the relevant heading rather than the body text -
> though I could be wrong about this, Eli might have some thoughts.
> 
> > +
> > +It is possible to emit @sc{gdb/mi} notifications from
> > +Python.  This is done with the @code{gdb.notify_mi} function.
> > +
> > +@defun gdb.notify_mi (name , data)
> > +Emit a @sc{gdb/mi} asynchronous notification.  @var{name} is the name of the
> > +notification, a string.  @var{data} are additional values emitted with the notification, passed
> > +as Python dictionary. The dictionary is converted to converted
> > +to a @sc{gdb/mi} @var{result-record} (@pxref{GDB/MI Output Syntax}) the same way
> > +as result of Python MI command (@pxref{GDB/MI Commands In Python}).
> > +
> > +If @var{data} is @code{None} then no additional values are emitted.
> > +@end defun
> > +
> > +Here is how to emit @code{=connection-removed} whenever a connection to remote
> > +GDB server is closed (see @pxref{Connections In Python}):
> > +
> > +@smallexample
> > +def notify_connection_removed (event):
> > +    data = @{ 'id'   : event.connection.num,
> > +              'type' : event.connection.type @}
> > +    gdb.notify_mi("connection-removed", data)
> 
> I've been trying to format my Python example code in the same style as
> `black` would reformat it.  Though I understand you might feel it is
> clearer not to do that.
> 
> Still I see two things I think should be fixed here.  First, you are
> inconsistent as to whether you use a space before the '('.  In two
> places you have a space, and in one case you don't.  I think you should
> standardise on no space -- that seems to be the Python way.
> 
> Then in the data layout, the spacing is off.  You use extra space so the
> ':' align, but when the info document is rendered, the '@' are not
> present, as a consequence the ':' will be slightly out of alignment.
> 

Thanks! Will fix that in V2. 

Jan
> Thanks,
> Andrew
> 
> > +
> > +gdb.events.connection_removed.connect (notify_connection_removed)
> > +@end smallexample
> > +
> > +Then, each time a connection is closed, there will be a notification on MI channel:
> > +
> > +@smallexample
> > +=connection-removed,id="1",type="remote"
> > +@end smallexample
> > +
> >  @node Parameters In Python
> >  @subsubsection Parameters In Python
> >  
> > diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
> > index 66dc6fb8a32..dc2ec5ddd0f 100644
> > --- a/gdb/python/py-mi.c
> > +++ b/gdb/python/py-mi.c
> > @@ -19,8 +19,14 @@
> >  
> >  #include "defs.h"
> >  #include "python-internal.h"
> > +#include "utils.h"
> > +#include "ui.h"
> >  #include "ui-out.h"
> > +#include "interps.h"
> > +#include "target.h"
> >  #include "mi/mi-parse.h"
> > +#include "mi/mi-console.h"
> > +#include "mi/mi-interp.h"
> >  
> >  /* A ui_out subclass that creates a Python object based on the data
> >     that is passed in.  */
> > @@ -296,3 +302,46 @@ gdbpy_execute_mi_command (PyObject *self, PyObject *args, PyObject *kw)
> >  
> >    return uiout.result ();
> >  }
> > +
> > +/* Implementation of the gdb.notify_mi function.  */
> > +
> > +PyObject *
> > +gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs)
> > +{
> > +  static const char *keywords[] = { "name", "data", nullptr };
> > +  const char *name;
> > +  PyObject *data;
> > +
> > +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "sO", keywords,
> > +					&name, &data))
> > +    return nullptr; // FIXME: is this the correct way to signal error?
> > +
> > +  SWITCH_THRU_ALL_UIS ()
> > +    {
> > +      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
> > +
> > +      if (mi == NULL)
> > +        continue;
> > +      
> > +      gdb_printf (mi->event_channel, "%s", name);
> > +      if (data != Py_None)      
> > +        {
> > +          /* At the top-level, the data must be a dictionary.  */
> > +          if (!PyDict_Check (data))
> > +            gdbpy_error (_("Data passed to notify_mi must be either None or a dictionary"));
> > +
> > +          target_terminal::scoped_restore_terminal_state term_state;
> > +          target_terminal::ours_for_output ();
> > +
> > +          ui_out *mi_uiout = mi->interp_ui_out ();
> > +          ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
> > +          scoped_restore restore_uiout
> > +            = make_scoped_restore (&current_uiout, mi_uiout);
> > +
> > +          serialize_mi_data (data);	  
> > +        }
> > +      gdb_flush (mi->event_channel);
> > +    }
> > +    
> > +  Py_RETURN_NONE;
> > +}
> > diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> > index 3f53b0ab6f0..2a7e8d68179 100644
> > --- a/gdb/python/python-internal.h
> > +++ b/gdb/python/python-internal.h
> > @@ -501,6 +501,9 @@ extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
> >  
> >  extern void serialize_mi_data (PyObject *result);
> >  
> > +extern PyObject *gdbpy_notify_mi (PyObject *self, PyObject *args,
> > +				  PyObject *kw);
> > +
> >  /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
> >     gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
> >     valid (see gdb.Progspace.is_valid), otherwise return the program_space
> > diff --git a/gdb/python/python.c b/gdb/python/python.c
> > index 6a978d632e9..faa7e0c217d 100644
> > --- a/gdb/python/python.c
> > +++ b/gdb/python/python.c
> > @@ -2669,6 +2669,10 @@ Return the name of the currently selected language." },
> >      "print_options () -> dict\n\
> >  Return the current print options." },
> >  
> > +  { "notify_mi", (PyCFunction) gdbpy_notify_mi,
> > +    METH_VARARGS | METH_KEYWORDS,
> > +    "notify_mi (name, data) -> None\n\
> > +Output async record to MI channels if any." },
> >    {NULL, NULL, 0, NULL}
> >  };
> >  
> > diff --git a/gdb/testsuite/gdb.python/py-mi-notify.exp b/gdb/testsuite/gdb.python/py-mi-notify.exp
> > new file mode 100644
> > index 00000000000..8221794c4f3
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-mi-notify.exp
> > @@ -0,0 +1,47 @@
> > +# Copyright (C) 2019-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>.
> > +
> > +# Test custom MI notifications implemented in Python.
> > +
> > +load_lib gdb-python.exp
> > +load_lib mi-support.exp
> > +set MIFLAGS "-i=mi"
> > +
> > +gdb_exit
> > +if {[mi_gdb_start]} {
> > +    return
> > +}
> > +
> > +if {[lsearch -exact [mi_get_features] python] < 0} {
> > +    unsupported "python support is disabled"
> > +    return -1
> > +}
> > +
> > +standard_testfile
> > +
> > +mi_gdb_test "set python print-stack full" \
> > +    ".*\\^done" \
> > +    "set python print-stack full"
> > +
> > +mi_gdb_test "python gdb.notify_mi('test-notification', None)" \
> > +    ".*=test-notification\r\n\\^done" \
> > +    "python notification, no additional data"
> > +
> > +mi_gdb_test "python gdb.notify_mi('test-notification', \{ 'data1' : 1 , 'data2' : 2 })" \
> > +    ".*=test-notification,data1=\"1\",data2=\"2\"\r\n\\^done" \
> > +    "python notification, with additional data"
> > +
> > +mi_gdb_test "python gdb.notify_mi('test-notification', 1)" \
> > +    ".*\\^error,msg=\".*\"" \
> > +    "python notification, invalid additional data"
> > -- 
> > 2.40.1

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Eli Zaretskii]

> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Jan Vrany <jan.vrany@labware.com>, Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 11 Sep 2023 13:42:14 +0100
> 
> > +@subsubsection @sc{gdb/mi} Notifications In Python
> > +
> > +@cindex MI notifications in python
> > +@cindex notifications in python, GDB/MI
> > +@cindex python notifications, GDB/MI
> 
> I think these @cindex entries are supposed to appear at least before the
> @subsubsection heading, that way, when following the link from the index
> you'll be taken to the relevant heading rather than the body text -
> though I could be wrong about this, Eli might have some thoughts.

No, what you say is correct for @item, but not for sectioning
commands.  In the latter case, we want to index entry to land the
reader on the text, not on the section name.

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Andrew Burgess]

Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:

> This commit adds a new Python function, gdb.notify_mi, that can be used
> to emit custom async notification to MI channel.  This can be used, among
> other things, to implement notifications about events MI does not support,
> such as remote connection closed or register change.
> ---
>  gdb/NEWS                                  |  3 ++
>  gdb/doc/python.texi                       | 38 ++++++++++++++++++
>  gdb/python/py-mi.c                        | 49 +++++++++++++++++++++++
>  gdb/python/python-internal.h              |  3 ++
>  gdb/python/python.c                       |  4 ++
>  gdb/testsuite/gdb.python/py-mi-notify.exp | 47 ++++++++++++++++++++++
>  6 files changed, 144 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.python/py-mi-notify.exp
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 98ff00d5efc..4a1f383a666 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -286,6 +286,9 @@ info main
>       might be array- or string-like, even if they do not have the
>       corresponding type code.
>  
> +  ** New function gdb.notify_mi(NAME, DATA), that emits custom
> +     GDB/MI async notification.
> +
>  *** Changes in GDB 13
>  
>  * MI version 1 is deprecated, and will be removed in GDB 14.
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index e9936991c49..cb2073976ba 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -4704,6 +4704,44 @@ Here is how this works using the commands from the example above:
>  @{'string': 'abc, def, ghi'@}
>  @end smallexample
>  
> +@node GDB/MI Notifications In Python
> +@subsubsection @sc{gdb/mi} Notifications In Python
> +
> +@cindex MI notifications in python
> +@cindex notifications in python, GDB/MI
> +@cindex python notifications, GDB/MI
> +
> +It is possible to emit @sc{gdb/mi} notifications from
> +Python.  This is done with the @code{gdb.notify_mi} function.
> +
> +@defun gdb.notify_mi (name , data)
> +Emit a @sc{gdb/mi} asynchronous notification.  @var{name} is the name of the
> +notification, a string.  @var{data} are additional values emitted with the notification, passed

I think this reads better:

  @var{data} is any additional data to be emitted with the notification,
  passed as a Python dictionary.

Eli already asked about which values of @var{name} are valid.  I can see
from the code that _any_ value is accepted, however, a user should
probably be careful not to reuse a name that GDB already uses -- or if
they do then they should ensure that all the required fields are
emitted.

Not sure exactly needs saying here, but we probably should at a minimum
include a cross-reference to 'GDB/MI Async Records' so users can find
the list of names that are currently in use.

I also wonder if we should provide some guidance about how user created
async notifications are named, something like:

  "User created notifications should be prefixed with a '_' character,
  e.g. gdb.notify_mi ('_foo', ....)"

And then GDB can promise to never create an internal Async record that
starts with a '_' character.

I'm thinking: the example you give creates a 'connection-removed' event,
but what if, at some future point, GDB actually, officially, adds a
connection-removed event, this is going to conflict with the user
defined notification.  If we require (or suggest) that user defined
notifications are placed into their own namespace in some way, then we
can ensure GDB's internal notifications, and user notifications will
always be separate.  What do you think?  (Obviously '_' is just the
first idea in my head, other strategies exist).

> +as Python dictionary. The dictionary is converted to converted
> +to a @sc{gdb/mi} @var{result-record} (@pxref{GDB/MI Output Syntax}) the same way
> +as result of Python MI command (@pxref{GDB/MI Commands In Python}).
> +
> +If @var{data} is @code{None} then no additional values are emitted.
> +@end defun
> +
> +Here is how to emit @code{=connection-removed} whenever a connection to remote
> +GDB server is closed (see @pxref{Connections In Python}):
> +
> +@smallexample
> +def notify_connection_removed (event):
> +    data = @{ 'id'   : event.connection.num,
> +              'type' : event.connection.type @}
> +    gdb.notify_mi("connection-removed", data)
> +
> +gdb.events.connection_removed.connect (notify_connection_removed)
> +@end smallexample
> +
> +Then, each time a connection is closed, there will be a notification on MI channel:
> +
> +@smallexample
> +=connection-removed,id="1",type="remote"
> +@end smallexample
> +
>  @node Parameters In Python
>  @subsubsection Parameters In Python
>  
> diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
> index 66dc6fb8a32..dc2ec5ddd0f 100644
> --- a/gdb/python/py-mi.c
> +++ b/gdb/python/py-mi.c
> @@ -19,8 +19,14 @@
>  
>  #include "defs.h"
>  #include "python-internal.h"
> +#include "utils.h"
> +#include "ui.h"
>  #include "ui-out.h"
> +#include "interps.h"
> +#include "target.h"
>  #include "mi/mi-parse.h"
> +#include "mi/mi-console.h"
> +#include "mi/mi-interp.h"
>  
>  /* A ui_out subclass that creates a Python object based on the data
>     that is passed in.  */
> @@ -296,3 +302,46 @@ gdbpy_execute_mi_command (PyObject *self, PyObject *args, PyObject *kw)
>  
>    return uiout.result ();
>  }
> +
> +/* Implementation of the gdb.notify_mi function.  */

This comment should be in the header file, and here we should say:

  /* See python-internal.h.  */

> +
> +PyObject *
> +gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs)
> +{
> +  static const char *keywords[] = { "name", "data", nullptr };
> +  const char *name;
> +  PyObject *data;
> +
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "sO", keywords,
> +					&name, &data))
> +    return nullptr; // FIXME: is this the correct way to signal error?

Yes it is.  If gdb_PyArg_ParseTupleAndKeywords encounters an error then
a Python exception will have already been set.  Returning nullptr causes
the Python runtime to throw the exception.

I wonder if we should make the data argument optional, with Py_None
being the default?  You can write:

  PyObject *data = Py_None;

  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|O", keywords,
  			                &name, &data))
    return nullptr;

And that should make it so.  Obviously you'd need a corresponding docs
update.

> +
> +  SWITCH_THRU_ALL_UIS ()
> +    {
> +      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
> +
> +      if (mi == NULL)

s/NULL/nullptr/

> +        continue;
> +      

The `continue` should use a tab to indent.

And the following line, and some other lines in this diff, have trailing
whitespace.  The .gitattributes files at the top-level should turn on
highlighting of whitespace errors like this.  OOI, if you 'git show'
this patch, are the errors not highlighted for you?  I only ask because
maybe we've not set up our .gitattributes correctly.

> +      gdb_printf (mi->event_channel, "%s", name);
> +      if (data != Py_None)      
> +        {
> +          /* At the top-level, the data must be a dictionary.  */
> +          if (!PyDict_Check (data))
> +            gdbpy_error (_("Data passed to notify_mi must be either None or a dictionary"));
> +
> +          target_terminal::scoped_restore_terminal_state term_state;
> +          target_terminal::ours_for_output ();
> +
> +          ui_out *mi_uiout = mi->interp_ui_out ();
> +          ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
> +          scoped_restore restore_uiout
> +            = make_scoped_restore (&current_uiout, mi_uiout);
> +
> +          serialize_mi_data (data);	  
> +        }
> +      gdb_flush (mi->event_channel);
> +    }
> +    
> +  Py_RETURN_NONE;
> +}
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 3f53b0ab6f0..2a7e8d68179 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -501,6 +501,9 @@ extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
>  
>  extern void serialize_mi_data (PyObject *result);
>  
> +extern PyObject *gdbpy_notify_mi (PyObject *self, PyObject *args,
> +				  PyObject *kw);
> +
>  /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
>     gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
>     valid (see gdb.Progspace.is_valid), otherwise return the program_space
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 6a978d632e9..faa7e0c217d 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -2669,6 +2669,10 @@ Return the name of the currently selected language." },
>      "print_options () -> dict\n\
>  Return the current print options." },
>  
> +  { "notify_mi", (PyCFunction) gdbpy_notify_mi,
> +    METH_VARARGS | METH_KEYWORDS,
> +    "notify_mi (name, data) -> None\n\
> +Output async record to MI channels if any." },
>    {NULL, NULL, 0, NULL}
>  };
>  
> diff --git a/gdb/testsuite/gdb.python/py-mi-notify.exp b/gdb/testsuite/gdb.python/py-mi-notify.exp
> new file mode 100644
> index 00000000000..8221794c4f3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-mi-notify.exp
> @@ -0,0 +1,47 @@
> +# Copyright (C) 2019-2023 Free Software Foundation, Inc.

Copyright data should probably be just '2023' here.

Thanks,
Andrew

> +# 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/>.
> +
> +# Test custom MI notifications implemented in Python.
> +
> +load_lib gdb-python.exp
> +load_lib mi-support.exp
> +set MIFLAGS "-i=mi"
> +
> +gdb_exit
> +if {[mi_gdb_start]} {
> +    return
> +}
> +
> +if {[lsearch -exact [mi_get_features] python] < 0} {
> +    unsupported "python support is disabled"
> +    return -1
> +}
> +
> +standard_testfile
> +
> +mi_gdb_test "set python print-stack full" \
> +    ".*\\^done" \
> +    "set python print-stack full"
> +
> +mi_gdb_test "python gdb.notify_mi('test-notification', None)" \
> +    ".*=test-notification\r\n\\^done" \
> +    "python notification, no additional data"
> +
> +mi_gdb_test "python gdb.notify_mi('test-notification', \{ 'data1' : 1 , 'data2' : 2 })" \
> +    ".*=test-notification,data1=\"1\",data2=\"2\"\r\n\\^done" \
> +    "python notification, with additional data"
> +
> +mi_gdb_test "python gdb.notify_mi('test-notification', 1)" \
> +    ".*\\^error,msg=\".*\"" \
> +    "python notification, invalid additional data"
> -- 
> 2.40.1

Re: [PATCH 1/2] gdb/python: generalize serialize_mi_result()

[Andrew Burgess]

Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:

> This commit renames serialize_mi_result() to serialize_mi_data() and makes
> it usable in different contexts than printing result of custom MI command
> (hence the rename).
>
> To do so, the check whether passed Python object is a dictionary has been
> moved to the caller - at the very least, different usages require different
> error messages. Also it seemed to me that py-utils.c is a better place
> for its code as it is now a utility function not specific to Python MI
> commands.

It's no longer just for MI commands, but it probably is only for MI
"stuff".  I wonder if moving it to py-mi.c would be better?

>
> This is a preparation for implementing Python support for sending custom
> MI async events.
> ---
>  gdb/python/py-micmd.c        | 179 ++---------------------------------
>  gdb/python/py-utils.c        | 145 ++++++++++++++++++++++++++++
>  gdb/python/python-internal.h |  15 +++
>  3 files changed, 166 insertions(+), 173 deletions(-)
>
> diff --git a/gdb/python/py-micmd.c b/gdb/python/py-micmd.c
> index 01fc6060ece..26231fc37e8 100644
> --- a/gdb/python/py-micmd.c
> +++ b/gdb/python/py-micmd.c
> @@ -173,178 +173,6 @@ extern PyTypeObject micmdpy_object_type
>  
>  static PyObject *invoke_cst;
>  
> -/* Convert KEY_OBJ into a string that can be used as a field name in MI
> -   output.  KEY_OBJ must be a Python string object, and must only contain
> -   characters suitable for use as an MI field name.
> -
> -   If KEY_OBJ is not a string, or if KEY_OBJ contains invalid characters,
> -   then an error is thrown.  Otherwise, KEY_OBJ is converted to a string
> -   and returned.  */
> -
> -static gdb::unique_xmalloc_ptr<char>
> -py_object_to_mi_key (PyObject *key_obj)
> -{
> -  /* The key must be a string.  */
> -  if (!PyUnicode_Check (key_obj))
> -    {
> -      gdbpy_ref<> key_repr (PyObject_Repr (key_obj));
> -      gdb::unique_xmalloc_ptr<char> key_repr_string;
> -      if (key_repr != nullptr)
> -	key_repr_string = python_string_to_target_string (key_repr.get ());
> -      if (key_repr_string == nullptr)
> -	gdbpy_handle_exception ();
> -
> -      gdbpy_error (_("non-string object used as key: %s"),
> -		   key_repr_string.get ());
> -    }
> -
> -  gdb::unique_xmalloc_ptr<char> key_string
> -    = python_string_to_target_string (key_obj);
> -  if (key_string == nullptr)
> -    gdbpy_handle_exception ();
> -
> -  /* Predicate function, returns true if NAME is a valid field name for use
> -     in MI result output, otherwise, returns false.  */
> -  auto is_valid_key_name = [] (const char *name) -> bool
> -  {
> -    gdb_assert (name != nullptr);
> -
> -    if (*name == '\0' || !isalpha (*name))
> -      return false;
> -
> -    for (; *name != '\0'; ++name)
> -      if (!isalnum (*name) && *name != '_' && *name != '-')
> -	return false;
> -
> -    return true;
> -  };
> -
> -  if (!is_valid_key_name (key_string.get ()))
> -    {
> -      if (*key_string.get () == '\0')
> -	gdbpy_error (_("Invalid empty key in MI result"));
> -      else
> -	gdbpy_error (_("Invalid key in MI result: %s"), key_string.get ());
> -    }
> -
> -  return key_string;
> -}
> -
> -/* Serialize RESULT and print it in MI format to the current_uiout.
> -   FIELD_NAME is used as the name of this result field.
> -
> -   RESULT can be a dictionary, a sequence, an iterator, or an object that
> -   can be converted to a string, these are converted to the matching MI
> -   output format (dictionaries as tuples, sequences and iterators as lists,
> -   and strings as named fields).
> -
> -   If anything goes wrong while formatting the output then an error is
> -   thrown.
> -
> -   This function is the recursive inner core of serialize_mi_result, and
> -   should only be called from that function.  */
> -
> -static void
> -serialize_mi_result_1 (PyObject *result, const char *field_name)
> -{
> -  struct ui_out *uiout = current_uiout;
> -
> -  if (PyDict_Check (result))
> -    {
> -      PyObject *key, *value;
> -      Py_ssize_t pos = 0;
> -      ui_out_emit_tuple tuple_emitter (uiout, field_name);
> -      while (PyDict_Next (result, &pos, &key, &value))
> -	{
> -	  gdb::unique_xmalloc_ptr<char> key_string
> -	    (py_object_to_mi_key (key));
> -	  serialize_mi_result_1 (value, key_string.get ());
> -	}
> -    }
> -  else if (PySequence_Check (result) && !PyUnicode_Check (result))
> -    {
> -      ui_out_emit_list list_emitter (uiout, field_name);
> -      Py_ssize_t len = PySequence_Size (result);
> -      if (len == -1)
> -	gdbpy_handle_exception ();
> -      for (Py_ssize_t i = 0; i < len; ++i)
> -	{
> -	  gdbpy_ref<> item (PySequence_ITEM (result, i));
> -	  if (item == nullptr)
> -	    gdbpy_handle_exception ();
> -	  serialize_mi_result_1 (item.get (), nullptr);
> -	}
> -    }
> -  else if (PyIter_Check (result))
> -    {
> -      gdbpy_ref<> item;
> -      ui_out_emit_list list_emitter (uiout, field_name);
> -      while (true)
> -	{
> -	  item.reset (PyIter_Next (result));
> -	  if (item == nullptr)
> -	    {
> -	      if (PyErr_Occurred () != nullptr)
> -		gdbpy_handle_exception ();
> -	      break;
> -	    }
> -	  serialize_mi_result_1 (item.get (), nullptr);
> -	}
> -    }
> -  else
> -    {
> -      if (PyLong_Check (result))
> -	{
> -	  int overflow = 0;
> -	  gdb_py_longest val = gdb_py_long_as_long_and_overflow (result,
> -								 &overflow);
> -	  if (PyErr_Occurred () != nullptr)
> -	    gdbpy_handle_exception ();
> -	  if (overflow == 0)
> -	    {
> -	      uiout->field_signed (field_name, val);
> -	      return;
> -	    }
> -	  /* Fall through to the string case on overflow.  */
> -	}
> -
> -      gdb::unique_xmalloc_ptr<char> string (gdbpy_obj_to_string (result));
> -      if (string == nullptr)
> -	gdbpy_handle_exception ();
> -      uiout->field_string (field_name, string.get ());
> -    }
> -}
> -
> -/* Serialize RESULT and print it in MI format to the current_uiout.
> -
> -   This function handles the top-level result initially returned from the
> -   invoke method of the Python command implementation.  At the top-level
> -   the result must be a dictionary.  The values within this dictionary can
> -   be a wider range of types.  Handling the values of the top-level
> -   dictionary is done by serialize_mi_result_1, see that function for more
> -   details.
> -
> -   If anything goes wrong while parsing and printing the MI output then an
> -   error is thrown.  */
> -
> -static void
> -serialize_mi_result (PyObject *result)
> -{
> -  /* At the top-level, the result must be a dictionary.  */
> -
> -  if (!PyDict_Check (result))
> -    gdbpy_error (_("Result from invoke must be a dictionary"));
> -
> -  PyObject *key, *value;
> -  Py_ssize_t pos = 0;
> -  while (PyDict_Next (result, &pos, &key, &value))
> -    {
> -      gdb::unique_xmalloc_ptr<char> key_string
> -	(py_object_to_mi_key (key));
> -      serialize_mi_result_1 (value, key_string.get ());
> -    }
> -}
> -
>  /* Called when the MI command is invoked.  PARSE contains the parsed
>     command line arguments from the user.  */
>  
> @@ -388,7 +216,12 @@ mi_command_py::invoke (struct mi_parse *parse) const
>      gdbpy_handle_exception ();
>  
>    if (result != Py_None)
> -    serialize_mi_result (result.get ());
> +    {
> +      /* At the top-level, the result must be a dictionary.  */
> +      if (!PyDict_Check (result.get ()))
> +        gdbpy_error (_("Result from invoke must be a dictionary"));
> +      serialize_mi_data (result.get ());
> +    }
>  }
>  
>  /* See declaration above.  */
> diff --git a/gdb/python/py-utils.c b/gdb/python/py-utils.c
> index d5b07a80d82..72a232f251d 100644
> --- a/gdb/python/py-utils.c
> +++ b/gdb/python/py-utils.c
> @@ -597,3 +597,148 @@ gdbpy_fix_doc_string_indentation (gdb::unique_xmalloc_ptr<char> doc)
>  
>    return doc;
>  }
> +
> +/* Convert KEY_OBJ into a string that can be used as a field name in MI
> +   output.  KEY_OBJ must be a Python string object, and must only contain
> +   characters suitable for use as an MI field name.
> +
> +   If KEY_OBJ is not a string, or if KEY_OBJ contains invalid characters,
> +   then an error is thrown.  Otherwise, KEY_OBJ is converted to a string
> +   and returned.  */
> +
> +static gdb::unique_xmalloc_ptr<char>
> +py_object_to_mi_key (PyObject *key_obj)
> +{
> +  /* The key must be a string.  */
> +  if (!PyUnicode_Check (key_obj))
> +    {
> +      gdbpy_ref<> key_repr (PyObject_Repr (key_obj));
> +      gdb::unique_xmalloc_ptr<char> key_repr_string;
> +      if (key_repr != nullptr)
> +	key_repr_string = python_string_to_target_string (key_repr.get ());
> +      if (key_repr_string == nullptr)
> +	gdbpy_handle_exception ();
> +
> +      gdbpy_error (_("non-string object used as key: %s"),
> +		   key_repr_string.get ());
> +    }
> +
> +  gdb::unique_xmalloc_ptr<char> key_string
> +    = python_string_to_target_string (key_obj);
> +  if (key_string == nullptr)
> +    gdbpy_handle_exception ();
> +
> +  /* Predicate function, returns true if NAME is a valid field name for use
> +     in MI result output, otherwise, returns false.  */
> +  auto is_valid_key_name = [] (const char *name) -> bool
> +  {
> +    gdb_assert (name != nullptr);
> +
> +    if (*name == '\0' || !isalpha (*name))
> +      return false;
> +
> +    for (; *name != '\0'; ++name)
> +      if (!isalnum (*name) && *name != '_' && *name != '-')
> +	return false;
> +
> +    return true;
> +  };
> +
> +  if (!is_valid_key_name (key_string.get ()))
> +    {
> +      if (*key_string.get () == '\0')
> +	gdbpy_error (_("Invalid empty key in MI result"));
> +      else
> +	gdbpy_error (_("Invalid key in MI result: %s"), key_string.get ());
> +    }
> +
> +  return key_string;
> +}
> +
> +static void
> +serialize_mi_data_1 (PyObject *data, const char *field_name)

The old version of this function `serialize_mi_result_1` had a comment,
which is now missing.

> +{
> +  struct ui_out *uiout = current_uiout;
> +
> +  if (PyDict_Check (data))
> +    {
> +      PyObject *key, *value;
> +      Py_ssize_t pos = 0;
> +      ui_out_emit_tuple tuple_emitter (uiout, field_name);
> +      while (PyDict_Next (data, &pos, &key, &value))
> +	{
> +	  gdb::unique_xmalloc_ptr<char> key_string
> +	    (py_object_to_mi_key (key));
> +	  serialize_mi_data_1 (value, key_string.get ());
> +	}
> +    }
> +  else if (PySequence_Check (data) && !PyUnicode_Check (data))
> +    {
> +      ui_out_emit_list list_emitter (uiout, field_name);
> +      Py_ssize_t len = PySequence_Size (data);
> +      if (len == -1)
> +	gdbpy_handle_exception ();
> +      for (Py_ssize_t i = 0; i < len; ++i)
> +	{
> +	  gdbpy_ref<> item (PySequence_ITEM (data, i));
> +	  if (item == nullptr)
> +	    gdbpy_handle_exception ();
> +	  serialize_mi_data_1 (item.get (), nullptr);
> +	}
> +    }
> +  else if (PyIter_Check (data))
> +    {
> +      gdbpy_ref<> item;
> +      ui_out_emit_list list_emitter (uiout, field_name);
> +      while (true)
> +	{
> +	  item.reset (PyIter_Next (data));
> +	  if (item == nullptr)
> +	    {
> +	      if (PyErr_Occurred () != nullptr)
> +		gdbpy_handle_exception ();
> +	      break;
> +	    }
> +	  serialize_mi_data_1 (item.get (), nullptr);
> +	}
> +    }
> +  else
> +    {
> +      if (PyLong_Check (data))
> +	{
> +	  int overflow = 0;
> +	  gdb_py_longest val = gdb_py_long_as_long_and_overflow (data,
> +								 &overflow);
> +	  if (PyErr_Occurred () != nullptr)
> +	    gdbpy_handle_exception ();
> +	  if (overflow == 0)
> +	    {
> +	      uiout->field_signed (field_name, val);
> +	      return;
> +	    }
> +	  /* Fall through to the string case on overflow.  */
> +	}
> +
> +      gdb::unique_xmalloc_ptr<char> string (gdbpy_obj_to_string (data));
> +      if (string == nullptr)
> +	gdbpy_handle_exception ();
> +      uiout->field_string (field_name, string.get ());
> +    }
> +}
> +
> +/* See python-internal.h.  */
> +
> +void
> +serialize_mi_data (PyObject *data)
> +{
> +  gdb_assert (PyDict_Check (data));
> +
> +  PyObject *key, *value;
> +  Py_ssize_t pos = 0;
> +  while (PyDict_Next (data, &pos, &key, &value))
> +    {
> +      gdb::unique_xmalloc_ptr<char> key_string
> +	(py_object_to_mi_key (key));
> +      serialize_mi_data_1 (value, key_string.get ());
> +    }
> +}
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 93217375cc5..3f53b0ab6f0 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -486,6 +486,21 @@ struct gdbarch *arch_object_to_gdbarch (PyObject *obj);
>  extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
>  					   PyObject *kw);
>  
> +/* Serialize DATA and print it in MI format to the current_uiout.
> +
> +   This function handles the top-level result initially returned from the
> +   invoke method of the Python command implementation.  At the top-level
> +   the data must be a dictionary.  Caller of this function is responsible
> +   for ensuring that.  The values within this dictionary can
> +   be a wider range of types.  Handling the values of the top-level
> +   dictionary is done by serialize_mi_data_1, see that function for more
> +   details.

Given the change coming in the next commit, I wonder if this comment
should be reworded to avoid talking about the 'invoke method'.  Maybe
just say how it formats DATA as an MI record.

> +
> +   If anything goes wrong while parsing and printing the MI output then an
> +   error is thrown.  */
> +
> +extern void serialize_mi_data (PyObject *result);

s/result/data/ here.

Thanks,
Andrew

> +
>  /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
>     gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
>     valid (see gdb.Progspace.is_valid), otherwise return the program_space
> -- 
> 2.40.1

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Andrew Burgess]

Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:

> This commit adds a new Python function, gdb.notify_mi, that can be used
> to emit custom async notification to MI channel.  This can be used, among
> other things, to implement notifications about events MI does not support,
> such as remote connection closed or register change.
> ---
>  gdb/NEWS                                  |  3 ++
>  gdb/doc/python.texi                       | 38 ++++++++++++++++++
>  gdb/python/py-mi.c                        | 49 +++++++++++++++++++++++
>  gdb/python/python-internal.h              |  3 ++
>  gdb/python/python.c                       |  4 ++
>  gdb/testsuite/gdb.python/py-mi-notify.exp | 47 ++++++++++++++++++++++
>  6 files changed, 144 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.python/py-mi-notify.exp
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 98ff00d5efc..4a1f383a666 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -286,6 +286,9 @@ info main
>       might be array- or string-like, even if they do not have the
>       corresponding type code.
>  
> +  ** New function gdb.notify_mi(NAME, DATA), that emits custom
> +     GDB/MI async notification.
> +
>  *** Changes in GDB 13
>  
>  * MI version 1 is deprecated, and will be removed in GDB 14.
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index e9936991c49..cb2073976ba 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -4704,6 +4704,44 @@ Here is how this works using the commands from the example above:
>  @{'string': 'abc, def, ghi'@}
>  @end smallexample
>  
> +@node GDB/MI Notifications In Python
> +@subsubsection @sc{gdb/mi} Notifications In Python
> +
> +@cindex MI notifications in python
> +@cindex notifications in python, GDB/MI
> +@cindex python notifications, GDB/MI
> +
> +It is possible to emit @sc{gdb/mi} notifications from
> +Python.  This is done with the @code{gdb.notify_mi} function.
> +
> +@defun gdb.notify_mi (name , data)
> +Emit a @sc{gdb/mi} asynchronous notification.  @var{name} is the name of the
> +notification, a string.  @var{data} are additional values emitted with the notification, passed
> +as Python dictionary. The dictionary is converted to converted
> +to a @sc{gdb/mi} @var{result-record} (@pxref{GDB/MI Output Syntax}) the same way
> +as result of Python MI command (@pxref{GDB/MI Commands In Python}).
> +
> +If @var{data} is @code{None} then no additional values are emitted.
> +@end defun
> +
> +Here is how to emit @code{=connection-removed} whenever a connection to remote
> +GDB server is closed (see @pxref{Connections In Python}):
> +
> +@smallexample
> +def notify_connection_removed (event):
> +    data = @{ 'id'   : event.connection.num,
> +              'type' : event.connection.type @}
> +    gdb.notify_mi("connection-removed", data)
> +
> +gdb.events.connection_removed.connect (notify_connection_removed)
> +@end smallexample
> +
> +Then, each time a connection is closed, there will be a notification on MI channel:
> +
> +@smallexample
> +=connection-removed,id="1",type="remote"
> +@end smallexample
> +
>  @node Parameters In Python
>  @subsubsection Parameters In Python
>  
> diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
> index 66dc6fb8a32..dc2ec5ddd0f 100644
> --- a/gdb/python/py-mi.c
> +++ b/gdb/python/py-mi.c
> @@ -19,8 +19,14 @@
>  
>  #include "defs.h"
>  #include "python-internal.h"
> +#include "utils.h"
> +#include "ui.h"
>  #include "ui-out.h"
> +#include "interps.h"
> +#include "target.h"
>  #include "mi/mi-parse.h"
> +#include "mi/mi-console.h"
> +#include "mi/mi-interp.h"
>  
>  /* A ui_out subclass that creates a Python object based on the data
>     that is passed in.  */
> @@ -296,3 +302,46 @@ gdbpy_execute_mi_command (PyObject *self, PyObject *args, PyObject *kw)
>  
>    return uiout.result ();
>  }
> +
> +/* Implementation of the gdb.notify_mi function.  */
> +
> +PyObject *
> +gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs)
> +{
> +  static const char *keywords[] = { "name", "data", nullptr };
> +  const char *name;
> +  PyObject *data;
> +
> +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "sO", keywords,
> +					&name, &data))
> +    return nullptr; // FIXME: is this the correct way to signal error?

I knew there was something else I wanted to say :)

I think you should add some validation of 'name' here too.  If you check
patch 1/2 in this series and look for 'is_valid_key_name', you should do
something similar for 'name', and document the restrictions. e.g. I
don't think a name like "  *** bad idea ***  " would be a great choice
for a notification name.

Thanks,
Andrew

> +
> +  SWITCH_THRU_ALL_UIS ()
> +    {
> +      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
> +
> +      if (mi == NULL)
> +        continue;
> +      
> +      gdb_printf (mi->event_channel, "%s", name);
> +      if (data != Py_None)      
> +        {
> +          /* At the top-level, the data must be a dictionary.  */
> +          if (!PyDict_Check (data))
> +            gdbpy_error (_("Data passed to notify_mi must be either None or a dictionary"));
> +
> +          target_terminal::scoped_restore_terminal_state term_state;
> +          target_terminal::ours_for_output ();
> +
> +          ui_out *mi_uiout = mi->interp_ui_out ();
> +          ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
> +          scoped_restore restore_uiout
> +            = make_scoped_restore (&current_uiout, mi_uiout);
> +
> +          serialize_mi_data (data);	  
> +        }
> +      gdb_flush (mi->event_channel);
> +    }
> +    
> +  Py_RETURN_NONE;
> +}
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 3f53b0ab6f0..2a7e8d68179 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -501,6 +501,9 @@ extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
>  
>  extern void serialize_mi_data (PyObject *result);
>  
> +extern PyObject *gdbpy_notify_mi (PyObject *self, PyObject *args,
> +				  PyObject *kw);
> +
>  /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
>     gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
>     valid (see gdb.Progspace.is_valid), otherwise return the program_space
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 6a978d632e9..faa7e0c217d 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -2669,6 +2669,10 @@ Return the name of the currently selected language." },
>      "print_options () -> dict\n\
>  Return the current print options." },
>  
> +  { "notify_mi", (PyCFunction) gdbpy_notify_mi,
> +    METH_VARARGS | METH_KEYWORDS,
> +    "notify_mi (name, data) -> None\n\
> +Output async record to MI channels if any." },
>    {NULL, NULL, 0, NULL}
>  };
>  
> diff --git a/gdb/testsuite/gdb.python/py-mi-notify.exp b/gdb/testsuite/gdb.python/py-mi-notify.exp
> new file mode 100644
> index 00000000000..8221794c4f3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-mi-notify.exp
> @@ -0,0 +1,47 @@
> +# Copyright (C) 2019-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/>.
> +
> +# Test custom MI notifications implemented in Python.
> +
> +load_lib gdb-python.exp
> +load_lib mi-support.exp
> +set MIFLAGS "-i=mi"
> +
> +gdb_exit
> +if {[mi_gdb_start]} {
> +    return
> +}
> +
> +if {[lsearch -exact [mi_get_features] python] < 0} {
> +    unsupported "python support is disabled"
> +    return -1
> +}
> +
> +standard_testfile
> +
> +mi_gdb_test "set python print-stack full" \
> +    ".*\\^done" \
> +    "set python print-stack full"
> +
> +mi_gdb_test "python gdb.notify_mi('test-notification', None)" \
> +    ".*=test-notification\r\n\\^done" \
> +    "python notification, no additional data"
> +
> +mi_gdb_test "python gdb.notify_mi('test-notification', \{ 'data1' : 1 , 'data2' : 2 })" \
> +    ".*=test-notification,data1=\"1\",data2=\"2\"\r\n\\^done" \
> +    "python notification, with additional data"
> +
> +mi_gdb_test "python gdb.notify_mi('test-notification', 1)" \
> +    ".*\\^error,msg=\".*\"" \
> +    "python notification, invalid additional data"
> -- 
> 2.40.1

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Andrew Burgess]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: Jan Vrany <jan.vrany@labware.com>, Eli Zaretskii <eliz@gnu.org>
>> Date: Mon, 11 Sep 2023 13:42:14 +0100
>> 
>> > +@subsubsection @sc{gdb/mi} Notifications In Python
>> > +
>> > +@cindex MI notifications in python
>> > +@cindex notifications in python, GDB/MI
>> > +@cindex python notifications, GDB/MI
>> 
>> I think these @cindex entries are supposed to appear at least before the
>> @subsubsection heading, that way, when following the link from the index
>> you'll be taken to the relevant heading rather than the body text -
>> though I could be wrong about this, Eli might have some thoughts.
>
> No, what you say is correct for @item, but not for sectioning
> commands.  In the latter case, we want to index entry to land the
> reader on the text, not on the section name.

OK, thanks.  I thought I should check with you :)

Thanks,
Andrew

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Jan Vraný]

On Mon, 2023-09-11 at 15:21 +0100, Andrew Burgess wrote:
> Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > This commit adds a new Python function, gdb.notify_mi, that can be used
> > to emit custom async notification to MI channel.  This can be used, among
> > other things, to implement notifications about events MI does not support,
> > such as remote connection closed or register change.
> > ---
> >  gdb/NEWS                                  |  3 ++
> >  gdb/doc/python.texi                       | 38 ++++++++++++++++++
> >  gdb/python/py-mi.c                        | 49 +++++++++++++++++++++++
> >  gdb/python/python-internal.h              |  3 ++
> >  gdb/python/python.c                       |  4 ++
> >  gdb/testsuite/gdb.python/py-mi-notify.exp | 47 ++++++++++++++++++++++
> >  6 files changed, 144 insertions(+)
> >  create mode 100644 gdb/testsuite/gdb.python/py-mi-notify.exp
> > 
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index 98ff00d5efc..4a1f383a666 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -286,6 +286,9 @@ info main
> >       might be array- or string-like, even if they do not have the
> >       corresponding type code.
> >  
> > +  ** New function gdb.notify_mi(NAME, DATA), that emits custom
> > +     GDB/MI async notification.
> > +
> >  *** Changes in GDB 13
> >  
> >  * MI version 1 is deprecated, and will be removed in GDB 14.
> > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> > index e9936991c49..cb2073976ba 100644
> > --- a/gdb/doc/python.texi
> > +++ b/gdb/doc/python.texi
> > @@ -4704,6 +4704,44 @@ Here is how this works using the commands from the example above:
> >  @{'string': 'abc, def, ghi'@}
> >  @end smallexample
> >  
> > +@node GDB/MI Notifications In Python
> > +@subsubsection @sc{gdb/mi} Notifications In Python
> > +
> > +@cindex MI notifications in python
> > +@cindex notifications in python, GDB/MI
> > +@cindex python notifications, GDB/MI
> > +
> > +It is possible to emit @sc{gdb/mi} notifications from
> > +Python.  This is done with the @code{gdb.notify_mi} function.
> > +
> > +@defun gdb.notify_mi (name , data)
> > +Emit a @sc{gdb/mi} asynchronous notification.  @var{name} is the name of the
> > +notification, a string.  @var{data} are additional values emitted with the notification, passed
> > +as Python dictionary. The dictionary is converted to converted
> > +to a @sc{gdb/mi} @var{result-record} (@pxref{GDB/MI Output Syntax}) the same way
> > +as result of Python MI command (@pxref{GDB/MI Commands In Python}).
> > +
> > +If @var{data} is @code{None} then no additional values are emitted.
> > +@end defun
> > +
> > +Here is how to emit @code{=connection-removed} whenever a connection to remote
> > +GDB server is closed (see @pxref{Connections In Python}):
> > +
> > +@smallexample
> > +def notify_connection_removed (event):
> > +    data = @{ 'id'   : event.connection.num,
> > +              'type' : event.connection.type @}
> > +    gdb.notify_mi("connection-removed", data)
> > +
> > +gdb.events.connection_removed.connect (notify_connection_removed)
> > +@end smallexample
> > +
> > +Then, each time a connection is closed, there will be a notification on MI channel:
> > +
> > +@smallexample
> > +=connection-removed,id="1",type="remote"
> > +@end smallexample
> > +
> >  @node Parameters In Python
> >  @subsubsection Parameters In Python
> >  
> > diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
> > index 66dc6fb8a32..dc2ec5ddd0f 100644
> > --- a/gdb/python/py-mi.c
> > +++ b/gdb/python/py-mi.c
> > @@ -19,8 +19,14 @@
> >  
> >  #include "defs.h"
> >  #include "python-internal.h"
> > +#include "utils.h"
> > +#include "ui.h"
> >  #include "ui-out.h"
> > +#include "interps.h"
> > +#include "target.h"
> >  #include "mi/mi-parse.h"
> > +#include "mi/mi-console.h"
> > +#include "mi/mi-interp.h"
> >  
> >  /* A ui_out subclass that creates a Python object based on the data
> >     that is passed in.  */
> > @@ -296,3 +302,46 @@ gdbpy_execute_mi_command (PyObject *self, PyObject *args, PyObject *kw)
> >  
> >    return uiout.result ();
> >  }
> > +
> > +/* Implementation of the gdb.notify_mi function.  */
> > +
> > +PyObject *
> > +gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs)
> > +{
> > +  static const char *keywords[] = { "name", "data", nullptr };
> > +  const char *name;
> > +  PyObject *data;
> > +
> > +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "sO", keywords,
> > +					&name, &data))
> > +    return nullptr; // FIXME: is this the correct way to signal error?
> 
> I knew there was something else I wanted to say :)
> 
> I think you should add some validation of 'name' here too.  If you check
> patch 1/2 in this series and look for 'is_valid_key_name', you should do
> something similar for 'name', and document the restrictions. e.g. I
> don't think a name like "  *** bad idea ***  " would be a great choice
> for a notification name.

Yes, that's exactly what I have added to (not yet submitted) V2. 

Thanks! Jan
> 
> Thanks,
> Andrew
> 
> > +
> > +  SWITCH_THRU_ALL_UIS ()
> > +    {
> > +      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
> > +
> > +      if (mi == NULL)
> > +        continue;
> > +      
> > +      gdb_printf (mi->event_channel, "%s", name);
> > +      if (data != Py_None)      
> > +        {
> > +          /* At the top-level, the data must be a dictionary.  */
> > +          if (!PyDict_Check (data))
> > +            gdbpy_error (_("Data passed to notify_mi must be either None or a dictionary"));
> > +
> > +          target_terminal::scoped_restore_terminal_state term_state;
> > +          target_terminal::ours_for_output ();
> > +
> > +          ui_out *mi_uiout = mi->interp_ui_out ();
> > +          ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
> > +          scoped_restore restore_uiout
> > +            = make_scoped_restore (&current_uiout, mi_uiout);
> > +
> > +          serialize_mi_data (data);	  
> > +        }
> > +      gdb_flush (mi->event_channel);
> > +    }
> > +    
> > +  Py_RETURN_NONE;
> > +}
> > diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> > index 3f53b0ab6f0..2a7e8d68179 100644
> > --- a/gdb/python/python-internal.h
> > +++ b/gdb/python/python-internal.h
> > @@ -501,6 +501,9 @@ extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
> >  
> >  extern void serialize_mi_data (PyObject *result);
> >  
> > +extern PyObject *gdbpy_notify_mi (PyObject *self, PyObject *args,
> > +				  PyObject *kw);
> > +
> >  /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
> >     gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
> >     valid (see gdb.Progspace.is_valid), otherwise return the program_space
> > diff --git a/gdb/python/python.c b/gdb/python/python.c
> > index 6a978d632e9..faa7e0c217d 100644
> > --- a/gdb/python/python.c
> > +++ b/gdb/python/python.c
> > @@ -2669,6 +2669,10 @@ Return the name of the currently selected language." },
> >      "print_options () -> dict\n\
> >  Return the current print options." },
> >  
> > +  { "notify_mi", (PyCFunction) gdbpy_notify_mi,
> > +    METH_VARARGS | METH_KEYWORDS,
> > +    "notify_mi (name, data) -> None\n\
> > +Output async record to MI channels if any." },
> >    {NULL, NULL, 0, NULL}
> >  };
> >  
> > diff --git a/gdb/testsuite/gdb.python/py-mi-notify.exp b/gdb/testsuite/gdb.python/py-mi-notify.exp
> > new file mode 100644
> > index 00000000000..8221794c4f3
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-mi-notify.exp
> > @@ -0,0 +1,47 @@
> > +# Copyright (C) 2019-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>.
> > +
> > +# Test custom MI notifications implemented in Python.
> > +
> > +load_lib gdb-python.exp
> > +load_lib mi-support.exp
> > +set MIFLAGS "-i=mi"
> > +
> > +gdb_exit
> > +if {[mi_gdb_start]} {
> > +    return
> > +}
> > +
> > +if {[lsearch -exact [mi_get_features] python] < 0} {
> > +    unsupported "python support is disabled"
> > +    return -1
> > +}
> > +
> > +standard_testfile
> > +
> > +mi_gdb_test "set python print-stack full" \
> > +    ".*\\^done" \
> > +    "set python print-stack full"
> > +
> > +mi_gdb_test "python gdb.notify_mi('test-notification', None)" \
> > +    ".*=test-notification\r\n\\^done" \
> > +    "python notification, no additional data"
> > +
> > +mi_gdb_test "python gdb.notify_mi('test-notification', \{ 'data1' : 1 , 'data2' : 2 })" \
> > +    ".*=test-notification,data1=\"1\",data2=\"2\"\r\n\\^done" \
> > +    "python notification, with additional data"
> > +
> > +mi_gdb_test "python gdb.notify_mi('test-notification', 1)" \
> > +    ".*\\^error,msg=\".*\"" \
> > +    "python notification, invalid additional data"
> > -- 
> > 2.40.1

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Jan Vraný]

On Mon, 2023-09-11 at 15:14 +0100, Andrew Burgess wrote:
> Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> > This commit adds a new Python function, gdb.notify_mi, that can be used
> > to emit custom async notification to MI channel.  This can be used, among
> > other things, to implement notifications about events MI does not support,
> > such as remote connection closed or register change.
> > ---
> >  gdb/NEWS                                  |  3 ++
> >  gdb/doc/python.texi                       | 38 ++++++++++++++++++
> >  gdb/python/py-mi.c                        | 49 +++++++++++++++++++++++
> >  gdb/python/python-internal.h              |  3 ++
> >  gdb/python/python.c                       |  4 ++
> >  gdb/testsuite/gdb.python/py-mi-notify.exp | 47 ++++++++++++++++++++++
> >  6 files changed, 144 insertions(+)
> >  create mode 100644 gdb/testsuite/gdb.python/py-mi-notify.exp
> > 
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index 98ff00d5efc..4a1f383a666 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -286,6 +286,9 @@ info main
> >       might be array- or string-like, even if they do not have the
> >       corresponding type code.
> >  
> > +  ** New function gdb.notify_mi(NAME, DATA), that emits custom
> > +     GDB/MI async notification.
> > +
> >  *** Changes in GDB 13
> >  
> >  * MI version 1 is deprecated, and will be removed in GDB 14.
> > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> > index e9936991c49..cb2073976ba 100644
> > --- a/gdb/doc/python.texi
> > +++ b/gdb/doc/python.texi
> > @@ -4704,6 +4704,44 @@ Here is how this works using the commands from the example above:
> >  @{'string': 'abc, def, ghi'@}
> >  @end smallexample
> >  
> > +@node GDB/MI Notifications In Python
> > +@subsubsection @sc{gdb/mi} Notifications In Python
> > +
> > +@cindex MI notifications in python
> > +@cindex notifications in python, GDB/MI
> > +@cindex python notifications, GDB/MI
> > +
> > +It is possible to emit @sc{gdb/mi} notifications from
> > +Python.  This is done with the @code{gdb.notify_mi} function.
> > +
> > +@defun gdb.notify_mi (name , data)
> > +Emit a @sc{gdb/mi} asynchronous notification.  @var{name} is the name of the
> > +notification, a string.  @var{data} are additional values emitted with the notification, passed
> 
> I think this reads better:
> 
>   @var{data} is any additional data to be emitted with the notification,
>   passed as a Python dictionary.
> 
> Eli already asked about which values of @var{name} are valid.  I can see
> from the code that _any_ value is accepted, however, a user should
> probably be careful not to reuse a name that GDB already uses -- or if
> they do then they should ensure that all the required fields are
> emitted.
> 
> Not sure exactly needs saying here, but we probably should at a minimum
> include a cross-reference to 'GDB/MI Async Records' so users can find
> the list of names that are currently in use.

Sure, will do. 

> 
> I also wonder if we should provide some guidance about how user created
> async notifications are named, something like:
> 
>   "User created notifications should be prefixed with a '_' character,
>   e.g. gdb.notify_mi ('_foo', ....)"
> 
> And then GDB can promise to never create an internal Async record that
> starts with a '_' character.
> 
> I'm thinking: the example you give creates a 'connection-removed' event,
> but what if, at some future point, GDB actually, officially, adds a
> connection-removed event, this is going to conflict with the user
> defined notification.  If we require (or suggest) that user defined
> notifications are placed into their own namespace in some way, then we
> can ensure GDB's internal notifications, and user notifications will
> always be separate.  What do you think?  (Obviously '_' is just the
> first idea in my head, other strategies exist).

Good point. In case of python-implemented MI commands, overriding existing
(built-in) command is forbidden. I'm inclined to do the same in case of 
notifications, mostly because it is consistent with MI commands and 
is relatively easy to implement (though it'd would require manually coded
list of built-in notifications). 

Suggesting python notification should go into separate namespace and promising 
built-in notifications never use that namespace is a good idea. 

As for requiring python notifications to be in separate namespace, I'm not sure.
Again, we did not required that for python MI commands so it feels it'd be a bit 
inconsistent. (Also, if at some future point GDB actually decides to add - say -
connection-removed event, why not implement it in python? But that's probably
a different discussion)

> 
> > +as Python dictionary. The dictionary is converted to converted
> > +to a @sc{gdb/mi} @var{result-record} (@pxref{GDB/MI Output Syntax}) the same way
> > +as result of Python MI command (@pxref{GDB/MI Commands In Python}).
> > +
> > +If @var{data} is @code{None} then no additional values are emitted.
> > +@end defun
> > +
> > +Here is how to emit @code{=connection-removed} whenever a connection to remote
> > +GDB server is closed (see @pxref{Connections In Python}):
> > +
> > +@smallexample
> > +def notify_connection_removed (event):
> > +    data = @{ 'id'   : event.connection.num,
> > +              'type' : event.connection.type @}
> > +    gdb.notify_mi("connection-removed", data)
> > +
> > +gdb.events.connection_removed.connect (notify_connection_removed)
> > +@end smallexample
> > +
> > +Then, each time a connection is closed, there will be a notification on MI channel:
> > +
> > +@smallexample
> > +=connection-removed,id="1",type="remote"
> > +@end smallexample
> > +
> >  @node Parameters In Python
> >  @subsubsection Parameters In Python
> >  
> > diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
> > index 66dc6fb8a32..dc2ec5ddd0f 100644
> > --- a/gdb/python/py-mi.c
> > +++ b/gdb/python/py-mi.c
> > @@ -19,8 +19,14 @@
> >  
> >  #include "defs.h"
> >  #include "python-internal.h"
> > +#include "utils.h"
> > +#include "ui.h"
> >  #include "ui-out.h"
> > +#include "interps.h"
> > +#include "target.h"
> >  #include "mi/mi-parse.h"
> > +#include "mi/mi-console.h"
> > +#include "mi/mi-interp.h"
> >  
> >  /* A ui_out subclass that creates a Python object based on the data
> >     that is passed in.  */
> > @@ -296,3 +302,46 @@ gdbpy_execute_mi_command (PyObject *self, PyObject *args, PyObject *kw)
> >  
> >    return uiout.result ();
> >  }
> > +
> > +/* Implementation of the gdb.notify_mi function.  */
> 
> This comment should be in the header file, and here we should say:
> 
>   /* See python-internal.h.  */
> 
> > +
> > +PyObject *
> > +gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs)
> > +{
> > +  static const char *keywords[] = { "name", "data", nullptr };
> > +  const char *name;
> > +  PyObject *data;
> > +
> > +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "sO", keywords,
> > +					&name, &data))
> > +    return nullptr; // FIXME: is this the correct way to signal error?
> 
> Yes it is.  If gdb_PyArg_ParseTupleAndKeywords encounters an error then
> a Python exception will have already been set.  Returning nullptr causes
> the Python runtime to throw the exception.
> 
> I wonder if we should make the data argument optional, with Py_None
> being the default?  You can write:
> 
>   PyObject *data = Py_None;
> 
>   if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|O", keywords,
>   			                &name, &data))
>     return nullptr;
> 
> And that should make it so.  Obviously you'd need a corresponding docs
> update.
> 
> > +
> > +  SWITCH_THRU_ALL_UIS ()
> > +    {
> > +      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
> > +
> > +      if (mi == NULL)
> 
> s/NULL/nullptr/
> 
> > +        continue;
> > +      
> 
> The `continue` should use a tab to indent.
> 
> And the following line, and some other lines in this diff, have trailing
> whitespace.  The .gitattributes files at the top-level should turn on
> highlighting of whitespace errors like this.  OOI, if you 'git show'
> this patch, are the errors not highlighted for you?  I only ask because
> maybe we've not set up our .gitattributes correctly.
> 
> > +      gdb_printf (mi->event_channel, "%s", name);
> > +      if (data != Py_None)      
> > +        {
> > +          /* At the top-level, the data must be a dictionary.  */
> > +          if (!PyDict_Check (data))
> > +            gdbpy_error (_("Data passed to notify_mi must be either None or a dictionary"));
> > +
> > +          target_terminal::scoped_restore_terminal_state term_state;
> > +          target_terminal::ours_for_output ();
> > +
> > +          ui_out *mi_uiout = mi->interp_ui_out ();
> > +          ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
> > +          scoped_restore restore_uiout
> > +            = make_scoped_restore (&current_uiout, mi_uiout);
> > +
> > +          serialize_mi_data (data);	  
> > +        }
> > +      gdb_flush (mi->event_channel);
> > +    }
> > +    
> > +  Py_RETURN_NONE;
> > +}
> > diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> > index 3f53b0ab6f0..2a7e8d68179 100644
> > --- a/gdb/python/python-internal.h
> > +++ b/gdb/python/python-internal.h
> > @@ -501,6 +501,9 @@ extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
> >  
> >  extern void serialize_mi_data (PyObject *result);
> >  
> > +extern PyObject *gdbpy_notify_mi (PyObject *self, PyObject *args,
> > +				  PyObject *kw);
> > +
> >  /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
> >     gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
> >     valid (see gdb.Progspace.is_valid), otherwise return the program_space
> > diff --git a/gdb/python/python.c b/gdb/python/python.c
> > index 6a978d632e9..faa7e0c217d 100644
> > --- a/gdb/python/python.c
> > +++ b/gdb/python/python.c
> > @@ -2669,6 +2669,10 @@ Return the name of the currently selected language." },
> >      "print_options () -> dict\n\
> >  Return the current print options." },
> >  
> > +  { "notify_mi", (PyCFunction) gdbpy_notify_mi,
> > +    METH_VARARGS | METH_KEYWORDS,
> > +    "notify_mi (name, data) -> None\n\
> > +Output async record to MI channels if any." },
> >    {NULL, NULL, 0, NULL}
> >  };
> >  
> > diff --git a/gdb/testsuite/gdb.python/py-mi-notify.exp b/gdb/testsuite/gdb.python/py-mi-notify.exp
> > new file mode 100644
> > index 00000000000..8221794c4f3
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-mi-notify.exp
> > @@ -0,0 +1,47 @@
> > +# Copyright (C) 2019-2023 Free Software Foundation, Inc.
> 
> Copyright data should probably be just '2023' here.
> 
> Thanks,
> Andrew
> 
> > +# 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>.
> > +
> > +# Test custom MI notifications implemented in Python.
> > +
> > +load_lib gdb-python.exp
> > +load_lib mi-support.exp
> > +set MIFLAGS "-i=mi"
> > +
> > +gdb_exit
> > +if {[mi_gdb_start]} {
> > +    return
> > +}
> > +
> > +if {[lsearch -exact [mi_get_features] python] < 0} {
> > +    unsupported "python support is disabled"
> > +    return -1
> > +}
> > +
> > +standard_testfile
> > +
> > +mi_gdb_test "set python print-stack full" \
> > +    ".*\\^done" \
> > +    "set python print-stack full"
> > +
> > +mi_gdb_test "python gdb.notify_mi('test-notification', None)" \
> > +    ".*=test-notification\r\n\\^done" \
> > +    "python notification, no additional data"
> > +
> > +mi_gdb_test "python gdb.notify_mi('test-notification', \{ 'data1' : 1 , 'data2' : 2 })" \
> > +    ".*=test-notification,data1=\"1\",data2=\"2\"\r\n\\^done" \
> > +    "python notification, with additional data"
> > +
> > +mi_gdb_test "python gdb.notify_mi('test-notification', 1)" \
> > +    ".*\\^error,msg=\".*\"" \
> > +    "python notification, invalid additional data"
> > -- 
> > 2.40.1

T

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Andrew Burgess]

Jan Vraný <Jan.Vrany@labware.com> writes:

> On Mon, 2023-09-11 at 15:14 +0100, Andrew Burgess wrote:
>> Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>> > This commit adds a new Python function, gdb.notify_mi, that can be used
>> > to emit custom async notification to MI channel.  This can be used, among
>> > other things, to implement notifications about events MI does not support,
>> > such as remote connection closed or register change.
>> > ---
>> >  gdb/NEWS                                  |  3 ++
>> >  gdb/doc/python.texi                       | 38 ++++++++++++++++++
>> >  gdb/python/py-mi.c                        | 49 +++++++++++++++++++++++
>> >  gdb/python/python-internal.h              |  3 ++
>> >  gdb/python/python.c                       |  4 ++
>> >  gdb/testsuite/gdb.python/py-mi-notify.exp | 47 ++++++++++++++++++++++
>> >  6 files changed, 144 insertions(+)
>> >  create mode 100644 gdb/testsuite/gdb.python/py-mi-notify.exp
>> > 
>> > diff --git a/gdb/NEWS b/gdb/NEWS
>> > index 98ff00d5efc..4a1f383a666 100644
>> > --- a/gdb/NEWS
>> > +++ b/gdb/NEWS
>> > @@ -286,6 +286,9 @@ info main
>> >       might be array- or string-like, even if they do not have the
>> >       corresponding type code.
>> >  
>> > +  ** New function gdb.notify_mi(NAME, DATA), that emits custom
>> > +     GDB/MI async notification.
>> > +
>> >  *** Changes in GDB 13
>> >  
>> >  * MI version 1 is deprecated, and will be removed in GDB 14.
>> > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
>> > index e9936991c49..cb2073976ba 100644
>> > --- a/gdb/doc/python.texi
>> > +++ b/gdb/doc/python.texi
>> > @@ -4704,6 +4704,44 @@ Here is how this works using the commands from the example above:
>> >  @{'string': 'abc, def, ghi'@}
>> >  @end smallexample
>> >  
>> > +@node GDB/MI Notifications In Python
>> > +@subsubsection @sc{gdb/mi} Notifications In Python
>> > +
>> > +@cindex MI notifications in python
>> > +@cindex notifications in python, GDB/MI
>> > +@cindex python notifications, GDB/MI
>> > +
>> > +It is possible to emit @sc{gdb/mi} notifications from
>> > +Python.  This is done with the @code{gdb.notify_mi} function.
>> > +
>> > +@defun gdb.notify_mi (name , data)
>> > +Emit a @sc{gdb/mi} asynchronous notification.  @var{name} is the name of the
>> > +notification, a string.  @var{data} are additional values emitted with the notification, passed
>> 
>> I think this reads better:
>> 
>>   @var{data} is any additional data to be emitted with the notification,
>>   passed as a Python dictionary.
>> 
>> Eli already asked about which values of @var{name} are valid.  I can see
>> from the code that _any_ value is accepted, however, a user should
>> probably be careful not to reuse a name that GDB already uses -- or if
>> they do then they should ensure that all the required fields are
>> emitted.
>> 
>> Not sure exactly needs saying here, but we probably should at a minimum
>> include a cross-reference to 'GDB/MI Async Records' so users can find
>> the list of names that are currently in use.
>
> Sure, will do. 
>
>> 
>> I also wonder if we should provide some guidance about how user created
>> async notifications are named, something like:
>> 
>>   "User created notifications should be prefixed with a '_' character,
>>   e.g. gdb.notify_mi ('_foo', ....)"
>> 
>> And then GDB can promise to never create an internal Async record that
>> starts with a '_' character.
>> 
>> I'm thinking: the example you give creates a 'connection-removed' event,
>> but what if, at some future point, GDB actually, officially, adds a
>> connection-removed event, this is going to conflict with the user
>> defined notification.  If we require (or suggest) that user defined
>> notifications are placed into their own namespace in some way, then we
>> can ensure GDB's internal notifications, and user notifications will
>> always be separate.  What do you think?  (Obviously '_' is just the
>> first idea in my head, other strategies exist).
>
> Good point. In case of python-implemented MI commands, overriding existing
> (built-in) command is forbidden. I'm inclined to do the same in case of 
> notifications, mostly because it is consistent with MI commands and 
> is relatively easy to implement (though it'd would require manually coded
> list of built-in notifications). 
>
> Suggesting python notification should go into separate namespace and promising 
> built-in notifications never use that namespace is a good idea. 
>
> As for requiring python notifications to be in separate namespace, I'm not sure.
> Again, we did not required that for python MI commands so it feels it'd be a bit 
> inconsistent. (Also, if at some future point GDB actually decides to add - say -
> connection-removed event, why not implement it in python? But that's probably
> a different discussion)

If we officially implemented it, then I think _how_ we implemented it
isn't important.  What's important, is we ideally want to make the user
a promise that if they follow some rule, GDB isn't going to come along
and conflict with them.

I agree with you that we shouldn't _prevent_ users from creating
possibly conflicting names, e.g. if they wanted to emit notifications
that already exist, or are happy to accept the risk that a later GDB
might conflict, then that's on them -- so long as they are aware that
they are taking on this risk, I'm happy with that.

Thanks,
Andrew


>
>> 
>> > +as Python dictionary. The dictionary is converted to converted
>> > +to a @sc{gdb/mi} @var{result-record} (@pxref{GDB/MI Output Syntax}) the same way
>> > +as result of Python MI command (@pxref{GDB/MI Commands In Python}).
>> > +
>> > +If @var{data} is @code{None} then no additional values are emitted.
>> > +@end defun
>> > +
>> > +Here is how to emit @code{=connection-removed} whenever a connection to remote
>> > +GDB server is closed (see @pxref{Connections In Python}):
>> > +
>> > +@smallexample
>> > +def notify_connection_removed (event):
>> > +    data = @{ 'id'   : event.connection.num,
>> > +              'type' : event.connection.type @}
>> > +    gdb.notify_mi("connection-removed", data)
>> > +
>> > +gdb.events.connection_removed.connect (notify_connection_removed)
>> > +@end smallexample
>> > +
>> > +Then, each time a connection is closed, there will be a notification on MI channel:
>> > +
>> > +@smallexample
>> > +=connection-removed,id="1",type="remote"
>> > +@end smallexample
>> > +
>> >  @node Parameters In Python
>> >  @subsubsection Parameters In Python
>> >  
>> > diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
>> > index 66dc6fb8a32..dc2ec5ddd0f 100644
>> > --- a/gdb/python/py-mi.c
>> > +++ b/gdb/python/py-mi.c
>> > @@ -19,8 +19,14 @@
>> >  
>> >  #include "defs.h"
>> >  #include "python-internal.h"
>> > +#include "utils.h"
>> > +#include "ui.h"
>> >  #include "ui-out.h"
>> > +#include "interps.h"
>> > +#include "target.h"
>> >  #include "mi/mi-parse.h"
>> > +#include "mi/mi-console.h"
>> > +#include "mi/mi-interp.h"
>> >  
>> >  /* A ui_out subclass that creates a Python object based on the data
>> >     that is passed in.  */
>> > @@ -296,3 +302,46 @@ gdbpy_execute_mi_command (PyObject *self, PyObject *args, PyObject *kw)
>> >  
>> >    return uiout.result ();
>> >  }
>> > +
>> > +/* Implementation of the gdb.notify_mi function.  */
>> 
>> This comment should be in the header file, and here we should say:
>> 
>>   /* See python-internal.h.  */
>> 
>> > +
>> > +PyObject *
>> > +gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs)
>> > +{
>> > +  static const char *keywords[] = { "name", "data", nullptr };
>> > +  const char *name;
>> > +  PyObject *data;
>> > +
>> > +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "sO", keywords,
>> > +					&name, &data))
>> > +    return nullptr; // FIXME: is this the correct way to signal error?
>> 
>> Yes it is.  If gdb_PyArg_ParseTupleAndKeywords encounters an error then
>> a Python exception will have already been set.  Returning nullptr causes
>> the Python runtime to throw the exception.
>> 
>> I wonder if we should make the data argument optional, with Py_None
>> being the default?  You can write:
>> 
>>   PyObject *data = Py_None;
>> 
>>   if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|O", keywords,
>>   			                &name, &data))
>>     return nullptr;
>> 
>> And that should make it so.  Obviously you'd need a corresponding docs
>> update.
>> 
>> > +
>> > +  SWITCH_THRU_ALL_UIS ()
>> > +    {
>> > +      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
>> > +
>> > +      if (mi == NULL)
>> 
>> s/NULL/nullptr/
>> 
>> > +        continue;
>> > +      
>> 
>> The `continue` should use a tab to indent.
>> 
>> And the following line, and some other lines in this diff, have trailing
>> whitespace.  The .gitattributes files at the top-level should turn on
>> highlighting of whitespace errors like this.  OOI, if you 'git show'
>> this patch, are the errors not highlighted for you?  I only ask because
>> maybe we've not set up our .gitattributes correctly.
>> 
>> > +      gdb_printf (mi->event_channel, "%s", name);
>> > +      if (data != Py_None)      
>> > +        {
>> > +          /* At the top-level, the data must be a dictionary.  */
>> > +          if (!PyDict_Check (data))
>> > +            gdbpy_error (_("Data passed to notify_mi must be either None or a dictionary"));
>> > +
>> > +          target_terminal::scoped_restore_terminal_state term_state;
>> > +          target_terminal::ours_for_output ();
>> > +
>> > +          ui_out *mi_uiout = mi->interp_ui_out ();
>> > +          ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
>> > +          scoped_restore restore_uiout
>> > +            = make_scoped_restore (&current_uiout, mi_uiout);
>> > +
>> > +          serialize_mi_data (data);	  
>> > +        }
>> > +      gdb_flush (mi->event_channel);
>> > +    }
>> > +    
>> > +  Py_RETURN_NONE;
>> > +}
>> > diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
>> > index 3f53b0ab6f0..2a7e8d68179 100644
>> > --- a/gdb/python/python-internal.h
>> > +++ b/gdb/python/python-internal.h
>> > @@ -501,6 +501,9 @@ extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
>> >  
>> >  extern void serialize_mi_data (PyObject *result);
>> >  
>> > +extern PyObject *gdbpy_notify_mi (PyObject *self, PyObject *args,
>> > +				  PyObject *kw);
>> > +
>> >  /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
>> >     gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
>> >     valid (see gdb.Progspace.is_valid), otherwise return the program_space
>> > diff --git a/gdb/python/python.c b/gdb/python/python.c
>> > index 6a978d632e9..faa7e0c217d 100644
>> > --- a/gdb/python/python.c
>> > +++ b/gdb/python/python.c
>> > @@ -2669,6 +2669,10 @@ Return the name of the currently selected language." },
>> >      "print_options () -> dict\n\
>> >  Return the current print options." },
>> >  
>> > +  { "notify_mi", (PyCFunction) gdbpy_notify_mi,
>> > +    METH_VARARGS | METH_KEYWORDS,
>> > +    "notify_mi (name, data) -> None\n\
>> > +Output async record to MI channels if any." },
>> >    {NULL, NULL, 0, NULL}
>> >  };
>> >  
>> > diff --git a/gdb/testsuite/gdb.python/py-mi-notify.exp b/gdb/testsuite/gdb.python/py-mi-notify.exp
>> > new file mode 100644
>> > index 00000000000..8221794c4f3
>> > --- /dev/null
>> > +++ b/gdb/testsuite/gdb.python/py-mi-notify.exp
>> > @@ -0,0 +1,47 @@
>> > +# Copyright (C) 2019-2023 Free Software Foundation, Inc.
>> 
>> Copyright data should probably be just '2023' here.
>> 
>> Thanks,
>> Andrew
>> 
>> > +# 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>.
>> > +
>> > +# Test custom MI notifications implemented in Python.
>> > +
>> > +load_lib gdb-python.exp
>> > +load_lib mi-support.exp
>> > +set MIFLAGS "-i=mi"
>> > +
>> > +gdb_exit
>> > +if {[mi_gdb_start]} {
>> > +    return
>> > +}
>> > +
>> > +if {[lsearch -exact [mi_get_features] python] < 0} {
>> > +    unsupported "python support is disabled"
>> > +    return -1
>> > +}
>> > +
>> > +standard_testfile
>> > +
>> > +mi_gdb_test "set python print-stack full" \
>> > +    ".*\\^done" \
>> > +    "set python print-stack full"
>> > +
>> > +mi_gdb_test "python gdb.notify_mi('test-notification', None)" \
>> > +    ".*=test-notification\r\n\\^done" \
>> > +    "python notification, no additional data"
>> > +
>> > +mi_gdb_test "python gdb.notify_mi('test-notification', \{ 'data1' : 1 , 'data2' : 2 })" \
>> > +    ".*=test-notification,data1=\"1\",data2=\"2\"\r\n\\^done" \
>> > +    "python notification, with additional data"
>> > +
>> > +mi_gdb_test "python gdb.notify_mi('test-notification', 1)" \
>> > +    ".*\\^error,msg=\".*\"" \
>> > +    "python notification, invalid additional data"
>> > -- 
>> > 2.40.1
>
> T

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Jan Vraný]

On Tue, 2023-09-12 at 14:07 +0100, Andrew Burgess wrote:
> Jan Vraný <Jan.Vrany@labware.com> writes:
> 
> > On Mon, 2023-09-11 at 15:14 +0100, Andrew Burgess wrote:
> > > Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
> > > 
> > > > This commit adds a new Python function, gdb.notify_mi, that can be used
> > > > to emit custom async notification to MI channel.  This can be used, among
> > > > other things, to implement notifications about events MI does not support,
> > > > such as remote connection closed or register change.
> > > > ---
> > > >  gdb/NEWS                                  |  3 ++
> > > >  gdb/doc/python.texi                       | 38 ++++++++++++++++++
> > > >  gdb/python/py-mi.c                        | 49 +++++++++++++++++++++++
> > > >  gdb/python/python-internal.h              |  3 ++
> > > >  gdb/python/python.c                       |  4 ++
> > > >  gdb/testsuite/gdb.python/py-mi-notify.exp | 47 ++++++++++++++++++++++
> > > >  6 files changed, 144 insertions(+)
> > > >  create mode 100644 gdb/testsuite/gdb.python/py-mi-notify.exp
> > > > 
> > > > diff --git a/gdb/NEWS b/gdb/NEWS
> > > > index 98ff00d5efc..4a1f383a666 100644
> > > > --- a/gdb/NEWS
> > > > +++ b/gdb/NEWS
> > > > @@ -286,6 +286,9 @@ info main
> > > >       might be array- or string-like, even if they do not have the
> > > >       corresponding type code.
> > > >  
> > > > +  ** New function gdb.notify_mi(NAME, DATA), that emits custom
> > > > +     GDB/MI async notification.
> > > > +
> > > >  *** Changes in GDB 13
> > > >  
> > > >  * MI version 1 is deprecated, and will be removed in GDB 14.
> > > > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> > > > index e9936991c49..cb2073976ba 100644
> > > > --- a/gdb/doc/python.texi
> > > > +++ b/gdb/doc/python.texi
> > > > @@ -4704,6 +4704,44 @@ Here is how this works using the commands from the example above:
> > > >  @{'string': 'abc, def, ghi'@}
> > > >  @end smallexample
> > > >  
> > > > +@node GDB/MI Notifications In Python
> > > > +@subsubsection @sc{gdb/mi} Notifications In Python
> > > > +
> > > > +@cindex MI notifications in python
> > > > +@cindex notifications in python, GDB/MI
> > > > +@cindex python notifications, GDB/MI
> > > > +
> > > > +It is possible to emit @sc{gdb/mi} notifications from
> > > > +Python.  This is done with the @code{gdb.notify_mi} function.
> > > > +
> > > > +@defun gdb.notify_mi (name , data)
> > > > +Emit a @sc{gdb/mi} asynchronous notification.  @var{name} is the name of the
> > > > +notification, a string.  @var{data} are additional values emitted with the notification, passed
> > > 
> > > I think this reads better:
> > > 
> > >   @var{data} is any additional data to be emitted with the notification,
> > >   passed as a Python dictionary.
> > > 
> > > Eli already asked about which values of @var{name} are valid.  I can see
> > > from the code that _any_ value is accepted, however, a user should
> > > probably be careful not to reuse a name that GDB already uses -- or if
> > > they do then they should ensure that all the required fields are
> > > emitted.
> > > 
> > > Not sure exactly needs saying here, but we probably should at a minimum
> > > include a cross-reference to 'GDB/MI Async Records' so users can find
> > > the list of names that are currently in use.
> > 
> > Sure, will do. 
> > 
> > > 
> > > I also wonder if we should provide some guidance about how user created
> > > async notifications are named, something like:
> > > 
> > >   "User created notifications should be prefixed with a '_' character,
> > >   e.g. gdb.notify_mi ('_foo', ....)"
> > > 
> > > And then GDB can promise to never create an internal Async record that
> > > starts with a '_' character.
> > > 
> > > I'm thinking: the example you give creates a 'connection-removed' event,
> > > but what if, at some future point, GDB actually, officially, adds a
> > > connection-removed event, this is going to conflict with the user
> > > defined notification.  If we require (or suggest) that user defined
> > > notifications are placed into their own namespace in some way, then we
> > > can ensure GDB's internal notifications, and user notifications will
> > > always be separate.  What do you think?  (Obviously '_' is just the
> > > first idea in my head, other strategies exist).
> > 
> > Good point. In case of python-implemented MI commands, overriding existing
> > (built-in) command is forbidden. I'm inclined to do the same in case of 
> > notifications, mostly because it is consistent with MI commands and 
> > is relatively easy to implement (though it'd would require manually coded
> > list of built-in notifications). 
> > 
> > Suggesting python notification should go into separate namespace and promising 
> > built-in notifications never use that namespace is a good idea. 
> > 
> > As for requiring python notifications to be in separate namespace, I'm not sure.
> > Again, we did not required that for python MI commands so it feels it'd be a bit 
> > inconsistent. (Also, if at some future point GDB actually decides to add - say -
> > connection-removed event, why not implement it in python? But that's probably
> > a different discussion)
> 
> If we officially implemented it, then I think _how_ we implemented it
> isn't important.  What's important, is we ideally want to make the user
> a promise that if they follow some rule, GDB isn't going to come along
> and conflict with them.

I agree.

> 
> I agree with you that we shouldn't _prevent_ users from creating
> possibly conflicting names, e.g. if they wanted to emit notifications
> that already exist, or are happy to accept the risk that a later GDB
> might conflict, then that's on them -- so long as they are aware that
> they are taking on this risk, I'm happy with that.

That's not exactly what I meant. What I meant is: 
   (i)    forbid user to emit notifications with the SAME name as existing, internal 
          (built-in) notifications (listed in documentation), but
   (ii)   allow user to emit notifications that may POSSIBLY conflict with future GDB
          version (such as =connection-closed)
   (iii)  promise that GDB would never add notifications in some namespace (say, with leading
          underscore). 

The reason for (i) forbidding existing notifications is to make it consistent with
MI commands - we decided to forbid replacing of existing internal (built-in) commands
with user ones. 

That being said, I do not feel strongly about it so if you think is okay to allow
users to emit currently existing notification, fine with me. 

Best,
Jan

> 
> Thanks,
> Andrew
> 
> 
> > 
> > > 
> > > > +as Python dictionary. The dictionary is converted to converted
> > > > +to a @sc{gdb/mi} @var{result-record} (@pxref{GDB/MI Output Syntax}) the same way
> > > > +as result of Python MI command (@pxref{GDB/MI Commands In Python}).
> > > > +
> > > > +If @var{data} is @code{None} then no additional values are emitted.
> > > > +@end defun
> > > > +
> > > > +Here is how to emit @code{=connection-removed} whenever a connection to remote
> > > > +GDB server is closed (see @pxref{Connections In Python}):
> > > > +
> > > > +@smallexample
> > > > +def notify_connection_removed (event):
> > > > +    data = @{ 'id'   : event.connection.num,
> > > > +              'type' : event.connection.type @}
> > > > +    gdb.notify_mi("connection-removed", data)
> > > > +
> > > > +gdb.events.connection_removed.connect (notify_connection_removed)
> > > > +@end smallexample
> > > > +
> > > > +Then, each time a connection is closed, there will be a notification on MI channel:
> > > > +
> > > > +@smallexample
> > > > +=connection-removed,id="1",type="remote"
> > > > +@end smallexample
> > > > +
> > > >  @node Parameters In Python
> > > >  @subsubsection Parameters In Python
> > > >  
> > > > diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
> > > > index 66dc6fb8a32..dc2ec5ddd0f 100644
> > > > --- a/gdb/python/py-mi.c
> > > > +++ b/gdb/python/py-mi.c
> > > > @@ -19,8 +19,14 @@
> > > >  
> > > >  #include "defs.h"
> > > >  #include "python-internal.h"
> > > > +#include "utils.h"
> > > > +#include "ui.h"
> > > >  #include "ui-out.h"
> > > > +#include "interps.h"
> > > > +#include "target.h"
> > > >  #include "mi/mi-parse.h"
> > > > +#include "mi/mi-console.h"
> > > > +#include "mi/mi-interp.h"
> > > >  
> > > >  /* A ui_out subclass that creates a Python object based on the data
> > > >     that is passed in.  */
> > > > @@ -296,3 +302,46 @@ gdbpy_execute_mi_command (PyObject *self, PyObject *args, PyObject *kw)
> > > >  
> > > >    return uiout.result ();
> > > >  }
> > > > +
> > > > +/* Implementation of the gdb.notify_mi function.  */
> > > 
> > > This comment should be in the header file, and here we should say:
> > > 
> > >   /* See python-internal.h.  */
> > > 
> > > > +
> > > > +PyObject *
> > > > +gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs)
> > > > +{
> > > > +  static const char *keywords[] = { "name", "data", nullptr };
> > > > +  const char *name;
> > > > +  PyObject *data;
> > > > +
> > > > +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "sO", keywords,
> > > > +					&name, &data))
> > > > +    return nullptr; // FIXME: is this the correct way to signal error?
> > > 
> > > Yes it is.  If gdb_PyArg_ParseTupleAndKeywords encounters an error then
> > > a Python exception will have already been set.  Returning nullptr causes
> > > the Python runtime to throw the exception.
> > > 
> > > I wonder if we should make the data argument optional, with Py_None
> > > being the default?  You can write:
> > > 
> > >   PyObject *data = Py_None;
> > > 
> > >   if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|O", keywords,
> > >   			                &name, &data))
> > >     return nullptr;
> > > 
> > > And that should make it so.  Obviously you'd need a corresponding docs
> > > update.
> > > 
> > > > +
> > > > +  SWITCH_THRU_ALL_UIS ()
> > > > +    {
> > > > +      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
> > > > +
> > > > +      if (mi == NULL)
> > > 
> > > s/NULL/nullptr/
> > > 
> > > > +        continue;
> > > > +      
> > > 
> > > The `continue` should use a tab to indent.
> > > 
> > > And the following line, and some other lines in this diff, have trailing
> > > whitespace.  The .gitattributes files at the top-level should turn on
> > > highlighting of whitespace errors like this.  OOI, if you 'git show'
> > > this patch, are the errors not highlighted for you?  I only ask because
> > > maybe we've not set up our .gitattributes correctly.
> > > 
> > > > +      gdb_printf (mi->event_channel, "%s", name);
> > > > +      if (data != Py_None)      
> > > > +        {
> > > > +          /* At the top-level, the data must be a dictionary.  */
> > > > +          if (!PyDict_Check (data))
> > > > +            gdbpy_error (_("Data passed to notify_mi must be either None or a dictionary"));
> > > > +
> > > > +          target_terminal::scoped_restore_terminal_state term_state;
> > > > +          target_terminal::ours_for_output ();
> > > > +
> > > > +          ui_out *mi_uiout = mi->interp_ui_out ();
> > > > +          ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
> > > > +          scoped_restore restore_uiout
> > > > +            = make_scoped_restore (&current_uiout, mi_uiout);
> > > > +
> > > > +          serialize_mi_data (data);	  
> > > > +        }
> > > > +      gdb_flush (mi->event_channel);
> > > > +    }
> > > > +    
> > > > +  Py_RETURN_NONE;
> > > > +}
> > > > diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> > > > index 3f53b0ab6f0..2a7e8d68179 100644
> > > > --- a/gdb/python/python-internal.h
> > > > +++ b/gdb/python/python-internal.h
> > > > @@ -501,6 +501,9 @@ extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
> > > >  
> > > >  extern void serialize_mi_data (PyObject *result);
> > > >  
> > > > +extern PyObject *gdbpy_notify_mi (PyObject *self, PyObject *args,
> > > > +				  PyObject *kw);
> > > > +
> > > >  /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
> > > >     gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
> > > >     valid (see gdb.Progspace.is_valid), otherwise return the program_space
> > > > diff --git a/gdb/python/python.c b/gdb/python/python.c
> > > > index 6a978d632e9..faa7e0c217d 100644
> > > > --- a/gdb/python/python.c
> > > > +++ b/gdb/python/python.c
> > > > @@ -2669,6 +2669,10 @@ Return the name of the currently selected language." },
> > > >      "print_options () -> dict\n\
> > > >  Return the current print options." },
> > > >  
> > > > +  { "notify_mi", (PyCFunction) gdbpy_notify_mi,
> > > > +    METH_VARARGS | METH_KEYWORDS,
> > > > +    "notify_mi (name, data) -> None\n\
> > > > +Output async record to MI channels if any." },
> > > >    {NULL, NULL, 0, NULL}
> > > >  };
> > > >  
> > > > diff --git a/gdb/testsuite/gdb.python/py-mi-notify.exp b/gdb/testsuite/gdb.python/py-mi-notify.exp
> > > > new file mode 100644
> > > > index 00000000000..8221794c4f3
> > > > --- /dev/null
> > > > +++ b/gdb/testsuite/gdb.python/py-mi-notify.exp
> > > > @@ -0,0 +1,47 @@
> > > > +# Copyright (C) 2019-2023 Free Software Foundation, Inc.
> > > 
> > > Copyright data should probably be just '2023' here.
> > > 
> > > Thanks,
> > > Andrew
> > > 
> > > > +# 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>.
> > > > +
> > > > +# Test custom MI notifications implemented in Python.
> > > > +
> > > > +load_lib gdb-python.exp
> > > > +load_lib mi-support.exp
> > > > +set MIFLAGS "-i=mi"
> > > > +
> > > > +gdb_exit
> > > > +if {[mi_gdb_start]} {
> > > > +    return
> > > > +}
> > > > +
> > > > +if {[lsearch -exact [mi_get_features] python] < 0} {
> > > > +    unsupported "python support is disabled"
> > > > +    return -1
> > > > +}
> > > > +
> > > > +standard_testfile
> > > > +
> > > > +mi_gdb_test "set python print-stack full" \
> > > > +    ".*\\^done" \
> > > > +    "set python print-stack full"
> > > > +
> > > > +mi_gdb_test "python gdb.notify_mi('test-notification', None)" \
> > > > +    ".*=test-notification\r\n\\^done" \
> > > > +    "python notification, no additional data"
> > > > +
> > > > +mi_gdb_test "python gdb.notify_mi('test-notification', \{ 'data1' : 1 , 'data2' : 2 })" \
> > > > +    ".*=test-notification,data1=\"1\",data2=\"2\"\r\n\\^done" \
> > > > +    "python notification, with additional data"
> > > > +
> > > > +mi_gdb_test "python gdb.notify_mi('test-notification', 1)" \
> > > > +    ".*\\^error,msg=\".*\"" \
> > > > +    "python notification, invalid additional data"
> > > > -- 
> > > > 2.40.1
> > 
> > T

Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications

[Andrew Burgess]

Jan Vraný <Jan.Vrany@labware.com> writes:

> On Tue, 2023-09-12 at 14:07 +0100, Andrew Burgess wrote:
>> Jan Vraný <Jan.Vrany@labware.com> writes:
>> 
>> > On Mon, 2023-09-11 at 15:14 +0100, Andrew Burgess wrote:
>> > > Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:
>> > > 
>> > > > This commit adds a new Python function, gdb.notify_mi, that can be used
>> > > > to emit custom async notification to MI channel.  This can be used, among
>> > > > other things, to implement notifications about events MI does not support,
>> > > > such as remote connection closed or register change.
>> > > > ---
>> > > >  gdb/NEWS                                  |  3 ++
>> > > >  gdb/doc/python.texi                       | 38 ++++++++++++++++++
>> > > >  gdb/python/py-mi.c                        | 49 +++++++++++++++++++++++
>> > > >  gdb/python/python-internal.h              |  3 ++
>> > > >  gdb/python/python.c                       |  4 ++
>> > > >  gdb/testsuite/gdb.python/py-mi-notify.exp | 47 ++++++++++++++++++++++
>> > > >  6 files changed, 144 insertions(+)
>> > > >  create mode 100644 gdb/testsuite/gdb.python/py-mi-notify.exp
>> > > > 
>> > > > diff --git a/gdb/NEWS b/gdb/NEWS
>> > > > index 98ff00d5efc..4a1f383a666 100644
>> > > > --- a/gdb/NEWS
>> > > > +++ b/gdb/NEWS
>> > > > @@ -286,6 +286,9 @@ info main
>> > > >       might be array- or string-like, even if they do not have the
>> > > >       corresponding type code.
>> > > >  
>> > > > +  ** New function gdb.notify_mi(NAME, DATA), that emits custom
>> > > > +     GDB/MI async notification.
>> > > > +
>> > > >  *** Changes in GDB 13
>> > > >  
>> > > >  * MI version 1 is deprecated, and will be removed in GDB 14.
>> > > > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
>> > > > index e9936991c49..cb2073976ba 100644
>> > > > --- a/gdb/doc/python.texi
>> > > > +++ b/gdb/doc/python.texi
>> > > > @@ -4704,6 +4704,44 @@ Here is how this works using the commands from the example above:
>> > > >  @{'string': 'abc, def, ghi'@}
>> > > >  @end smallexample
>> > > >  
>> > > > +@node GDB/MI Notifications In Python
>> > > > +@subsubsection @sc{gdb/mi} Notifications In Python
>> > > > +
>> > > > +@cindex MI notifications in python
>> > > > +@cindex notifications in python, GDB/MI
>> > > > +@cindex python notifications, GDB/MI
>> > > > +
>> > > > +It is possible to emit @sc{gdb/mi} notifications from
>> > > > +Python.  This is done with the @code{gdb.notify_mi} function.
>> > > > +
>> > > > +@defun gdb.notify_mi (name , data)
>> > > > +Emit a @sc{gdb/mi} asynchronous notification.  @var{name} is the name of the
>> > > > +notification, a string.  @var{data} are additional values emitted with the notification, passed
>> > > 
>> > > I think this reads better:
>> > > 
>> > >   @var{data} is any additional data to be emitted with the notification,
>> > >   passed as a Python dictionary.
>> > > 
>> > > Eli already asked about which values of @var{name} are valid.  I can see
>> > > from the code that _any_ value is accepted, however, a user should
>> > > probably be careful not to reuse a name that GDB already uses -- or if
>> > > they do then they should ensure that all the required fields are
>> > > emitted.
>> > > 
>> > > Not sure exactly needs saying here, but we probably should at a minimum
>> > > include a cross-reference to 'GDB/MI Async Records' so users can find
>> > > the list of names that are currently in use.
>> > 
>> > Sure, will do. 
>> > 
>> > > 
>> > > I also wonder if we should provide some guidance about how user created
>> > > async notifications are named, something like:
>> > > 
>> > >   "User created notifications should be prefixed with a '_' character,
>> > >   e.g. gdb.notify_mi ('_foo', ....)"
>> > > 
>> > > And then GDB can promise to never create an internal Async record that
>> > > starts with a '_' character.
>> > > 
>> > > I'm thinking: the example you give creates a 'connection-removed' event,
>> > > but what if, at some future point, GDB actually, officially, adds a
>> > > connection-removed event, this is going to conflict with the user
>> > > defined notification.  If we require (or suggest) that user defined
>> > > notifications are placed into their own namespace in some way, then we
>> > > can ensure GDB's internal notifications, and user notifications will
>> > > always be separate.  What do you think?  (Obviously '_' is just the
>> > > first idea in my head, other strategies exist).
>> > 
>> > Good point. In case of python-implemented MI commands, overriding existing
>> > (built-in) command is forbidden. I'm inclined to do the same in case of 
>> > notifications, mostly because it is consistent with MI commands and 
>> > is relatively easy to implement (though it'd would require manually coded
>> > list of built-in notifications). 
>> > 
>> > Suggesting python notification should go into separate namespace and promising 
>> > built-in notifications never use that namespace is a good idea. 
>> > 
>> > As for requiring python notifications to be in separate namespace, I'm not sure.
>> > Again, we did not required that for python MI commands so it feels it'd be a bit 
>> > inconsistent. (Also, if at some future point GDB actually decides to add - say -
>> > connection-removed event, why not implement it in python? But that's probably
>> > a different discussion)
>> 
>> If we officially implemented it, then I think _how_ we implemented it
>> isn't important.  What's important, is we ideally want to make the user
>> a promise that if they follow some rule, GDB isn't going to come along
>> and conflict with them.
>
> I agree.
>
>> 
>> I agree with you that we shouldn't _prevent_ users from creating
>> possibly conflicting names, e.g. if they wanted to emit notifications
>> that already exist, or are happy to accept the risk that a later GDB
>> might conflict, then that's on them -- so long as they are aware that
>> they are taking on this risk, I'm happy with that.
>
> That's not exactly what I meant. What I meant is: 
>    (i)    forbid user to emit notifications with the SAME name as existing, internal 
>           (built-in) notifications (listed in documentation), but
>    (ii)   allow user to emit notifications that may POSSIBLY conflict with future GDB
>           version (such as =connection-closed)
>    (iii)  promise that GDB would never add notifications in some namespace (say, with leading
>           underscore). 
>
> The reason for (i) forbidding existing notifications is to make it consistent with
> MI commands - we decided to forbid replacing of existing internal (built-in) commands
> with user ones. 
>
> That being said, I do not feel strongly about it so if you think is okay to allow
> users to emit currently existing notification, fine with me.

I wouldn't bother.  My reason being that there's no easy way to check if
some string is an existing notification or not -- without including a
new list of strings, which just feels like it's going to become out of
date at some point.

I would just add a caveat to the docs that emitting an existing
notification (with cross link to the list in the docs) might break
existing front ends that expect GDB's existing behaviour.

Then just treat (i) as (ii) -- we allow it, but user beware.

Thanks,
Andrew


>
> Best,
> Jan
>
>> 
>> Thanks,
>> Andrew
>> 
>> 
>> > 
>> > > 
>> > > > +as Python dictionary. The dictionary is converted to converted
>> > > > +to a @sc{gdb/mi} @var{result-record} (@pxref{GDB/MI Output Syntax}) the same way
>> > > > +as result of Python MI command (@pxref{GDB/MI Commands In Python}).
>> > > > +
>> > > > +If @var{data} is @code{None} then no additional values are emitted.
>> > > > +@end defun
>> > > > +
>> > > > +Here is how to emit @code{=connection-removed} whenever a connection to remote
>> > > > +GDB server is closed (see @pxref{Connections In Python}):
>> > > > +
>> > > > +@smallexample
>> > > > +def notify_connection_removed (event):
>> > > > +    data = @{ 'id'   : event.connection.num,
>> > > > +              'type' : event.connection.type @}
>> > > > +    gdb.notify_mi("connection-removed", data)
>> > > > +
>> > > > +gdb.events.connection_removed.connect (notify_connection_removed)
>> > > > +@end smallexample
>> > > > +
>> > > > +Then, each time a connection is closed, there will be a notification on MI channel:
>> > > > +
>> > > > +@smallexample
>> > > > +=connection-removed,id="1",type="remote"
>> > > > +@end smallexample
>> > > > +
>> > > >  @node Parameters In Python
>> > > >  @subsubsection Parameters In Python
>> > > >  
>> > > > diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
>> > > > index 66dc6fb8a32..dc2ec5ddd0f 100644
>> > > > --- a/gdb/python/py-mi.c
>> > > > +++ b/gdb/python/py-mi.c
>> > > > @@ -19,8 +19,14 @@
>> > > >  
>> > > >  #include "defs.h"
>> > > >  #include "python-internal.h"
>> > > > +#include "utils.h"
>> > > > +#include "ui.h"
>> > > >  #include "ui-out.h"
>> > > > +#include "interps.h"
>> > > > +#include "target.h"
>> > > >  #include "mi/mi-parse.h"
>> > > > +#include "mi/mi-console.h"
>> > > > +#include "mi/mi-interp.h"
>> > > >  
>> > > >  /* A ui_out subclass that creates a Python object based on the data
>> > > >     that is passed in.  */
>> > > > @@ -296,3 +302,46 @@ gdbpy_execute_mi_command (PyObject *self, PyObject *args, PyObject *kw)
>> > > >  
>> > > >    return uiout.result ();
>> > > >  }
>> > > > +
>> > > > +/* Implementation of the gdb.notify_mi function.  */
>> > > 
>> > > This comment should be in the header file, and here we should say:
>> > > 
>> > >   /* See python-internal.h.  */
>> > > 
>> > > > +
>> > > > +PyObject *
>> > > > +gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs)
>> > > > +{
>> > > > +  static const char *keywords[] = { "name", "data", nullptr };
>> > > > +  const char *name;
>> > > > +  PyObject *data;
>> > > > +
>> > > > +  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "sO", keywords,
>> > > > +					&name, &data))
>> > > > +    return nullptr; // FIXME: is this the correct way to signal error?
>> > > 
>> > > Yes it is.  If gdb_PyArg_ParseTupleAndKeywords encounters an error then
>> > > a Python exception will have already been set.  Returning nullptr causes
>> > > the Python runtime to throw the exception.
>> > > 
>> > > I wonder if we should make the data argument optional, with Py_None
>> > > being the default?  You can write:
>> > > 
>> > >   PyObject *data = Py_None;
>> > > 
>> > >   if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|O", keywords,
>> > >   			                &name, &data))
>> > >     return nullptr;
>> > > 
>> > > And that should make it so.  Obviously you'd need a corresponding docs
>> > > update.
>> > > 
>> > > > +
>> > > > +  SWITCH_THRU_ALL_UIS ()
>> > > > +    {
>> > > > +      struct mi_interp *mi = as_mi_interp (top_level_interpreter ());
>> > > > +
>> > > > +      if (mi == NULL)
>> > > 
>> > > s/NULL/nullptr/
>> > > 
>> > > > +        continue;
>> > > > +      
>> > > 
>> > > The `continue` should use a tab to indent.
>> > > 
>> > > And the following line, and some other lines in this diff, have trailing
>> > > whitespace.  The .gitattributes files at the top-level should turn on
>> > > highlighting of whitespace errors like this.  OOI, if you 'git show'
>> > > this patch, are the errors not highlighted for you?  I only ask because
>> > > maybe we've not set up our .gitattributes correctly.
>> > > 
>> > > > +      gdb_printf (mi->event_channel, "%s", name);
>> > > > +      if (data != Py_None)      
>> > > > +        {
>> > > > +          /* At the top-level, the data must be a dictionary.  */
>> > > > +          if (!PyDict_Check (data))
>> > > > +            gdbpy_error (_("Data passed to notify_mi must be either None or a dictionary"));
>> > > > +
>> > > > +          target_terminal::scoped_restore_terminal_state term_state;
>> > > > +          target_terminal::ours_for_output ();
>> > > > +
>> > > > +          ui_out *mi_uiout = mi->interp_ui_out ();
>> > > > +          ui_out_redirect_pop redir (mi_uiout, mi->event_channel);
>> > > > +          scoped_restore restore_uiout
>> > > > +            = make_scoped_restore (&current_uiout, mi_uiout);
>> > > > +
>> > > > +          serialize_mi_data (data);	  
>> > > > +        }
>> > > > +      gdb_flush (mi->event_channel);
>> > > > +    }
>> > > > +    
>> > > > +  Py_RETURN_NONE;
>> > > > +}
>> > > > diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
>> > > > index 3f53b0ab6f0..2a7e8d68179 100644
>> > > > --- a/gdb/python/python-internal.h
>> > > > +++ b/gdb/python/python-internal.h
>> > > > @@ -501,6 +501,9 @@ extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args,
>> > > >  
>> > > >  extern void serialize_mi_data (PyObject *result);
>> > > >  
>> > > > +extern PyObject *gdbpy_notify_mi (PyObject *self, PyObject *args,
>> > > > +				  PyObject *kw);
>> > > > +
>> > > >  /* Convert Python object OBJ to a program_space pointer.  OBJ must be a
>> > > >     gdb.Progspace reference.  Return nullptr if the gdb.Progspace is not
>> > > >     valid (see gdb.Progspace.is_valid), otherwise return the program_space
>> > > > diff --git a/gdb/python/python.c b/gdb/python/python.c
>> > > > index 6a978d632e9..faa7e0c217d 100644
>> > > > --- a/gdb/python/python.c
>> > > > +++ b/gdb/python/python.c
>> > > > @@ -2669,6 +2669,10 @@ Return the name of the currently selected language." },
>> > > >      "print_options () -> dict\n\
>> > > >  Return the current print options." },
>> > > >  
>> > > > +  { "notify_mi", (PyCFunction) gdbpy_notify_mi,
>> > > > +    METH_VARARGS | METH_KEYWORDS,
>> > > > +    "notify_mi (name, data) -> None\n\
>> > > > +Output async record to MI channels if any." },
>> > > >    {NULL, NULL, 0, NULL}
>> > > >  };
>> > > >  
>> > > > diff --git a/gdb/testsuite/gdb.python/py-mi-notify.exp b/gdb/testsuite/gdb.python/py-mi-notify.exp
>> > > > new file mode 100644
>> > > > index 00000000000..8221794c4f3
>> > > > --- /dev/null
>> > > > +++ b/gdb/testsuite/gdb.python/py-mi-notify.exp
>> > > > @@ -0,0 +1,47 @@
>> > > > +# Copyright (C) 2019-2023 Free Software Foundation, Inc.
>> > > 
>> > > Copyright data should probably be just '2023' here.
>> > > 
>> > > Thanks,
>> > > Andrew
>> > > 
>> > > > +# 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>.
>> > > > +
>> > > > +# Test custom MI notifications implemented in Python.
>> > > > +
>> > > > +load_lib gdb-python.exp
>> > > > +load_lib mi-support.exp
>> > > > +set MIFLAGS "-i=mi"
>> > > > +
>> > > > +gdb_exit
>> > > > +if {[mi_gdb_start]} {
>> > > > +    return
>> > > > +}
>> > > > +
>> > > > +if {[lsearch -exact [mi_get_features] python] < 0} {
>> > > > +    unsupported "python support is disabled"
>> > > > +    return -1
>> > > > +}
>> > > > +
>> > > > +standard_testfile
>> > > > +
>> > > > +mi_gdb_test "set python print-stack full" \
>> > > > +    ".*\\^done" \
>> > > > +    "set python print-stack full"
>> > > > +
>> > > > +mi_gdb_test "python gdb.notify_mi('test-notification', None)" \
>> > > > +    ".*=test-notification\r\n\\^done" \
>> > > > +    "python notification, no additional data"
>> > > > +
>> > > > +mi_gdb_test "python gdb.notify_mi('test-notification', \{ 'data1' : 1 , 'data2' : 2 })" \
>> > > > +    ".*=test-notification,data1=\"1\",data2=\"2\"\r\n\\^done" \
>> > > > +    "python notification, with additional data"
>> > > > +
>> > > > +mi_gdb_test "python gdb.notify_mi('test-notification', 1)" \
>> > > > +    ".*\\^error,msg=\".*\"" \
>> > > > +    "python notification, invalid additional data"
>> > > > -- 
>> > > > 2.40.1
>> > 
>> > T

Re: [PATCH 1/2] gdb/python: generalize serialize_mi_result()

[Tom Tromey]

>>>>> "Jan" == Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:

Jan> This commit renames serialize_mi_result() to serialize_mi_data() and makes
Jan> it usable in different contexts than printing result of custom MI command
Jan> (hence the rename).

There's some missing context here, which is you wanted a
registers-changed MI event.

I think it's fine to add new MI events, so if this is just in support of
that, you may prefer to do it directly.

Jan> To do so, the check whether passed Python object is a dictionary has been
Jan> moved to the caller - at the very least, different usages require different
Jan> error messages. Also it seemed to me that py-utils.c is a better place
Jan> for its code as it is now a utility function not specific to Python MI
Jan> commands.

It's still MI-specific, py-utils is really for utility things used by
the other Python-layer code in gdb.

I didn't really read the patch.

Tom