Re: [PATCHv3 0/2] Improve 'maint set target-async off' for remote targets

[John Baldwin <jhb@FreeBSD.org>]

On 12/1/21 2:40 AM, Andrew Burgess via Gdb-patches wrote:
> I pushed patches #1 to #5 from the original series, but it turned out
> that the assert added in patch #5 would trigger in some tests.  I
> checked my cached results and I definitiely wasn't seeing that assert
> during my testing, so my original assumption was that patch #6 fixed
> the cause of the assert.
> 
> However, upon retesting I found that this was not the case.  After
> rebuilding my development branch, the assert was present.  But it
> hadn't triggered during my testing.
> 
> I honestly have no idea what I originally tested - it clearly showed
> the expected improvement for native-extended-remote, so it contained
> some version of my work, but obviously not patch #5.
> 
> Which is why there's been a delay getting a new version of this patch
> on the list, I've retested the remaining patches, then double checked.
> 
> The old patch #5 is now patch #2 in this series, and the old patch #6
> is now patch #1.  Patch #1 contains a fix that allows the assert in
> patch #2, that previously caused problems, to now be just fine.
> 
> Changes since v2:
> 
>    - Previous patches #1 to #4 are now committed,
> 
>    - Previous patch #5 is now patch #2,
> 
>    - Previous patch #6 is now patch #1,
> 
>    - Change in remote_target::push_stop_reply, the async event token is
>      now only marked if the target _is_ in async mode, rather than if
>      the target _can_ be in async mode.  When async mode is later
>      enabled we were already marking the event token, so this should
>      not be a change in most cases.  During remote_target::start_remote
>      however, we push events, then process them without ever enabling
>      async mode, this use case was what was previously triggering the
>      assert in patch #2.
> 
>    - Code is otherwise unchanged.
> 
> Changes since v1:
> 
>    - The old patch #2 is removed, as Simon pointed out, this was just wrong.
> 
>    - The old patch #3, which was approved, is now patch #5, and is unchanged.
> 
>    - The old patch #4, is now patch #6, and still needs a review.
> 
>    - The old patch #1 has been split up into #1, #2, #3, and #4.  Given
>      significant changes since V1 these should probably be reviewed
>      afresh.
>      
> ---
> 
> Andrew Burgess (2):
>    gdb/remote: some fixes for 'maint set target-async off'
>    gdb: add assert in remote_target::wait relating to async being off
> 
>   gdb/remote.c | 192 +++++++++++++++++++++++----------------------------
>   1 file changed, 87 insertions(+), 105 deletions(-)

Patch 2 is straightforward and seems obviously correct to me.  I think Patch 1
also looks ok, and the description sounds sensible.  I'm just not super familiar
enough with the remote target in general to give a detailed review.  I do think
storing the "parsed" stop reply state in an existing queue is probably cleaner
overall (and my guess is the queue was added later and this cached reply just
wasn't converted to use the queue at the time?)

-- 
John Baldwin