gdb: handle the `ptid.is_pid ()` case in registers_changed_ptid

As reported by Tom here [1], commit 888bdb2b7445 ("gdb: change regcache
list to be a map") overlooked an important case, causing a regression.
When registers_changed_ptid is called with a pid-like ptid, it used to
clear all the regcaches for that pid.  That commit accidentally removed
that behavior.  We need to handle the `ptid.is_pid ()` case in
registers_changed_ptid.

The most trivial way of fixing it would be to iterate on all ptids of a
target and delete the entries where the ptid match the pid.  But the
point of that commit was to avoid having to iterate on ptids to
invalidate regcaches, so that would feel like a step backwards.

The only logical solution I see is to add yet another map level, so that
we now have:

  target -> (pid -> (ptid -> regcaches))

This patch implements that and adds a test for the case of calling
registers_changed_ptid with a pid-like ptid.

[1] https://sourceware.org/pipermail/gdb-patches/2020-August/171222.html

gdb/ChangeLog:

	* regcache.c (pid_ptid_regcache_map): New type.
	(target_ptid_regcache_map): Remove.
	(target_pid_ptid_regcache_map): New type.
	(regcaches): Change type to target_pid_ptid_regcache_map.
	(get_thread_arch_aspace_regcache): Update.
	(regcache_thread_ptid_changed): Update, handle pid-like ptid
	case.
	(regcaches_size): Update.
	(regcache_count): Update.
	(registers_changed_ptid_target_pid_test): New.
	(_initialize_regcache): Register new test.

Change-Id: I4c46e26d8225c177dbac9488b549eff4c68fa0d8

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 330d7f3ada06205bc02a1f46d9c5d526bece1b8b..70a8f643fb1155180a7b1447938f82526abf7287 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,17 @@
+2020-08-20  Simon Marchi  <simon.marchi@polymtl.ca>
+
+       * regcache.c (pid_ptid_regcache_map): New type.
+       (target_ptid_regcache_map): Remove.
+       (target_pid_ptid_regcache_map): New type.
+       (regcaches): Change type to target_pid_ptid_regcache_map.
+       (get_thread_arch_aspace_regcache): Update.
+       (regcache_thread_ptid_changed): Update, handle pid-like ptid
+       case.
+       (regcaches_size): Update.
+       (regcache_count): Update.
+       (registers_changed_ptid_target_pid_test): New.
+       (_initialize_regcache): Register new test.
+
 2020-08-20  Simon Marchi  <simon.marchi@polymtl.ca>
 
        * regcache.c (regcache_count): New.

diff --git a/gdb/regcache.c b/gdb/regcache.c
index d9af4539ab9aff3eb8eca4e405100230fc6ec6c2..91d3202b94baa87573c1b903bd6759dd83f99981 100644 (file)
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -319,11 +319,14 @@ reg_buffer::assert_regnum (int regnum) const
 using ptid_regcache_map
   = std::unordered_multimap<ptid_t, regcache_up, hash_ptid>;
 
-/* Type to map a target to a ptid_regcache_map, holding the regcaches for the
-   threads defined by that target.  */
+/* Type holding regcaches for a given pid.  */
 
-using target_ptid_regcache_map
-  = std::unordered_map<process_stratum_target *, ptid_regcache_map>;
+using pid_ptid_regcache_map = std::unordered_map<int, ptid_regcache_map>;
+
+/* Type holding regcaches for a given target.  */
+
+using target_pid_ptid_regcache_map
+  = std::unordered_map<process_stratum_target *, pid_ptid_regcache_map>;
 
 /* Global structure containing the existing regcaches.  */
 
@@ -331,7 +334,7 @@ using target_ptid_regcache_map
    recording if the register values have been changed (eg. by the
    user).  Therefore all registers must be written back to the
    target when appropriate.  */
-static target_ptid_regcache_map regcaches;
+static target_pid_ptid_regcache_map regcaches;
 
 struct regcache *
 get_thread_arch_aspace_regcache (process_stratum_target *target,
@@ -340,8 +343,11 @@ get_thread_arch_aspace_regcache (process_stratum_target *target,
 {
   gdb_assert (target != nullptr);
 
-  /* Find the ptid -> regcache map for this target.  */
-  auto &ptid_regc_map = regcaches[target];
+  /* Find the map for this target.  */
+  pid_ptid_regcache_map &pid_ptid_regc_map = regcaches[target];
+
+  /* Find the map for this pid.  */
+  ptid_regcache_map &ptid_regc_map = pid_ptid_regc_map[ptid.pid ()];
 
   /* Check first if a regcache for this arch already exists.  */
   auto range = ptid_regc_map.equal_range (ptid);
@@ -436,12 +442,19 @@ static void
 regcache_thread_ptid_changed (process_stratum_target *target,
                              ptid_t old_ptid, ptid_t new_ptid)
 {
-  auto ptid_regc_map_it = regcaches.find (target);
+  /* Look up map for target.  */
+  auto pid_ptid_regc_map_it = regcaches.find (target);
+  if (pid_ptid_regc_map_it == regcaches.end ())
+    return;
 
-  if (ptid_regc_map_it == regcaches.end ())
+ /* Look up map for pid.  */
+  pid_ptid_regcache_map &pid_ptid_regc_map = pid_ptid_regc_map_it->second;
+  auto ptid_regc_map_it = pid_ptid_regc_map.find (old_ptid.pid ());
+  if (ptid_regc_map_it == pid_ptid_regc_map.end ())
     return;
 
-  auto &ptid_regc_map = ptid_regc_map_it->second;
+  /* Update all regcaches belonging to old_ptid.  */
+  ptid_regcache_map &ptid_regc_map = ptid_regc_map_it->second;
   auto range = ptid_regc_map.equal_range (old_ptid);
   for (auto it = range.first; it != range.second;)
     {
@@ -478,15 +491,43 @@ registers_changed_ptid (process_stratum_target *target, ptid_t ptid)
       /* Delete all the regcaches of all targets.  */
       regcaches.clear ();
     }
+  else if (ptid.is_pid ())
+    {
+      /* Non-NULL target and pid ptid, delete all regcaches belonging
+        to this (TARGET, PID).  */
+
+      /* Look up map for target.  */
+      auto pid_ptid_regc_map_it = regcaches.find (target);
+      if (pid_ptid_regc_map_it != regcaches.end ())
+       {
+         pid_ptid_regcache_map &pid_ptid_regc_map
+           = pid_ptid_regc_map_it->second;
+
+         pid_ptid_regc_map.erase (ptid.pid ());
+       }
+    }
   else if (ptid != minus_one_ptid)
     {
       /* Non-NULL target and non-minus_one_ptid, delete all regcaches belonging
-       to this (TARGET, PTID).  */
-      auto ptid_regc_map_it = regcaches.find (target);
-      if (ptid_regc_map_it != regcaches.end ())
+        to this (TARGET, PTID).  */
+
+      /* Look up map for target.  */
+      auto pid_ptid_regc_map_it = regcaches.find (target);
+      if (pid_ptid_regc_map_it != regcaches.end ())
        {
-         auto &ptid_regc_map = ptid_regc_map_it->second;
-         ptid_regc_map.erase (ptid);
+         pid_ptid_regcache_map &pid_ptid_regc_map
+           = pid_ptid_regc_map_it->second;
+
+         /* Look up map for pid.  */
+         auto ptid_regc_map_it
+           = pid_ptid_regc_map.find (ptid.pid ());
+         if (ptid_regc_map_it != pid_ptid_regc_map.end ())
+           {
+             ptid_regcache_map &ptid_regc_map
+               = ptid_regc_map_it->second;
+
+             ptid_regc_map.erase (ptid);
+           }
        }
     }
   else
@@ -1479,10 +1520,23 @@ static size_t
 regcaches_size ()
 {
   size_t size = 0;
-  for (auto it = regcaches.begin (); it != regcaches.end (); ++it)
+
+  for (auto pid_ptid_regc_map_it = regcaches.cbegin ();
+       pid_ptid_regc_map_it != regcaches.cend ();
+       ++pid_ptid_regc_map_it)
     {
-      auto &ptid_regc_map = it->second;
-      size += ptid_regc_map.size ();
+      const pid_ptid_regcache_map &pid_ptid_regc_map
+       = pid_ptid_regc_map_it->second;
+
+      for (auto ptid_regc_map_it = pid_ptid_regc_map.cbegin ();
+          ptid_regc_map_it != pid_ptid_regc_map.cend ();
+          ++ptid_regc_map_it)
+       {
+         const ptid_regcache_map &ptid_regc_map
+           = ptid_regc_map_it->second;
+
+         size += ptid_regc_map.size ();
+       }
     }
 
   return size;
@@ -1493,13 +1547,21 @@ regcaches_size ()
 static int
 regcache_count (process_stratum_target *target, ptid_t ptid)
 {
-  auto ptid_regc_map_it = regcaches.find (target);
-  if (ptid_regc_map_it != regcaches.end ())
+  /* Look up map for target.  */
+  auto pid_ptid_regc_map_it = regcaches.find (target);
+  if (pid_ptid_regc_map_it != regcaches.end ())
     {
-      auto &ptid_regc_map = ptid_regc_map_it->second;
-      auto range = ptid_regc_map.equal_range (ptid);
+      pid_ptid_regcache_map &pid_ptid_regc_map = pid_ptid_regc_map_it->second;
 
-      return std::distance (range.first, range.second);
+      /* Look map for pid.  */
+      auto ptid_regc_map_it = pid_ptid_regc_map.find (ptid.pid ());
+      if (ptid_regc_map_it != pid_ptid_regc_map.end ())
+       {
+         ptid_regcache_map &ptid_regc_map = ptid_regc_map_it->second;
+         auto range = ptid_regc_map.equal_range (ptid);
+
+         return std::distance (range.first, range.second);
+       }
     }
 
   return 0;
@@ -1622,6 +1684,22 @@ registers_changed_ptid_target_test ()
   SELF_CHECK (regcache_count (&data->test_target2, ptid_t (2, 2)) == 1);
 }
 
+/* Test marking regcaches of a specific (target, pid) as changed.  */
+
+static void
+registers_changed_ptid_target_pid_test ()
+{
+  regcache_test_data_up data = populate_regcaches_for_test ();
+
+  registers_changed_ptid (&data->test_target1, ptid_t (2));
+  SELF_CHECK (regcaches_size () == 9);
+
+  /* Regcaches from target1 should not exist, while regcaches from target2
+     should exist.  */
+  SELF_CHECK (regcache_count (&data->test_target1, ptid_t (2, 2)) == 0);
+  SELF_CHECK (regcache_count (&data->test_target2, ptid_t (2, 2)) == 1);
+}
+
 /* Test marking regcaches of a specific (target, ptid) as changed.  */
 
 static void
@@ -2012,10 +2090,12 @@ _initialize_regcache ()
                            selftests::get_thread_arch_aspace_regcache_test);
   selftests::register_test ("registers_changed_ptid_all",
                            selftests::registers_changed_ptid_all_test);
+  selftests::register_test ("registers_changed_ptid_target",
+                           selftests::registers_changed_ptid_target_test);
+  selftests::register_test ("registers_changed_ptid_target_pid",
+                           selftests::registers_changed_ptid_target_pid_test);
   selftests::register_test ("registers_changed_ptid_target_ptid",
                            selftests::registers_changed_ptid_target_ptid_test);
-  selftests::register_test ("registers_changed_ptid_target",
-                           selftests::registers_changed_ptid_target_test);
 
   selftests::register_test_foreach_arch ("regcache::cooked_read_test",
                                         selftests::cooked_read_test);