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 --git a/gdb/ChangeLog b/gdb/ChangeLog
index 66d08fc13c0bf30659844c516a4a7ae4eaa00205..fa1120e89e630ccf37fc1527d6a2c5f500fab2f7 100644 (file)
--- 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 ca850f81c43ba6f2ac34309a6feddc3d2d7a9acf..7be1fed76df21d1fad1aa460d13bf6b654025aee 100644 (file)
--- 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 070cf0f18e5197bad9c41e9fe7521dfb91fc9a0b..da0a9b12f74caab06a11bafa0e78f5f654937f01 100644 (file)
--- 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 3c460f255fd35796d226c4a4a5250c09bb156a39..71e528f9b60f7b3d27b758ea648faabfe1cb733f 100644 (file)
--- 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 &regcache : 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 4dce1ef82aaf5ad2dde8ea690f09047da22607ef..c915407581fb6fba7bdf00968d31d8259557ddb8 100644 (file)
--- 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.  */