[PATCHv3 3/3] gdb/python: add gdb.RemoteTargetConnection.send_packet
Andrew Burgess
andrew.burgess@embecosm.com
Fri Oct 22 11:08:25 GMT 2021
Thanks for the review.
I've posted a new version of the patch, but I wanted to reply to one
comment specifically, which I've done inline below.
* Simon Marchi <simon.marchi@polymtl.ca> [2021-10-20 22:43:01 -0400]:
>
>
> On 2021-10-19 06:17, Andrew Burgess wrote:
> > This commits adds a new sub-class of gdb.TargetConnection,
> > gdb.RemoteTargetConnection. This sub-class is created for all
> > 'remote' and 'extended-remote' targets.
> >
> > This new sub-class has one additional method over its base class,
> > 'send_packet'. This new method is equivalent to the 'maint
> > packet' CLI command, it allows a custom packet to be sent to a remote
> > target.
> >
> > The result of calling RemoteTargetConnection.send_packet is a string
> > containing the reply that came from the remote.
> > ---
> > gdb/NEWS | 8 +-
> > gdb/doc/gdb.texinfo | 1 +
> > gdb/doc/python.texi | 28 +++-
> > gdb/python/py-connection.c | 163 +++++++++++++++++++-
> > gdb/testsuite/gdb.python/py-send-packet.c | 22 +++
> > gdb/testsuite/gdb.python/py-send-packet.exp | 59 +++++++
> > gdb/testsuite/gdb.python/py-send-packet.py | 84 ++++++++++
> > 7 files changed, 358 insertions(+), 7 deletions(-)
> > create mode 100644 gdb/testsuite/gdb.python/py-send-packet.c
> > create mode 100644 gdb/testsuite/gdb.python/py-send-packet.exp
> > create mode 100644 gdb/testsuite/gdb.python/py-send-packet.py
> >
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index 43b81df526d..78ac8beb7fd 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -46,7 +46,9 @@ maint show internal-warning backtrace
> > before GDB starts to clean up its internal state.
> >
> > ** New gdb.TargetConnection object type that represents a connection
> > - (as displayed by the 'info connections' command).
> > + (as displayed by the 'info connections' command). A sub-class,
> > + gdb.RemoteTargetConnection, is used to represent 'remote' and
> > + 'extended-remote' connections.
> >
> > ** The gdb.Inferior type now has a 'connection' property which is an
> > instance of gdb.TargetConnection, the connection used by this
> > @@ -61,6 +63,10 @@ maint show internal-warning backtrace
> > ** New gdb.connections() function that returns a list of all
> > currently active connections.
> >
> > + ** New gdb.RemoteTargetConnection.send_packet(STRING) method. This
> > + is equivalent to the existing 'maint packet' CLI command; it
> > + allows a user specified packet to be sent to the remote target.
> > +
> > *** Changes in GDB 11
> >
> > * The 'set disassembler-options' command now supports specifying options
> > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> > index 631a7c03b31..89cf86210ab 100644
> > --- a/gdb/doc/gdb.texinfo
> > +++ b/gdb/doc/gdb.texinfo
> > @@ -39269,6 +39269,7 @@
> > error stream. This is @samp{on} by default for @code{internal-error}
> > and @samp{off} by default for @code{internal-warning}.
> >
> > +@anchor{maint packet}
> > @kindex maint packet
> > @item maint packet @var{text}
> > If @value{GDBN} is talking to an inferior via the serial protocol,
> > diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> > index 599c411b11d..97c1dd595bf 100644
> > --- a/gdb/doc/python.texi
> > +++ b/gdb/doc/python.texi
> > @@ -5980,9 +5980,9 @@
> > Examples of different connection types are @samp{native} and
> > @samp{remote}. @xref{Inferiors Connections and Programs}.
> >
> > -@value{GDBN} uses the @code{gdb.TargetConnection} object type to
> > -represent a connection in Python code. To get a list of all
> > -connections use @code{gdb.connections}
> > +Connections in @value{GDBN} are represented as instances of
> > +@code{gdb.TargetConnection}, or as one of its sub-classes. To get a
> > +list of all connections use @code{gdb.connections}
> > (@pxref{gdbpy_connections,,gdb.connections}).
> >
> > To get the connection for a single @code{gdb.Inferior} read its
> > @@ -6034,6 +6034,28 @@
> > to the remote target.
> > @end defvar
> >
> > +The @code{gdb.RemoteTargetConnection} class is a sub-class of
> > +@code{gdb.TargetConnection}, and is used to represent @samp{remote}
> > +and @samp{extended-remote} connections. In addition to the attributes
> > +and methods available from the @code{gdb.TargetConnection} base class,
> > +a @code{gdb.RemoteTargetConnection} has the following additional method:
> > +
> > +@kindex maint packet
> > +@defun RemoteTargetConnection.send_packet (@var{packet})
> > +This method sends @var{packet}, which should be a non-empty string, to
> > +the remote target and returns the response as a string. If
> > +@var{packet} is not a string, or is the empty string, then an
> > +exception of type @code{gdb.error} is thrown.
>
> I think that the standard TypeError and ValueError would be appropriate
> here.
>
> Can the remote return some bytes that can't be decoded as a string (like
> the reply of the X packet)? In that case, maybe the return value of
> send_packet should be `bytes` (at least for Python 3).
>
>
> > @@ -80,8 +85,15 @@ target_to_connection_object (process_stratum_target *target)
> > gdbpy_ref <connection_object> conn_obj = all_connection_objects[target];
> > if (conn_obj == nullptr)
> > {
> > - conn_obj.reset (PyObject_New (connection_object,
> > - &connection_object_type));
> > + PyTypeObject *type;
> > +
> > + std::string connection_type = target->shortname ();
> > + if (connection_type == "remote" || connection_type == "extended-remote")
> > + type = &remote_connection_object_type;
> > + else
> > + type = &connection_object_type;
>
> I imagined that we could use a dynamic cast here, same as we do in
> get_current_remote_target:
>
> dynamic_cast<remote_target *> (target)
This is what I originally wanted to do. The problem is that this
requires that the remote_target type be visible inside
py-connection.c, and currently remote_target is defined in remote.c.
We could move the declaration of remote_target, but in the end I
figured that the string comparison was just as good, and I didn't see
the names changing - and even if they did, we can fix py-connections.c
with the new names, and this wouldn't break the external API.
Thanks,
Andrew
>
> But, comparing the strings works too, it's not like these names are
> going to change.
>
> > +/* Implement RemoteTargetConnection.send_packet function. Send a packet to
> > + the target identified by SELF. The connection must still be valid, and
> > + the packet to be sent must be non-empty, otherwise an exception will be
> > + thrown. */
> > +
> > +static PyObject *
> > +connpy_send_packet (PyObject *self, PyObject *args, PyObject *kw)
> > +{
> > + connection_object *conn = (connection_object *) self;
> > +
> > + CONNPY_REQUIRE_VALID (conn);
> > +
> > + static const char *keywords[] = {"packet", nullptr};
> > + const char *packet_str = nullptr;
> > +
> > + if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s", keywords,
> > + &packet_str))
> > + return nullptr;
> > +
> > + if (packet_str == nullptr || *packet_str == '\0')
> > + {
> > + PyErr_SetString (PyExc_ValueError, _("Invalid remote packet"));
> > + return nullptr;
> > + }
> > +
> > + try
> > + {
> > + scoped_restore_current_thread restore_thread;
> > + switch_to_target_no_thread (conn->target);
> > +
> > + py_send_packet_callbacks callbacks;
> > + send_remote_packet (packet_str, &callbacks);
> > + return callbacks.result ().release ();
>
> Looking at py_send_packet_callbacks::received, it looks like m_result
> can stay nullptr. So here, we would return nullptr, which indicates to
> the Python interpreter that we are throwing an exception. But are we
> sure the error indicator has been set? If PyUnicode_Decode fails, I
> suppose it will. But if m_result stays nullptr because buf is empty, I
> guess we'll return nullptr without setting the error indicator.
>
> > diff --git a/gdb/testsuite/gdb.python/py-send-packet.c b/gdb/testsuite/gdb.python/py-send-packet.c
> > new file mode 100644
> > index 00000000000..9811b15f06d
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-send-packet.c
> > @@ -0,0 +1,22 @@
> > +/* This testcase is part of GDB, the GNU debugger.
> > +
> > + Copyright 2021 Free Software Foundation, Inc.
> > +
> > + 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/>. */
> > +
> > +int
> > +main ()
>
> main (void)
>
> > +{
> > + return 0;
> > +}
> > diff --git a/gdb/testsuite/gdb.python/py-send-packet.exp b/gdb/testsuite/gdb.python/py-send-packet.exp
> > new file mode 100644
> > index 00000000000..89ab28d003b
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.python/py-send-packet.exp
> > @@ -0,0 +1,59 @@
> > +# Copyright (C) 2021 Free Software Foundation, Inc.
> > +
> > +# 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 the gdb.RemoteTargetConnection.send_packet API. This is done
> > +# by connecting to a remote target and fetching the thread list in two
> > +# ways, first, we manually send the packets required to read the
> > +# thread list using gdb.TargetConnection.send_packet, then we compare
> > +# the results to the thread list using the standard API calls.
> > +
> > +load_lib gdb-python.exp
> > +load_lib gdbserver-support.exp
> > +
> > +standard_testfile
> > +
> > +if {[skip_gdbserver_tests]} {
> > + return 0
> > +}
> > +
> > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> > + return -1
> > +}
> > +
> > +gdb_exit
> > +gdb_start
>
> You could avoid an unnecessary starting and exiting GDB by using
> build_executable instead of prepare_for_testing.
>
> Simon
More information about the Gdb-patches
mailing list