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

[Pedro Alves <pedro@palves.net>]

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.

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