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

[Jan Kiszka]

Lionel Landwerlin wrote:
> 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 <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
>>
> 
> 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 <address@hidden>
---

 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, ...)
 CPUState *cpu_copy(CPUState *env)
 {
     CPUState *new_env = cpu_init(env->cpu_model_str);
-    /* preserve chaining and index */
     CPUState *next_cpu = new_env->next_cpu;
     int cpu_index = 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 = next_cpu;
     new_env->cpu_index = 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;
 }

signature.asc
Description: OpenPGP digital signature