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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3814 bytes --]

Lionel Landwerlin wrote:
> Le samedi 13 décembre 2008 à 14:49 +0100, Jan Kiszka a écrit :
>> Lionel Landwerlin wrote:
>> 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 <jan.kiszka@web.de>
>> ---
>>
>>  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;
>>  }
>>  
>>
> 
> Jan,
> 
> Well the patch seems pretty better as qemu does not crash anymore :)
> There might be other problems, because gdbstub doesn't stop where I know
> it should. I'm investigating...

OK. If you have a testcase, I would also look into this next week.

> 
> You might want to add this patch too, there is something strange with
> TAILQ 'first' structure member. It's not updated on deletion of
> all/first elements.
> 
> Regards,
> 
>>From 78ba0dbf0c9e5d73022fecdbf1869274b8224949 Mon Sep 17 00:00:00 2001
> From: Lionel Landwerlin <lionel.landwerlin@openwide.fr>
> Date: Sat, 13 Dec 2008 14:05:18 +0100
> Subject: [PATCH] Fix suspicious TAILQ management
> 
>     TAILQ first pointer is not updated when the last element is
>     removed.
> ---
>  sys-queue.h |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/sys-queue.h b/sys-queue.h
> index ad5c8fb..37bedde 100644
> --- a/sys-queue.h
> +++ b/sys-queue.h
> @@ -202,7 +202,8 @@ struct {                                             \
>                      (elm)->field.tqe_prev;                              \
>          else                                                            \
>                  (head)->tqh_last = (elm)->field.tqe_prev;               \
> -        *(elm)->field.tqe_prev = (elm)->field.tqe_next;                 \
> +        if ((head)->tqh_first == (elm))                                 \
> +                (head)->tqh_first = (elm)->field.tqe_next;              \

That's fishy. The elm's prev field should point to the head, thus the
head should be updated to elm's next (ie. NULL). Could you dig deeper
what the state of all involved structures are and maybe track down when
they become inconsistent? Alternatively, please provide a testcase.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 258 bytes --]