Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Peter Maydell]

On Mon, 31 Jul 2023 at 07:59, Joel Stanley <joel@jms.id.au> wrote:
>
> On Sun, 30 Jul 2023 at 09:43, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > On Wed Jul 26, 2023 at 4:35 PM AEST, Joel Stanley wrote:
> > > On Wed, 12 Jul 2023 at 02:12, Nicholas Piggin <npiggin@gmail.com> wrote:
> > > >
> > > > On Tue Jul 11, 2023 at 9:03 PM AEST, Matheus Tavares Bernardino wrote:
> > > > > > Nicholas Piggin <npiggin@gmail.com> wrote:
> > > > > >
> > > > > > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > > > > > index 6911b73c07..ce8b42eb15 100644
> > > > > > --- a/gdbstub/gdbstub.c
> > > > > > +++ b/gdbstub/gdbstub.c
> > > > > > @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
> > > > > >              return;
> > > > > >      }
> > > > > >      if (runstate_is_running()) {
> > > > > > -        /* when the CPU is running, we cannot do anything except 
> > > > > > stop
> > > > > > -           it when receiving a char */
> > > > > > +        /*
> > > > > > +         * When the CPU is running, we cannot do anything except 
> > > > > > stop
> > > > > > +         * it when receiving a char. This is expected on a Ctrl-C 
> > > > > > in the
> > > > > > +         * gdb client. Because we are in all-stop mode, gdb sends a
> > > > > > +         * 0x03 byte which is not a usual packet, so we handle it 
> > > > > > specially
> > > > > > +         * here, but it does expect a stop reply.
> > > > > > +         */
> > > > > > +        if (ch != 0x03) {
> > > > > > +            warn_report("gdbstub: client sent packet while target 
> > > > > > running\n");
> > > > > > +        }
> > > > > > +        gdbserver_state.allow_stop_reply = true;
> > > > > >          vm_stop(RUN_STATE_PAUSED);
> > > > > >      } else
> > > > > >  #endif
> > > > >
> > > > > Makes sense to me, but shouldn't we send the stop-reply packet only 
> > > > > for
> > > > > Ctrl+C/0x03?
> > > >
> > > > Good question.
> > > >
> > > > I think if we get a character here that's not a 3, we're already in
> > > > trouble, and we eat it so even worse. Since we only send a stop packet
> > > > back when the vm stops, then if we don't send one now we might never
> > > > send it. At least if we send one then the client might have some chance
> > > > to get back to a sane state. And this does at least take us back to
> > > > behaviour before the stop filtering patch.
> > > >
> > > > Could go further and only stop the machine if it was a 3, or send a
> > > > stop packet even if we were stopped, etc. but all that get further from
> > > > a minimal fix.
> > >
> > > I was taking a look at -rc1 and it looks like this hasn't made it in.
> > > Is it something we want to propose including?
> > >
> > > As a user of qemu I'd vote for it to go in.
> >
> > I think it should, gdb is hardly usable without it.
>
> I think I hit this issue when debugging u-boot in the aspeed arm
> machines. Your patch fixed things:
>
> Tested-by: Joel Stanley <joel@jms.id.au>
>
> Alex, Philippe, can we get this queued for 8.1?

I'm doing a pullreq today anyway, so I can grab it.

thanks
-- PMM