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

Tom Tromey
2020-08-12T12:52:20Z

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