From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.efficios.com (mail.efficios.com [167.114.26.124]) by sourceware.org (Postfix) with ESMTPS id A35D53850410 for ; Thu, 30 Jul 2020 15:28:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A35D53850410 Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id 1FA712CC510; Thu, 30 Jul 2020 11:28:02 -0400 (EDT) Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id J4raNvShnGxg; Thu, 30 Jul 2020 11:28:01 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by mail.efficios.com (Postfix) with ESMTP id CB8352CC50F; Thu, 30 Jul 2020 11:28:01 -0400 (EDT) DKIM-Filter: OpenDKIM Filter v2.10.3 mail.efficios.com CB8352CC50F X-Virus-Scanned: amavisd-new at efficios.com Received: from mail.efficios.com ([127.0.0.1]) by localhost (mail03.efficios.com [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id BHyvmQfUFJSg; Thu, 30 Jul 2020 11:28:01 -0400 (EDT) Received: from [10.0.0.11] (173-246-6-90.qc.cable.ebox.net [173.246.6.90]) by mail.efficios.com (Postfix) with ESMTPSA id 996082CC731; Thu, 30 Jul 2020 11:28:01 -0400 (EDT) Subject: Re: [PATCH 3/4] gdb: pass target to thread_ptid_changed observable To: Pedro Alves , gdb-patches@sourceware.org Cc: Laurent References: <20200720204101.2849535-1-simon.marchi@efficios.com> <20200720204101.2849535-4-simon.marchi@efficios.com> From: Simon Marchi Message-ID: <6bd06f25-c89b-58b5-3639-94813a7284f5@efficios.com> Date: Thu, 30 Jul 2020 11:27:54 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: fr Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 30 Jul 2020 15:28:04 -0000 On 2020-07-23 4:42 p.m., Pedro Alves wrote: >> @@ -1817,6 +1818,58 @@ cooked_write_test (struct gdbarch *gdbarch) >> } >> } >> >> +/* Verify that when two threads with the same ptid exist (from two different >> + targets) and one of them changes ptid, we only update the appropriate >> + regcaches. */ >> + >> +static void >> +regcache_thread_ptid_changed () >> +{ >> + /* Any arch will do. */ >> + gdbarch *arch = current_inferior ()->gdbarch; >> + >> + /* Prepare two targets with one thread each, with the same ptid. */ >> + scoped_mock_context target1 (arch); >> + scoped_mock_context target2 (arch); >> + target2.mock_inferior.next = &target1.mock_inferior; >> + >> + ptid_t old_ptid (111, 222); >> + ptid_t new_ptid (111, 333); >> + >> + target1.mock_inferior.pid = old_ptid.pid (); >> + target1.mock_thread.ptid = old_ptid; >> + target2.mock_inferior.pid = old_ptid.pid (); >> + target2.mock_thread.ptid = old_ptid; >> + >> + gdb_assert (regcaches.empty ()); > > This will fail if you debug something, e.g., run to main, > and then do: > > (gdb) maint selftest regcache_thread_ptid_changed I don't know, do we really want to support this? I don't really see the point. We can design the selftests to make it possible, but is there any advantage? In the selftests command, we could ensure that you are not debugging something, and otherwise error out with "You can't run selftests while debugging.". > Maybe call registers_changed() before ? > > Actually trying that made me notice that I completely missed > making cooked_write_test in 236ef0346d88efffd1ca1da1a5d80724cb145660 ... > Fixing that would get rid of these fails: > > Self test failed: arch i386: target already pushed > ... I can do this, but I'll wait for your reply on the suggestion above to disallow running selftests while debugging first. >> + >> + /* Populate the regcaches container. */ >> + get_thread_arch_aspace_regcache (&target1.mock_target, old_ptid, arch, NULL); >> + get_thread_arch_aspace_regcache (&target2.mock_target, old_ptid, arch, NULL); > > nullptr? ;-) Yes! I actually appreciate you pointing it out, if we chose one convention and follow it, we don't have to always wonder which one to use! >> + >> + /* Return whether a regcache for (TARGET, PTID) exists in REGCACHES. */ >> + auto regcache_exists = [] (process_stratum_target *target, ptid_t ptid) >> + { >> + for (regcache *rc : regcaches) >> + { >> + if (rc->target () == target && rc->ptid () == ptid) >> + return true; >> + } >> + >> + return false; >> + }; >> + >> + gdb_assert (regcaches_size () == 2); >> + gdb_assert (regcache_exists (&target1.mock_target, old_ptid)); >> + gdb_assert (regcache_exists (&target2.mock_target, old_ptid)); > > For completeness, I'd add: > > gdb_assert (!regcache_exists (&target1.mock_target, new_ptid)); > gdb_assert (!regcache_exists (&target2.mock_target, new_ptid)); > >> + >> + thread_change_ptid (&target1.mock_target, old_ptid, new_ptid); >> + >> + gdb_assert (regcaches_size () == 2); >> + gdb_assert (regcache_exists (&target1.mock_target, new_ptid)); >> + gdb_assert (regcache_exists (&target2.mock_target, old_ptid)); > > Similarly here. I thought that with `gdb_assert (regcaches_size () == 2)`, but more asserts is not bad, it could catch some bug, we never know. I've added them. Simon