From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LBUsg-0002hL-HL for qemu-devel@nongnu.org; Sat, 13 Dec 2008 08:50:06 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LBUse-0002gd-Bj for qemu-devel@nongnu.org; Sat, 13 Dec 2008 08:50:06 -0500 Received: from [199.232.76.173] (port=54141 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LBUsd-0002gY-OL for qemu-devel@nongnu.org; Sat, 13 Dec 2008 08:50:03 -0500 Received: from fmmailgate03.web.de ([217.72.192.234]:37079) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LBUsc-0001GF-3G for qemu-devel@nongnu.org; Sat, 13 Dec 2008 08:50:03 -0500 Received: from smtp07.web.de (fmsmtp07.dlan.cinetic.de [172.20.5.215]) by fmmailgate03.web.de (Postfix) with ESMTP id 4D48FF561233 for ; Sat, 13 Dec 2008 14:49:57 +0100 (CET) Received: from [88.64.11.10] (helo=[192.168.1.198]) by smtp07.web.de with asmtp (TLSv1:AES256-SHA:256) (WEB.DE 4.109 #226) id 1LBUsX-0002Np-00 for qemu-devel@nongnu.org; Sat, 13 Dec 2008 14:49:57 +0100 Message-ID: <4943BD66.60109@web.de> Date: Sat, 13 Dec 2008 14:49:26 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <1229125944.3898.39.camel@cocoduo.atr> <1229126410.3898.42.camel@cocoduo.atr> <49438B8B.8050709@web.de> <1229171501.3898.53.camel@cocoduo.atr> <4943B1B6.9010707@web.de> <1229174473.3898.61.camel@cocoduo.atr> In-Reply-To: <1229174473.3898.61.camel@cocoduo.atr> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig7B0C26140D4C8EAFE93A9151" Sender: jan.kiszka@web.de Subject: [Qemu-devel] Re: [linux-user] Fixed Qemu crash using Gdbstub Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig7B0C26140D4C8EAFE93A9151 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Lionel Landwerlin wrote: > Le samedi 13 d=C3=A9cembre 2008 =C3=A0 13:59 +0100, Jan Kiszka a =C3=A9= crit : >> Lionel Landwerlin wrote: >>> Le samedi 13 d=C3=A9cembre 2008 =C3=A0 11:16 +0100, Jan Kiszka a =C3=A9= crit : >>>> Lionel Landwerlin wrote: >>>>> I just forgot to remove 2 printf ... >>>>> Here the good patch : >>>>> >>>>> >>>>> >>>>> >>>>> >From 2b3fe65ea3f2ee8dd3efbb52b66a2f4e53b788ea Mon Sep 17 00:00:00 = 2001 >>>>> From: Lionel Landwerlin >>>>> Date: Sat, 13 Dec 2008 00:32:04 +0100 >>>>> Subject: [PATCH] [linux-user] Fixed Qemu crash using Gdbstub >>>>> >>>>> When using gdb with qemu (via gdbstub), if your emulated >>>>> application is multithreaded and does a segfault then qemu >>>>> crashes. >>>>> >>>>> Qemu crashes because the break/watch points are shared betwee= n >>>>> cpus. The TAILQ structure which handles the list of break/wat= ch >>>>> points is copied inside each CPUState structure. When the las= t >>>>> breakpoint is removed (this happens on a segfault), it is >>>>> removed across all cpus but because of the copied TAILQ >>>>> structure a same breakpoint can be freed N times with N the >>>>> current number of cpus. >>>> OK, now I got the problem: user space emulation spawns additional VC= PUs >>>> to emulate fork. Those VCPUs are cloned via cpu_copy which simply >>>> duplicates the CPUState of the parent, including the breakpoint and >>>> watchpoint TAILQ headers. This is doomed to fail. >>>> >>>> But your approach to let the cloned VCPU point to the same TAILQ hea= der >>>> as its parent is not correct as well. It will cause troubles to gdbs= tub >>>> which manages breakpoints on all VCPUs by adding duplicate instances= on >>>> a per-VCPU base. If you inject a breakpoint before a fork and then >>>> remove it afterwards, gdbstub will report an error because it will o= nly >>>> find the breakpoint once, not n times (n =3D number of VCPUs). >>>> >>>> What you have to do is to cleanly duplicate the breakpoint and >>>> watchpoint lists on cpu_copy (filter out BP_CPU types for cleanness >>>> reasons, although they do not occur in user emulation ATM). >>> Hello Jan, >>> >>> Thanks for reviewing my patch. >>> >>> Duplication of all break/watchpoints will makes the patch bigger, >>> because it will required break/watchpoint_copy functions etc... >>> >>> Another problem is that threads are also emulated by vcpus in user >>> emulation. But we also need to share break/watchpoints between thread= s. >>> This explain the way my patch do the thing. >>> >>> Finally, this makes the modification a lot more complicated than what= I >>> expected, because breakpoints on emulated forks should not apply. >> Sorry, but shouldn't we prefer correct solutions over simpler but brok= en >> ones...? >> >> Before my gdbstub changes, break/watchpoints were per-VCPU and >> automatically duplicated on cpu_copy (as they were stored in a static >> array inside CPUState). Now they are kept in lists, but still per-VCPU= =2E >> All that has to be done now is to fix cpu_copy to take this into >> account. If that takes additional simple helpers to clone breakpoints,= >> so what? >> >> Jan >> >=20 > Ok, I will do that Guess I was too fast - could you try this one: --------> Subject: [PATCH] Adopt cpu_copy to new breakpoint API Latest changes to the cpu_breakpoint/watchpoint API broke cpu_copy. This patch fixes it by cloning the breakpoint and watchpoint lists appropriately. Thanks to Lionel Landwerlin for pointing out. Signed-off-by: Jan Kiszka --- exec.c | 24 +++++++++++++++++++++++- 1 files changed, 23 insertions(+), 1 deletions(-) diff --git a/exec.c b/exec.c index 44f6a42..193a43c 100644 --- a/exec.c +++ b/exec.c @@ -1654,12 +1654,34 @@ void cpu_abort(CPUState *env, const char *fmt, ..= =2E) CPUState *cpu_copy(CPUState *env) { CPUState *new_env =3D cpu_init(env->cpu_model_str); - /* preserve chaining and index */ CPUState *next_cpu =3D new_env->next_cpu; int cpu_index =3D new_env->cpu_index; +#if defined(TARGET_HAS_ICE) + CPUBreakpoint *bp; + CPUWatchpoint *wp; +#endif + memcpy(new_env, env, sizeof(CPUState)); + + /* Preserve chaining and index. */ new_env->next_cpu =3D next_cpu; new_env->cpu_index =3D cpu_index; + + /* Clone all break/watchpoints. + Note: Once we support ptrace with hw-debug register access, make = sure + BP_CPU break/watchpoints are handled correctly on clone. */ + TAILQ_INIT(&env->breakpoints); + TAILQ_INIT(&env->watchpoints); +#if defined(TARGET_HAS_ICE) + TAILQ_FOREACH(bp, &env->breakpoints, entry) { + cpu_breakpoint_insert(new_env, bp->pc, bp->flags, NULL); + } + TAILQ_FOREACH(wp, &env->watchpoints, entry) { + cpu_watchpoint_insert(new_env, wp->vaddr, (~wp->len_mask) + 1, + wp->flags, NULL); + } +#endif + return new_env; } =20 --------------enig7B0C26140D4C8EAFE93A9151 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 iEYEARECAAYFAklDvWkACgkQniDOoMHTA+mvngCfeU2WzuO4k9Nn9V49tci/SXrB VcQAn1pe9FQHpXUPNr9UfYlSvqqgWoRg =fq1D -----END PGP SIGNATURE----- --------------enig7B0C26140D4C8EAFE93A9151--