| Message ID | 20170601144915.20778-4-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | some gdbstub fixes for debug and vcont | expand |
On Thu, 1 Jun 2017 15:49:14 +0100 Alex Bennée <alex.bennee@linaro.org> wrote: > This was only used by the gdbstub and even then was only being set for > subsequent threads. Rather the continue duplicating the number just > make the gdbstub get the information from TaskState structure. > > Now the tid is correctly reported for all threads the bug I was seeing > with "vCont;C04:0;c" packets is fixed as the correct tid is reported > to gdb. > > I moved cpu_gdb_index into the gdbstub to facilitate easy access to > the TaskState which is used elsewhere in gdbstub. > FWIW, this change would make more sense in patch 2 since all users are in gdbstub.c and it would avoid to change things twice. No big deal compared to the benefit of dropping the broken @host_tid :) > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- In any case. Reviewed-by: Greg Kurz <groug@kaod.org> > gdbstub.c | 15 +++++++++++++++ > include/exec/gdbstub.h | 14 -------------- > include/qom/cpu.h | 2 -- > linux-user/syscall.c | 1 - > 4 files changed, 15 insertions(+), 17 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index 026d1fe6bb..45a3a0b16b 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -55,6 +55,21 @@ static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr, > return cpu_memory_rw_debug(cpu, addr, buf, len, is_write); > } > > +/* Return the GDB index for a given vCPU state. > + * > + * For user mode this is simply the thread id. In system mode GDB > + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index. > + */ > +static inline int cpu_gdb_index(CPUState *cpu) > +{ > +#if defined(CONFIG_USER_ONLY) > + TaskState *ts = (TaskState *) cpu->opaque; > + return ts->ts_tid; > +#else > + return cpu->cpu_index + 1; > +#endif > +} > + > enum { > GDB_SIGNAL_0 = 0, > GDB_SIGNAL_INT = 2, > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h > index c4fe567600..9aa7756d92 100644 > --- a/include/exec/gdbstub.h > +++ b/include/exec/gdbstub.h > @@ -58,20 +58,6 @@ void gdb_register_coprocessor(CPUState *cpu, > gdb_reg_cb get_reg, gdb_reg_cb set_reg, > int num_regs, const char *xml, int g_pos); > > -/* Return the GDB index for a given vCPU state. > - * > - * For user mode this is simply the thread id. In system mode GDB > - * numbers CPUs from 1 as 0 is reserved as an "any cpu" index. > - */ > -static inline int cpu_gdb_index(CPUState *cpu) > -{ > -#if defined(CONFIG_USER_ONLY) > - return cpu->host_tid; > -#else > - return cpu->cpu_index + 1; > -#endif > -} > - > /* The GDB remote protocol transfers values in target byte order. This means > * we can use the raw memory access routines to access the value buffer. > * Conveniently, these also handle the case where the buffer is mis-aligned. > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 55214ce131..909e7ae994 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -266,7 +266,6 @@ struct qemu_work_item; > * @nr_cores: Number of cores within this CPU package. > * @nr_threads: Number of threads within this CPU. > * @numa_node: NUMA node this CPU is belonging to. > - * @host_tid: Host thread ID. > * @running: #true if CPU is currently running (lockless). > * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end; > * valid under cpu_list_lock. > @@ -321,7 +320,6 @@ struct CPUState { > HANDLE hThread; > #endif > int thread_id; > - uint32_t host_tid; > bool running, has_waiter; > struct QemuCond *halt_cond; > bool thread_kicked; > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index cec8428589..cada188e58 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -6216,7 +6216,6 @@ static void *clone_func(void *arg) > thread_cpu = cpu; > ts = (TaskState *)cpu->opaque; > info->tid = gettid(); > - cpu->host_tid = info->tid; > task_settid(ts); > if (info->child_tidptr) > put_user_u32(info->tid, info->child_tidptr);
On 06/01/2017 11:49 AM, Alex Bennée wrote: > This was only used by the gdbstub and even then was only being set for > subsequent threads. Rather the continue duplicating the number just > make the gdbstub get the information from TaskState structure. > > Now the tid is correctly reported for all threads the bug I was seeing > with "vCont;C04:0;c" packets is fixed as the correct tid is reported > to gdb. > > I moved cpu_gdb_index into the gdbstub to facilitate easy access to > the TaskState which is used elsewhere in gdbstub. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > gdbstub.c | 15 +++++++++++++++ > include/exec/gdbstub.h | 14 -------------- > include/qom/cpu.h | 2 -- > linux-user/syscall.c | 1 - > 4 files changed, 15 insertions(+), 17 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index 026d1fe6bb..45a3a0b16b 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -55,6 +55,21 @@ static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr, > return cpu_memory_rw_debug(cpu, addr, buf, len, is_write); > } > > +/* Return the GDB index for a given vCPU state. > + * > + * For user mode this is simply the thread id. In system mode GDB > + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index. > + */ > +static inline int cpu_gdb_index(CPUState *cpu) > +{ > +#if defined(CONFIG_USER_ONLY) > + TaskState *ts = (TaskState *) cpu->opaque; > + return ts->ts_tid; > +#else > + return cpu->cpu_index + 1; > +#endif > +} > + > enum { > GDB_SIGNAL_0 = 0, > GDB_SIGNAL_INT = 2, > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h > index c4fe567600..9aa7756d92 100644 > --- a/include/exec/gdbstub.h > +++ b/include/exec/gdbstub.h > @@ -58,20 +58,6 @@ void gdb_register_coprocessor(CPUState *cpu, > gdb_reg_cb get_reg, gdb_reg_cb set_reg, > int num_regs, const char *xml, int g_pos); > > -/* Return the GDB index for a given vCPU state. > - * > - * For user mode this is simply the thread id. In system mode GDB > - * numbers CPUs from 1 as 0 is reserved as an "any cpu" index. > - */ > -static inline int cpu_gdb_index(CPUState *cpu) > -{ > -#if defined(CONFIG_USER_ONLY) > - return cpu->host_tid; > -#else > - return cpu->cpu_index + 1; > -#endif > -} > - > /* The GDB remote protocol transfers values in target byte order. This means > * we can use the raw memory access routines to access the value buffer. > * Conveniently, these also handle the case where the buffer is mis-aligned. > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 55214ce131..909e7ae994 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -266,7 +266,6 @@ struct qemu_work_item; > * @nr_cores: Number of cores within this CPU package. > * @nr_threads: Number of threads within this CPU. > * @numa_node: NUMA node this CPU is belonging to. > - * @host_tid: Host thread ID. > * @running: #true if CPU is currently running (lockless). > * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end; > * valid under cpu_list_lock. > @@ -321,7 +320,6 @@ struct CPUState { > HANDLE hThread; > #endif > int thread_id; > - uint32_t host_tid; > bool running, has_waiter; > struct QemuCond *halt_cond; > bool thread_kicked; > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index cec8428589..cada188e58 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -6216,7 +6216,6 @@ static void *clone_func(void *arg) > thread_cpu = cpu; > ts = (TaskState *)cpu->opaque; > info->tid = gettid(); > - cpu->host_tid = info->tid; > task_settid(ts); > if (info->child_tidptr) > put_user_u32(info->tid, info->child_tidptr); >
On Thu, 1 Jun 2017 17:32:13 +0200 Greg Kurz <groug@kaod.org> wrote: > On Thu, 1 Jun 2017 15:49:14 +0100 > Alex Bennée <alex.bennee@linaro.org> wrote: > > > This was only used by the gdbstub and even then was only being set > > for subsequent threads. Rather the continue duplicating the number > > just make the gdbstub get the information from TaskState structure. > > > > Now the tid is correctly reported for all threads the bug I was > > seeing with "vCont;C04:0;c" packets is fixed as the correct tid is > > reported to gdb. > > > > I moved cpu_gdb_index into the gdbstub to facilitate easy access to > > the TaskState which is used elsewhere in gdbstub. > > > > FWIW, this change would make more sense in patch 2 since all users > are in gdbstub.c and it would avoid to change things twice. No big > deal compared to the benefit of dropping the broken @host_tid :) I agree with this > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > > In any case. > > Reviewed-by: Greg Kurz <groug@kaod.org> and me too Reviewed-by: Claudio Imbrenda <imbrenda@linux.vnet.ibm.com> > > gdbstub.c | 15 +++++++++++++++ > > include/exec/gdbstub.h | 14 -------------- > > include/qom/cpu.h | 2 -- > > linux-user/syscall.c | 1 - > > 4 files changed, 15 insertions(+), 17 deletions(-) > > > > diff --git a/gdbstub.c b/gdbstub.c > > index 026d1fe6bb..45a3a0b16b 100644 > > --- a/gdbstub.c > > +++ b/gdbstub.c > > @@ -55,6 +55,21 @@ static inline int > > target_memory_rw_debug(CPUState *cpu, target_ulong addr, return > > cpu_memory_rw_debug(cpu, addr, buf, len, is_write); } > > > > +/* Return the GDB index for a given vCPU state. > > + * > > + * For user mode this is simply the thread id. In system mode GDB > > + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index. > > + */ > > +static inline int cpu_gdb_index(CPUState *cpu) > > +{ > > +#if defined(CONFIG_USER_ONLY) > > + TaskState *ts = (TaskState *) cpu->opaque; > > + return ts->ts_tid; > > +#else > > + return cpu->cpu_index + 1; > > +#endif > > +} > > + > > enum { > > GDB_SIGNAL_0 = 0, > > GDB_SIGNAL_INT = 2, > > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h > > index c4fe567600..9aa7756d92 100644 > > --- a/include/exec/gdbstub.h > > +++ b/include/exec/gdbstub.h > > @@ -58,20 +58,6 @@ void gdb_register_coprocessor(CPUState *cpu, > > gdb_reg_cb get_reg, gdb_reg_cb > > set_reg, int num_regs, const char *xml, int g_pos); > > > > -/* Return the GDB index for a given vCPU state. > > - * > > - * For user mode this is simply the thread id. In system mode GDB > > - * numbers CPUs from 1 as 0 is reserved as an "any cpu" index. > > - */ > > -static inline int cpu_gdb_index(CPUState *cpu) > > -{ > > -#if defined(CONFIG_USER_ONLY) > > - return cpu->host_tid; > > -#else > > - return cpu->cpu_index + 1; > > -#endif > > -} > > - > > /* The GDB remote protocol transfers values in target byte order. > > This means > > * we can use the raw memory access routines to access the value > > buffer. > > * Conveniently, these also handle the case where the buffer is > > mis-aligned. diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > index 55214ce131..909e7ae994 100644 > > --- a/include/qom/cpu.h > > +++ b/include/qom/cpu.h > > @@ -266,7 +266,6 @@ struct qemu_work_item; > > * @nr_cores: Number of cores within this CPU package. > > * @nr_threads: Number of threads within this CPU. > > * @numa_node: NUMA node this CPU is belonging to. > > - * @host_tid: Host thread ID. > > * @running: #true if CPU is currently running (lockless). > > * @has_waiter: #true if a CPU is currently waiting for the > > cpu_exec_end; > > * valid under cpu_list_lock. > > @@ -321,7 +320,6 @@ struct CPUState { > > HANDLE hThread; > > #endif > > int thread_id; > > - uint32_t host_tid; > > bool running, has_waiter; > > struct QemuCond *halt_cond; > > bool thread_kicked; > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > > index cec8428589..cada188e58 100644 > > --- a/linux-user/syscall.c > > +++ b/linux-user/syscall.c > > @@ -6216,7 +6216,6 @@ static void *clone_func(void *arg) > > thread_cpu = cpu; > > ts = (TaskState *)cpu->opaque; > > info->tid = gettid(); > > - cpu->host_tid = info->tid; > > task_settid(ts); > > if (info->child_tidptr) > > put_user_u32(info->tid, info->child_tidptr); >
Le 01/06/2017 à 16:49, Alex Bennée a écrit : > This was only used by the gdbstub and even then was only being set for > subsequent threads. Rather the continue duplicating the number just > make the gdbstub get the information from TaskState structure. > > Now the tid is correctly reported for all threads the bug I was seeing > with "vCont;C04:0;c" packets is fixed as the correct tid is reported > to gdb. > > I moved cpu_gdb_index into the gdbstub to facilitate easy access to > the TaskState which is used elsewhere in gdbstub. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Reviewed-by: Laurent Vivier <laurent@vivier.eu> > --- > gdbstub.c | 15 +++++++++++++++ > include/exec/gdbstub.h | 14 -------------- > include/qom/cpu.h | 2 -- > linux-user/syscall.c | 1 - > 4 files changed, 15 insertions(+), 17 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index 026d1fe6bb..45a3a0b16b 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -55,6 +55,21 @@ static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr, > return cpu_memory_rw_debug(cpu, addr, buf, len, is_write); > } > > +/* Return the GDB index for a given vCPU state. > + * > + * For user mode this is simply the thread id. In system mode GDB > + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index. > + */ > +static inline int cpu_gdb_index(CPUState *cpu) > +{ > +#if defined(CONFIG_USER_ONLY) > + TaskState *ts = (TaskState *) cpu->opaque; > + return ts->ts_tid; > +#else > + return cpu->cpu_index + 1; > +#endif > +} > + > enum { > GDB_SIGNAL_0 = 0, > GDB_SIGNAL_INT = 2, > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h > index c4fe567600..9aa7756d92 100644 > --- a/include/exec/gdbstub.h > +++ b/include/exec/gdbstub.h > @@ -58,20 +58,6 @@ void gdb_register_coprocessor(CPUState *cpu, > gdb_reg_cb get_reg, gdb_reg_cb set_reg, > int num_regs, const char *xml, int g_pos); > > -/* Return the GDB index for a given vCPU state. > - * > - * For user mode this is simply the thread id. In system mode GDB > - * numbers CPUs from 1 as 0 is reserved as an "any cpu" index. > - */ > -static inline int cpu_gdb_index(CPUState *cpu) > -{ > -#if defined(CONFIG_USER_ONLY) > - return cpu->host_tid; > -#else > - return cpu->cpu_index + 1; > -#endif > -} > - > /* The GDB remote protocol transfers values in target byte order. This means > * we can use the raw memory access routines to access the value buffer. > * Conveniently, these also handle the case where the buffer is mis-aligned. > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 55214ce131..909e7ae994 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -266,7 +266,6 @@ struct qemu_work_item; > * @nr_cores: Number of cores within this CPU package. > * @nr_threads: Number of threads within this CPU. > * @numa_node: NUMA node this CPU is belonging to. > - * @host_tid: Host thread ID. > * @running: #true if CPU is currently running (lockless). > * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end; > * valid under cpu_list_lock. > @@ -321,7 +320,6 @@ struct CPUState { > HANDLE hThread; > #endif > int thread_id; > - uint32_t host_tid; > bool running, has_waiter; > struct QemuCond *halt_cond; > bool thread_kicked; > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index cec8428589..cada188e58 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -6216,7 +6216,6 @@ static void *clone_func(void *arg) > thread_cpu = cpu; > ts = (TaskState *)cpu->opaque; > info->tid = gettid(); > - cpu->host_tid = info->tid; > task_settid(ts); > if (info->child_tidptr) > put_user_u32(info->tid, info->child_tidptr); >
diff --git a/gdbstub.c b/gdbstub.c index 026d1fe6bb..45a3a0b16b 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -55,6 +55,21 @@ static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr, return cpu_memory_rw_debug(cpu, addr, buf, len, is_write); } +/* Return the GDB index for a given vCPU state. + * + * For user mode this is simply the thread id. In system mode GDB + * numbers CPUs from 1 as 0 is reserved as an "any cpu" index. + */ +static inline int cpu_gdb_index(CPUState *cpu) +{ +#if defined(CONFIG_USER_ONLY) + TaskState *ts = (TaskState *) cpu->opaque; + return ts->ts_tid; +#else + return cpu->cpu_index + 1; +#endif +} + enum { GDB_SIGNAL_0 = 0, GDB_SIGNAL_INT = 2, diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h index c4fe567600..9aa7756d92 100644 --- a/include/exec/gdbstub.h +++ b/include/exec/gdbstub.h @@ -58,20 +58,6 @@ void gdb_register_coprocessor(CPUState *cpu, gdb_reg_cb get_reg, gdb_reg_cb set_reg, int num_regs, const char *xml, int g_pos); -/* Return the GDB index for a given vCPU state. - * - * For user mode this is simply the thread id. In system mode GDB - * numbers CPUs from 1 as 0 is reserved as an "any cpu" index. - */ -static inline int cpu_gdb_index(CPUState *cpu) -{ -#if defined(CONFIG_USER_ONLY) - return cpu->host_tid; -#else - return cpu->cpu_index + 1; -#endif -} - /* The GDB remote protocol transfers values in target byte order. This means * we can use the raw memory access routines to access the value buffer. * Conveniently, these also handle the case where the buffer is mis-aligned. diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 55214ce131..909e7ae994 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -266,7 +266,6 @@ struct qemu_work_item; * @nr_cores: Number of cores within this CPU package. * @nr_threads: Number of threads within this CPU. * @numa_node: NUMA node this CPU is belonging to. - * @host_tid: Host thread ID. * @running: #true if CPU is currently running (lockless). * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end; * valid under cpu_list_lock. @@ -321,7 +320,6 @@ struct CPUState { HANDLE hThread; #endif int thread_id; - uint32_t host_tid; bool running, has_waiter; struct QemuCond *halt_cond; bool thread_kicked; diff --git a/linux-user/syscall.c b/linux-user/syscall.c index cec8428589..cada188e58 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -6216,7 +6216,6 @@ static void *clone_func(void *arg) thread_cpu = cpu; ts = (TaskState *)cpu->opaque; info->tid = gettid(); - cpu->host_tid = info->tid; task_settid(ts); if (info->child_tidptr) put_user_u32(info->tid, info->child_tidptr);
This was only used by the gdbstub and even then was only being set for subsequent threads. Rather the continue duplicating the number just make the gdbstub get the information from TaskState structure. Now the tid is correctly reported for all threads the bug I was seeing with "vCont;C04:0;c" packets is fixed as the correct tid is reported to gdb. I moved cpu_gdb_index into the gdbstub to facilitate easy access to the TaskState which is used elsewhere in gdbstub. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- gdbstub.c | 15 +++++++++++++++ include/exec/gdbstub.h | 14 -------------- include/qom/cpu.h | 2 -- linux-user/syscall.c | 1 - 4 files changed, 15 insertions(+), 17 deletions(-) -- 2.13.0