From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id DEB733858D3C for ; Tue, 12 Sep 2023 13:07:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DEB733858D3C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1694524067; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eGBJjElBGh1/Hg+RoWO/Qpck1YbWxgcmiDJbhUVJr/o=; b=J3uqQ01TAM74wo1iXTlfjGXmcYTXmw5MHsSOKBNgNtmpsvQLMF01E0TqTOVcisvD5Bgjdl mDnBM2RjmLYyd1n33kqYeudCtLKV74F0SdqSYm2Y2OLohDqLvUTq0erO+Gq2cTTo/YHSF7 SGhLdqOKGNuS54nX9S6R7KkxLoHe0gc= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-252-Plq6L4ZIM9-loCqrhq6-Zw-1; Tue, 12 Sep 2023 09:07:45 -0400 X-MC-Unique: Plq6L4ZIM9-loCqrhq6-Zw-1 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-31f46ccee0fso2637648f8f.1 for ; Tue, 12 Sep 2023 06:07:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694524063; x=1695128863; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:to:from:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=hcOU4a2xdxaHlFcgk/A/6fm4kUD3QwdkwnCyZwLVfao=; b=jxxDIXzxNf7v9/bWZeblAdo8X6UzSLwedE2qjFcL+xKKfHpkPHbDw5p8MhxI9P1hPy F1L2fUa5exgAxFSji/b993IvZN9oDIQJw2x0CUSYlKffANFlUh4l/5qAEMu6O5mxDt8b vvQb+OO59CMS2+KfBtn+hsOy4hExftPKitEBXx+Z8hzIQCl1FIDWakYTqcmBfiUYYJ8s 6OiK+z6LA/JLuFNEbe9PJe1ydsRNucbnDjmb3r3rl7Ks+PHXlC5YXA+WYNNjyzcUP4iF 2OUnAzSkJuYhl5+L3FkbSD4zP2uKL2nkCIMyEd+zjUDtqAxRrQ2JVn6UB7/Ixdr1syS1 uY6g== X-Gm-Message-State: AOJu0YzA7mQcETJhVjSSofC4XIEJZ9V27/HBdmkNytfnG4GeHvqYHdvJ fpxKq6st8wlygnoe1i57ddq15ukfFsb9TAg6FoCscbFS4QRLyiqmeU/UZ/EBZclQZK/yGXPtgab 6cyxPkS50QWZUpqywDFpq0GR8Nra+lQ== X-Received: by 2002:adf:cf08:0:b0:31d:cb4b:ccd3 with SMTP id o8-20020adfcf08000000b0031dcb4bccd3mr1786153wrj.21.1694524063416; Tue, 12 Sep 2023 06:07:43 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGQsfekDmOFcuWR/Oje7QX+a/0E2kfBNc3GRzaTCpUBc4gMcYXbNNgWTz1ANgcFMPvZZlWyPA== X-Received: by 2002:adf:cf08:0:b0:31d:cb4b:ccd3 with SMTP id o8-20020adfcf08000000b0031dcb4bccd3mr1786125wrj.21.1694524062876; Tue, 12 Sep 2023 06:07:42 -0700 (PDT) Received: from localhost (197.126.90.146.dyn.plus.net. [146.90.126.197]) by smtp.gmail.com with ESMTPSA id z2-20020a5d4c82000000b0031aca6cc69csm12839093wrs.2.2023.09.12.06.07.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Sep 2023 06:07:42 -0700 (PDT) From: Andrew Burgess To: Jan =?utf-8?Q?Vran=C3=BD?= , "gdb-patches@sourceware.org" Subject: Re: [PATCH 2/2] gdb/python: implement support for sending custom MI async notifications In-Reply-To: References: <20230908210504.89194-1-jan.vrany@labware.com> <20230908210504.89194-2-jan.vrany@labware.com> <877cowlrv8.fsf@redhat.com> Date: Tue, 12 Sep 2023 14:07:41 +0100 Message-ID: <87ttrzilpe.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Jan Vran=C3=BD writes: > On Mon, 2023-09-11 at 15:14 +0100, Andrew Burgess wrote: >> Jan Vrany via Gdb-patches writes: >>=20 >> > This commit adds a new Python function, gdb.notify_mi, that can be use= d >> > to emit custom async notification to MI channel. This can be used, am= ong >> > other things, to implement notifications about events MI does not supp= ort, >> > 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 >> >=20 >> > 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. >> > =20 >> > + ** New function gdb.notify_mi(NAME, DATA), that emits custom >> > + GDB/MI async notification. >> > + >> > *** Changes in GDB 13 >> > =20 >> > * 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 >> > =20 >> > +@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 wit= h the notification, passed >>=20 >> I think this reads better: >>=20 >> @var{data} is any additional data to be emitted with the notification, >> passed as a Python dictionary. >>=20 >> 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. >>=20 >> 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.=20 > >>=20 >> I also wonder if we should provide some guidance about how user created >> async notifications are named, something like: >>=20 >> "User created notifications should be prefixed with a '_' character, >> e.g. gdb.notify_mi ('_foo', ....)" >>=20 >> And then GDB can promise to never create an internal Async record that >> starts with a '_' character. >>=20 >> 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 existin= g > (built-in) command is forbidden.=C2=A0I'm inclined to do the same in case= of=C2=A0 > notifications, mostly because it is consistent with MI commands and=C2=A0 > is relatively easy to implement (though it'd would require manually coded > list of built-in notifications).=20 > > Suggesting python notification should go into separate namespace and prom= ising=C2=A0 > built-in notifications never use that namespace is a good idea.=20 > > As for requiring python notifications to be in separate namespace, I'm no= t sure. > Again, we did not required that for python MI commands so it feels it'd b= e a bit=C2=A0 > 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 prob= ably > 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 > >>=20 >> > +as Python dictionary. The dictionary is converted to converted >> > +to a @sc{gdb/mi} @var{result-record} (@pxref{GDB/MI Output Syntax}) t= he 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{=3Dconnection-removed} whenever a connectio= n to remote >> > +GDB server is closed (see @pxref{Connections In Python}): >> > + >> > +@smallexample >> > +def notify_connection_removed (event): >> > + data =3D @{ '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 >> > +=3Dconnection-removed,id=3D"1",type=3D"remote" >> > +@end smallexample >> > + >> > @node Parameters In Python >> > @subsubsection Parameters In Python >> > =20 >> > 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 @@ >> > =20 >> > #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" >> > =20 >> > /* 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, PyObjec= t *args, PyObject *kw) >> > =20 >> > return uiout.result (); >> > } >> > + >> > +/* Implementation of the gdb.notify_mi function. */ >>=20 >> This comment should be in the header file, and here we should say: >>=20 >> /* See python-internal.h. */ >>=20 >> > + >> > +PyObject * >> > +gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs) >> > +{ >> > + static const char *keywords[] =3D { "name", "data", nullptr }; >> > + const char *name; >> > + PyObject *data; >> > + >> > + if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "sO", keywords, >> > +=09=09=09=09=09&name, &data)) >> > + return nullptr; // FIXME: is this the correct way to signal error= ? >>=20 >> 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. >>=20 >> I wonder if we should make the data argument optional, with Py_None >> being the default? You can write: >>=20 >> PyObject *data =3D Py_None; >>=20 >> if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|O", keywords, >> =09=09=09 &name, &data)) >> return nullptr; >>=20 >> And that should make it so. Obviously you'd need a corresponding docs >> update. >>=20 >> > + >> > + SWITCH_THRU_ALL_UIS () >> > + { >> > + struct mi_interp *mi =3D as_mi_interp (top_level_interpreter ()= ); >> > + >> > + if (mi =3D=3D NULL) >>=20 >> s/NULL/nullptr/ >>=20 >> > + continue; >> > + =20 >>=20 >> The `continue` should use a tab to indent. >>=20 >> 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. >>=20 >> > + gdb_printf (mi->event_channel, "%s", name); >> > + if (data !=3D Py_None) =20 >> > + { >> > + /* At the top-level, the data must be a dictionary. */ >> > + if (!PyDict_Check (data)) >> > + gdbpy_error (_("Data passed to notify_mi must be either N= one or a dictionary")); >> > + >> > + target_terminal::scoped_restore_terminal_state term_state; >> > + target_terminal::ours_for_output (); >> > + >> > + ui_out *mi_uiout =3D mi->interp_ui_out (); >> > + ui_out_redirect_pop redir (mi_uiout, mi->event_channel); >> > + scoped_restore restore_uiout >> > + =3D make_scoped_restore (¤t_uiout, mi_uiout); >> > + >> > + serialize_mi_data (data);=09 =20 >> > + } >> > + gdb_flush (mi->event_channel); >> > + } >> > + =20 >> > + 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 (PyObjec= t *self, PyObject *args, >> > =20 >> > extern void serialize_mi_data (PyObject *result); >> > =20 >> > +extern PyObject *gdbpy_notify_mi (PyObject *self, PyObject *args, >> > +=09=09=09=09 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 n= ot >> > valid (see gdb.Progspace.is_valid), otherwise return the program_s= pace >> > 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 langu= age." }, >> > "print_options () -> dict\n\ >> > Return the current print options." }, >> > =20 >> > + { "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} >> > }; >> > =20 >> > 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. >>=20 >> Copyright data should probably be just '2023' here. >>=20 >> Thanks, >> Andrew >>=20 >> > +# This program is free software; you can redistribute it and/or modif= y >> > +# it under the terms of the GNU General Public License as published b= y >> > +# 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 = . >> > + >> > +# Test custom MI notifications implemented in Python. >> > + >> > +load_lib gdb-python.exp >> > +load_lib mi-support.exp >> > +set MIFLAGS "-i=3Dmi" >> > + >> > +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)" \ >> > + ".*=3Dtest-notification\r\n\\^done" \ >> > + "python notification, no additional data" >> > + >> > +mi_gdb_test "python gdb.notify_mi('test-notification', \{ 'data1' : 1= , 'data2' : 2 })" \ >> > + ".*=3Dtest-notification,data1=3D\"1\",data2=3D\"2\"\r\n\\^done" \ >> > + "python notification, with additional data" >> > + >> > +mi_gdb_test "python gdb.notify_mi('test-notification', 1)" \ >> > + ".*\\^error,msg=3D\".*\"" \ >> > + "python notification, invalid additional data" >> > --=20 >> > 2.40.1 > > T