[Qemu-devel] [PATCH] fix gdbstub support for multiple threads in usermode, v2

[Nathan Froyd]

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