| Message ID | 20170531150933.10156-3-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | some gdbstub fixes for debug and vcont | expand |
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; > }
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.
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
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
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. > > >
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. > > > > > > >
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.
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
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.
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
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 --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; }
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