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

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

[Peter Maydell]

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.

Signed-off-by: Peter Maydell <peter.maydell@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

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

[Max Filippov]

Hi Peter,

On Fri, Sep 12, 2014 at 11:04 AM, Peter Maydell
<peter.maydell@linaro.org> 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.

I've tested xtensa part and have noticed no difference with or without
this patch:
gdb connected to qemu gdbstub always stops right after the watched instruction.
I guess that without this patch I should have seen gdb stopping not right after
the watched instruction, but one instruction later, right?

> We set this flag for the CPU architectures which set
> gdbarch_have_nonsteppable_watchpoint in gdb 7.7:
> ARM, CRIS, LM32, MIPS and Xtensa.
>
> Signed-off-by: Peter Maydell <peter.maydell@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.

-- 
Thanks.
-- Max

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

[Peter Maydell]

On 15 September 2014 20:59, Max Filippov <jcmvbkbc@gmail.com> wrote:
> I've tested xtensa part and have noticed no difference with or without
> this patch:
> gdb connected to qemu gdbstub always stops right after the watched instruction.
> I guess that without this patch I should have seen gdb stopping not right after
> the watched instruction, but one instruction later, right?

Yes, that is the behaviour I saw on ARM. Perhaps for
Xtensa gdb thinks it has no watchpoint support at all
and is using its "singlestep-and-test" fallback?

-- PMM

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

[Max Filippov]

On Mon, Sep 15, 2014 at 9:15 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 15 September 2014 20:59, Max Filippov <jcmvbkbc@gmail.com> wrote:
>> I've tested xtensa part and have noticed no difference with or without
>> this patch:
>> gdb connected to qemu gdbstub always stops right after the watched instruction.
>> I guess that without this patch I should have seen gdb stopping not right after
>> the watched instruction, but one instruction later, right?
>
> Yes, that is the behaviour I saw on ARM. Perhaps for
> Xtensa gdb thinks it has no watchpoint support at all
> and is using its "singlestep-and-test" fallback?

Hmmm, interesting. Yes, I see gdb single-stepping and reading watched buffer.
With the patch it first stops at the store instruction, without it --
at the next one.
In both cases the following single-step request does nothing. Need to look at
it some more.

-- 
Thanks.
-- Max

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 <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Thanks,
Edgar


> 
> Signed-off-by: Peter Maydell <peter.maydell@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
> 
> 

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

[Max Filippov]

On Tue, Sep 16, 2014 at 9:16 AM, Max Filippov <jcmvbkbc@gmail.com> wrote:
> On Mon, Sep 15, 2014 at 9:15 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 15 September 2014 20:59, Max Filippov <jcmvbkbc@gmail.com> wrote:
>>> I've tested xtensa part and have noticed no difference with or without
>>> this patch:
>>> gdb connected to qemu gdbstub always stops right after the watched instruction.
>>> I guess that without this patch I should have seen gdb stopping not right after
>>> the watched instruction, but one instruction later, right?
>>
>> Yes, that is the behaviour I saw on ARM. Perhaps for
>> Xtensa gdb thinks it has no watchpoint support at all
>> and is using its "singlestep-and-test" fallback?
>
> Hmmm, interesting. Yes, I see gdb single-stepping and reading watched buffer.
> With the patch it first stops at the store instruction, without it --
> at the next one.
> In both cases the following single-step request does nothing. Need to look at
> it some more.

Looks like I won't be able to look at it in a reasonable time. So, here's my

Tested-by: Max Filippov <jcmvbkbc@gmail.com>

to signify that it at least doesn't have visible negative effect on
target-xtensa.
I'll still try to figure out why xtensa doesn't exhibit the issue that
the patch is
supposed to fix.

-- 
Thanks.
-- Max

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

[Michael Walle]

Am Freitag, 12. September 2014, 19:04:17 schrieb Peter Maydell:
> 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.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Hi Peter,

i've tested your patch on the lm32 target.

My test program was like the following:

mvhi r1, hi(0x10000000)
ori r1, r1, lo(0x10000000)
nop
nop
nop
nop
sw (r1+0), r0  ; (1) store some value to 0x10000000
nop            ; (2)
nop            ; (3)
nop
nop
1: bi 1b       ; loop forever

I can confirm that your patch makes qemu stop one instruction earlier. Without 
your patch the program is stopped at (3). With your patch applied the program 
is stopped at (2). But I guess the correct point to stop is (1), right?

I think there is still some mistake in the lm32 target code in qemu. I'll look 
into it soon.

-- 
-michael

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

[Peter Maydell]

On 5 October 2014 22:00, Michael Walle <michael@walle.cc> wrote:
> i've tested your patch on the lm32 target.
>
> My test program was like the following:
>
> mvhi r1, hi(0x10000000)
> ori r1, r1, lo(0x10000000)
> nop
> nop
> nop
> nop
> sw (r1+0), r0  ; (1) store some value to 0x10000000
> nop            ; (2)
> nop            ; (3)
> nop
> nop
> 1: bi 1b       ; loop forever
>
> I can confirm that your patch makes qemu stop one instruction earlier. Without
> your patch the program is stopped at (3). With your patch applied the program
> is stopped at (2). But I guess the correct point to stop is (1), right?

No, gdb wants execution to stop with the PC just after the
instruction which issued the memory access, with whatever
effects the instruction had having already taken place.
So (2) is correct. (I think nicer UI would indeed be to
stop at (1) but you can't get that effect on CPUs like
x86 which only stop after the wp insn has executed, and
they'd rather be consistent.)

thanks
-- PMM

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

[Peter Maydell]

On 5 October 2014 22:36, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 October 2014 22:00, Michael Walle <michael@walle.cc> wrote:
>> I can confirm that your patch makes qemu stop one instruction earlier. Without
>> your patch the program is stopped at (3). With your patch applied the program
>> is stopped at (2). But I guess the correct point to stop is (1), right?
>
> No, gdb wants execution to stop with the PC just after the
> instruction which issued the memory access, with whatever
> effects the instruction had having already taken place.
> So (2) is correct. (I think nicer UI would indeed be to
> stop at (1) but you can't get that effect on CPUs like
> x86 which only stop after the wp insn has executed, and
> they'd rather be consistent.)

...and incidentally the way it achieves this for "stop before
wp insn" CPU targets is that it unsets the watchpoint
and automatically steps one instruction before returning
control to the gdb user. (You can see this if you turn
gdb's remote-protocol debug on.)

-- PMM

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

[Michael Walle]

Am Sonntag, 5. Oktober 2014, 22:48:05 schrieb Peter Maydell:
> On 5 October 2014 22:36, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 5 October 2014 22:00, Michael Walle <michael@walle.cc> wrote:
> >> I can confirm that your patch makes qemu stop one instruction earlier.
> >> Without your patch the program is stopped at (3). With your patch
> >> applied the program is stopped at (2). But I guess the correct point to
> >> stop is (1), right?> 
> > No, gdb wants execution to stop with the PC just after the
> > instruction which issued the memory access, with whatever
> > effects the instruction had having already taken place.
> > So (2) is correct. (I think nicer UI would indeed be to
> > stop at (1) but you can't get that effect on CPUs like
> > x86 which only stop after the wp insn has executed, and
> > they'd rather be consistent.)
> 
> ...and incidentally the way it achieves this for "stop before
> wp insn" CPU targets is that it unsets the watchpoint
> and automatically steps one instruction before returning
> control to the gdb user. (You can see this if you turn
> gdb's remote-protocol debug on.)

Ah, now it makes sense :)

Tested-by: Michael Walle <michael@walle.cc> (for lm32)

-- 
-michael

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

[Peter Maydell]

On 5 October 2014 23:07, Michael Walle <michael@walle.cc> wrote:
> Am Sonntag, 5. Oktober 2014, 22:48:05 schrieb Peter Maydell:
>> On 5 October 2014 22:36, Peter Maydell <peter.maydell@linaro.org> wrote:
>> > On 5 October 2014 22:00, Michael Walle <michael@walle.cc> wrote:
>> >> I can confirm that your patch makes qemu stop one instruction earlier.
>> >> Without your patch the program is stopped at (3). With your patch
>> >> applied the program is stopped at (2). But I guess the correct point to
>> >> stop is (1), right?>
>> > No, gdb wants execution to stop with the PC just after the
>> > instruction which issued the memory access, with whatever
>> > effects the instruction had having already taken place.
>> > So (2) is correct. (I think nicer UI would indeed be to
>> > stop at (1) but you can't get that effect on CPUs like
>> > x86 which only stop after the wp insn has executed, and
>> > they'd rather be consistent.)
>>
>> ...and incidentally the way it achieves this for "stop before
>> wp insn" CPU targets is that it unsets the watchpoint
>> and automatically steps one instruction before returning
>> control to the gdb user. (You can see this if you turn
>> gdb's remote-protocol debug on.)
>
> Ah, now it makes sense :)
>
> Tested-by: Michael Walle <michael@walle.cc> (for lm32)

Thanks to all for review/testing; applied to master.

-- PMM