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

[Lionel Landwerlin]

Le dimanche 14 décembre 2008 à 15:17 +0100, Jan Kiszka a écrit :
> 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 <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;
> >>  }
> >>  
> >>
> > 
> > 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 <address@hidden>
> > 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.

In fact when you're not using gdbstub, there is no break/watch points.
So this problem never appears. And when you're using gdbstub, this
problem only appears, when all break/watch points are removed (ie when a
SIGSEGV is raised). In this case, you're almost everytime restart qemu
anytime soon, so break/watch points (and first member of the TAILQ
lists) are not used anymore.


My current test case is a piece a proprietary software, so I don't think
I will be able to give it to you, but I can probably rewrite something
simple using ~10/15 threads. I'm testing on sh4 binary, do you think you
need a sh4 root filesystem to reproduce the problem ?


-- 
Lione Landwerlin                                         

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