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 >>>>> 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 --- 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; }