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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Pedro Alves <palves@redhat.com> [2020-03-02 19:07:33 +0000]:

> 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 for the explanation.

Andrew