Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: don't fail on vCont; C04:0; c packets

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 2086 bytes --]

On Wed, 31 May 2017 18:17:37 +0200
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> On Wed, 31 May 2017 16:09:33 +0100
> Alex Bennée <alex.bennee@linaro.org> wrote:
> 
> > The thread-id of 0 means any CPU but we then ignore the fact we find
> > the first_cpu in this case who can have an index of 0. Instead of
> > bailing out just test if we have managed to match up thread-id to a
> > CPU.
> > 
> > Otherwise you get:
> >   gdb_handle_packet: command='vCont;C04:0;c'
> >   put_packet: reply='E22'
> > 
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > ---
> >  gdbstub.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/gdbstub.c b/gdbstub.c
> > index a249846954..29c9ed3002 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -934,8 +934,8 @@ static int gdb_handle_vcont(GDBState *s, const
> > char *p)
> >               * CPU first, and only then we can use its index.
> >               */
> >              cpu = find_cpu(idx);
> > -            /* invalid CPU/thread specified */
> > -            if (!idx || !cpu) {
> > +            /* invalid thread specified, cpu not found. */
> > +            if (!cpu) {
> >                  res = -EINVAL;
> >                  goto out;
> >              }  
> 
> This is strange. cpu_index() is defined as:
> 
> static inline int cpu_index(CPUState *cpu)
> {
> #if defined(CONFIG_USER_ONLY)
>     return cpu->host_tid;
> #else
>     return cpu->cpu_index + 1;
> #endif
> }
> 
> therefore it shouldn't return 0 under any circumstance, and

I think it is 0 for first_cpu in user mode.

> find_cpu(idx) should also fail if idx == 0, because internally it also
> uses cpu_index()
> 
> on the other hand, you say that the patch does fix the problem for you,
> which really confuses me.
> 
> 
> 
> (probably) completely unrelatedly, this:
> 
> res = qemu_strtoul(p + 1, &p, 16, &tmp);
> 
> should be like this instead:
> 
> res = qemu_strtoul(p, &p, 16, &tmp);
> 
> but this shouldn't impact you in any way.
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]