[binutils-gdb] gdb: pass target to thread_ptid_changed observable
Simon Marchi
simark@sourceware.org
Fri Aug 7 15:20:32 GMT 2020
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b161a60d1fe2a7383c7940815687c6100b97204e
commit b161a60d1fe2a7383c7940815687c6100b97204e
Author: Simon Marchi <simon.marchi@efficios.com>
Date: Fri Aug 7 10:59:33 2020 -0400
gdb: pass target to thread_ptid_changed observable
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
Diff:
---
gdb/ChangeLog | 16 ++++++++++++
gdb/infrun.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
gdb/observable.h | 6 +++--
gdb/regcache.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++--
gdb/thread.c | 2 +-
5 files changed, 167 insertions(+), 7 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 66d08fc13c0..fa1120e89e6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@
+2020-08-07 Simon Marchi <simon.marchi@efficios.com>
+
+ * 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.
+
2020-08-06 Caroline Tice <cmtice@google.com>
* dwarf2/read.c (struct dwo_file): Update comment on 'sections' field.
diff --git a/gdb/infrun.c b/gdb/infrun.c
index ca850f81c43..7be1fed76df 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -67,6 +67,9 @@
#include "gdbsupport/gdb_select.h"
#include <unordered_map>
#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;
}
@@ -9455,6 +9460,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<test_target_ops> target1 (arch);
+ scoped_mock_context<test_target_ops> 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<test_target_ops> target1 (arch);
+ scoped_mock_context<test_target_ops> 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 ()
@@ -9756,4 +9825,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 070cf0f18e5..da0a9b12f74 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<struct gdbarch */* newarch */> 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<ptid_t /* old_ptid */, ptid_t /* new_ptid */>
- thread_ptid_changed;
+extern observable<process_stratum_target * /* target */,
+ 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 3c460f255fd..71e528f9b60 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,71 @@ 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 ()
+{
+ /* This test relies on the global regcache list to initially be empty. */
+ registers_changed ();
+
+ /* Any arch will do. */
+ gdbarch *arch = current_inferior ()->gdbarch;
+
+ /* Prepare two targets with one thread each, with the same ptid. */
+ scoped_mock_context<test_target_ops> target1 (arch);
+ scoped_mock_context<test_target_ops> 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,
+ nullptr);
+ get_thread_arch_aspace_regcache (&target2.mock_target, old_ptid, arch,
+ 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 (&target1.mock_target, new_ptid));
+ gdb_assert (regcache_exists (&target2.mock_target, old_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, old_ptid));
+ gdb_assert (regcache_exists (&target1.mock_target, new_ptid));
+ gdb_assert (regcache_exists (&target2.mock_target, old_ptid));
+ gdb_assert (!regcache_exists (&target2.mock_target, new_ptid));
+
+ /* Leave the regcache list empty. */
+ registers_changed ();
+ gdb_assert (regcaches.empty ());
+}
+
} // namespace selftests
#endif /* GDB_SELF_TEST */
@@ -1840,5 +1906,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 4dce1ef82aa..c915407581f 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. */
More information about the Gdb-cvs
mailing list