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

[Alex Bennée]

Claudio Imbrenda <address@hidden> writes:

> 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

This only applies to newly cloned threads. The first is unset.

>
>> > 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.
>> >
>> >
>> >
>>


--
Alex Bennée