Re: [Qemu-devel] [PATCH] gdbstub: Allow target CPUs to specify watchpoint STOP_BEFORE_ACCESS flag

[Edgar E. Iglesias]

On Fri, Sep 12, 2014 at 07:04:17PM +0100, Peter Maydell wrote:
> GDB assumes that watchpoint set via the gdbstub remote protocol will
> behave in the same way as hardware watchpoints for the target. In
> particular, whether the CPU stops with the PC before or after the insn
> which triggers the watchpoint is target dependent. Allow guest CPU
> code to specify which behaviour to use. This fixes a bug where with
> guest CPUs which stop before the accessing insn GDB would manually
> step forward over what it thought was the insn and end up one insn
> further forward than it should be.
> 
> We set this flag for the CPU architectures which set
> gdbarch_have_nonsteppable_watchpoint in gdb 7.7:
> ARM, CRIS, LM32, MIPS and Xtensa.

I took a look at this and indeed, it fixes the bug you describe
for CRIS. Nice catch!

Reviewed-by: Edgar E. Iglesias <address@hidden>
Tested-by: Edgar E. Iglesias <address@hidden>

Thanks,
Edgar


> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> I have tested ARM and confirmed that this patch is needed
> to give correct watchpoint behaviour with a gdb attached to
> our debug stub. I haven't tested the other targets beyond
> compile testing, but I have checked the gdb sources to get
> the list of which targets needed changes.
> 
> 
>  gdbstub.c           | 32 +++++++++++++++++++++++---------
>  include/qom/cpu.h   |  3 +++
>  target-arm/cpu.c    |  1 +
>  target-cris/cpu.c   |  1 +
>  target-lm32/cpu.c   |  1 +
>  target-mips/cpu.c   |  1 +
>  target-xtensa/cpu.c |  1 +
>  7 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index 8afe0b7..d500cc5 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -625,11 +625,23 @@ void gdb_register_coprocessor(CPUState *cpu,
>  }
>  
>  #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,
> -};
> +/* Translate GDB watchpoint type to a flags value for cpu_watchpoint_* */
> +static inline int xlat_gdb_type(CPUState *cpu, int gdbtype)
> +{
> +    static const int xlat[] = {
> +        [GDB_WATCHPOINT_WRITE]  = BP_GDB | BP_MEM_WRITE,
> +        [GDB_WATCHPOINT_READ]   = BP_GDB | BP_MEM_READ,
> +        [GDB_WATCHPOINT_ACCESS] = BP_GDB | BP_MEM_ACCESS,
> +    };
> +
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    int cputype = xlat[gdbtype];
> +
> +    if (cc->gdb_stop_before_watchpoint) {
> +        cputype |= BP_STOP_BEFORE_ACCESS;
> +    }
> +    return cputype;
> +}
>  #endif
>  
>  static int gdb_breakpoint_insert(target_ulong addr, target_ulong len, int 
> type)
> @@ -656,10 +668,11 @@ static int gdb_breakpoint_insert(target_ulong addr, 
> target_ulong len, int type)
>      case GDB_WATCHPOINT_READ:
>      case GDB_WATCHPOINT_ACCESS:
>          CPU_FOREACH(cpu) {
> -            err = cpu_watchpoint_insert(cpu, addr, len, xlat_gdb_type[type],
> -                                        NULL);
> -            if (err)
> +            err = cpu_watchpoint_insert(cpu, addr, len,
> +                                        xlat_gdb_type(cpu, type), NULL);
> +            if (err) {
>                  break;
> +            }
>          }
>          return err;
>  #endif
> @@ -692,7 +705,8 @@ static int gdb_breakpoint_remove(target_ulong addr, 
> target_ulong len, int type)
>      case GDB_WATCHPOINT_READ:
>      case GDB_WATCHPOINT_ACCESS:
>          CPU_FOREACH(cpu) {
> -            err = cpu_watchpoint_remove(cpu, addr, len, xlat_gdb_type[type]);
> +            err = cpu_watchpoint_remove(cpu, addr, len,
> +                                        xlat_gdb_type(cpu, type));
>              if (err)
>                  break;
>          }
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 370b3eb..24aa0aa 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -99,6 +99,8 @@ struct TranslationBlock;
>   * @vmsd: State description for migration.
>   * @gdb_num_core_regs: Number of core registers accessible to GDB.
>   * @gdb_core_xml_file: File name for core registers GDB XML description.
> + * @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop
> + *           before the insn which triggers a watchpoint rather than after 
> it.
>   *
>   * Represents a CPU family or model.
>   */
> @@ -149,6 +151,7 @@ typedef struct CPUClass {
>      const struct VMStateDescription *vmsd;
>      int gdb_num_core_regs;
>      const char *gdb_core_xml_file;
> +    bool gdb_stop_before_watchpoint;
>  } CPUClass;
>  
>  #ifdef HOST_WORDS_BIGENDIAN
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 5b5531a..47ebf77 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -1066,6 +1066,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void 
> *data)
>  #endif
>      cc->gdb_num_core_regs = 26;
>      cc->gdb_core_xml_file = "arm-core.xml";
> +    cc->gdb_stop_before_watchpoint = true;
>      cc->debug_excp_handler = arm_debug_excp_handler;
>  }
>  
> diff --git a/target-cris/cpu.c b/target-cris/cpu.c
> index 20d8809..355bba0 100644
> --- a/target-cris/cpu.c
> +++ b/target-cris/cpu.c
> @@ -290,6 +290,7 @@ static void cris_cpu_class_init(ObjectClass *oc, void 
> *data)
>  #endif
>  
>      cc->gdb_num_core_regs = 49;
> +    cc->gdb_stop_before_watchpoint = true;
>  }
>  
>  static const TypeInfo cris_cpu_type_info = {
> diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
> index 419d664..a51490f 100644
> --- a/target-lm32/cpu.c
> +++ b/target-lm32/cpu.c
> @@ -272,6 +272,7 @@ static void lm32_cpu_class_init(ObjectClass *oc, void 
> *data)
>      cc->vmsd = &vmstate_lm32_cpu;
>  #endif
>      cc->gdb_num_core_regs = 32 + 7;
> +    cc->gdb_stop_before_watchpoint = true;
>      cc->debug_excp_handler = lm32_debug_excp_handler;
>  }
>  
> diff --git a/target-mips/cpu.c b/target-mips/cpu.c
> index b3e0e6c..a23e0b7 100644
> --- a/target-mips/cpu.c
> +++ b/target-mips/cpu.c
> @@ -150,6 +150,7 @@ static void mips_cpu_class_init(ObjectClass *c, void 
> *data)
>  #endif
>  
>      cc->gdb_num_core_regs = 73;
> +    cc->gdb_stop_before_watchpoint = true;
>  }
>  
>  static const TypeInfo mips_cpu_type_info = {
> diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
> index 936d526..ca95878 100644
> --- a/target-xtensa/cpu.c
> +++ b/target-xtensa/cpu.c
> @@ -146,6 +146,7 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void 
> *data)
>      cc->set_pc = xtensa_cpu_set_pc;
>      cc->gdb_read_register = xtensa_cpu_gdb_read_register;
>      cc->gdb_write_register = xtensa_cpu_gdb_write_register;
> +    cc->gdb_stop_before_watchpoint = true;
>  #ifndef CONFIG_USER_ONLY
>      cc->do_unaligned_access = xtensa_cpu_do_unaligned_access;
>      cc->get_phys_page_debug = xtensa_cpu_get_phys_page_debug;
> -- 
> 1.9.1
> 
>