Re: [Qemu-devel] Re: [linux-user] Fixed Qemu crash using Gdbstub

[Lionel Landwerlin <lionel.landwerlin@openwide.fr>]

Le samedi 13 décembre 2008 à 13:59 +0100, Jan Kiszka a écrit :
> Lionel Landwerlin wrote:
> > Le samedi 13 décembre 2008 à 11:16 +0100, Jan Kiszka a é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 <lionel.landwerlin@openwide.fr>
> >>> 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 between
> >>>       cpus. The TAILQ structure which handles the list of break/watch
> >>>       points is copied inside each CPUState structure. When the last
> >>>       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 VCPUs
> >> 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 header
> >> as its parent is not correct as well. It will cause troubles to gdbstub
> >> 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 only
> >> find the breakpoint once, not n times (n = 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 threads.
> > 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 broken
> 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.
> 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
> 

Ok, I will do that

Regards,

-- 
Lione Landwerlin                                         

O p e n W i d e                    14, rue Gaillon 75002 Paris