Re: [PATCHv3 2/2] gdb/remote: Restore support for 'S' stop reply packet

[Pedro Alves <palves at redhat dot com>]

On 3/2/20 3:19 PM, Andrew Burgess wrote:
> * Pedro Alves <palves@redhat.com> [2020-03-02 12:25:12 +0000]:

>> "inferior" -> "target".
>>
>> The "inferior" -> "target" distinction I'm making in these
>> small remarks above matters, because say the remote server
>> is debugging two single-threaded inferiors.  We still want to
>> (and do) warn.
> 
> I hadn't really considered this case, however, this raises a
> follow on question:
> 
> Before entering the target wait code we call
> switch_to_inferior_no_thread, partly, as I understand it because
> having inferior_ptid pointing at a thread leads to invalid code
> that relies on this thread being _the_ event thread, when really we
> need to extract the inferior and thread-id from the stop event.

The main reason we switch the inferior is that the target stack is a
property of the inferior now.  Each inferior has its own target stack
(see inferior::m_target_stack).  Note, target stack, not target.
A single target may be used by different inferiors.  E.g.,
when remote debugging, if you're debugging two inferiors under control
of the same gdbserver, each inferior will have a pointer to the
same remote target instance in its target stack.

To call any target method, we must be sure to make the relevant
inferior current, because target methods consult the inferior's
target stack to know which target to call.  When target methods
defer to the target beneath, they'll again consult the target stack
stored in the current inferior (the "this->beneath ()->foo()" calls
in target-delegates.c).

So to call the target_wait method to poll events out of some target,
you need to switch to some representative inferior that uses the
process_stratum target you want to poll.

However, a reason we need to switch to all inferiors in turn and not
just a subset sufficient to cover all instantiated process_stratum
targets, is the strata above process_stratum, like record_stratum.
Those targets aren't shared between inferiors, and they generate
target events as well.

I suspect it may be possible to come up with a cleaner design here,
but I haven't been able to come up with one that's as simple as the
current one, given all the constraints of the rest of current
gdb's design.  I didn't try very hard though.

Since we're switching the inferior around, the question 
of -- which thread in the inferior should we pick? -- immediately
arises.  And the best answer is "none".  Switching to no-thread also
has the property that it "catches" target backends relying on
inferior_ptid to infer things about the next stopping event,
when they should not.  (Another point that helps with seeing how that's
wrong is to consider that inferior_ptid includes no field
that would indicate which target instance the ptid came from.
So e.g., if you're debugging against two remote stubs, it could well
be the case that when you get to target_wait, inferior_ptid points at
a thread of the other target, even though it seemingly looks
like a valid thread in the current target.)

> 
> So, why do we allow the current_inferior to remain valid while we
> perform the wait?  Instead, why don't we clear both current_inferior
> and inferior_ptid, and then enter the wait code?

It's impossible to clear current_inferior with the current design.
There must always be a current inferior.

Thanks,
Pedro Alves