[PATCH 2/2] gdb/python: implement support for sending custom MI async notifications
Jan Vraný
Jan.Vrany@labware.com
Tue Sep 12 13:45:29 GMT 2023
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 (¤t_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
More information about the Gdb-patches
mailing list