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

[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

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

[Paul Brook]

On Tuesday 02 June 2009, 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.

Really? Why doesn't GDB get confused on real machines when the PID wraps?
Is the real bug that we're missing some sort of thread creation/destruction 
event reporting?

Paul

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

[Daniel Jacobowitz]

On Tue, Jun 02, 2009 at 09:08:14PM +0100, Paul Brook wrote:
> On Tuesday 02 June 2009, 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.
> 
> Really? Why doesn't GDB get confused on real machines when the PID wraps?

It would, if using gdbserver and no one had done 'info threads' in between.

> Is the real bug that we're missing some sort of thread creation/destruction 
> event reporting?

The remote protocol doesn't have creation/destruction events (yet;
it's a known limitation).

-- 
Daniel Jacobowitz
CodeSourcery

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

[Nathan Froyd]

On Tue, Jun 02, 2009 at 09:08:14PM +0100, Paul Brook wrote:
> On Tuesday 02 June 2009, Nathan Froyd wrote:
> >   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.
> 
> Really? Why doesn't GDB get confused on real machines when the PID wraps?
> Is the real bug that we're missing some sort of thread creation/destruction 
> event reporting?

Hm, this is a good point.  I think the bug is that:

1) gdb_exit, called at thread exit, just blasts out a 'W' packet, when
   such a packet is really only defined to be sent in response to a '?'
   packet.  If I understand gdb's remote.c correctly, an unrequested 'W'
   packet is just going to get dropped on the floor.  This behavior is
   probably part of the reason GDB never sees threads die until it
   queries the target again, along with...

2) The response to a '?' packet is, well, not smart:

    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);
        put_packet(s, buf);

   which is, um, inaccurate.

I'm assuming that GDB handles wrapping PIDs correctly...it's possible
that it doesn't.  (For instance:

GDB has PIDs 12 and 14
GDB runs the target
PID 12 exits (I don't think a status notification gets sent here)
PID 12 gets reused by the process (no status notification here, either?)
target stops
GDB sees PID 12 still present

but I am not a Linux target GDB expert.)

I'm also not sure what to do differently that doesn't involve making
QEMU remember what happened to all the threads it's seen until GDB asks
about them.  Ideas?

-Nathan

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

[Paul Brook]

> > > thread #3 will have the same cpu_index as thread #1, which would
> > > confuse GDB.
> >
> > Really? Why doesn't GDB get confused on real machines when the PID wraps?
> > Is the real bug that we're missing some sort of thread
> > creation/destruction event reporting?
>
> Hm, this is a good point.  I think the bug is that:
>...
> I'm also not sure what to do differently that doesn't involve making
> QEMU remember what happened to all the threads it's seen until GDB asks
> about them.  Ideas?

Ok, from Daniel's response it sounds like this bit of gdb is broken.

Could we use the real TID? Seems silly to invent a new value when the host has 
already found one for us...

Paul

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

[Nathan Froyd]

On Tue, Jun 02, 2009 at 10:14:22PM +0100, Paul Brook wrote:
> > > Really? Why doesn't GDB get confused on real machines when the PID wraps?
> > > Is the real bug that we're missing some sort of thread
> > > creation/destruction event reporting?
> >
> > Hm, this is a good point.  I think the bug is that:
> >...
> > I'm also not sure what to do differently that doesn't involve making
> > QEMU remember what happened to all the threads it's seen until GDB asks
> > about them.  Ideas?
> 
> Ok, from Daniel's response it sounds like this bit of gdb is broken.
> 
> Could we use the real TID? Seems silly to invent a new value when the host has 
> already found one for us...

That would work for threaded usermode emulation.  But for multiple-cpu
system-mode emulation, the CPUs are unlikely to have unique TID values
(e.g. r2 on powerpc or what have you).  And if you're going to support
hotplugging someday, you're going to have support generation of unique
IDs somewhere along the way.  Using the same code for usermode and
system mode seems like the better, more robust/future-proof option.

-Nathan

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

[Paul Brook]

> > Could we use the real TID? Seems silly to invent a new value when the
> > host has already found one for us...
>
> That would work for threaded usermode emulation.  But for multiple-cpu
> system-mode emulation, the CPUs are unlikely to have unique TID values
> (e.g. r2 on powerpc or what have you).  And if you're going to support
> hotplugging someday, you're going to have support generation of unique
> IDs somewhere along the way.  Using the same code for usermode and
> system mode seems like the better, more robust/future-proof option.

Using threads for CPU emulation is just plain wrong.

Paul

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

[Jan Kiszka]

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

Nathan Froyd wrote:
> On Tue, Jun 02, 2009 at 10:14:22PM +0100, Paul Brook wrote:
>>>> Really? Why doesn't GDB get confused on real machines when the PID wraps?
>>>> Is the real bug that we're missing some sort of thread
>>>> creation/destruction event reporting?
>>> Hm, this is a good point.  I think the bug is that:
>>> ...
>>> I'm also not sure what to do differently that doesn't involve making
>>> QEMU remember what happened to all the threads it's seen until GDB asks
>>> about them.  Ideas?
>> Ok, from Daniel's response it sounds like this bit of gdb is broken.
>>
>> Could we use the real TID? Seems silly to invent a new value when the host has 
>> already found one for us...
> 
> That would work for threaded usermode emulation.  But for multiple-cpu
> system-mode emulation, the CPUs are unlikely to have unique TID values
> (e.g. r2 on powerpc or what have you).  And if you're going to support
> hotplugging someday, you're going to have support generation of unique
> IDs somewhere along the way.  Using the same code for usermode and
> system mode seems like the better, more robust/future-proof option.

For system-mode emulation, this is not that problematic. The threaded
model assumes that all CPUs use the same memory mapping (for the code of
interest at least) and are programmed with the same set of
break/watchpoints. Pulling one and reinserting it will not require gdb
to handle the newly plugged one differently within this model.

Once we have a true multicore model for gdb and proper protocol
extensions to handle more complex use cases, I bet we will also have a
CPU hotplug event channel for gdb.

Jan


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

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

[Antti P Miettinen]

Paul Brook <paul@codesourcery.com> writes:
> Using threads for CPU emulation is just plain wrong.
>
> Paul

Could you elaborate a bit? I've sort of assumed that to get reasonable
performance out of emulating a parallel target on a parallel host,
this would be the way to go. At least the parallel SimOS thesis by
Bob Lanz sort of suggets this [1]. Are there alternative approaches?

[1] http://cs.stanford.edu/~rlantz/papers/lantz-thesis.pdf

-- 
http://www.iki.fi/~ananaza/

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

[Paul Brook]

On Tuesday 16 June 2009, Antti P Miettinen wrote:
> Paul Brook <paul@codesourcery.com> writes:
> > Using threads for CPU emulation is just plain wrong.
> >
> Could you elaborate a bit? I've sort of assumed that to get reasonable
> performance out of emulating a parallel target on a parallel host,

You have taken this comment out of context.

Exposing multiple CPUs as threads to GDB is incorrect. Each CPU has its own 
MMU (i.e. address space), so they should be exposed as processes, not threads.

Paul

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

[Antti P Miettinen]

Paul Brook <paul@codesourcery.com> writes:
> You have taken this comment out of context.
>
> Exposing multiple CPUs as threads to GDB is incorrect. Each CPU has its own 
> MMU (i.e. address space), so they should be exposed as processes,
> not threads.
>
> Paul

Ah - OK - sorry. I misinterpreted your comment.

--
http://www.iki.fi/~ananaza/

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

[Jan Kiszka]

Paul Brook wrote:
> On Tuesday 16 June 2009, Antti P Miettinen wrote:
>> Paul Brook <paul@codesourcery.com> writes:
>>> Using threads for CPU emulation is just plain wrong.
>>>
>> Could you elaborate a bit? I've sort of assumed that to get reasonable
>> performance out of emulating a parallel target on a parallel host,
> 
> You have taken this comment out of context.
> 
> Exposing multiple CPUs as threads to GDB is incorrect. Each CPU has its own 
> MMU (i.e. address space), so they should be exposed as processes, not threads.

Exposing them as processes isn't fitting either. As stated by Daniel, we
need a "multicore" model (or however we may call it) for gdb.

Until someone "finds" the resources to work on this, qemu's current
approach to expose VCPUs as threads is a pragmatic workaround that
covers many use cases completely and even quite a few more when you deal
with the restrictions properly.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux