From: Nicholas Piggin <npiggin@gmail.com>

The final part of the reverse step and break handling is to bring
the machine back to a debug stop state. gdb expects a response.

A gdb 'rsi' command hangs forever because the gdbstub filters out
the response (also observable with reverse_debugging.py avocado
tests).

Fix by setting allow_stop_reply for the gdb backward packets.

Fixes: 758370052fb ("gdbstub: only send stop-reply packets when allowed to")
Cc: qemu-stable@nongnu.org
Cc: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Taylor Simpson <tsimpson@quicinc.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Acked-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Message-Id: <20230623035304.279833-1-npiggin@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20230630180423.558337-30-alex.bennee@linaro.org>

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index be18568d0a..9496d7b175 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1814,6 +1814,7 @@ static int gdb_handle_packet(const char *line_buf)
                 .handler = handle_backward,
                 .cmd = "b",
                 .cmd_startswith = 1,
+                .allow_stop_reply = true,
                 .schema = "o0"
             };
             cmd_parser = &backward_cmd_desc;
-- 
2.39.2

03.07.2023 16:44, Alex Bennée wrote:
> From: Nicholas Piggin <npiggin@gmail.com>
> 
> The final part of the reverse step and break handling is to bring
> the machine back to a debug stop state. gdb expects a response.
> 
> A gdb 'rsi' command hangs forever because the gdbstub filters out
> the response (also observable with reverse_debugging.py avocado
> tests).
> 
> Fix by setting allow_stop_reply for the gdb backward packets.
> 
> Fixes: 758370052fb ("gdbstub: only send stop-reply packets when allowed to")
> Cc: qemu-stable@nongnu.org

Hi!

Are you guys sure this needs to be in -stable?

To me it looks a sort of "partial revert" of a previous commit:

commit 758370052fb602f9f23c3b8ae26a6133373c78e6
Author: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Date:   Thu May 4 12:37:31 2023 -0300
Subject: gdbstub: only send stop-reply packets when allowed to

which introduced `allow_stop_reply' field in GdbCmdParseEntry.
This change ("gdbstub: Permit..") does not work in 8.0 without
the above mentioned "gdbstub: only send" commit, and I guess
it is *not* supposed to be in stable. Or is it?

I'm not applying this one to stable for now.

Thanks,

/mjt

> Cc: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Taylor Simpson <tsimpson@quicinc.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Acked-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Message-Id: <20230623035304.279833-1-npiggin@gmail.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20230630180423.558337-30-alex.bennee@linaro.org>
> 
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index be18568d0a..9496d7b175 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -1814,6 +1814,7 @@ static int gdb_handle_packet(const char *line_buf)
>                   .handler = handle_backward,
>                   .cmd = "b",
>                   .cmd_startswith = 1,
> +                .allow_stop_reply = true,
>                   .schema = "o0"
>               };
>               cmd_parser = &backward_cmd_desc;

Michael Tokarev <mjt@tls.msk.ru> writes:

> 03.07.2023 16:44, Alex Bennée wrote:
>> From: Nicholas Piggin <npiggin@gmail.com>
>> The final part of the reverse step and break handling is to bring
>> the machine back to a debug stop state. gdb expects a response.
>> A gdb 'rsi' command hangs forever because the gdbstub filters out
>> the response (also observable with reverse_debugging.py avocado
>> tests).
>> Fix by setting allow_stop_reply for the gdb backward packets.
>> Fixes: 758370052fb ("gdbstub: only send stop-reply packets when
>> allowed to")
>> Cc: qemu-stable@nongnu.org
>
> Hi!
>
> Are you guys sure this needs to be in -stable?
>
> To me it looks a sort of "partial revert" of a previous commit:
>
> commit 758370052fb602f9f23c3b8ae26a6133373c78e6
> Author: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Date:   Thu May 4 12:37:31 2023 -0300
> Subject: gdbstub: only send stop-reply packets when allowed to
>
> which introduced `allow_stop_reply' field in GdbCmdParseEntry.
> This change ("gdbstub: Permit..") does not work in 8.0 without
> the above mentioned "gdbstub: only send" commit, and I guess
> it is *not* supposed to be in stable. Or is it?
>
> I'm not applying this one to stable for now.

Good catch - your right it's purely fixing something that has been
merged in the current cycle.

>
> Thanks,
>
> /mjt
>
>> Cc: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
>> Cc: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Taylor Simpson <tsimpson@quicinc.com>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> Acked-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
>> Message-Id: <20230623035304.279833-1-npiggin@gmail.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20230630180423.558337-30-alex.bennee@linaro.org>
>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>> index be18568d0a..9496d7b175 100644
>> --- a/gdbstub/gdbstub.c
>> +++ b/gdbstub/gdbstub.c
>> @@ -1814,6 +1814,7 @@ static int gdb_handle_packet(const char *line_buf)
>>                   .handler = handle_backward,
>>                   .cmd = "b",
>>                   .cmd_startswith = 1,
>> +                .allow_stop_reply = true,
>>                   .schema = "o0"
>>               };
>>               cmd_parser = &backward_cmd_desc;


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

08.07.2023 13:10, Alex Bennée wrote:
...
> Good catch - your right it's purely fixing something that has been
> merged in the current cycle.

That's entirely okay, - it's better to tag extra as "for-stable" and
reject things than to omit something important.

This is not a good catch actually - it immediately leads to a build
failure, plain and obvious, and looking at previous changes in this
area immediately leads to the other commit.

Thank you for confirming my suspicions, and please keep the good work!

/mjt