From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 71ED53858D39 for ; Thu, 21 Oct 2021 02:00:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 71ED53858D39 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 19L20ck2014154 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 20 Oct 2021 22:00:42 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 19L20ck2014154 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id A10BF1E813; Wed, 20 Oct 2021 22:00:37 -0400 (EDT) Message-ID: <2e4bfc98-842e-2035-2cf3-26c8fddd15dd@polymtl.ca> Date: Wed, 20 Oct 2021 22:00:37 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCHv3 1/3] gdb/python: introduce gdb.TargetConnection object type Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org References: From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 21 Oct 2021 02:00:38 +0000 X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Oct 2021 02:00:46 -0000 On 2021-10-19 06:17, Andrew Burgess wrote: > This commit adds a new object type gdb.TargetConnection. This new > type represents a connection within GDB (a connection as displayed by > 'info connections'). > > There's three ways to find a gdb.TargetConnection, there's a new > 'gdb.connections()' function, which returns a list of all currently > active connections. > > Or you can read the new 'connection' property on the gdb.Inferior > object type, this contains the connection for that inferior (or None > if the inferior has no connection, for example, it is exited). > > Finally, there's a new gdb.events.connection_removed event registry, > this emits a new gdb.ConnectionEvent whenever a connection is removed > from GDB (this happens when all inferiors using a connection exit). > The gdb.ConnectionEvent has a 'connection' property, which is the > gdb.TargetConnection being removed from GDB. > > The gdb.TargetConnection has a 'is_valid()' method. A connection > object becomes invalid when the underlying connection is removed from > GDB (all inferiors using the connection exit). > > The gdb.TargetConnection has the following read-only properties: > > 'num': The number for this connection, > > 'type': e.g. 'native', 'remove', 'sim', etc > > 'description': The longer description as seen in the 'info > connections' command output. > > 'details': A string or None. Extra details for the connection, for > example, a remote connection's details might be > 'hostname:port'. > --- > gdb/Makefile.in | 1 + > gdb/NEWS | 16 + > gdb/doc/python.texi | 92 ++++- > gdb/observable.c | 1 + > gdb/observable.h | 3 + > gdb/python/py-all-events.def | 1 + > gdb/python/py-connection.c | 366 ++++++++++++++++++ > gdb/python/py-event-types.def | 5 + > gdb/python/py-inferior.c | 16 + > gdb/python/python-internal.h | 6 + > gdb/python/python.c | 5 + > gdb/target-connection.c | 4 + > .../gdb.multi/multi-target-info-inferiors.exp | 38 ++ > .../gdb.multi/multi-target-info-inferiors.py | 63 +++ > gdb/testsuite/gdb.python/py-connection.c | 22 ++ > gdb/testsuite/gdb.python/py-connection.exp | 63 +++ > gdb/testsuite/gdb.python/py-inferior.exp | 20 +- > 17 files changed, 718 insertions(+), 4 deletions(-) > create mode 100644 gdb/python/py-connection.c > create mode 100644 gdb/testsuite/gdb.multi/multi-target-info-inferiors.py > create mode 100644 gdb/testsuite/gdb.python/py-connection.c > create mode 100644 gdb/testsuite/gdb.python/py-connection.exp > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index 4201f65e68d..5ec58aba439 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -392,6 +392,7 @@ SUBDIR_PYTHON_SRCS = \ > python/py-breakpoint.c \ > python/py-cmd.c \ > python/py-continueevent.c \ > + python/py-connection.c \ This is not in alphabetical order :). > python/py-event.c \ > python/py-evtregistry.c \ > python/py-evts.c \ > diff --git a/gdb/NEWS b/gdb/NEWS > index bd26d2b1ec2..43b81df526d 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -45,6 +45,22 @@ maint show internal-warning backtrace > event is triggered once GDB decides it is going to exit, but > 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). > + > + ** The gdb.Inferior type now has a 'connection' property which is an > + instance of gdb.TargetConnection, the connection used by this > + inferior. This can be None if the inferior has no connection > + (for example, when exited). "when exited" is not completely accurate (see below, I wrote that other comment before). > +A @code{gdb.TargetConnection} has the following method: > + > +@defun TargetConnection.is_valid () > +Return @code{True} if the @code{gdb.TargetConnection} object is valid, > +@code{False} if not. A @code{gdb.TargetConnection} will become > +invalid if the connection no longer exists within @value{GDBN}, this > +will happen when all the inferiors using that connection exit. I don't think the last part is completely accurate. In some cases, an inferior will keep its process target (and therefore keep the connection alive) after exiting. For instance, if you push the native target explicitly, or if you connect using the extended-remote protocol. You could say "this will happen when no inferior use the connection" or something along those lines. > +gdbpy_ref<> > +target_to_connection_object (process_stratum_target *target) > +{ > + if (target == nullptr) > + return gdbpy_ref<>::new_reference (Py_None); > + > + gdbpy_ref conn_obj = all_connection_objects[target]; > + if (conn_obj == nullptr) > + { > + conn_obj.reset (PyObject_New (connection_object, > + &connection_object_type)); > + if (conn_obj == nullptr) > + return nullptr; > + > + conn_obj->target = target; > + > + /* PyObject_New initializes the new object with a refcount of 1. This > + counts for the reference we are keeping in the target_ops data. */ > + all_connection_objects[target] = conn_obj; I find this comment more confusing than helpful. The strong reference created by PyObject_New belongs to conn_obj, which is then returned by this function. The strong reference belonging to the map (what I presume you mean by "target_ops data") is created when copying conn_obj into the map. In the end all references are the same, I just find it a bit confusing. > +/* Callback for the connection_removed observer. */ > + > +static void > +connpy_connection_removed (process_stratum_target *target) > +{ > + if (!gdb_python_initialized) > + return; > + > + gdbpy_enter enter_py (get_current_arch (), current_language); > + > + if (!evregpy_no_listeners_p (gdb_py_events.connection_removed)) > + if (emit_connection_event (target, gdb_py_events.connection_removed) < 0) > + gdbpy_print_stack (); > + > + gdbpy_ref conn_obj = all_connection_objects[target]; > + if (conn_obj != nullptr) > + { > + conn_obj->target = nullptr; > + all_connection_objects.erase (target); > + } Watch out with using operator[], that will insert an item for `target` with value nullptr in the map, if there isn't one already. Maybe it doesn't matter in the end, but it brings the situation where a target without a corresponding Python object can exist in two different states: no entry in the map, and an entry with value nullptr. I think it would be easier to understand / less error prone if all entries in the map had non-nullptr values, and targets without corresponding Python object had no entries in the map. > diff --git a/gdb/testsuite/gdb.python/py-connection.exp b/gdb/testsuite/gdb.python/py-connection.exp > new file mode 100644 > index 00000000000..b994cec01eb > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-connection.exp > @@ -0,0 +1,63 @@ > +# 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 . > + > +# This file is for testing the gdb.TargetConnection API. This API is > +# already tested in gdb.multi/multi-target-info-inferiors.exp and > +# gdb.python/py-inferior.exp, this file just covers some edge cases > +# that are not tested in other places. > + > +load_lib gdb-python.exp > + > +standard_testfile > + > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } { > + return -1 > +} > + > +# Skip all tests if Python scripting is not enabled. > +if { [skip_python_tests] } { continue } > + > +if ![runto_main] then { > + fail "can't run to main" > + return 0 > +} Since 4dfef5be6812 ("gdb/testsuite: make runto_main not pass no-message to runto"), you don't have to make an explicit fail when runto_main fails. > + > +# Create a gdb.TargetConnection object and check it becomes invalid > +# once the connection has gone away. > +gdb_test_no_output "python conn = gdb.selected_inferior().connection" > +gdb_test "python print(conn)" \ > + "" \ > + "print gdb.TargetConnection while it is still valid" > +gdb_test "python print(conn.is_valid())" "True" "is_valid returns True" > +gdb_test "info connections" "\r\n\\* 1 .*" \ > + "info connections while the connection is still around" > +gdb_test "kill" "" "kill the inferior" \ > + "Kill the program being debugged.*y or n. $" "y" > +gdb_test "info connections" "No connections\\." \ > + "info connections now all the connections have gone" The above will fail with the native-extended-gdbserver board, as the connection will not be gone. You could maybe use "disconnect", which should pop the process target, whatever it is. Simon