Re: [PATCHv3 1/2] gdb/remote: some fixes for 'maint set target-async off'

[Andrew Burgess <aburgess@redhat.com>]

* Pedro Alves <pedro@palves.net> [2021-12-17 14:05:55 +0000]:

> On 2021-12-17 13:35, Andrew Burgess wrote:
> > * Pedro Alves <pedro@palves.net> [2021-12-16 21:15:31 +0000]:
> > 
> >> On 2021-12-01 10:40, Andrew Burgess via Gdb-patches wrote:
> >>
> >>> This problem can clearly be seen I feel by looking at the
> >>> remote_state::cached_wait_status flag.  This flag tells GDB if there
> >>> is a wait status cached in remote_state::buf.  However, in
> >>> remote_target::putpkt_binary and remote_target::getpkt_or_notif_sane_1
> >>> this flag is just set back to 0, doing this immediately discards any
> >>> cached data.
> >>>
> >>> I don't know if this scheme ever made sense, maybe once upon a time,
> >>> GDB would, when it found it had no cached stop, simply re-request the
> >>> stop information from the target, however, this certainly isn't the
> >>> case now, and resetting the cached_wait_status is (I claim) a bad
> >>> thing.
> >>
> >> I don't think that was ever the case.  Take a look at 2d717e4f8a54,
> >> where the cached status was introduced to handle "attach".  It was simply the case
> >> back then that nothing would talk to the target between the initial attach
> >> and consuming the event.  It's not clear to me why putpkt/getpkt would
> >> need to clear the flag back then.  Looks more like a "just in case" safeguard.
> > 
> > Thanks for this insight.  I've updated the commit message to try and
> > describe the history here a little more accurately, including adding
> > the commit SHA you gave above, which should help if anyone needs to
> > dig into this code in the future.
> > 
> >>
> >>> So, finally, in this commit, I propose to remove the
> >>> remote_state::cached_wait_status flag and to stop using the ::buf to
> >>> cache stop replies.  Instead, stop replies will now always be stored
> >>> in the ::stop_reply_queue.
> >>
> >> To be honest, I don't recall exactly why I didn't do that when introducing
> >> the stop reply queue.
> >>
> >>> @@ -8163,15 +8151,12 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status,
> >>>  
> >>>    stop_reply = queued_stop_reply (ptid);
> >>>    if (stop_reply != NULL)
> >>> -    return process_stop_reply (stop_reply, status);
> >>> -
> >>> -  if (rs->cached_wait_status)
> >>> -    /* Use the cached wait status, but only once.  */
> >>> -    rs->cached_wait_status = 0;
> >>> +    {
> >>> +      rs->waiting_for_stop_reply = 0;
> >>
> >> This is a difference described in the commit log, but looking at the resulting code,
> >> I don't understand why clearing this flag is needed here, it looks like dead code to me.
> >> I mean, if we have a cached status already, then we're not waiting for a stop reply
> >> from the target.  Did you run into a case where it was needed?
> > 
> > No I never hit a case where it was definitely needed.  Honestly, I
> > just looked at the different code paths, and saw that it was pretty
> > easy to merge the paths and carry out all the actions, so did that.
> > 
> > However, you're right to call me out on that.  It's exactly this sort
> > of "just in case" code that was causing the cached packets to be
> > discarded originally without warning.
> > 
> > So, I've changed the unconditional "set flag to false" with an assert,
> > and a comment.  If it turns out we are wrong in our understanding of
> > the situation, then hopefully, the problem will get reported, and we
> > can figure out what's going on at that point!
> 
> Yeah.  I don't imagine how this will ever trigger, but definitely can't hurt
> having an assert.
> 
> - If we have an event queued it must mean that we have gotten the reply from the target
>   already.  Recall this is all-stop RSP, vCont is synchronous.
> 
> - And if we see a stop reply, and have it queued, we can't resume the target before
>   the queued event is processed, otherwise when we get to process the queued event,
>   the target is already running past what the stop the event was for!  If we do ever
>   manage to sent a vCont with a pending queued event, then that's a bug.
> 
> > 
> > Below is the updated patch.  The only code change is the assert I
> > mention above.  All other changes are in the commit message.
> > 
> > I'll give this a few days in case you want to follow up, then I'll
> > push this.
> > 
> 
> Thanks, this is OK.  Just noticed a tiny typo in the new comment:
> 
> > +    {
> > +      /* Currently non of the paths that push a stop reply onto the queue
> 
> 
> non -> none.  I'd argue that "Currently" is redundant, and gives a false
> impression that that might change without much consequence other than hitting
> this assertion.  I think you should remove that word.

Thanks for catching that.  I made the changes you suggested, and
pushed this patch.

Thanks,
Andrew


> 
> > +	 will have set the waiting_for_stop_reply flag.  */
> > +      gdb_assert (!rs->waiting_for_stop_reply);
> > +      event_ptid = process_stop_reply (stop_reply, status);
> > +    }
>