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

[Pedro Alves <pedro@palves.net>]

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.

> 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?

Otherwise the patch LGTM.  Thanks for doing this.