* [Qemu-devel] [PATCH] fix gdbstub support for multiple threads in usermode, v3
@ 2009-06-03 18:33 Nathan Froyd
2009-06-03 19:40 ` [Qemu-devel] " Jan Kiszka
2009-06-04 16:03 ` [Qemu-devel] [PATCH w/S-o-b] " Nathan Froyd
0 siblings, 2 replies; 3+ messages in thread
From: Nathan Froyd @ 2009-06-03 18:33 UTC (permalink / raw)
To: qemu-devel
When debugging multi-threaded programs, QEMU's gdb stub would report the
correct number of threads (the qfThreadInfo and qsThreadInfo packets).
However, the stub was unable to actually switch between threads (the T
packet), since it would report every thread except the first as being
dead. Furthermore, the stub relied upon cpu_index as a reliable means
of assigning IDs to the threads. This was a bad idea; if you have this
sequence of events:
initial thread created
new thread #1
new thread #2
thread #1 exits
new thread #3
thread #3 will have the same cpu_index as thread #1, which would confuse
GDB. (This problem is partly due to the remote protocol not having a
good way to send thread creation/destruction events.)
We fix this by using the host thread ID for the identifier passed to GDB
when debugging a multi-threaded userspace program. The thread ID might
wrap, but the same sort of problems with wrapping thread IDs would come
up with debugging programs natively, so this doesn't represent a
problem.
---
cpu-defs.h | 1 +
exec.c | 2 +-
gdbstub.c | 69 +++++++++++++++++++++++++++++++------------------
linux-user/syscall.c | 4 ++-
4 files changed, 49 insertions(+), 27 deletions(-)
Changes from earlier versions: use the host thread ID as the unique ID
to pass to GDB instead of inventing a new ID that tries to be robust
against wrapping.
diff --git a/cpu-defs.h b/cpu-defs.h
index 0a82197..768f164 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -207,6 +207,7 @@ typedef struct CPUWatchpoint {
\
CPUState *next_cpu; /* next CPU sharing TB cache */ \
int cpu_index; /* CPU index (informative) */ \
+ uint32_t host_tid; /* host thread ID */ \
int numa_node; /* NUMA node this cpu is belonging to */ \
int running; /* Nonzero if cpu is currently running(usermode). */ \
/* user data */ \
diff --git a/exec.c b/exec.c
index c5c9280..5b1ae47 100644
--- a/exec.c
+++ b/exec.c
@@ -550,7 +550,7 @@ void cpu_exec_init(CPUState *env)
penv = &first_cpu;
cpu_index = 0;
while (*penv != NULL) {
- penv = (CPUState **)&(*penv)->next_cpu;
+ penv = &(*penv)->next_cpu;
cpu_index++;
}
env->cpu_index = cpu_index;
diff --git a/gdbstub.c b/gdbstub.c
index 3c34741..190ff31 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1535,11 +1535,34 @@ static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
#endif
}
+static inline int gdb_id(CPUState *env)
+{
+#if defined(CONFIG_USER_ONLY) && defined(USE_NPTL)
+ return env->host_tid;
+#else
+ return env->cpu_index + 1;
+#endif
+}
+
+static CPUState *find_cpu(uint32_t thread_id)
+{
+ CPUState *env;
+
+ for (env = first_cpu; env != NULL; env = env->next_cpu) {
+ if (gdb_id(env) == thread_id) {
+ return env;
+ }
+ }
+
+ return NULL;
+}
+
static int gdb_handle_packet(GDBState *s, const char *line_buf)
{
CPUState *env;
const char *p;
- int ch, reg_size, type, res, thread;
+ uint32_t thread;
+ int ch, reg_size, type, res;
char buf[MAX_PACKET_LENGTH];
uint8_t mem_buf[MAX_PACKET_LENGTH];
uint8_t *registers;
@@ -1554,7 +1577,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
case '?':
/* TODO: Make this return the correct value for user-mode. */
snprintf(buf, sizeof(buf), "T%02xthread:%02x;", GDB_SIGNAL_TRAP,
- s->c_cpu->cpu_index+1);
+ gdb_id(s->c_cpu));
put_packet(s, buf);
/* Remove all the breakpoints when this query is issued,
* because gdb is doing and initial connect and the state
@@ -1718,9 +1741,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
put_packet(s, "OK");
break;
}
- for (env = first_cpu; env != NULL; env = env->next_cpu)
- if (env->cpu_index + 1 == thread)
- break;
+ env = find_cpu(thread);
if (env == NULL) {
put_packet(s, "E22");
break;
@@ -1741,14 +1762,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
break;
case 'T':
thread = strtoull(p, (char **)&p, 16);
-#ifndef CONFIG_USER_ONLY
- if (thread > 0 && thread < smp_cpus + 1)
-#else
- if (thread == 1)
-#endif
- put_packet(s, "OK");
- else
+ env = find_cpu(thread);
+
+ if (env != NULL) {
+ put_packet(s, "OK");
+ } else {
put_packet(s, "E22");
+ }
break;
case 'q':
case 'Q':
@@ -1786,7 +1806,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
} else if (strcmp(p,"sThreadInfo") == 0) {
report_cpuinfo:
if (s->query_cpu) {
- snprintf(buf, sizeof(buf), "m%x", s->query_cpu->cpu_index+1);
+ snprintf(buf, sizeof(buf), "m%x", gdb_id(s->query_cpu));
put_packet(s, buf);
s->query_cpu = s->query_cpu->next_cpu;
} else
@@ -1794,16 +1814,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
break;
} else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) {
thread = strtoull(p+16, (char **)&p, 16);
- for (env = first_cpu; env != NULL; env = env->next_cpu)
- if (env->cpu_index + 1 == thread) {
- cpu_synchronize_state(env, 0);
- len = snprintf((char *)mem_buf, sizeof(mem_buf),
- "CPU#%d [%s]", env->cpu_index,
- env->halted ? "halted " : "running");
- memtohex(buf, mem_buf, len);
- put_packet(s, buf);
- break;
- }
+ env = find_cpu(thread);
+ if (env != NULL) {
+ cpu_synchronize_state(env, 0);
+ len = snprintf((char *)mem_buf, sizeof(mem_buf),
+ "CPU#%d [%s]", env->cpu_index,
+ env->halted ? "halted " : "running");
+ memtohex(buf, mem_buf, len);
+ put_packet(s, buf);
+ }
break;
}
#ifdef CONFIG_USER_ONLY
@@ -1933,7 +1952,7 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
}
snprintf(buf, sizeof(buf),
"T%02xthread:%02x;%swatch:" TARGET_FMT_lx ";",
- GDB_SIGNAL_TRAP, env->cpu_index+1, type,
+ GDB_SIGNAL_TRAP, gdb_id(env), type,
env->watchpoint_hit->vaddr);
put_packet(s, buf);
env->watchpoint_hit = NULL;
@@ -1944,7 +1963,7 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
} else {
ret = GDB_SIGNAL_INT;
}
- snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, env->cpu_index+1);
+ snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, gdb_id(env));
put_packet(s, buf);
}
#endif
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0bc9902..cfa99d2 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3202,6 +3202,7 @@ static void *clone_func(void *arg)
env = info->env;
thread_env = env;
info->tid = gettid();
+ env->host_tid = info->tid;
if (info->child_tidptr)
put_user_u32(info->tid, info->child_tidptr);
if (info->parent_tidptr)
@@ -3792,6 +3793,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
/* FIXME: This probably breaks if a signal arrives. We should probably
be disabling signals. */
if (first_cpu->next_cpu) {
+ TaskState *ts;
CPUState **lastp;
CPUState *p;
@@ -3809,7 +3811,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
/* Remove the CPU from the list. */
*lastp = p->next_cpu;
cpu_list_unlock();
- TaskState *ts = ((CPUState *)cpu_env)->opaque;
+ ts = ((CPUState *)cpu_env)->opaque;
if (ts->child_tidptr) {
put_user_u32(0, ts->child_tidptr);
sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX,
--
1.6.0.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [Qemu-devel] Re: [PATCH] fix gdbstub support for multiple threads in usermode, v3
2009-06-03 18:33 [Qemu-devel] [PATCH] fix gdbstub support for multiple threads in usermode, v3 Nathan Froyd
@ 2009-06-03 19:40 ` Jan Kiszka
2009-06-04 16:03 ` [Qemu-devel] [PATCH w/S-o-b] " Nathan Froyd
1 sibling, 0 replies; 3+ messages in thread
From: Jan Kiszka @ 2009-06-03 19:40 UTC (permalink / raw)
To: Nathan Froyd; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1658 bytes --]
Nathan Froyd wrote:
> When debugging multi-threaded programs, QEMU's gdb stub would report the
> correct number of threads (the qfThreadInfo and qsThreadInfo packets).
> However, the stub was unable to actually switch between threads (the T
> packet), since it would report every thread except the first as being
> dead. Furthermore, the stub relied upon cpu_index as a reliable means
> of assigning IDs to the threads. This was a bad idea; if you have this
> sequence of events:
>
> initial thread created
> new thread #1
> new thread #2
> thread #1 exits
> new thread #3
>
> thread #3 will have the same cpu_index as thread #1, which would confuse
> GDB. (This problem is partly due to the remote protocol not having a
> good way to send thread creation/destruction events.)
>
> We fix this by using the host thread ID for the identifier passed to GDB
> when debugging a multi-threaded userspace program. The thread ID might
> wrap, but the same sort of problems with wrapping thread IDs would come
> up with debugging programs natively, so this doesn't represent a
> problem.
> ---
> cpu-defs.h | 1 +
> exec.c | 2 +-
> gdbstub.c | 69 +++++++++++++++++++++++++++++++------------------
> linux-user/syscall.c | 4 ++-
> 4 files changed, 49 insertions(+), 27 deletions(-)
>
> Changes from earlier versions: use the host thread ID as the unique ID
> to pass to GDB instead of inventing a new ID that tries to be robust
> against wrapping.
Looks good to me, has additionally some nice cleanup aspects, and
survived a quick regression test in system mode.
Jan
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH w/S-o-b] fix gdbstub support for multiple threads in usermode, v3
2009-06-03 18:33 [Qemu-devel] [PATCH] fix gdbstub support for multiple threads in usermode, v3 Nathan Froyd
2009-06-03 19:40 ` [Qemu-devel] " Jan Kiszka
@ 2009-06-04 16:03 ` Nathan Froyd
1 sibling, 0 replies; 3+ messages in thread
From: Nathan Froyd @ 2009-06-04 16:03 UTC (permalink / raw)
To: qemu-devel
When debugging multi-threaded programs, QEMU's gdb stub would report the
correct number of threads (the qfThreadInfo and qsThreadInfo packets).
However, the stub was unable to actually switch between threads (the T
packet), since it would report every thread except the first as being
dead. Furthermore, the stub relied upon cpu_index as a reliable means
of assigning IDs to the threads. This was a bad idea; if you have this
sequence of events:
initial thread created
new thread #1
new thread #2
thread #1 exits
new thread #3
thread #3 will have the same cpu_index as thread #1, which would confuse
GDB. (This problem is partly due to the remote protocol not having a
good way to send thread creation/destruction events.)
We fix this by using the host thread ID for the identifier passed to GDB
when debugging a multi-threaded userspace program. The thread ID might
wrap, but the same sort of problems with wrapping thread IDs would come
up with debugging programs natively, so this doesn't represent a
problem.
Signed-off-by: Nathan Froyd <froydnj@codesourcery.com>
---
cpu-defs.h | 1 +
exec.c | 2 +-
gdbstub.c | 69 +++++++++++++++++++++++++++++++------------------
linux-user/syscall.c | 4 ++-
4 files changed, 49 insertions(+), 27 deletions(-)
Sorry for not including Signed-off-by before. I fail at patchsubmission.
diff --git a/cpu-defs.h b/cpu-defs.h
index 0a82197..768f164 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -207,6 +207,7 @@ typedef struct CPUWatchpoint {
\
CPUState *next_cpu; /* next CPU sharing TB cache */ \
int cpu_index; /* CPU index (informative) */ \
+ uint32_t host_tid; /* host thread ID */ \
int numa_node; /* NUMA node this cpu is belonging to */ \
int running; /* Nonzero if cpu is currently running(usermode). */ \
/* user data */ \
diff --git a/exec.c b/exec.c
index c5c9280..5b1ae47 100644
--- a/exec.c
+++ b/exec.c
@@ -550,7 +550,7 @@ void cpu_exec_init(CPUState *env)
penv = &first_cpu;
cpu_index = 0;
while (*penv != NULL) {
- penv = (CPUState **)&(*penv)->next_cpu;
+ penv = &(*penv)->next_cpu;
cpu_index++;
}
env->cpu_index = cpu_index;
diff --git a/gdbstub.c b/gdbstub.c
index 3c34741..190ff31 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1535,11 +1535,34 @@ static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
#endif
}
+static inline int gdb_id(CPUState *env)
+{
+#if defined(CONFIG_USER_ONLY) && defined(USE_NPTL)
+ return env->host_tid;
+#else
+ return env->cpu_index + 1;
+#endif
+}
+
+static CPUState *find_cpu(uint32_t thread_id)
+{
+ CPUState *env;
+
+ for (env = first_cpu; env != NULL; env = env->next_cpu) {
+ if (gdb_id(env) == thread_id) {
+ return env;
+ }
+ }
+
+ return NULL;
+}
+
static int gdb_handle_packet(GDBState *s, const char *line_buf)
{
CPUState *env;
const char *p;
- int ch, reg_size, type, res, thread;
+ uint32_t thread;
+ int ch, reg_size, type, res;
char buf[MAX_PACKET_LENGTH];
uint8_t mem_buf[MAX_PACKET_LENGTH];
uint8_t *registers;
@@ -1554,7 +1577,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
case '?':
/* TODO: Make this return the correct value for user-mode. */
snprintf(buf, sizeof(buf), "T%02xthread:%02x;", GDB_SIGNAL_TRAP,
- s->c_cpu->cpu_index+1);
+ gdb_id(s->c_cpu));
put_packet(s, buf);
/* Remove all the breakpoints when this query is issued,
* because gdb is doing and initial connect and the state
@@ -1718,9 +1741,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
put_packet(s, "OK");
break;
}
- for (env = first_cpu; env != NULL; env = env->next_cpu)
- if (env->cpu_index + 1 == thread)
- break;
+ env = find_cpu(thread);
if (env == NULL) {
put_packet(s, "E22");
break;
@@ -1741,14 +1762,13 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
break;
case 'T':
thread = strtoull(p, (char **)&p, 16);
-#ifndef CONFIG_USER_ONLY
- if (thread > 0 && thread < smp_cpus + 1)
-#else
- if (thread == 1)
-#endif
- put_packet(s, "OK");
- else
+ env = find_cpu(thread);
+
+ if (env != NULL) {
+ put_packet(s, "OK");
+ } else {
put_packet(s, "E22");
+ }
break;
case 'q':
case 'Q':
@@ -1786,7 +1806,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
} else if (strcmp(p,"sThreadInfo") == 0) {
report_cpuinfo:
if (s->query_cpu) {
- snprintf(buf, sizeof(buf), "m%x", s->query_cpu->cpu_index+1);
+ snprintf(buf, sizeof(buf), "m%x", gdb_id(s->query_cpu));
put_packet(s, buf);
s->query_cpu = s->query_cpu->next_cpu;
} else
@@ -1794,16 +1814,15 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
break;
} else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) {
thread = strtoull(p+16, (char **)&p, 16);
- for (env = first_cpu; env != NULL; env = env->next_cpu)
- if (env->cpu_index + 1 == thread) {
- cpu_synchronize_state(env, 0);
- len = snprintf((char *)mem_buf, sizeof(mem_buf),
- "CPU#%d [%s]", env->cpu_index,
- env->halted ? "halted " : "running");
- memtohex(buf, mem_buf, len);
- put_packet(s, buf);
- break;
- }
+ env = find_cpu(thread);
+ if (env != NULL) {
+ cpu_synchronize_state(env, 0);
+ len = snprintf((char *)mem_buf, sizeof(mem_buf),
+ "CPU#%d [%s]", env->cpu_index,
+ env->halted ? "halted " : "running");
+ memtohex(buf, mem_buf, len);
+ put_packet(s, buf);
+ }
break;
}
#ifdef CONFIG_USER_ONLY
@@ -1933,7 +1952,7 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
}
snprintf(buf, sizeof(buf),
"T%02xthread:%02x;%swatch:" TARGET_FMT_lx ";",
- GDB_SIGNAL_TRAP, env->cpu_index+1, type,
+ GDB_SIGNAL_TRAP, gdb_id(env), type,
env->watchpoint_hit->vaddr);
put_packet(s, buf);
env->watchpoint_hit = NULL;
@@ -1944,7 +1963,7 @@ static void gdb_vm_state_change(void *opaque, int running, int reason)
} else {
ret = GDB_SIGNAL_INT;
}
- snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, env->cpu_index+1);
+ snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, gdb_id(env));
put_packet(s, buf);
}
#endif
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0bc9902..cfa99d2 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3202,6 +3202,7 @@ static void *clone_func(void *arg)
env = info->env;
thread_env = env;
info->tid = gettid();
+ env->host_tid = info->tid;
if (info->child_tidptr)
put_user_u32(info->tid, info->child_tidptr);
if (info->parent_tidptr)
@@ -3792,6 +3793,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
/* FIXME: This probably breaks if a signal arrives. We should probably
be disabling signals. */
if (first_cpu->next_cpu) {
+ TaskState *ts;
CPUState **lastp;
CPUState *p;
@@ -3809,7 +3811,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
/* Remove the CPU from the list. */
*lastp = p->next_cpu;
cpu_list_unlock();
- TaskState *ts = ((CPUState *)cpu_env)->opaque;
+ ts = ((CPUState *)cpu_env)->opaque;
if (ts->child_tidptr) {
put_user_u32(0, ts->child_tidptr);
sys_futex(g2h(ts->child_tidptr), FUTEX_WAKE, INT_MAX,
--
1.6.3.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2009-06-04 23:52 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-03 18:33 [Qemu-devel] [PATCH] fix gdbstub support for multiple threads in usermode, v3 Nathan Froyd
2009-06-03 19:40 ` [Qemu-devel] " Jan Kiszka
2009-06-04 16:03 ` [Qemu-devel] [PATCH w/S-o-b] " Nathan Froyd
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.