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 69B1738313B9 for ; Thu, 5 Oct 2023 14:19:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 69B1738313B9 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=1696515598; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=lehJB2MlHeZ1L6IHwzhQfcYiVQVvECS+ltIbO7LyrN4=; b=Di87LitlUURJgkvBbPAyTLRIr02fIKqz9lsyL/TW1XB0M46V8sj8lBwSJF5r+OYgq/Psho UtFqpulIt56m9AjlDUAiZsEV9WT6Iih2Wbgde1wGHvsbDY7zFGMyVNtJch0Hi6JveWyhQd E4ddCB2rXwvS4C+VFRnXCGQ9XAOFl5U= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-179-dY4cTSMpNtipEYq87pOSHg-1; Thu, 05 Oct 2023 10:19:42 -0400 X-MC-Unique: dY4cTSMpNtipEYq87pOSHg-1 Received: by mail-qk1-f199.google.com with SMTP id af79cd13be357-77578227e4bso120262685a.2 for ; Thu, 05 Oct 2023 07:19:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696515582; x=1697120382; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=lehJB2MlHeZ1L6IHwzhQfcYiVQVvECS+ltIbO7LyrN4=; b=snN1f77mE9o/KSU2t2SSKIFj1Gnhnu/YqzhU4rJRL37mDr6XCy/e6+7b6x0vSiQHb+ MyEEPWuQy28UTj5fZHR6c3ZRUrSjVPXWs7Qsjs8w8z1gPyO8FiEN9gCzwuGXdq+5bz+6 Wx49mARv2ReL/hH/YppZXL4o2EvjAHQ43nQvEzfBHfIWhqAwXSsH6xU6g5kJoYNUvbkf Wa9JDdAr+35tjzWDPD3pSB/AZi+BDSH7XrEGQVwBlxlmj3NsDsR7XqKASQXj7isQi/JM y4fp/vZs8wv0CkjWoSZsdhn/NTLnBzlPMCsfAtvS/s8c3ZV8Hv+ugMJDVbSkiNxQ3SWN 8UZw== X-Gm-Message-State: AOJu0YyPoNxFbUswiIiFQkZ6elwi2t/3cKxYBCDpRBfGbIR7d+UJnTp/ gDhXt8/AL3RPHf/a5x2yYjylHzOcjGD010bIbSqW56mrOBWk8j9wNVBfouofBk/5YUiYJQOc2Bb zOaMtRILsxtxzZnKpyMpmMySe2UZQV9jUHuPLUqmLmEYBYdbSuEAt65tVkg8UsSVrB+bjXWe5c8 wywMl0rA== X-Received: by 2002:a05:620a:44c8:b0:774:16fc:65e0 with SMTP id y8-20020a05620a44c800b0077416fc65e0mr6040472qkp.12.1696515581654; Thu, 05 Oct 2023 07:19:41 -0700 (PDT) X-Google-Smtp-Source: AGHT+IESIw8PY+1i2ay57lgPtdbDkbQAfkVPL0roYgNioMZDJNRa/dKQPP0B8mgf4WXoFTyVy9rCrw== X-Received: by 2002:a05:620a:44c8:b0:774:16fc:65e0 with SMTP id y8-20020a05620a44c800b0077416fc65e0mr6040444qkp.12.1696515581162; Thu, 05 Oct 2023 07:19:41 -0700 (PDT) Received: from localhost ([31.111.84.209]) by smtp.gmail.com with ESMTPSA id os7-20020a05620a810700b00767e2668536sm516714qkn.17.2023.10.05.07.19.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 Oct 2023 07:19:40 -0700 (PDT) From: Andrew Burgess To: Jan Vrany via Gdb-patches , gdb-patches@sourceware.org Cc: Jan Vrany Subject: Re: [PATCH v2 2/2] gdb/python: implement support for sending custom MI async notifications In-Reply-To: <20230913143843.185997-3-jan.vrany@labware.com> References: <20230913143843.185997-1-jan.vrany@labware.com> <20230913143843.185997-3-jan.vrany@labware.com> Date: Thu, 05 Oct 2023 15:19:39 +0100 Message-ID: <87bkddtamc.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-10.9 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,RCVD_IN_SORBS_WEB,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 Vrany via Gdb-patches 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. Thanks for working on this. I have a couple of minor formatting nits, but also a couple of small changes that I think are needed. But this looks super close now. See the details inline. > --- > gdb/NEWS | 3 + > gdb/doc/python.texi | 45 ++++++++++++++ > gdb/python/py-mi.c | 71 +++++++++++++++++++++++ > gdb/python/python-internal.h | 5 ++ > gdb/python/python.c | 4 ++ > gdb/testsuite/gdb.python/py-mi-notify.exp | 71 +++++++++++++++++++++++ > 6 files changed, 199 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 5b13958aeaf..7764101e521 100644 > --- a/gdb/doc/python.texi > +++ b/gdb/doc/python.texi > @@ -211,6 +211,7 @@ optional arguments while skipping others. Example: > * Recordings In Python:: Accessing recordings from Python. > * CLI Commands In Python:: Implementing new CLI commands in Python. > * GDB/MI Commands In Python:: Implementing new @sc{gdb/mi} commands in Python. > +* GDB/MI Notifications In Python:: Implementing new @sc{gdb/mi} notifications in Python. > * Parameters In Python:: Adding new @value{GDBN} parameters. > * Functions In Python:: Writing new convenience functions. > * Progspaces In Python:: Program spaces. > @@ -4706,6 +4707,50 @@ 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. Use the @code{gdb.notify_mi} function to do that. > + > +@defun gdb.notify_mi (name @r{[}, data@r{]}) > +Emit a @sc{gdb/mi} asynchronous notification. @var{name} is the name of the > +notification, consisting of alphanumeric characters and a hyphen (@code{-}). > +@var{data} is any additional data to be emitted with the notification, passed > +as a Python dictionary. This argument is optional. The dictionary is converted > +to a @sc{gdb/mi} @var{result} records (@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 > + > +While using existing notification names (@pxref{GDB/MI Async Records}) with > +@code{gdb.notify_mi} is allowed, users are encouraged to prefix user-defined > +notification with a hyphen (@code{-}) to avoid possible conflict. @value{GDBN} > +will never introduce notification starting with hyphen. > + > +Here is how to emit @code{=-connection-removed} whenever a connection to remote > +GDB server is closed (@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 36bcb6ceece..77479e31f30 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. */ > @@ -455,3 +461,68 @@ serialize_mi_results (PyObject *results) > serialize_mi_result_1 (value, key_string.get ()); > } > } > + > +/* See python-internal.h. */ > + > +PyObject * > +gdbpy_notify_mi (PyObject *self, PyObject *args, PyObject *kwargs) > +{ > + static const char *keywords[] = { "name", "data", nullptr }; > + char *name = nullptr; > + PyObject *data = Py_None; > + > + if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "s|O", keywords, > + &name, &data)) > + return nullptr; > + > + /* Validate notification name */ /* Validate notification name. */ > + const int name_len = strlen (name); > + if (name_len == 0) > + { > + PyErr_SetString (PyExc_ValueError, _("MI notification name is empty.")); > + return nullptr; > + } > + for (int i = 0; i < name_len; i++) > + { > + if (!isalnum (name[i]) && name[i] != '-') > + { > + PyErr_Format (PyExc_ValueError, > + _("MI notification name contains invalid character: %c."), > + name[i]); The line containing the string is too long (82 characters), if you reformat like this: PyErr_Format (PyExc_ValueError, _("MI notification name contains invalid character: %c."), name[i]); Then you should be good. > + return nullptr; > + } > + } > + > + /* Validate additional data */ /* Validate additional data. */ > + if (! (data == Py_None || PyDict_Check (data))) No space after '!'. > + { > + PyErr_SetString (PyExc_ValueError, > + _("MI notification data must be either None or a dictionary")); Again, the error string line is too long. Also, I'd be tempted to include the incorrect type name, like this: PyErr_Format (PyExc_ValueError, _("MI notification data must be either None or a dictionary, not %s"), Py_TYPE (data)->tp_name); > + return nullptr; > + } > + > + SWITCH_THRU_ALL_UIS () > + { > + struct mi_interp *mi = as_mi_interp (top_level_interpreter ()); > + > + if (mi == nullptr) > + continue; > + > + gdb_printf (mi->event_channel, "%s", name); > + if (data != Py_None) > + { > + target_terminal::scoped_restore_terminal_state term_state; > + target_terminal::ours_for_output (); I was surprised to see these here, when we've already written out some output. Should these not appear earlier, after the 'mi == nullptr' check? > + > + 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_results (data); > + } > + gdb_flush (mi->event_channel); > + } > + > + Py_RETURN_NONE; > +} > diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h > index 785dc4a5743..c7ac6817bd3 100644 > --- a/gdb/python/python-internal.h > +++ b/gdb/python/python-internal.h > @@ -499,6 +499,11 @@ extern PyObject *gdbpy_execute_mi_command (PyObject *self, PyObject *args, > > extern void serialize_mi_results (PyObject *results); > > +/* Implementation of the gdb.notify_mi function. */ > + > +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..7c99f7c93b5 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-mi-notify.exp > @@ -0,0 +1,71 @@ > +# Copyright (C) 2023-2023 Free Software Foundation, Inc. Just '2023' here. > +# 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 . > + > +# Test custom MI notifications implemented in Python. You should add a test for the error cases: - MI notification name is empty. - MI notification name contains invalid character: %c. - MI notification data must be either None or a dictionary I know for the last two you can't test _every_ possible wrong input, but you can send at least one, just to check that the error works. Thanks, Andrew > + > +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')" \ > + ".*=-test-notification\r\n\\^done" \ > + "python notification, no additional data parameter" > + > +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" > + > +mi_gdb_test "python gdb.notify_mi('', 1)" \ > + ".*\\^error,msg=\".*\"" \ > + "python notification, empty notification name" > + > +mi_gdb_test "python gdb.notify_mi('**invalid**', 1)" \ > + ".*\\^error,msg=\".*\"" \ > + "python notification, invalid notification name" > + > +mi_gdb_test "python gdb.notify_mi(\[1,2,3\], 1)" \ > + ".*\\^error,msg=\".*\"" \ > + "python notification, non-string notification name" > + > +mi_gdb_test "python gdb.notify_mi()" \ > + ".*\\^error,msg=\".*\"" \ > + "python notification, no arguments passed" > + > +mi_gdb_test "python gdb.notify_mi('thread-group-added', \{'id' : 'i2'\})" \ > + ".*=thread-group-added,id=\"i2\"\r\n\\^done" \ > + "python notification, using existing internal notification name" > -- > 2.40.1