Re: [Qemu-devel] [PATCH 03/12] Refactor and enhance break/watchpoint API

[Anthony Liguori]

Jan Kiszka wrote:
> Index: b/gdbstub.c
> ===================================================================
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1145,10 +1145,64 @@ void gdb_register_coprocessor(CPUState *
>      }
>  }
>  
> +/* GDB breakpoint/watchpoint types */
> +#define GDB_BREAKPOINT_SW        0
> +#define GDB_BREAKPOINT_HW        1
> +#define GDB_WATCHPOINT_WRITE     2
> +#define GDB_WATCHPOINT_READ      3
> +#define GDB_WATCHPOINT_ACCESS    4
> +
> +#ifndef CONFIG_USER_ONLY
> +static const int xlat_gdb_type[] = {
> +    [GDB_WATCHPOINT_WRITE]  = BP_GDB | BP_MEM_WRITE,
> +    [GDB_WATCHPOINT_READ]   = BP_GDB | BP_MEM_READ,
> +    [GDB_WATCHPOINT_ACCESS] = BP_GDB | BP_MEM_ACCESS,
> +};
> +#endif
> +
> +static int gdb_breakpoint_insert(CPUState *env, target_ulong addr,
> +                                 target_ulong len, int type)
> +{
> +    switch (type) {
> +    case GDB_BREAKPOINT_SW ... GDB_BREAKPOINT_HW:
>   

We've avoided this GCCism pretty much.  I don't think the code is 
significantly cleaner with it so I think it's best to avoid it.

> +typedef struct CPUWatchpoint {
> +    target_ulong vaddr;
> +    target_ulong len_mask;
> +    int flags; /* BP_* */
> +    struct CPUWatchpoint *prev, *next;
> +} CPUWatchpoint;
> +
>  #define CPU_TEMP_BUF_NLONGS 128
>  #define CPU_COMMON                                                      \
>      struct TranslationBlock *current_tb; /* currently executing TB  */  \
> @@ -174,16 +185,11 @@ typedef struct icount_decr_u16 {
>                                                                          \
>      /* from this point: preserved by CPU reset */                       \
>      /* ice debug support */                                             \
> -    target_ulong breakpoints[MAX_BREAKPOINTS];                          \
> -    int nb_breakpoints;                                                 \
> +    CPUBreakpoint *breakpoints;                                         \
>      int singlestep_enabled;                                             \
>                                                                          \
> -    struct {                                                            \
> -        target_ulong vaddr;                                             \
> -        int type; /* PAGE_READ/PAGE_WRITE */                            \
> -    } watchpoint[MAX_WATCHPOINTS];                                      \
> -    int nb_watchpoints;                                                 \
> -    int watchpoint_hit;                                                 \
> +    CPUWatchpoint *watchpoints;                                         \
> +    CPUWatchpoint *watchpoint_hit;                                      \
>   

The previous patch doesn't build because this patch is where you 
introduce flags into watch points.  Please make sure the whole series 
builds and works at each patch.  Otherwise bisecting breaks.

Regards,

Anthony Liguori