From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 86EAF38582B5 for ; Thu, 22 Sep 2022 15:00:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 86EAF38582B5 Received: from [10.0.0.11] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 07CCC1E0D3; Thu, 22 Sep 2022 11:00:23 -0400 (EDT) Message-ID: <9a31d60a-01a8-e527-71d9-0f5908be2961@simark.ca> Date: Thu, 22 Sep 2022 11:00:23 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [PATCH] gdb: fix target_ops reference count for some cases Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org References: <20220921131200.3983844-1-aburgess@redhat.com> From: Simon Marchi In-Reply-To: <20220921131200.3983844-1-aburgess@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, 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 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, 22 Sep 2022 15:01:33 -0000 > diff --git a/gdb/inferior.c b/gdb/inferior.c > index 7eb2bd97907..0c0b4b78ff5 100644 > --- a/gdb/inferior.c > +++ b/gdb/inferior.c > @@ -70,6 +70,16 @@ inferior::~inferior () > { > inferior *inf = this; > > + /* Before the inferior is deleted, all target_ops should be popped from > + the target stack, this leaves just the dummy_target behind. If this > + is not done, then any target left in the target stack will be left > + with an artificially high reference count. As the dummy_target is > + still on the target stack then we are about to loose a reference to loose -> lose? > @@ -191,6 +201,26 @@ delete_inferior (struct inferior *inf) > > gdb::observers::inferior_removed.notify (inf); > > + { > + /* Limit the change of inferior to an inner scope so that the original > + inferior and program space will have been restored by the time that > + we delete the inferior INF and (possibly) its related program two spaces between related and program. > + space. */ > + scoped_restore_current_pspace_and_thread restore_pspace_and_thread; Just wondering, why do we need to restore explicitly the current pspace, instead of using just scoped_restore_current_thread? scoped_restore_current_pspace_and_thread's doc says: /* Save/restore the current program space, thread, inferior and frame. Use this when you need to call switch_to_program_space_and_thread. */ ... but you are not using switch_to_program_space_and_thread here. Maybe it's ok and I just don't understand. Same in ~scoped_mock_context. > diff --git a/gdb/testsuite/gdb.python/py-connection-removed.exp b/gdb/testsuite/gdb.python/py-connection-removed.exp > new file mode 100644 > index 00000000000..1b139cedc0d > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-connection-removed.exp > @@ -0,0 +1,92 @@ > +# Copyright (C) 2022 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 . > + > +# Check that the gdb.connect_removed event triggers when we expect it > +# too. too -> to > +# > +# Checking this event has wider implications that simply some corner simply -> imply? Or I don't get what you mean. > +# of the Python API working or not. The connection_removed event > +# triggers when the reference count of a process_stratum_target > +# reaches zero. If these events stop triggering when expected then > +# GDB might be getting the reference counting on target_ops objects > +# wrong. > + > +load_lib gdb-python.exp > + > +standard_testfile py-connection.c > + > +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 { > + return 0 > +} > + > +# Register a callback that will trigger when a connection is removed > +# (deleted) within GDB. > +gdb_test_multiline "Add connection_removed event" \ > + "python" "" \ > + "def connection_removed_handler(event):" "" \ > + " num = event.connection.num" "" \ > + " type = event.connection.type" "" \ > + " print(f'Connection {num} ({type}) removed')" "" \ I think unfortunately need to support Python versions that don't know about f-strings. Simon