Re: [PATCH 3/4] gdb: pass target to thread_ptid_changed observable

Pedro Alves
2020-07-23T20:42:53Z

On 7/20/20 9:41 PM, Simon Marchi via Gdb-patches wrote:
> 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.

Yes, it's definitely a bug. Thanks for fixing this.

>
> 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.

I think the latter is unlikely to be a bug in practice, because inferior_ptid
will be pointing at a thread of the current target, and thread_change_ptid
will be called in the context of the same target. But it doesn't hurt to
make it explicit, of course.

>
> 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 <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;
> }
>
> @@ -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<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 ()
> @@ -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<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 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 &regcache : 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<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 ());

This will fail if you debug something, e.g., run to main,
and then do:

(gdb) maint selftest regcache_thread_ptid_changed

Maybe call registers_changed() before ?

Actually trying that made me notice that I completely missed
making cooked_write_test in 236ef0346d88efffd1ca1da1a5d80724cb145660 ...
Fixing that would get rid of these fails:

Self test failed: arch i386: target already pushed
...

> +
> + /* 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);

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 (&target2.mock_target, old_ptid));

For completeness, I'd add:

gdb_assert (!regcache_exists (&target1.mock_target, new_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, new_ptid));
> + gdb_assert (regcache_exists (&target2.mock_target, old_ptid));

Similarly here.

> +}
> +
> } // 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. */
>

Otherwise LGTM.

Thanks,
Pedro Alves