From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MBa3i-0006K8-Kp for qemu-devel@nongnu.org; Tue, 02 Jun 2009 15:54:06 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MBa3e-0006Ew-8U for qemu-devel@nongnu.org; Tue, 02 Jun 2009 15:54:06 -0400 Received: from [199.232.76.173] (port=60716 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MBa3e-0006EX-1V for qemu-devel@nongnu.org; Tue, 02 Jun 2009 15:54:02 -0400 Received: from mx20.gnu.org ([199.232.41.8]:63238) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MBa3d-00037b-CN for qemu-devel@nongnu.org; Tue, 02 Jun 2009 15:54:01 -0400 Received: from mail.codesourcery.com ([65.74.133.4]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MBa3b-0002wk-FF for qemu-devel@nongnu.org; Tue, 02 Jun 2009 15:53:59 -0400 From: Nathan Froyd Date: Tue, 2 Jun 2009 12:53:49 -0700 Message-Id: <1243972429-7972-1-git-send-email-froydnj@codesourcery.com> Subject: [Qemu-devel] [PATCH] fix gdbstub support for multiple threads in usermode, v2 List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org 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. We fix this by adding a stable gdbstub_index field for each CPU. To support the possibility of billions upon billions of threads being created, we maintain the cpu list in sorted order by the new field. Instead of using the next_cpu field to thread the linked list, we introduce a separate field for this purpose. Once we have a stable gdbstub_index field, the stub needs to use this field instead of cpu_index for communicating with GDB. --- cpu-defs.h | 2 + exec-all.h | 1 + exec.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++- gdbstub.c | 30 ++++++++++++++--------- linux-user/syscall.c | 22 +++-------------- 5 files changed, 85 insertions(+), 31 deletions(-) Changes from v1: Instead of maintaining a `gdbstub_next_index' variable, we simply maintain a list sorted by gdbstub_index and walk through it to discover what the next index should be. This is slightly slower, but more robust. I considered using a bitmap to track free IDs, but doing that so it wasn't a memory hog for O(small # of threads) cases seems more complicated than this approach. Code from linux-user has also been moved to exec.c; it doesn't properly belong in linux-user and will likely be needed by other usermode emulators or something like cpu hotplugging. diff --git a/cpu-defs.h b/cpu-defs.h index 0a82197..fa89ff2 100644 --- a/cpu-defs.h +++ b/cpu-defs.h @@ -206,7 +206,9 @@ typedef struct CPUWatchpoint { int exception_index; \ \ CPUState *next_cpu; /* next CPU sharing TB cache */ \ + CPUState *gdbstub_next_cpu; /* next CPU, sorted by gdbstub_index */ \ int cpu_index; /* CPU index (informative) */ \ + uint32_t gdbstub_index; /* index for gdbstub T and H packets */ \ 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-all.h b/exec-all.h index f91e646..21cd7c0 100644 --- a/exec-all.h +++ b/exec-all.h @@ -80,6 +80,7 @@ TranslationBlock *tb_gen_code(CPUState *env, target_ulong pc, target_ulong cs_base, int flags, int cflags); void cpu_exec_init(CPUState *env); +void cpu_exec_fini(CPUState *env); void QEMU_NORETURN cpu_loop_exit(void); int page_unprotect(target_ulong address, unsigned long pc, void *puc); void tb_invalidate_phys_page_range(target_phys_addr_t start, target_phys_addr_t end, diff --git a/exec.c b/exec.c index c5c9280..07b7298 100644 --- a/exec.c +++ b/exec.c @@ -126,6 +126,7 @@ ram_addr_t last_ram_offset; #endif CPUState *first_cpu; +static CPUState *first_gdbstub_cpu; /* current CPU in the current thread. It is only valid inside cpu_exec() */ CPUState *cpu_single_env; @@ -538,6 +539,33 @@ static int cpu_common_load(QEMUFile *f, void *opaque, int version_id) } #endif +/* Assign a stable index for the gdb stub. The CPU list is kept in + sorted, increasing order by the gdbstub_next_cpu field, keyed on the + gdbstub_index field. */ +static void assign_gdbstub_index(CPUState *env) +{ + CPUState **penv; + CPUState **prev; + uint32_t next_index; + + env->gdbstub_next_cpu = NULL; + penv = &first_gdbstub_cpu; + prev = penv; + next_index = 0; + while (1) { + if (*penv == NULL || (*penv)->gdbstub_index != next_index) { + env->gdbstub_index = next_index; + env->gdbstub_next_cpu = (*penv); + *prev = env; + break; + } else { + next_index++; + } + prev = penv; + penv = &(*penv)->gdbstub_next_cpu; + } +} + void cpu_exec_init(CPUState *env) { CPUState **penv; @@ -550,10 +578,11 @@ 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; + assign_gdbstub_index(env); env->numa_node = 0; TAILQ_INIT(&env->breakpoints); TAILQ_INIT(&env->watchpoints); @@ -569,6 +598,35 @@ void cpu_exec_init(CPUState *env) #endif } +#if defined(CONFIG_USER_ONLY) +void cpu_exec_fini(CPUState *env) +{ + CPUState **lastp; + CPUState *p; + + cpu_list_lock(); +#define FROB(first, field) \ + do { \ + lastp = &first; \ + p = first; \ + while (p && p != env) { \ + lastp = &p->field; \ + p = p->field; \ + } \ + /* CPU not found? That's not good. */ \ + if (!p) \ + abort(); \ + /* Remove the CPU from the list. */ \ + *lastp = p->next_cpu; \ + } while (0) + + FROB(first_cpu, next_cpu); + FROB(first_gdbstub_cpu, gdbstub_next_cpu); +#undef FROB + cpu_list_unlock(); +} +#endif + static inline void invalidate_page_bitmap(PageDesc *p) { if (p->code_bitmap) { @@ -1711,6 +1769,7 @@ CPUState *cpu_copy(CPUState *env) wp->flags, NULL); } #endif + assign_gdbstub_index(new_env); return new_env; } diff --git a/gdbstub.c b/gdbstub.c index 3c34741..1930e7b 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -1718,9 +1718,11 @@ 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) + for (env = first_cpu; env != NULL; env = env->next_cpu) { + if (env->gdbstub_index == thread) { break; + } + } if (env == NULL) { put_packet(s, "E22"); break; @@ -1741,14 +1743,17 @@ 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 + for (env = first_cpu; env != NULL; env = env->next_cpu) { + if (env->gdbstub_index == thread) { + break; + } + } + + if (env != NULL) { + put_packet(s, "OK"); + } else { put_packet(s, "E22"); + } break; case 'q': case 'Q': @@ -1786,7 +1791,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", s->query_cpu->gdbstub_index); put_packet(s, buf); s->query_cpu = s->query_cpu->next_cpu; } else @@ -1794,8 +1799,8 @@ 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) { + for (env = first_cpu; env != NULL; env = env->next_cpu) { + if (env->gdbstub_index == thread) { cpu_synchronize_state(env, 0); len = snprintf((char *)mem_buf, sizeof(mem_buf), "CPU#%d [%s]", env->cpu_index, @@ -1804,6 +1809,7 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) put_packet(s, buf); break; } + } break; } #ifdef CONFIG_USER_ONLY diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 0bc9902..763d6a3 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -3792,24 +3792,10 @@ 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) { - CPUState **lastp; - CPUState *p; - - cpu_list_lock(); - lastp = &first_cpu; - p = first_cpu; - while (p && p != (CPUState *)cpu_env) { - lastp = &p->next_cpu; - p = p->next_cpu; - } - /* If we didn't find the CPU for this thread then something is - horribly wrong. */ - if (!p) - abort(); - /* Remove the CPU from the list. */ - *lastp = p->next_cpu; - cpu_list_unlock(); - TaskState *ts = ((CPUState *)cpu_env)->opaque; + TaskState *ts; + + cpu_exec_fini((CPUState *)cpu_env); + 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