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

[Andrew Burgess <aburgess@redhat.com>]

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