From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from barracuda.ebox.ca (barracuda.ebox.ca [96.127.255.19]) by sourceware.org (Postfix) with ESMTPS id C5B823860C35 for ; Mon, 20 Jul 2020 20:41:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C5B823860C35 X-ASG-Debug-ID: 1595277677-0c856e180d10bcb0001-fS2M51 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id dwyEk3yQyuyC9kGi (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 20 Jul 2020 16:41:17 -0400 (EDT) X-Barracuda-Envelope-From: simon.marchi@efficios.com X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from smarchi-efficios.internal.efficios.com (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) by smtp.ebox.ca (Postfix) with ESMTP id 04754441D66; Mon, 20 Jul 2020 16:41:16 -0400 (EDT) From: Simon Marchi X-Barracuda-RBL-IP: 192.222.181.218 X-Barracuda-Effective-Source-IP: 192-222-181-218.qc.cable.ebox.net[192.222.181.218] X-Barracuda-Apparent-Source-IP: 192.222.181.218 To: gdb-patches@sourceware.org Cc: Morichetti, Laurent , Simon Marchi Subject: [PATCH 3/4] gdb: pass target to thread_ptid_changed observable Date: Mon, 20 Jul 2020 16:41:00 -0400 X-ASG-Orig-Subj: [PATCH 3/4] gdb: pass target to thread_ptid_changed observable Message-Id: <20200720204101.2849535-4-simon.marchi@efficios.com> X-Mailer: git-send-email 2.26.2 In-Reply-To: <20200720204101.2849535-1-simon.marchi@efficios.com> References: <20200720204101.2849535-1-simon.marchi@efficios.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Barracuda-Connect: smtp.ebox.ca[96.127.255.82] X-Barracuda-Start-Time: 1595277677 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at ebox.ca X-Barracuda-Scan-Msg-Size: 9840 X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.50 X-Barracuda-Spam-Status: No, SCORE=0.50 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests=BSF_RULE7568M X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.83339 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.50 BSF_RULE7568M Custom Rule 7568M X-Spam-Status: No, score=-24.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_SOFTFAIL, 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: Mon, 20 Jul 2020 20:41:24 -0000 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. 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. 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 + 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 ()); + + /* 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); + + /* 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)); + + 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)); +} + } // 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. */ -- 2.26.2