[Qemu-devel] [5743] gdbstub: manage CPUs as threads (Jan Kiszka)

[Qemu-devel] [5743] gdbstub: manage CPUs as threads (Jan Kiszka)

[Anthony Liguori]

Revision: 5743
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5743
Author:   aliguori
Date:     2008-11-18 20:30:24 +0000 (Tue, 18 Nov 2008)

Log Message:
-----------
gdbstub: manage CPUs as threads (Jan Kiszka)

This patch enhances QEMU's built-in debugger for SMP guest debugging.
Using the thread support of the gdb remote protocol, each VCPU is mapped
on a pseudo thread and exposed to the gdb frontend. This way you can
easy switch the focus of gdb between the VCPUs and observe their states.
On breakpoint hit, the focus is automatically adjusted just as for
normal multi-threaded application under gdb control.

Furthermore, the patch propagates breakpoint and watchpoint insertions
or removals to all CPUs, not just the current one as it was the case so
far. Without this, SMP guest debugging was practically unfeasible.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Modified Paths:
--------------
    trunk/gdbstub.c
    trunk/gdbstub.h
    trunk/vl.c

Modified: trunk/gdbstub.c
===================================================================
--- trunk/gdbstub.c	2008-11-18 20:26:41 UTC (rev 5742)
+++ trunk/gdbstub.c	2008-11-18 20:30:24 UTC (rev 5743)
@@ -69,7 +69,9 @@
     RS_SYSCALL,
 };
 typedef struct GDBState {
-    CPUState *env; /* current CPU */
+    CPUState *c_cpu; /* current CPU for step/continue ops */
+    CPUState *g_cpu; /* current CPU for other ops */
+    CPUState *query_cpu; /* for q{f|s}ThreadInfo */
     enum RSState state; /* parsing state */
     char line_buf[MAX_PACKET_LENGTH];
     int line_buf_index;
@@ -90,6 +92,8 @@
  */
 static int sstep_flags = SSTEP_ENABLE|SSTEP_NOIRQ|SSTEP_NOTIMER;
 
+static GDBState *gdbserver_state;
+
 /* This is an ugly hack to cope with both new and old gdb.
    If gdb sends qXfer:features:read then assume we're talking to a newish
    gdb that understands target descriptions.  */
@@ -99,9 +103,6 @@
 /* XXX: This is not thread safe.  Do we care?  */
 static int gdbserver_fd = -1;
 
-/* XXX: remove this hack.  */
-static GDBState gdbserver_state;
-
 static int get_char(GDBState *s)
 {
     uint8_t ch;
@@ -126,8 +127,6 @@
 }
 #endif
 
-/* GDB stub state for use by semihosting syscalls.  */
-static GDBState *gdb_syscall_state;
 static gdb_syscall_complete_cb gdb_current_syscall_cb;
 
 enum {
@@ -141,8 +140,8 @@
 int use_gdb_syscalls(void)
 {
     if (gdb_syscall_mode == GDB_SYS_UNKNOWN) {
-        gdb_syscall_mode = (gdb_syscall_state ? GDB_SYS_ENABLED
-                                              : GDB_SYS_DISABLED);
+        gdb_syscall_mode = (gdbserver_state ? GDB_SYS_ENABLED
+                                            : GDB_SYS_DISABLED);
     }
     return gdb_syscall_mode == GDB_SYS_ENABLED;
 }
@@ -1031,7 +1030,7 @@
     return p - buf;
 }
 
-const char *get_feature_xml(CPUState *env, const char *p, const char **newp)
+const char *get_feature_xml(const char *p, const char **newp)
 {
     extern const char *const xml_builtin[][2];
     size_t len;
@@ -1057,7 +1056,7 @@
                      "<xi:include href=\"%s\"/>",
                      GDB_CORE_XML);
 
-            for (r = env->gdb_regs; r; r = r->next) {
+            for (r = first_cpu->gdb_regs; r; r = r->next) {
                 strcat(target_xml, "<xi:include href=\"");
                 strcat(target_xml, r->xml);
                 strcat(target_xml, "\"/>");
@@ -1160,55 +1159,84 @@
 };
 #endif
 
-static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
-                                 target_ulong len, int type)
+static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int type)
 {
+    CPUState *env;
+    int err = 0;
+
     switch (type) {
     case GDB_BREAKPOINT_SW:
     case GDB_BREAKPOINT_HW:
-        return cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_breakpoint_insert(env, addr, BP_GDB, NULL);
+            if (err)
+                break;
+        }
+        return err;
 #ifndef CONFIG_USER_ONLY
     case GDB_WATCHPOINT_WRITE:
     case GDB_WATCHPOINT_READ:
     case GDB_WATCHPOINT_ACCESS:
-        return cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
-                                     NULL);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_watchpoint_insert(env, addr, len, xlat_gdb_type[type],
+                                        NULL);
+            if (err)
+                break;
+        }
+        return err;
 #endif
     default:
         return -ENOSYS;
     }
 }
 
-static int gdb_breakpoint_remove(CPUState *env, target_ulong addr,
-                                 target_ulong len, int type)
+static int gdb_breakpoint_remove(target_ulong addr, target_ulong len, int type)
 {
+    CPUState *env;
+    int err = 0;
+
     switch (type) {
     case GDB_BREAKPOINT_SW:
     case GDB_BREAKPOINT_HW:
-        return cpu_breakpoint_remove(env, addr, BP_GDB);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_breakpoint_remove(env, addr, BP_GDB);
+            if (err)
+                break;
+        }
+        return err;
 #ifndef CONFIG_USER_ONLY
     case GDB_WATCHPOINT_WRITE:
     case GDB_WATCHPOINT_READ:
     case GDB_WATCHPOINT_ACCESS:
-        return cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+        for (env = first_cpu; env != NULL; env = env->next_cpu) {
+            err = cpu_watchpoint_remove(env, addr, len, xlat_gdb_type[type]);
+            if (err)
+                break;
+        }
+        return err;
 #endif
     default:
         return -ENOSYS;
     }
 }
 
-static void gdb_breakpoint_remove_all(CPUState *env)
+static void gdb_breakpoint_remove_all(void)
 {
-    cpu_breakpoint_remove_all(env, BP_GDB);
+    CPUState *env;
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        cpu_breakpoint_remove_all(env, BP_GDB);
 #ifndef CONFIG_USER_ONLY
-    cpu_watchpoint_remove_all(env, BP_GDB);
+        cpu_watchpoint_remove_all(env, BP_GDB);
 #endif
+    }
 }
 
-static int gdb_handle_packet(GDBState *s, CPUState *env, const char *line_buf)
+static int gdb_handle_packet(GDBState *s, const char *line_buf)
 {
+    CPUState *env;
     const char *p;
-    int ch, reg_size, type, res;
+    int ch, reg_size, type, res, thread;
     char buf[MAX_PACKET_LENGTH];
     uint8_t mem_buf[MAX_PACKET_LENGTH];
     uint8_t *registers;
@@ -1222,32 +1250,33 @@
     switch(ch) {
     case '?':
         /* TODO: Make this return the correct value for user-mode.  */
-        snprintf(buf, sizeof(buf), "S%02x", SIGTRAP);
+        snprintf(buf, sizeof(buf), "T%02xthread:%02x;", SIGTRAP,
+                 s->c_cpu->cpu_index+1);
         put_packet(s, buf);
         /* Remove all the breakpoints when this query is issued,
          * because gdb is doing and initial connect and the state
          * should be cleaned up.
          */
-        gdb_breakpoint_remove_all(env);
+        gdb_breakpoint_remove_all();
         break;
     case 'c':
         if (*p != '\0') {
             addr = strtoull(p, (char **)&p, 16);
 #if defined(TARGET_I386)
-            env->eip = addr;
+            s->c_cpu->eip = addr;
 #elif defined (TARGET_PPC)
-            env->nip = addr;
+            s->c_cpu->nip = addr;
 #elif defined (TARGET_SPARC)
-            env->pc = addr;
-            env->npc = addr + 4;
+            s->c_cpu->pc = addr;
+            s->c_cpu->npc = addr + 4;
 #elif defined (TARGET_ARM)
-            env->regs[15] = addr;
+            s->c_cpu->regs[15] = addr;
 #elif defined (TARGET_SH4)
-            env->pc = addr;
+            s->c_cpu->pc = addr;
 #elif defined (TARGET_MIPS)
-            env->active_tc.PC = addr;
+            s->c_cpu->active_tc.PC = addr;
 #elif defined (TARGET_CRIS)
-            env->pc = addr;
+            s->c_cpu->pc = addr;
 #endif
         }
         gdb_continue(s);
@@ -1262,7 +1291,7 @@
         exit(0);
     case 'D':
         /* Detach packet */
-        gdb_breakpoint_remove_all(env);
+        gdb_breakpoint_remove_all();
         gdb_continue(s);
         put_packet(s, "OK");
         break;
@@ -1270,23 +1299,23 @@
         if (*p != '\0') {
             addr = strtoull(p, (char **)&p, 16);
 #if defined(TARGET_I386)
-            env->eip = addr;
+            s->c_cpu->eip = addr;
 #elif defined (TARGET_PPC)
-            env->nip = addr;
+            s->c_cpu->nip = addr;
 #elif defined (TARGET_SPARC)
-            env->pc = addr;
-            env->npc = addr + 4;
+            s->c_cpu->pc = addr;
+            s->c_cpu->npc = addr + 4;
 #elif defined (TARGET_ARM)
-            env->regs[15] = addr;
+            s->c_cpu->regs[15] = addr;
 #elif defined (TARGET_SH4)
-            env->pc = addr;
+            s->c_cpu->pc = addr;
 #elif defined (TARGET_MIPS)
-            env->active_tc.PC = addr;
+            s->c_cpu->active_tc.PC = addr;
 #elif defined (TARGET_CRIS)
-            env->pc = addr;
+            s->c_cpu->pc = addr;
 #endif
         }
-        cpu_single_step(env, sstep_flags);
+        cpu_single_step(s->c_cpu, sstep_flags);
         gdb_continue(s);
 	return RS_IDLE;
     case 'F':
@@ -1305,7 +1334,7 @@
                 p++;
             type = *p;
             if (gdb_current_syscall_cb)
-                gdb_current_syscall_cb(s->env, ret, err);
+                gdb_current_syscall_cb(s->c_cpu, ret, err);
             if (type == 'C') {
                 put_packet(s, "T02");
             } else {
@@ -1316,7 +1345,7 @@
     case 'g':
         len = 0;
         for (addr = 0; addr < num_g_regs; addr++) {
-            reg_size = gdb_read_register(env, mem_buf + len, addr);
+            reg_size = gdb_read_register(s->g_cpu, mem_buf + len, addr);
             len += reg_size;
         }
         memtohex(buf, mem_buf, len);
@@ -1327,7 +1356,7 @@
         len = strlen(p) / 2;
         hextomem((uint8_t *)registers, p, len);
         for (addr = 0; addr < num_g_regs && len > 0; addr++) {
-            reg_size = gdb_write_register(env, registers, addr);
+            reg_size = gdb_write_register(s->g_cpu, registers, addr);
             len -= reg_size;
             registers += reg_size;
         }
@@ -1338,7 +1367,7 @@
         if (*p == ',')
             p++;
         len = strtoull(p, NULL, 16);
-        if (cpu_memory_rw_debug(env, addr, mem_buf, len, 0) != 0) {
+        if (cpu_memory_rw_debug(s->g_cpu, addr, mem_buf, len, 0) != 0) {
             put_packet (s, "E14");
         } else {
             memtohex(buf, mem_buf, len);
@@ -1353,7 +1382,7 @@
         if (*p == ':')
             p++;
         hextomem(mem_buf, p, len);
-        if (cpu_memory_rw_debug(env, addr, mem_buf, len, 1) != 0)
+        if (cpu_memory_rw_debug(s->g_cpu, addr, mem_buf, len, 1) != 0)
             put_packet(s, "E14");
         else
             put_packet(s, "OK");
@@ -1365,7 +1394,7 @@
         if (!gdb_has_xml)
             goto unknown_command;
         addr = strtoull(p, (char **)&p, 16);
-        reg_size = gdb_read_register(env, mem_buf, addr);
+        reg_size = gdb_read_register(s->g_cpu, mem_buf, addr);
         if (reg_size) {
             memtohex(buf, mem_buf, reg_size);
             put_packet(s, buf);
@@ -1381,7 +1410,7 @@
             p++;
         reg_size = strlen(p) / 2;
         hextomem(mem_buf, p, reg_size);
-        gdb_write_register(env, mem_buf, addr);
+        gdb_write_register(s->g_cpu, mem_buf, addr);
         put_packet(s, "OK");
         break;
     case 'Z':
@@ -1394,9 +1423,9 @@
             p++;
         len = strtoull(p, (char **)&p, 16);
         if (ch == 'Z')
-            res = gdb_breakpoint_insert(env, addr, len, type);
+            res = gdb_breakpoint_insert(addr, len, type);
         else
-            res = gdb_breakpoint_remove(env, addr, len, type);
+            res = gdb_breakpoint_remove(addr, len, type);
         if (res >= 0)
              put_packet(s, "OK");
         else if (res == -ENOSYS)
@@ -1404,6 +1433,45 @@
         else
             put_packet(s, "E22");
         break;
+    case 'H':
+        type = *p++;
+        thread = strtoull(p, (char **)&p, 16);
+        if (thread == -1 || thread == 0) {
+            put_packet(s, "OK");
+            break;
+        }
+        for (env = first_cpu; env != NULL; env = env->next_cpu)
+            if (env->cpu_index + 1 == thread)
+                break;
+        if (env == NULL) {
+            put_packet(s, "E22");
+            break;
+        }
+        switch (type) {
+        case 'c':
+            s->c_cpu = env;
+            put_packet(s, "OK");
+            break;
+        case 'g':
+            s->g_cpu = env;
+            put_packet(s, "OK");
+            break;
+        default:
+             put_packet(s, "E22");
+             break;
+        }
+        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
+            put_packet(s, "E22");
+        break;
     case 'q':
     case 'Q':
         /* parse any 'q' packets here */
@@ -1429,10 +1497,39 @@
             sstep_flags = type;
             put_packet(s, "OK");
             break;
+        } else if (strcmp(p,"C") == 0) {
+            /* "Current thread" remains vague in the spec, so always return
+             *  the first CPU (gdb returns the first thread). */
+            put_packet(s, "QC1");
+            break;
+        } else if (strcmp(p,"fThreadInfo") == 0) {
+            s->query_cpu = first_cpu;
+            goto report_cpuinfo;
+        } else if (strcmp(p,"sThreadInfo") == 0) {
+        report_cpuinfo:
+            if (s->query_cpu) {
+                snprintf(buf, sizeof(buf), "m%x", s->query_cpu->cpu_index+1);
+                put_packet(s, buf);
+                s->query_cpu = s->query_cpu->next_cpu;
+            } else
+                put_packet(s, "l");
+            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) {
+                    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;
+                }
+            break;
         }
 #ifdef CONFIG_LINUX_USER
         else if (strncmp(p, "Offsets", 7) == 0) {
-            TaskState *ts = env->opaque;
+            TaskState *ts = s->c_cpu->opaque;
 
             snprintf(buf, sizeof(buf),
                      "Text=" TARGET_ABI_FMT_lx ";Data=" TARGET_ABI_FMT_lx
@@ -1459,7 +1556,7 @@
 
             gdb_has_xml = 1;
             p += 19;
-            xml = get_feature_xml(env, p, &p);
+            xml = get_feature_xml(p, &p);
             if (!xml) {
                 snprintf(buf, sizeof(buf), "E00");
                 put_packet(s, buf);
@@ -1507,10 +1604,17 @@
 
 extern void tb_flush(CPUState *env);
 
+void gdb_set_stop_cpu(CPUState *env)
+{
+    gdbserver_state->c_cpu = env;
+    gdbserver_state->g_cpu = env;
+}
+
 #ifndef CONFIG_USER_ONLY
 static void gdb_vm_stopped(void *opaque, int reason)
 {
-    GDBState *s = opaque;
+    GDBState *s = gdbserver_state;
+    CPUState *env = s->c_cpu;
     char buf[256];
     const char *type;
     int ret;
@@ -1519,11 +1623,11 @@
         return;
 
     /* disable single step if it was enable */
-    cpu_single_step(s->env, 0);
+    cpu_single_step(env, 0);
 
     if (reason == EXCP_DEBUG) {
-        if (s->env->watchpoint_hit) {
-            switch (s->env->watchpoint_hit->flags & BP_MEM_ACCESS) {
+        if (env->watchpoint_hit) {
+            switch (env->watchpoint_hit->flags & BP_MEM_ACCESS) {
             case BP_MEM_READ:
                 type = "r";
                 break;
@@ -1534,20 +1638,22 @@
                 type = "";
                 break;
             }
-            snprintf(buf, sizeof(buf), "T%02x%swatch:" TARGET_FMT_lx ";",
-                     SIGTRAP, type, s->env->watchpoint_hit->vaddr);
+            snprintf(buf, sizeof(buf),
+                     "T%02xthread:%02x;%swatch:" TARGET_FMT_lx ";",
+                     SIGTRAP, env->cpu_index+1, type,
+                     env->watchpoint_hit->vaddr);
             put_packet(s, buf);
-            s->env->watchpoint_hit = NULL;
+            env->watchpoint_hit = NULL;
             return;
         }
-	tb_flush(s->env);
+	tb_flush(env);
         ret = SIGTRAP;
     } else if (reason == EXCP_INTERRUPT) {
         ret = SIGINT;
     } else {
         ret = 0;
     }
-    snprintf(buf, sizeof(buf), "S%02x", ret);
+    snprintf(buf, sizeof(buf), "T%02xthread:%02x;", ret, env->cpu_index+1);
     put_packet(s, buf);
 }
 #endif
@@ -1566,7 +1672,7 @@
     uint64_t i64;
     GDBState *s;
 
-    s = gdb_syscall_state;
+    s = gdbserver_state;
     if (!s)
         return;
     gdb_current_syscall_cb = cb;
@@ -1611,15 +1717,14 @@
     va_end(va);
     put_packet(s, buf);
 #ifdef CONFIG_USER_ONLY
-    gdb_handlesig(s->env, 0);
+    gdb_handlesig(s->c_cpu, 0);
 #else
-    cpu_interrupt(s->env, CPU_INTERRUPT_EXIT);
+    cpu_interrupt(s->c_cpu, CPU_INTERRUPT_EXIT);
 #endif
 }
 
 static void gdb_read_byte(GDBState *s, int ch)
 {
-    CPUState *env = s->env;
     int i, csum;
     uint8_t reply;
 
@@ -1685,7 +1790,7 @@
             } else {
                 reply = '+';
                 put_buffer(s, &reply, 1);
-                s->state = gdb_handle_packet(s, env, s->line_buf);
+                s->state = gdb_handle_packet(s, s->line_buf);
             }
             break;
         default:
@@ -1702,7 +1807,7 @@
   char buf[256];
   int n;
 
-  s = &gdbserver_state;
+  s = gdbserver_state;
   if (gdbserver_fd < 0 || s->fd < 0)
     return sig;
 
@@ -1750,7 +1855,7 @@
   GDBState *s;
   char buf[4];
 
-  s = &gdbserver_state;
+  s = gdbserver_state;
   if (gdbserver_fd < 0 || s->fd < 0)
     return;
 
@@ -1759,7 +1864,7 @@
 }
 
 
-static void gdb_accept(void *opaque)
+static void gdb_accept(void)
 {
     GDBState *s;
     struct sockaddr_in sockaddr;
@@ -1781,13 +1886,20 @@
     val = 1;
     setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char *)&val, sizeof(val));
 
-    s = &gdbserver_state;
+    s = qemu_mallocz(sizeof(GDBState));
+    if (!s) {
+        errno = ENOMEM;
+        perror("accept");
+        return;
+    }
+
     memset (s, 0, sizeof (GDBState));
-    s->env = first_cpu; /* XXX: allow to change CPU */
+    s->c_cpu = first_cpu;
+    s->g_cpu = first_cpu;
     s->fd = fd;
     gdb_has_xml = 0;
 
-    gdb_syscall_state = s;
+    gdbserver_state = s;
 
     fcntl(fd, F_SETFL, O_NONBLOCK);
 }
@@ -1829,7 +1941,7 @@
     if (gdbserver_fd < 0)
         return -1;
     /* accept connections */
-    gdb_accept (NULL);
+    gdb_accept();
     return 0;
 }
 #else
@@ -1842,11 +1954,10 @@
 
 static void gdb_chr_receive(void *opaque, const uint8_t *buf, int size)
 {
-    GDBState *s = opaque;
     int i;
 
     for (i = 0; i < size; i++) {
-        gdb_read_byte(s, buf[i]);
+        gdb_read_byte(gdbserver_state, buf[i]);
     }
 }
 
@@ -1855,7 +1966,6 @@
     switch (event) {
     case CHR_EVENT_RESET:
         vm_stop(EXCP_INTERRUPT);
-        gdb_syscall_state = opaque;
         gdb_has_xml = 0;
         break;
     default:
@@ -1890,11 +2000,13 @@
     if (!s) {
         return -1;
     }
-    s->env = first_cpu; /* XXX: allow to change CPU */
+    s->c_cpu = first_cpu;
+    s->g_cpu = first_cpu;
     s->chr = chr;
+    gdbserver_state = s;
     qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
-                          gdb_chr_event, s);
-    qemu_add_vm_stop_handler(gdb_vm_stopped, s);
+                          gdb_chr_event, NULL);
+    qemu_add_vm_stop_handler(gdb_vm_stopped, NULL);
     return 0;
 }
 #endif

Modified: trunk/gdbstub.h
===================================================================
--- trunk/gdbstub.h	2008-11-18 20:26:41 UTC (rev 5742)
+++ trunk/gdbstub.h	2008-11-18 20:30:24 UTC (rev 5743)
@@ -8,6 +8,7 @@
 
 void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...);
 int use_gdb_syscalls(void);
+void gdb_set_stop_cpu(CPUState *env);
 #ifdef CONFIG_USER_ONLY
 int gdb_handlesig (CPUState *, int);
 void gdb_exit(CPUState *, int);

Modified: trunk/vl.c
===================================================================
--- trunk/vl.c	2008-11-18 20:26:41 UTC (rev 5742)
+++ trunk/vl.c	2008-11-18 20:30:24 UTC (rev 5743)
@@ -3720,6 +3720,7 @@
                 ret = EXCP_INTERRUPT;
             }
             if (unlikely(ret == EXCP_DEBUG)) {
+                gdb_set_stop_cpu(cur_cpu);
                 vm_stop(EXCP_DEBUG);
             }
             /* If all cpus are halted then wait until the next IRQ */

Re: [Qemu-devel] [5743] gdbstub: manage CPUs as threads (Jan Kiszka)

[Paul Brook]

On Tuesday 18 November 2008, Anthony Liguori wrote:
> Revision: 5743
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5743
> Author:   aliguori
> Date:     2008-11-18 20:30:24 +0000 (Tue, 18 Nov 2008)
>
> Log Message:
> -----------
> gdbstub: manage CPUs as threads (Jan Kiszka)

This is wrong. CPUs do not share the same address space, so should be modelled 
as processes, not threads.

Paul

Re: [Qemu-devel] [5743] gdbstub: manage CPUs as threads (Jan Kiszka)

[Anthony Liguori]

Paul Brook wrote:
> On Tuesday 18 November 2008, Anthony Liguori wrote:
>   
>> Revision: 5743
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5743
>> Author:   aliguori
>> Date:     2008-11-18 20:30:24 +0000 (Tue, 18 Nov 2008)
>>
>> Log Message:
>> -----------
>> gdbstub: manage CPUs as threads (Jan Kiszka)
>>     
>
> This is wrong. CPUs do not share the same address space, so should be modelled 
> as processes, not threads.
>   

They share the same physical address space.  How well does GDB support 
debugging processes verses threads?  A cursory look seems to suggest the 
thread debugging support is slightly better (thread events) than the 
process debugging support.  They look very similar though.

Regards,

Anthony Liguori

> Paul
>   

Re: [Qemu-devel] [5743] gdbstub: manage CPUs as threads (Jan Kiszka)

[François Revol]

> Paul Brook wrote:
> > On Tuesday 18 November 2008, Anthony Liguori wrote:
> >
> >> Revision: 5743
> >>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5743
> > > > >> Author:   aliguori
> >> Date:     2008-11-18 20:30:24 +0000 (Tue, 18 Nov 2008)
> >>
> >> Log Message:
> >> -----------
> >> gdbstub: manage CPUs as threads (Jan Kiszka)
> >>
> >
> > This is wrong. CPUs do not share the same address space, so should
> >  be modelled
> > as processes, not threads.
> >
>
> They share the same physical address space.  How well does GDB
> support
> debugging processes verses threads?  A cursory look seems to suggest
> the
> thread debugging support is slightly better (thread events) than the
> process debugging support.  They look very similar though.

Well they do if you except cache (but those are supposed to stay
coherent anyway), and NUMA systems, does QEMU emulate any NUMA machine
yet ?

François.

Re: [Qemu-devel] [5743] gdbstub: manage CPUs as threads (Jan Kiszka)

[Paul Brook]

On Tuesday 18 November 2008, Anthony Liguori wrote:
> Paul Brook wrote:
> > On Tuesday 18 November 2008, Anthony Liguori wrote:
> >> Revision: 5743
> >>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5743
> >> Author:   aliguori
> >> Date:     2008-11-18 20:30:24 +0000 (Tue, 18 Nov 2008)
> >>
> >> Log Message:
> >> -----------
> >> gdbstub: manage CPUs as threads (Jan Kiszka)
> >
> > This is wrong. CPUs do not share the same address space, so should be
> > modelled as processes, not threads.
>
> They share the same physical address space.  

GDB works with virtual address spaces. Each CPU has its own MMU, so as soon as 
you load a multitasking OS the chances are that these will look significantly 
different.

> How well does GDB support 
> debugging processes verses threads?  A cursory look seems to suggest the
> thread debugging support is slightly better (thread events) than the
> process debugging support.  They look very similar though.

The thread model can't describe what's actually happening. gdb multiprocess 
support definitely exists (and if it doesn't work right now it's being 
actively worked on), so we should use it.

Paul

[Qemu-devel] Re: [5743] gdbstub: manage CPUs as threads (Jan Kiszka)

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

Paul Brook wrote:
> On Tuesday 18 November 2008, Anthony Liguori wrote:
>> Paul Brook wrote:
>>> On Tuesday 18 November 2008, Anthony Liguori wrote:
>>>> Revision: 5743
>>>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5743
>>>> Author:   aliguori
>>>> Date:     2008-11-18 20:30:24 +0000 (Tue, 18 Nov 2008)
>>>>
>>>> Log Message:
>>>> -----------
>>>> gdbstub: manage CPUs as threads (Jan Kiszka)
>>> This is wrong. CPUs do not share the same address space, so should be
>>> modelled as processes, not threads.
>> They share the same physical address space.  
> 
> GDB works with virtual address spaces. Each CPU has its own MMU, so as soon as 
> you load a multitasking OS the chances are that these will look significantly 
> different.
> 
>> How well does GDB support 
>> debugging processes verses threads?  A cursory look seems to suggest the
>> thread debugging support is slightly better (thread events) than the
>> process debugging support.  They look very similar though.
> 
> The thread model can't describe what's actually happening. gdb multiprocess 
> support definitely exists (and if it doesn't work right now it's being 
> actively worked on), so we should use it.

The thread model describes well what happens in many common OSes that
have their interesting part mapped identically to all CPUs - take Linux
as an example. For sure, you run into problems when trying to follow the
OS into dynamically mapped code (e.g. userspace under Linux) - but that
can already happen with _single_ CPU systems. And that is definitely not
the point where I suggest qemu or kvm as your favorite debugger. That's
what the guest itself can (generally) handle much better.

Well, maybe gdb will further improve toward mulitprocess debugging over
remote connections, and we will be able to use this instead. Right now I
don't see that this is the case (please correct me by drafting an
equivalent model based on mulitprocess debugging). Also, I'm a bit
skeptical about the maturity of multiprocess support - given the
experience I collected with multi-threading in gdb (just recently become
truly usable for our scenario here).

So, while CPU-to-thread mapping may not be the optimal model, it is the
most usable one right now, finally enabling qemu for (many) SMP guest
debugging scenarios. Believe me, it's _marvelously_ useful in this state
for analyzing tricky SMP issues, specifically on x86 where you generally
don't have hardware debuggers.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]