Re: [PATCH 4/4] gdb: change regcache list to be a map

[Tom Tromey <tom@tromey.com>]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> The function registers_changed_ptid deletes all regcaches related to a
Simon> given (target, ptid).  We must now handle the different cases
Simon> separately:

[...]
Simon> - non-NULL target and non-minus_one_ptid: we delete all the entries
Simon>   associated to that tuple, this is done efficiently
Simon> - a non-NULL target and minus_one_ptid: we delete all the entries
Simon>   associated to that target, whatever the ptid.  This is the slightly
Simon>   annoying case, as we can't easily look up all items having this target
Simon>   in their key.  I implemented it by walking the list, which is not
Simon>   ideal.

This patch caused a regression for Ravenscar targets.  I spent a bit of
time tracking it down, and I think the issue is that there was a change
in the semantics in registers_changed_ptid.

Formerly it did this:

Simon> -      if ((target == nullptr || regcache->target () == target)
Simon> -	  && regcache->ptid ().matches (ptid))
Simon> -	{
Simon> -	  delete regcache;
Simon> -	  it = regcaches.erase_after (oit);

But now it does:

      if (target == nullptr)
    ...
      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 ())
            {
              auto &ptid_regc_map = ptid_regc_map_it->second;
              ptid_regc_map.erase (ptid);
            }
        }
      else
    ...

The difference being the call to ptid::matches.  This method will return
true if the ptid in question is a PID:

	    || (filter.is_pid () && m_pid == filter.pid ())

... but in the new code, no provision is made for the PID case.

This comes up because, at least in the case I am debugging,
target_resume is called with a PID and not -1.  See
user_visible_resume_ptid:

  else if (!sched_multi && target_supports_multi_process ())
    {
      /* Resume all threads of the current process (and none of other
	 processes).  */
      resume_ptid = ptid_t (inferior_ptid.pid ());
    }

At first I wasn't sure if this semantic change was really a bug; but now
it seems to me that it must be.  The non-stop and scheduler-locking
cases are handled earlier in user_visible_resume_ptid, so at this point
it intends to resume all the threads of the process.  And, in this
situation, I think the register caches for all threads in the process
ought to be cleared by the registers_changed_ptid call in target_resume.

A simple fix would be to fall back to iterating over the map in the
is_pid case.  I don't know whether this would reintroduce the
performance issue that prompted the patch, though.

I'd like to know whether you agree with this analysis ... if so, what
should we do; and if not, what is incorrect?

thanks,
Tom