diff mbox series

[v1,2/2] gdbstub: don't fail on vCont; C04:0; c packets

Message ID 20170531150933.10156-3-alex.bennee@linaro.org
State New
Headers show
Series some gdbstub fixes for debug and vcont | expand

Commit Message

Alex Bennée May 31, 2017, 3:09 p.m. UTC
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(-)

-- 
2.13.0

Comments

Greg Kurz May 31, 2017, 4:17 p.m. UTC | #1
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


The index can never be 0 in system mode actually, but you're right that this
check doesn't make sense.

The code still looks a bit convoluted IMHO. What about something like the
following ?

            /* 0 means any thread, so we pick the first valid CPU */
            cpu = tmp ? find_cpu(tmp) : first_cpu;

            /* invalid CPU/thread specified */
            if (!cpu) {
                res = -EINVAL;
                goto out;
            }

Anyway, the fix looks ok.

Reviewed-by: Greg Kurz <groug@kaod.org>


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

>              }
Claudio Imbrenda May 31, 2017, 4:17 p.m. UTC | #2
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
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 May 31, 2017, 4:26 p.m. UTC | #3
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:

> 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

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


Hmm that was me assuming cpu_index(cpu) was the same as cpu->cpu_index.
However in the CONFIG_USER_ONLY case we return host_tid which is only
set by clone_func() so I think will not be set for the first cpu.

>

>

>

> (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
Alex Bennée May 31, 2017, 4:27 p.m. UTC | #4
Greg Kurz <groug@kaod.org> writes:

> 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

>

> The index can never be 0 in system mode actually, but you're right that this

> check doesn't make sense.

>

> The code still looks a bit convoluted IMHO. What about something like the

> following ?

>

>             /* 0 means any thread, so we pick the first valid CPU */

>             cpu = tmp ? find_cpu(tmp) : first_cpu;

>

>             /* invalid CPU/thread specified */

>             if (!cpu) {

>                 res = -EINVAL;

>                 goto out;

>             }


Yeah that will make it cleaner, I'll apply to v2.

>

> Anyway, the fix looks ok.

>

> Reviewed-by: Greg Kurz <groug@kaod.org>

>

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

>>              }



--
Alex Bennée
Greg Kurz May 31, 2017, 4:33 p.m. UTC | #5
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.

> 

> 

>
Claudio Imbrenda May 31, 2017, 4:51 p.m. UTC | #6
On Wed, 31 May 2017 18:33:24 +0200
Greg Kurz <groug@kaod.org> wrote:

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


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.

> > 

> > 

> >   

>
Greg Kurz May 31, 2017, 5:06 p.m. UTC | #7
On Wed, 31 May 2017 18:51:06 +0200
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:
[...]
> > > 

> > > 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 is correct but these lines are in clone_func(). This gets called for
all threads but the "main" thread which I believe to be associated to
first_cpu.
Alex Bennée May 31, 2017, 5:23 p.m. UTC | #8
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:

> On Wed, 31 May 2017 18:33:24 +0200

> Greg Kurz <groug@kaod.org> wrote:

>

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

>

> 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
Claudio Imbrenda May 31, 2017, 5:40 p.m. UTC | #9
On Wed, 31 May 2017 19:06:29 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Wed, 31 May 2017 18:51:06 +0200

> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> [...]

> > > > 

> > > > 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 is correct but these lines are in clone_func(). This gets called

> for all threads but the "main" thread which I believe to be

> associated to first_cpu.


then IMHO that is a bug and it needs to be corrected. the host_tid
should be, well, the host tid, and not 0, which is never a valid
tid for Linux.

the current behaviour is simply the easiest for the "any CPU" case.
Picking the last CPU or a random one would still be correct, and in
that case there would be no way to explicitly address the first CPU.
Alex Bennée May 31, 2017, 6:16 p.m. UTC | #10
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> writes:

> On Wed, 31 May 2017 19:06:29 +0200

> Greg Kurz <groug@kaod.org> wrote:

>

>> On Wed, 31 May 2017 18:51:06 +0200

>> Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

>> [...]

>> > > >

>> > > > 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 is correct but these lines are in clone_func(). This gets called

>> for all threads but the "main" thread which I believe to be

>> associated to first_cpu.

>

> then IMHO that is a bug and it needs to be corrected. the host_tid

> should be, well, the host tid, and not 0, which is never a valid

> tid for Linux.

>

> the current behaviour is simply the easiest for the "any CPU" case.

> Picking the last CPU or a random one would still be correct, and in

> that case there would be no way to explicitly address the first CPU.


OK I'll include a fix in the next iteration.

--
Alex Bennée
Greg Kurz May 31, 2017, 6:33 p.m. UTC | #11
On Wed, 31 May 2017 19:40:46 +0200
Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> On Wed, 31 May 2017 19:06:29 +0200

> Greg Kurz <groug@kaod.org> wrote:

> 

> > On Wed, 31 May 2017 18:51:06 +0200

> > Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> wrote:

> > [...]  

> > > > > 

> > > > > 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 is correct but these lines are in clone_func(). This gets called

> > for all threads but the "main" thread which I believe to be

> > associated to first_cpu.  

> 

> then IMHO that is a bug and it needs to be corrected. the host_tid

> should be, well, the host tid, and not 0, which is never a valid

> tid for Linux.

> 


I tend to agree indeed. It isn't a problem for user mode though since it
doesn't use @host_tid. Only gdbstub does.

$ git grep host_tid
include/exec/gdbstub.h:    return cpu->host_tid;
include/qom/cpu.h: * @host_tid: Host thread ID.
include/qom/cpu.h:    uint32_t host_tid;
linux-user/syscall.c:    cpu->host_tid = info->tid;

> the current behaviour is simply the easiest for the "any CPU" case.

> Picking the last CPU or a random one would still be correct, and in

> that case there would be no way to explicitly address the first CPU.

> 


I'm not familiar enough with gdbstub to know if this is a real problem.
But I guess it is possible to add a "first_cpu->host_tid = gettid();" line
somewhere in linux-user/main.c.
diff mbox series

Patch

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;
             }