[PATCH 2/2] gdb: avoid sending unnecessary wildcard vCont actions

[Andrew Burgess]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-05-13 13:55:05 -0400]:

> On 2021-05-11 10:39 a.m., Andrew Burgess wrote:
> > Looking at remote_target::commit_resumed the algorithm for sending
> > wildcard actions starts by assuming that we can send a wildcard
> > action, and then looks for pending events, or resume requests, that
> > prevent wildcard actions (either global, or inferior specific).
> > 
> > This is fine, and there's no real problems with this, but it does lead
> > to one slight weirdness, if we are in non-stop mode, and
> > displaced-stepping is off, then, when we do a step, all threads should
> > be stopped, and we should step only the one thread that we wish to
> > have move on.  However, in the situation where there the inferior only
> > has a single thread then we actually end up sending a packet like
> > this:
> > 
> >   [remote] Sending packet: $vCont;s:pPID.TID;c#XX
> > 
> > Notice the trailing ';c'.
> > 
> > Now, there's nothing technically wrong here, we are correctly asking
> > the one and only thread PID.TID to perform a step, there are no other
> > threads, so telling everyone else to continue is harmless.
> > 
> > However, this annoys me.  It annoys me because we know we want only
> > the one, and only one, thread PID.TID to step, why send over redundant
> > information!
> > 
> > This commit fixes this tiny blip by keeping a counter of the number of
> > threads that could be resumed in each inferior.  We count up as we see
> > threads that need to be resumed, and then count down when non-wildcard
> > actions are sent (e.g. the s:pPID.TID in our example above).  Then,
> > when we are considering sending wildcard actions, we only send one if
> > there are any threads that will be impacted by the wildcard action.
> > 
> > Now, in the above situation we send out
> > 
> >   [remote] Sending packet: $vCont;s:pPID.TID#XX
> > 
> > There should be no change in behaviour when considering gdb/gdbserver
> > working together after this commit.
> 
> The code looks correct, but in my opinion this little oddity is not
> significant enough to justify making this code more complex (it's
> already complex enough as it is).

Thanks for the feedback.

I spotted this oddity when looking at gdb/27830 [1], this bug boils
down to what should happen when a new thread is started.  Right now we
(almost) always assume that any new thread should be set running, but
this just isn't true.  If we are single stepping a thread in non-stop
mode, and with displaced stepping off, then, should a new thread
appear we should report the new thread as created/stopped rather than
setting it running.

Solving this for native targets is easy enough, but we don't really
have an easy/obvious way in the remote protocol to specify how new
threads should be handled (well, we have QThreadEvents, which might be
the solution).

Anyway, I wondered if it was possible to infer from the last vCont
packet how new threads should be handled, which got me looking at
these packets...

Given I don't, right now, know that fixing this is a requirement, I'll
hold off pushing this until either (a) I know it's required, or (b) I
can figure out a simpler way to have write the logic so we can send
the exactly correct contents.

Thanks,
Andrew


[1] https://sourceware.org/bugzilla/show_bug.cgi?id=27830