From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by sourceware.org (Postfix) with ESMTPS id 60FDF3857C5B for ; Thu, 23 Jul 2020 20:42:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 60FDF3857C5B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wr1-f67.google.com with SMTP id y3so6392967wrl.4 for ; Thu, 23 Jul 2020 13:42:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=x3H0339UK6PAKON1NAz/gXeX/j+bxEb4b2opvE3RNCQ=; b=XHcyTnyNhgvTwbF7TqGPcZ8REMUwddlOpZXfA0BXIRxkOCZNCNXjwvUypzklFx2SXK pxG80vB9O/dzdoCOeMPfUtCW0i6bLG1iXP2hYFzx+FqkQ1JOWeWEkwPSNAIr1kb74/yl HOWxgA9vs+QljZ2Xb7QJ8ZjC5lEYQoHfnAVx4x0y7wA1SDLI9H1tTlM9shoOo2B93B8S aJmI+7cNeVMmEv0wP8rbg5DMry8MT/vLxtBvdpnqFZViQeO1ozgZ1lN4mvlN2a9dxNpz XP4LxP9FeSKJeSAoRE51ke8wi2ZlHzOostsliBIjO8xhClCOx/9UFEM0TF/UpLU7h1v1 F8jA== X-Gm-Message-State: AOAM531k1M4b+aeqXEx4bQYlMx5YKpftTOzp5WslqD34ddrEjkHNSIJR 0A3fVsALJ8gJxsAubDP9vok= X-Google-Smtp-Source: ABdhPJztYbPzO4tQfUYJl/z9KwunKVMJmUKPxCkE2qqZHOYkEmWiuw0/PxMQKhEZQqGeIRwRt7hLwA== X-Received: by 2002:adf:eb05:: with SMTP id s5mr6162465wrn.0.1595536972333; Thu, 23 Jul 2020 13:42:52 -0700 (PDT) Received: from ?IPv6:2001:8a0:f91a:c400:56ee:75ff:fe8d:232b? ([2001:8a0:f91a:c400:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id r11sm4556737wmh.1.2020.07.23.13.42.50 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 23 Jul 2020 13:42:51 -0700 (PDT) Subject: Re: [PATCH 3/4] gdb: pass target to thread_ptid_changed observable To: Simon Marchi , gdb-patches@sourceware.org References: <20200720204101.2849535-1-simon.marchi@efficios.com> <20200720204101.2849535-4-simon.marchi@efficios.com> Cc: Laurent From: Pedro Alves Message-ID: Date: Thu, 23 Jul 2020 21:42:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20200720204101.2849535-4-simon.marchi@efficios.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.7 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, 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, 23 Jul 2020 20:42:55 -0000 On 7/20/20 9:41 PM, Simon Marchi via Gdb-patches wrote: > I noticed what I think is a potential bug. I did not observe it nor was > I able to reproduce it using actual debugging. It's quite unlikely, > because it involves multi-target and ptid clashes. I added selftests > that demonstrate it though. Yes, it's definitely a bug. Thanks for fixing this. > > The thread_ptid_changed observer says that thread with OLD_PTID now has > NEW_PTID. Now, if for some reason we happen to have two targets > defining a thread with OLD_PTID, the observers don't know which thread > this is about. > > regcache::regcache_thread_ptid_changed changes all regcaches with > OLD_PTID. If there is a regcache for a thread with ptid OLD_PTID, but > that belongs to a different target, this regcache will be erroneously > changed. > > Similarly, infrun_thread_ptid_changed updates inferior_ptid if > inferior_ptid matches OLD_PTID. But if inferior_ptid currently refers > not to the thread is being changed, but to a thread with the same ptid > belonging to a different target, then inferior_ptid will erroneously be > changed. I think the latter is unlikely to be a bug in practice, because inferior_ptid will be pointing at a thread of the current target, and thread_change_ptid will be called in the context of the same target. But it doesn't hurt to make it explicit, of course. > > This patch adds a `process_stratum_target *` parameter to the > `thread_ptid_changed` observable and makes the two observers use it. > Tests for both are added, which would fail if the corresponding fix > wasn't done. > > gdb/ChangeLog: > > * observable.h (thread_ptid_changed): Add parameter > `process_stratum_target *`. > * infrun.c (infrun_thread_ptid_changed): Add parameter > `process_stratum_target *` and use it. > (selftests): New namespace. > (infrun_thread_ptid_changed): New function. > (_initialize_infrun): Register selftest. > * regcache.c (regcache_thread_ptid_changed): Add parameter > `process_stratum_target *` and use it. > (regcache_thread_ptid_changed): New function. > (_initialize_regcache): Register selftest. > * thread.c (thread_change_ptid): Pass target to > thread_ptid_changed observable. > > Change-Id: I0599e61224b6d154a7b55088a894cb88298c3c71 > --- > gdb/infrun.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++-- > gdb/observable.h | 6 ++-- > gdb/regcache.c | 59 ++++++++++++++++++++++++++++++++++-- > gdb/thread.c | 2 +- > 4 files changed, 138 insertions(+), 7 deletions(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 31266109a6d3..3fbe45efb8ca 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -67,6 +67,9 @@ > #include "gdbsupport/gdb_select.h" > #include > #include "async-event.h" > +#include "gdbsupport/selftest.h" > +#include "scoped-mock-context.h" > +#include "test-target.h" > > /* Prototypes for local functions */ > > @@ -2068,9 +2071,11 @@ start_step_over (void) > /* Update global variables holding ptids to hold NEW_PTID if they were > holding OLD_PTID. */ > static void > -infrun_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid) > +infrun_thread_ptid_changed (process_stratum_target *target, > + ptid_t old_ptid, ptid_t new_ptid) > { > - if (inferior_ptid == old_ptid) > + if (inferior_ptid == old_ptid > + && current_inferior ()->process_target () == target) > inferior_ptid = new_ptid; > } > > @@ -9467,6 +9472,70 @@ infrun_async_inferior_event_handler (gdb_client_data data) > inferior_event_handler (INF_REG_EVENT); > } > > +namespace selftests > +{ > + > +/* Verify that when two threads with the same ptid exist (from two different > + targets) and one of them changes ptid, we only update inferior_ptid if > + it is appropriate. */ > + > +static void > +infrun_thread_ptid_changed () > +{ > + gdbarch *arch = current_inferior ()->gdbarch; > + > + /* The thread which inferior_ptid represents changes ptid. */ > + { > + scoped_restore_current_pspace_and_thread restore; > + > + 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; > + > + auto restore_inferior_ptid = make_scoped_restore (&inferior_ptid, old_ptid); > + set_current_inferior (&target1.mock_inferior); > + > + thread_change_ptid (&target1.mock_target, old_ptid, new_ptid); > + > + gdb_assert (inferior_ptid == new_ptid); > + } > + > + /* A thread with the same ptid as inferior_ptid, but from another target, > + changes ptid. */ > + { > + scoped_restore_current_pspace_and_thread restore; > + > + 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; > + > + auto restore_inferior_ptid = make_scoped_restore (&inferior_ptid, old_ptid); > + set_current_inferior (&target2.mock_inferior); > + > + thread_change_ptid (&target1.mock_target, old_ptid, new_ptid); > + > + gdb_assert (inferior_ptid == old_ptid); > + } > +} > + > +} /* namespace selftests */ > + > void _initialize_infrun (); > void > _initialize_infrun () > @@ -9768,4 +9837,9 @@ or signalled."), > show_observer_mode, > &setlist, > &showlist); > + > +#if GDB_SELF_TEST > + selftests::register_test ("infrun_thread_ptid_changed", > + selftests::infrun_thread_ptid_changed); > +#endif > } > diff --git a/gdb/observable.h b/gdb/observable.h > index 070cf0f18e51..da0a9b12f74c 100644 > --- a/gdb/observable.h > +++ b/gdb/observable.h > @@ -27,6 +27,7 @@ struct so_list; > struct objfile; > struct thread_info; > struct inferior; > +struct process_stratum_target; > struct trace_state_variable; > > namespace gdb > @@ -165,8 +166,9 @@ extern observable architecture_changed; > > /* The thread's ptid has changed. The OLD_PTID parameter specifies > the old value, and NEW_PTID specifies the new value. */ > -extern observable > - thread_ptid_changed; > +extern observable + ptid_t /* old_ptid */, ptid_t /* new_ptid */> > + thread_ptid_changed; > > /* The inferior INF has been added to the list of inferiors. At > this point, it might not be associated with any process. */ > diff --git a/gdb/regcache.c b/gdb/regcache.c > index 54354fe2c161..fb20250d20f0 100644 > --- a/gdb/regcache.c > +++ b/gdb/regcache.c > @@ -414,11 +414,12 @@ regcache_observer_target_changed (struct target_ops *target) > > /* Update regcaches related to OLD_PTID to now use NEW_PTID. */ > static void > -regcache_thread_ptid_changed (ptid_t old_ptid, ptid_t new_ptid) > +regcache_thread_ptid_changed (process_stratum_target *target, > + ptid_t old_ptid, ptid_t new_ptid) > { > for (auto ®cache : regcaches) > { > - if (regcache->ptid () == old_ptid) > + if (regcache->ptid () == old_ptid && regcache->target () == target) > regcache->set_ptid (new_ptid); > } > } > @@ -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 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 ... > + > + /* 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? ;-) > + > + /* 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. > +} > + > } // namespace selftests > #endif /* GDB_SELF_TEST */ > > @@ -1840,5 +1893,7 @@ _initialize_regcache () > selftests::cooked_read_test); > selftests::register_test_foreach_arch ("regcache::cooked_write_test", > selftests::cooked_write_test); > + selftests::register_test ("regcache_thread_ptid_changed", > + selftests::regcache_thread_ptid_changed); > #endif > } > diff --git a/gdb/thread.c b/gdb/thread.c > index 4dce1ef82aaf..c915407581fb 100644 > --- a/gdb/thread.c > +++ b/gdb/thread.c > @@ -775,7 +775,7 @@ thread_change_ptid (process_stratum_target *targ, > tp = find_thread_ptid (inf, old_ptid); > tp->ptid = new_ptid; > > - gdb::observers::thread_ptid_changed.notify (old_ptid, new_ptid); > + gdb::observers::thread_ptid_changed.notify (targ, old_ptid, new_ptid); > } > > /* See gdbthread.h. */ > Otherwise LGTM. Thanks, Pedro Alves