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

[Pedro Alves <pedro@palves.net>]

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