Re: [Qemu-devel] [PATCH 02/13] Refactor and enhance break/watchpoint API

[Glauber Costa]

>
> Index: b/exec.c
> ===================================================================
> --- a/exec.c
> +++ b/exec.c
> @@ -537,7 +537,6 @@ void cpu_exec_init(CPUState *env)
>         cpu_index++;
>     }
>     env->cpu_index = cpu_index;
> -    env->nb_watchpoints = 0;
>     *penv = env;
>  #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
>     register_savevm("cpu_common", cpu_index, CPU_COMMON_SAVE_VERSION,
> @@ -1311,107 +1310,150 @@ static void breakpoint_invalidate(CPUSta
>  #endif
>
>  /* Add a watchpoint.  */
> -int cpu_watchpoint_insert(CPUState *env, target_ulong addr, int type)
> +int cpu_watchpoint_insert(CPUState *env, target_ulong addr, target_ulong len,
> +                          int flags, CPUWatchpoint **watchpoint)
>  {
> -    int i;
> +    CPUWatchpoint *wp;
>
> -    for (i = 0; i < env->nb_watchpoints; i++) {
> -        if (addr == env->watchpoint[i].vaddr)
> -            return 0;
> -    }
> -    if (env->nb_watchpoints >= MAX_WATCHPOINTS)
> -        return -1;
> +    wp = qemu_malloc(sizeof(*wp));
> +    if (!wp)
> +        return -ENOBUFS;
> +
> +    wp->vaddr = addr;
> +    wp->len = len;
> +    wp->flags = flags;
> +
> +    wp->next = env->watchpoints;
> +    wp->prev = NULL;
> +    if (wp->next)
> +        wp->next->prev = wp;
> +    env->watchpoints = wp;
>
> -    i = env->nb_watchpoints++;
> -    env->watchpoint[i].vaddr = addr;
> -    env->watchpoint[i].type = type;
>     tlb_flush_page(env, addr);
>     /* FIXME: This flush is needed because of the hack to make memory ops
>        terminate the TB.  It can be removed once the proper IO trap and
>        re-execute bits are in.  */
>     tb_flush(env);

> Index: b/cpu-defs.h
> +typedef struct CPUBreakpoint {
> +    target_ulong pc;
> +    int flags; /* BP_* */
> +    struct CPUBreakpoint *prev, *next;
> +} CPUBreakpoint;
> +
> +typedef struct CPUWatchpoint {
> +    target_ulong vaddr;
> +    target_ulong len;
> +    int flags; /* BP_* */
> +    struct CPUWatchpoint *prev, *next;
> +} CPUWatchpoint;
> +

Most of the time, you are transversing the list in a single direction.
So any particular reason to use a double linked list?

By the way, /me thinks it is about time for us to have a generic
linked list implementation


-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."