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.129.124]) by sourceware.org (Postfix) with ESMTPS id 11DD23858D3C for ; Tue, 12 Sep 2023 13:53:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 11DD23858D3C 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=1694526817; 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=ru0DunN5EVz7aKrf/umvt3+4mko6ZvyfGB4h77RYfBI=; b=J0UMG7UyJh7TXFvliv21VcIHD75xYd36Mb4Sfuiru10txWoCDBFVqg9s+OxGD1jM7ngpiF 6SjgH4RPTLurdHs7qHUAVv9zikLsDHB4+Nvun6TRin/RY3mlbDYZpI1SRReNeJh59N2csc uTcZUOh7X9mjW59ApMeQ0i/N1ahNtxU= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-627-49-CxtOHPQmsK4pD2enhfA-1; Tue, 12 Sep 2023 09:53:35 -0400 X-MC-Unique: 49-CxtOHPQmsK4pD2enhfA-1 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-3fd0fa4d08cso42748205e9.1 for ; Tue, 12 Sep 2023 06:53:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694526814; x=1695131614; 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=C4tchx3BORBrD8xvg/UvfBHfMZFKBSEueMFjEDlt8po=; b=Z7JQHK+UNmn0jRCvsuFlYi/LWbA23r+NYE4Dw9F9jpS/xyafBBk9ZxkWTw/WKMC9Ig GXw8+T7E+VO4ImJJ38NbovJTkI+AXHmNRx/OaQFQ9gfRXXa4/P6AyNCTrWAZVYn9c+IP 6LYpylLTOXRQVZQ4qpgcsouvzYcYAO6lYYlx5z/4Bu+x/31fFn5QJ/EPIOb9RD1yjl0j fkMvU15bPqj9XPF4Syiwk+tAP3XQNlfgOX8nvMPqYdN0TzfynsDfxwq8clu2eZdOx+od 47nk+YwEtQ65dnOaM0z1byyxS4RUitIbMepNix6yXbH9sQYZGdjsv3bSNmj/alUIlrqh IUsg== X-Gm-Message-State: AOJu0Yx1DSq5pSUfh2ewKe9DdRTCNOMuKpivBpbZS7T6wLHjmuAvhXWw 4AIoPYanxDAdCEZ3yrAP6e33e+VAk+GDHiaDZJTvfyutB7WD18Lsc6D4letjJPK9ebe+NStFD6c qP2sxqZfUk3qmn4GPAEGIs2PmsUMBUQ== X-Received: by 2002:a1c:4b0a:0:b0:400:c0e8:18c6 with SMTP id y10-20020a1c4b0a000000b00400c0e818c6mr10909723wma.18.1694526814375; Tue, 12 Sep 2023 06:53:34 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFRgx2kq5j5vGGwe3Ijg6wWBV8kA/57QkxdTbfV3FCgg0TOYzigK+FcHySjHr1Y82y4tNJtZA== X-Received: by 2002:a1c:4b0a:0:b0:400:c0e8:18c6 with SMTP id y10-20020a1c4b0a000000b00400c0e818c6mr10909705wma.18.1694526813889; Tue, 12 Sep 2023 06:53:33 -0700 (PDT) Received: from localhost (197.126.90.146.dyn.plus.net. [146.90.126.197]) by smtp.gmail.com with ESMTPSA id n8-20020a7bcbc8000000b003fe29f6b61bsm12966754wmi.46.2023.09.12.06.53.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Sep 2023 06:53:33 -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: <74e472763ab2a0c34353f2767606408ee7e92862.camel@labware.com> References: <20230908210504.89194-1-jan.vrany@labware.com> <20230908210504.89194-2-jan.vrany@labware.com> <877cowlrv8.fsf@redhat.com> <87ttrzilpe.fsf@redhat.com> <74e472763ab2a0c34353f2767606408ee7e92862.camel@labware.com> Date: Tue, 12 Sep 2023 14:53:32 +0100 Message-ID: <87ledbijkz.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_H4,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 Tue, 2023-09-12 at 14:07 +0100, Andrew Burgess wrote: >> Jan Vran=C3=BD writes: >>=20 >> > 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= 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 >> > > >=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 f= rom 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= with the notification, passed >> > >=20 >> > > I think this reads better: >> > >=20 >> > > @var{data} is any additional data to be emitted with the notificat= ion, >> > > 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 mini= mum >> > > include a cross-reference to 'GDB/MI Async Records' so users can fin= d >> > > the list of names that are currently in use. >> >=20 >> > Sure, will do.=20 >> >=20 >> > >=20 >> > > I also wonder if we should provide some guidance about how user crea= ted >> > > async notifications are named, something like: >> > >=20 >> > > "User created notifications should be prefixed with a '_' characte= r, >> > > e.g. gdb.notify_mi ('_foo', ....)" >> > >=20 >> > > And then GDB can promise to never create an internal Async record th= at >> > > starts with a '_' character. >> > >=20 >> > > I'm thinking: the example you give creates a 'connection-removed' ev= ent, >> > > 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). >> >=20 >> > Good point. In case of python-implemented MI commands, overriding exis= ting >> > (built-in) command is forbidden.=C2=A0I'm inclined to do the same in c= ase 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 co= ded >> > list of built-in notifications).=20 >> >=20 >> > Suggesting python notification should go into separate namespace and p= romising=C2=A0 >> > built-in notifications never use that namespace is a good idea.=20 >> >=20 >> > 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=C2=A0 >> > inconsistent. (Also, if at some future point GDB actually decides to a= dd - say - >> > connection-removed event, why not implement it in python? But that's p= robably >> > a different discussion) >>=20 >> 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. > >>=20 >> 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:=C2=A0 > (i) forbid user to emit notifications with the SAME name as existin= g, internal=C2=A0 > (built-in) notifications (listed in documentation), but > (ii) allow user to emit notifications that may POSSIBLY conflict wit= h future GDB > version (such as =3Dconnection-closed) > (iii) promise that GDB would never add notifications in some namespac= e (say, with leading > underscore).=20 > > The reason for (i) forbidding existing notifications is to make it consis= tent with > MI commands - we decided to forbid replacing of existing internal (built-= in) commands > with user ones.=20 > > 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 > >>=20 >> Thanks, >> Andrew >>=20 >>=20 >> >=20 >> > >=20 >> > > > +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 emitte= d. >> > > > +@end defun >> > > > + >> > > > +Here is how to emit @code{=3Dconnection-removed} whenever a conne= ction 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 notificat= ion 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 da= ta >> > > > that is passed in. */ >> > > > @@ -296,3 +302,46 @@ gdbpy_execute_mi_command (PyObject *self, PyO= bject *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", keywo= rds, >> > > > +=09=09=09=09=09&name, &data)) >> > > > + return nullptr; // FIXME: is this the correct way to signal e= rror? >> > >=20 >> > > Yes it is. If gdb_PyArg_ParseTupleAndKeywords encounters an error t= hen >> > > a Python exception will have already been set. Returning nullptr ca= uses >> > > 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", keyword= s, >> > > =09=09=09 &name, &data)) >> > > return nullptr; >> > >=20 >> > > And that should make it so. Obviously you'd need a corresponding do= cs >> > > update. >> > >=20 >> > > > + >> > > > + SWITCH_THRU_ALL_UIS () >> > > > + { >> > > > + struct mi_interp *mi =3D as_mi_interp (top_level_interprete= r ()); >> > > > + >> > > > + 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 trai= ling >> > > whitespace. The .gitattributes files at the top-level should turn o= n >> > > highlighting of whitespace errors like this. OOI, if you 'git show' >> > > this patch, are the errors not highlighted for you? I only ask beca= use >> > > 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 eith= er None or a dictionary")); >> > > > + >> > > > + target_terminal::scoped_restore_terminal_state term_sta= te; >> > > > + 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-inte= rnal.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 (PyO= bject *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 mus= t be a >> > > > gdb.Progspace reference. Return nullptr if the gdb.Progspace = is not >> > > > valid (see gdb.Progspace.is_valid), otherwise return the progr= am_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 l= anguage." }, >> > > > "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/tests= uite/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 m= odify >> > > > +# it under the terms of the GNU General Public License as publish= ed 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 Licen= se >> > > > +# 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\\^don= e" \ >> > > > + "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 >> >=20 >> > T