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 <edgar.igles...@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
Thanks,
Edgar
>
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> ---
> 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
>
>
