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

[Claudio Imbrenda]

On Wed, 31 May 2017 18:33:24 +0200
Greg Kurz <address@hidden> wrote:

> On Wed, 31 May 2017 18:17:37 +0200
> Claudio Imbrenda <address@hidden> wrote:
> 
> > On Wed, 31 May 2017 16:09:33 +0100
> > Alex Bennée <address@hidden> 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 <address@hidden>
> > > ---
> > >  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.

in linux-user/syscall.c:

info->tid = gettid();
cpu->host_tid = info->tid;

kernel thread-ids are system-wide unique and can't be 0
 
> > 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.
> > 
> > 
> >   
>