| Message ID | 20230703134427.1389440-30-alex.bennee@linaro.org |
|---|---|
| State | Accepted |
| Commit | 3b72d68162f63ae4abc78edaa40775b045128ff6 |
| Headers | show |
| Series | [PULL,01/38] gitlab: explicit set artifacts publishing criteria | expand |
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;
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
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;