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

[Simon Marchi <simon.marchi@efficios.com>]

On 2020-07-23 4:42 p.m., Pedro Alves wrote:
>> @@ -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

I don't know, do we really want to support this?  I don't really see the point.  We can design
the selftests to make it possible, but is there any advantage?  In the selftests command, we
could ensure that you are not debugging something, and otherwise error out with "You can't run
selftests while debugging.".

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

I can do this, but I'll wait for your reply on the suggestion above to disallow running selftests
while debugging first.

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

Yes!  I actually appreciate you pointing it out, if we chose one convention and follow it, we
don't have to always wonder which one to use!

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

I thought that with `gdb_assert (regcaches_size () == 2)`, but more asserts
is not bad, it could catch some bug, we never know.  I've added them.

Simon