From b70e516e89d95d06252cb04ded3397ec0a0a3212 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Thu, 20 Aug 2020 10:11:09 -0400 Subject: [PATCH] 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 --- gdb/ChangeLog | 14 ++++++ gdb/regcache.c | 130 +++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 119 insertions(+), 25 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 330d7f3ada0..70a8f643fb1 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,17 @@ +2020-08-20 Simon Marchi + + * 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 * regcache.c (regcache_count): New. diff --git a/gdb/regcache.c b/gdb/regcache.c index d9af4539ab9..91d3202b94b 100644 --- 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; -/* 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; +using pid_ptid_regcache_map = std::unordered_map; + +/* Type holding regcaches for a given target. */ + +using target_pid_ptid_regcache_map + = std::unordered_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); -- 2.30.2