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

[Jan Kiszka]

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 <address@hidden>
>>> 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

signature.asc
Description: OpenPGP digital signature