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 DA3A33858400 for ; Thu, 22 Sep 2022 17:24:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DA3A33858400 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_128_GCM_SHA256) id us-mta-125-GDGda2wXN5qMn6HQjsMrzg-1; Thu, 22 Sep 2022 13:24:11 -0400 X-MC-Unique: GDGda2wXN5qMn6HQjsMrzg-1 Received: by mail-wm1-f71.google.com with SMTP id n7-20020a1c2707000000b003a638356355so4357805wmn.2 for ; Thu, 22 Sep 2022 10:24:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date; bh=Bwlo13XNiEZNzwAW/pFFndK39z0N5To/6R6Yg2nve+8=; b=v5wjWJqgU+UfYFGnp3mjFcvTaQI9KM1Mn4jpDfYYAhp404O1hCTl+t9OQ0AdLm9rcU EgHUM6wQwWZWE08I3v4hTDsig82OyRRTvKFeELTsA/9LOXwNiMdwF7JswVO+DaZAQI5l 1T2y+pXFAEYVzm+iO80eF1x4qSHPNBGgl8q/x31DDR4bAoQWb4fMfEU1W83U2HXyiB7Z 7ZPsbSZzJqM+TbzaaKKtX/KhSj6kXy8NQeDD2E89po+sGK7ICy6A6zBQci0thOGlvvES tkmAJlkcg0EtWFQ1TwG8O11WyFEr+y72U+G+BGx7K5EqZnh6JpLUM/9cl9cGZzwbeQnN NYTA== X-Gm-Message-State: ACrzQf1nmn9mfjnrQxTeiXmTUm9UHzMPgwC8Id5E0pEQw6/IJHkeK3pJ dDLmzC3qE/WjLmHmOHpSyED/hkoAk9XiybdpWumlqDMwM1Xgem6mxpQb4w39Gu0u9XPtMzIa6VQ IlPM6lF50estInegpFJ3XXw== X-Received: by 2002:a05:600c:2d05:b0:3b4:7788:9944 with SMTP id x5-20020a05600c2d0500b003b477889944mr10435874wmf.57.1663867449992; Thu, 22 Sep 2022 10:24:09 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6hcrAR9avs4uiuea3n+IHAY+ImcK/Eo4nLC7W/reI/P4YX6FJdrqJQ53Th3gLd6PNFkvkcuw== X-Received: by 2002:a05:600c:2d05:b0:3b4:7788:9944 with SMTP id x5-20020a05600c2d0500b003b477889944mr10435858wmf.57.1663867449746; Thu, 22 Sep 2022 10:24:09 -0700 (PDT) Received: from localhost (92.40.178.45.threembb.co.uk. [92.40.178.45]) by smtp.gmail.com with ESMTPSA id m12-20020a5d56cc000000b00228cd9f6349sm5294152wrw.106.2022.09.22.10.24.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 22 Sep 2022 10:24:09 -0700 (PDT) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: fix target_ops reference count for some cases In-Reply-To: <9a31d60a-01a8-e527-71d9-0f5908be2961@simark.ca> References: <20220921131200.3983844-1-aburgess@redhat.com> <9a31d60a-01a8-e527-71d9-0f5908be2961@simark.ca> Date: Thu, 22 Sep 2022 18:24:07 +0100 Message-ID: <87a66re30o.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=-12.6 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_LOW, 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 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 17:24:14 -0000 Simon Marchi writes: >> 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. I suspect the comment you quote is just out of date. switch_to_program_space_and_thread can end up calling switch_to_inferior_no_thread if there are no running threads in the program space being switched too. But, even if switch_to_thread does end up being called we: - set the program space, - set the inferior, - set the current thread, - reinit the frame cache, By comparison, switch_to_inferior_no_thread does: - sets the program space, - sets the inferior, - sets the current thread (to nullptr this time though), - reinits the frame cache, As you can see they do the same set of things, all of which I think should be reverted once we leave the scope, hence scoped_restore_current_pspace_and_thread seems like the way to go. > >> 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. OK. Thanks, Andrew > > Simon