Re: [Qemu-devel] [PATCH w/S-o-b] fix gdbstub support for multiple threads in usermode, v3

[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.  (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 <address@hidden>
---
 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