From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1M3yzG-0001BH-TC for qemu-devel@nongnu.org; Tue, 12 May 2009 16:54:06 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1M3yzC-0001A5-Ba for qemu-devel@nongnu.org; Tue, 12 May 2009 16:54:06 -0400 Received: from [199.232.76.173] (port=48412 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1M3yzC-0001A2-7n for qemu-devel@nongnu.org; Tue, 12 May 2009 16:54:02 -0400 Received: from fmmailgate02.web.de ([217.72.192.227]:47672) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1M3yzA-0005Gp-Tm for qemu-devel@nongnu.org; Tue, 12 May 2009 16:54:01 -0400 Message-ID: <4A09E1C2.9020302@web.de> Date: Tue, 12 May 2009 22:53:22 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1242156943-27850-1-git-send-email-froydnj@codesourcery.com> In-Reply-To: <1242156943-27850-1-git-send-email-froydnj@codesourcery.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig73642A8D0BF547974D9FF821" Sender: jan.kiszka@web.de Subject: [Qemu-devel] Re: [PATCH] fix gdbstub support for multiple threads in usermode List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nathan Froyd Cc: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig73642A8D0BF547974D9FF821 Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: quoted-printable Nathan Froyd wrote: > When debugging multi-threaded programs, QEMU's gdb stub would report th= e > 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: >=20 > initial thread created > new thread #1 > new thread #2 > thread #1 exits > new thread #3 >=20 > thread #3 will have the same cpu_index as thread #1, which would confus= e > GDB. >=20 > We fix this by adding a stable gdbstub_index field for each CPU; the > index is incremented for every CPU (thread) created. We ignore > wraparound issues for now. Once we have this field, the stub needs to > use this field instead of cpu_index for communicating with GDB. >=20 > Signed-off-by: Nathan Froyd > --- > cpu-defs.h | 1 + > exec.c | 5 +++++ > gdbstub.c | 32 +++++++++++++++++++++++--------- > 3 files changed, 29 insertions(+), 9 deletions(-) >=20 > diff --git a/cpu-defs.h b/cpu-defs.h > index 0a82197..c923708 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) */ = \ > + int 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.c b/exec.c > index c5c9280..63e3411 100644 > --- a/exec.c > +++ b/exec.c > @@ -538,6 +538,8 @@ static int cpu_common_load(QEMUFile *f, void *opaqu= e, int version_id) > } > #endif > =20 > +static int next_gdbstub_index; > + > void cpu_exec_init(CPUState *env) > { > CPUState **penv; > @@ -554,6 +556,7 @@ void cpu_exec_init(CPUState *env) > cpu_index++; > } > env->cpu_index =3D cpu_index; > + env->gdbstub_index =3D ++next_gdbstub_index; While this is simple and sufficient for most cases, making next_gdbstub_index robust against collisions due to overflow is not much more complicated - so why not do this right from the beginning? > env->numa_node =3D 0; > TAILQ_INIT(&env->breakpoints); > TAILQ_INIT(&env->watchpoints); > @@ -1711,6 +1714,8 @@ CPUState *cpu_copy(CPUState *env) > wp->flags, NULL); > } > #endif > + /* Need to assign a new thread ID for the gdbstub */ > + new_env->gdbstub_index =3D ++next_gdbstub_index; > =20 > return new_env; > } > diff --git a/gdbstub.c b/gdbstub.c > index 3c34741..61b065b 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 =3D first_cpu; env !=3D NULL; env =3D env->next_cpu) > - if (env->cpu_index + 1 =3D=3D thread) > + for (env =3D first_cpu; env !=3D NULL; env =3D env->next_cpu) = { > + if (env->gdbstub_index =3D=3D thread) { > break; > + } > + } > if (env =3D=3D NULL) { > put_packet(s, "E22"); > break; > @@ -1741,14 +1743,25 @@ static int gdb_handle_packet(GDBState *s, const= char *line_buf) > break; > case 'T': > thread =3D strtoull(p, (char **)&p, 16); > -#ifndef CONFIG_USER_ONLY > - if (thread > 0 && thread < smp_cpus + 1) > +#if defined(CONFIG_USER_ONLY) > + { > + for (env =3D first_cpu; env !=3D NULL; env =3D env->next_c= pu) { > + if (env->gdbstub_index =3D=3D thread) { > + break; > + } > + } > + > + if (env !=3D NULL) > + put_packet(s, "OK"); > + else > + put_packet(s, "E22"); > + } > #else > - if (thread =3D=3D 1) > -#endif > + if (thread > 0 && thread < smp_cpus + 1) > put_packet(s, "OK"); > else > put_packet(s, "E22"); > +#endif I don't think we need these #ifdefs here. You assign continuously increasing IDs also to system-mode CPUs, so we can handle them identically (we have to anyway once cpu hotplugging hits upstream). > break; > case 'q': > case 'Q': > @@ -1786,7 +1799,7 @@ static int gdb_handle_packet(GDBState *s, const c= har *line_buf) > } else if (strcmp(p,"sThreadInfo") =3D=3D 0) { > report_cpuinfo: > if (s->query_cpu) { > - snprintf(buf, sizeof(buf), "m%x", s->query_cpu->cpu_in= dex+1); > + snprintf(buf, sizeof(buf), "m%x", s->query_cpu->gdbstu= b_index); > put_packet(s, buf); > s->query_cpu =3D s->query_cpu->next_cpu; > } else > @@ -1794,8 +1807,8 @@ static int gdb_handle_packet(GDBState *s, const c= har *line_buf) > break; > } else if (strncmp(p,"ThreadExtraInfo,", 16) =3D=3D 0) { > thread =3D strtoull(p+16, (char **)&p, 16); > - for (env =3D first_cpu; env !=3D NULL; env =3D env->next_c= pu) > - if (env->cpu_index + 1 =3D=3D thread) { > + for (env =3D first_cpu; env !=3D NULL; env =3D env->next_c= pu) { > + if (env->gdbstub_index =3D=3D thread) { > cpu_synchronize_state(env, 0); > len =3D snprintf((char *)mem_buf, sizeof(mem_buf),= > "CPU#%d [%s]", env->cpu_index, > @@ -1804,6 +1817,7 @@ static int gdb_handle_packet(GDBState *s, const c= har *line_buf) > put_packet(s, buf); > break; > } > + } > break; > } > #ifdef CONFIG_USER_ONLY Hmm, I bet you now have some use for my good-old vCont patch (=3D>continu= e single-stepping on different CPU / in different thread). Will repost soon= =2E Jan --------------enig73642A8D0BF547974D9FF821 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iEYEARECAAYFAkoJ4ecACgkQniDOoMHTA+kLXQCfetuNq6KGuLtRyrpI6xL3/VXW 1p8An0yDs3qvle3/XtE6SJPlFnCHfoMg =SIpf -----END PGP SIGNATURE----- --------------enig73642A8D0BF547974D9FF821--