[RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Joel Brobecker]

Hello!

Re: question about ARM watchpoints
    https://www.sourceware.org/ml/gdb/2014-09/msg00000.html

This patch fixes an issue with watchpoints on ARM targets, where
the debugger stops 2 instructions after the instruction causing
the watchpoint. GDB is expected to stop at the next instruction.

The problem is caused by the fact that GDB does an extra single-step
after receiving the watchpoint notification, because the
have_nonsteppable_watchpoint gdbarch attribute is set for ARM
targets. Our experiments indicate that this is incorrect, at
least for the versions of ARM that we tested on (ARMv7). We tried
to get confirmation of this through the ARM documentation, but
did not manage to get a clear answer. So, in light of evidence
that the current code is wrong for some versions of ARM, and
with the lack of evidence regarding the versions of ARM we could
not test on, this patch simply unsets this gdbarch attribute
for all versions of ARM.  The plan is to refine this later on
if/when we find that some systems behave differently.

gdb/ChangeLog:

        * arm-tdep.c (arm_gdbarch_init): Remove call to
        set_gdbarch_have_nonsteppable_watchpoint.

Unless there are comments, I will commit the patch in a week.

Thank you!

---
 gdb/arm-tdep.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index f9feb52..990c4ad 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -10446,9 +10446,6 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   if (tdep->arm_abi == ARM_ABI_AUTO)
     tdep->arm_abi = ARM_ABI_APCS;
 
-  /* Watchpoints are not steppable.  */
-  set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
-
   /* We used to default to FPA for generic ARM, but almost nobody
      uses that now, and we now provide a way for the user to force
      the model.  So default to the most useful variant.  */
-- 
1.9.1

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Yao Qi]

Joel Brobecker <brobecker@adacore.com> writes:

> Hello!
>
> Re: question about ARM watchpoints
>     https://www.sourceware.org/ml/gdb/2014-09/msg00000.html
>
> This patch fixes an issue with watchpoints on ARM targets, where
> the debugger stops 2 instructions after the instruction causing
> the watchpoint. GDB is expected to stop at the next instruction.
>
> The problem is caused by the fact that GDB does an extra single-step
> after receiving the watchpoint notification, because the
> have_nonsteppable_watchpoint gdbarch attribute is set for ARM
> targets. Our experiments indicate that this is incorrect, at
> least for the versions of ARM that we tested on (ARMv7). We tried

Joel,
Can you elaborate your experiments?  Do you do experiments on qemu, arm
bare metal targets or arm linux targets?

I find Peter tries to fix the same problem we encounter in qemu side,

  [Qemu-devel] [PATCH] gdbstub: Allow target CPUs to specify watchpoint STOP_BEFORE_ACCESS flag
  http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg02665.html

and this patch isn't accepted yet.

Without this patch, program stops two instructions after the variable is
updated on qemu trunk,

   0x000001ae <+10>:    str     r3, [r7, #12]
   0x000001b0 <+12>:    ldr     r3, [r7, #4]
=> 0x000001b2 <+14>:    cmp     r3, #1
   0x000001b4 <+16>:    bne.n   0x1ba <recurse+22>

however, with Peter's patch applied, program stops one instruction after
the variable is updated,

(gdb) watch b
Hardware watchpoint 3: b
(gdb) c
Continuing.
Hardware watchpoint 3: b

Old value = 1283
New value = 0
recurse (a=10) at ../../../../git/gdb/testsuite/gdb.base/recurse.c:15
15        if (a == 1)
(gdb) disassemble recurse
Dump of assembler code for function recurse:
   0x000001a4 <+0>:     push    {r7, lr}
   0x000001a6 <+2>:     sub     sp, #16
   0x000001a8 <+4>:     add     r7, sp, #0
   0x000001aa <+6>:     str     r0, [r7, #4]
   0x000001ac <+8>:     movs    r3, #0
   0x000001ae <+10>:    str     r3, [r7, #12]
=> 0x000001b0 <+12>:    ldr     r3, [r7, #4]
   0x000001b2 <+14>:    cmp     r3, #1

Note that with patched qemu, two fails in gdb.base/recurse.exp are
fixed.  At least, gdb and qemu should be in sync on this.

-- 
Yao (齐尧)

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Joel Brobecker]

> Can you elaborate your experiments?  Do you do experiments on qemu, arm
> bare metal targets or arm linux targets?

My experiments were on QEMU, but others tried on bare-metal. I tried
on GNU/Linux targets as well, but hardware watchpoints simply did
not work (no signal).

> I find Peter tries to fix the same problem we encounter in qemu side,
> 
>   [Qemu-devel] [PATCH] gdbstub: Allow target CPUs to specify watchpoint STOP_BEFORE_ACCESS flag
>   http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg02665.html
> 
> and this patch isn't accepted yet.
[...]
> Note that with patched qemu, two fails in gdb.base/recurse.exp are
> fixed.  At least, gdb and qemu should be in sync on this.

I think the experiments that were run showed that QEMU is in fact
correct and should NOT be changed.

-- 
Joel

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Luis Machado]

On 09/16/2014 08:59 AM, Joel Brobecker wrote:
>> Can you elaborate your experiments?  Do you do experiments on qemu, arm
>> bare metal targets or arm linux targets?
>
> My experiments were on QEMU, but others tried on bare-metal. I tried
> on GNU/Linux targets as well, but hardware watchpoints simply did
> not work (no signal).
>
>> I find Peter tries to fix the same problem we encounter in qemu side,
>>
>>    [Qemu-devel] [PATCH] gdbstub: Allow target CPUs to specify watchpoint STOP_BEFORE_ACCESS flag
>>    http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg02665.html
>>
>> and this patch isn't accepted yet.
> [...]
>> Note that with patched qemu, two fails in gdb.base/recurse.exp are
>> fixed.  At least, gdb and qemu should be in sync on this.
>
> I think the experiments that were run showed that QEMU is in fact
> correct and should NOT be changed.
>

Do we know what the Linux kernel's behavior on this one is? I wonder 
what the stopped data address shows.

Someone with access to a board with a relatively new kernel could try 
that and rule it out, otherwise we risk fixing something for QEMU/bare 
metal and breaking things for Linux.

Luis

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Joel Brobecker]

> >I think the experiments that were run showed that QEMU is in fact
> >correct and should NOT be changed.
> 
> Do we know what the Linux kernel's behavior on this one is? I wonder
> what the stopped data address shows.
> 
> Someone with access to a board with a relatively new kernel could
> try that and rule it out, otherwise we risk fixing something for
> QEMU/bare metal and breaking things for Linux.

When I tested on GNU/Linux, watchpoints simply did not work
(silently ignored, no signal).  I was using an old kernel (2012),
though; but that's all I had access to.  But, all in all, baremetal
should be our most reliable source of info, though,no? - no software
layer to murky the waters.

-- 
Joel

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Luis Machado]

On 09/16/2014 09:48 AM, Joel Brobecker wrote:
>>> I think the experiments that were run showed that QEMU is in fact
>>> correct and should NOT be changed.
>>
>> Do we know what the Linux kernel's behavior on this one is? I wonder
>> what the stopped data address shows.
>>
>> Someone with access to a board with a relatively new kernel could
>> try that and rule it out, otherwise we risk fixing something for
>> QEMU/bare metal and breaking things for Linux.
>
> When I tested on GNU/Linux, watchpoints simply did not work
> (silently ignored, no signal).  I was using an old kernel (2012),
> though; but that's all I had access to.  But, all in all, baremetal
> should be our most reliable source of info, though,no? - no software
> layer to murky the waters.
>

It is hard to tell. ARM's documentation is not clear. For example, this 
is probably where the stopped data address should be coming from:

--

WFAR - Watchpoint Fault Address Register

The WFAR is updated to indicate the address of the instruction that 
accessed the watchpointed address:

- the address of the instruction + 8 in ARM state
- the address of the instruction + 4 in Thumb® state

--

So it seems in line with what we are seeing? The program being trapped 
two instructions after the data fault?

If it stops just a single instruction after the data fault, then someone 
(probe, emulator or kernel) may be trying to help GDB by decrementing 
the data fault address.

Luis

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Pedro Alves]

Hi Terry, Marcus,

Can someone at ARM shed some light on this, please?

This thread is here:

 https://sourceware.org/ml/gdb-patches/2014-09/msg00498.html

And the discussion started in another thread here:

  https://sourceware.org/ml/gdb/2014-09/msg00000.html

I've just added a test that hopefully helps with this, btw:

 https://sourceware.org/ml/gdb-patches/2014-09/msg00535.html

I'm also wondering whether Aarch64 needs adjustment as well.

Thanks,
Pedro Alves

On 09/16/2014 02:09 PM, Luis Machado wrote:
> On 09/16/2014 09:48 AM, Joel Brobecker wrote:
>>>> I think the experiments that were run showed that QEMU is in fact
>>>> correct and should NOT be changed.
>>>
>>> Do we know what the Linux kernel's behavior on this one is? I wonder
>>> what the stopped data address shows.
>>>
>>> Someone with access to a board with a relatively new kernel could
>>> try that and rule it out, otherwise we risk fixing something for
>>> QEMU/bare metal and breaking things for Linux.
>>
>> When I tested on GNU/Linux, watchpoints simply did not work
>> (silently ignored, no signal).  I was using an old kernel (2012),
>> though; but that's all I had access to.  But, all in all, baremetal
>> should be our most reliable source of info, though,no? - no software
>> layer to murky the waters.
>>
> 
> It is hard to tell. ARM's documentation is not clear. For example, this 
> is probably where the stopped data address should be coming from:
> 
> --
> 
> WFAR - Watchpoint Fault Address Register
> 
> The WFAR is updated to indicate the address of the instruction that 
> accessed the watchpointed address:
> 
> - the address of the instruction + 8 in ARM state
> - the address of the instruction + 4 in Thumb® state
> 
> --
> 
> So it seems in line with what we are seeing? The program being trapped 
> two instructions after the data fault?
> 
> If it stops just a single instruction after the data fault, then someone 
> (probe, emulator or kernel) may be trying to help GDB by decrementing 
> the data fault address.
> 
> Luis
> 

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Marcus Shawcroft]

On 16 September 2014 16:21, Pedro Alves <palves@redhat.com> wrote:
> Hi Terry, Marcus,
>
> Can someone at ARM shed some light on this, please?
>
> This thread is here:
>
>  https://sourceware.org/ml/gdb-patches/2014-09/msg00498.html
>
> And the discussion started in another thread here:
>
>   https://sourceware.org/ml/gdb/2014-09/msg00000.html
>
> I've just added a test that hopefully helps with this, btw:
>
>  https://sourceware.org/ml/gdb-patches/2014-09/msg00535.html
>
> I'm also wondering whether Aarch64 needs adjustment as well.
>
> Thanks,
> Pedro Alves


Hi,

In aarch32 execution state a watch point event is taken as a data
abort with the PC containing the address of the faulting instruction +
8 irrespective of thumb mode.

The linux kernel adjusts the reported PC by subtracting 8 such that
the ptrace interface will indicate the address of the faulting
instruction.

Peter Maydell's proposed qemu patch referenced in the thread above
appears to me to align the gdbstub behaviour in qemu with the linux
kernel ptrace() interface behaviour.

w.r.t DBGWFAR, it's use is described as deprecated in  ARM ARMv7-A&R
Issue C.c  c11.11.45. It is not used by linux kernel.

Cheers
/Marcus

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Pedro Alves]

Hi Marcus,

On 09/18/2014 12:40 PM, Marcus Shawcroft wrote:
> On 16 September 2014 16:21, Pedro Alves <palves@redhat.com> wrote:
>> Hi Terry, Marcus,
>>
>> Can someone at ARM shed some light on this, please?
>>
>> This thread is here:
>>
>>  https://sourceware.org/ml/gdb-patches/2014-09/msg00498.html
>>
>> And the discussion started in another thread here:
>>
>>   https://sourceware.org/ml/gdb/2014-09/msg00000.html
>>
>> I've just added a test that hopefully helps with this, btw:
>>
>>  https://sourceware.org/ml/gdb-patches/2014-09/msg00535.html
>>
>> I'm also wondering whether Aarch64 needs adjustment as well.
>>

> In aarch32 execution state a watch point event is taken as a data
> abort with the PC containing the address of the faulting instruction +
> 8 irrespective of thumb mode.

So the documentation is actually wrong for thumb?  Or you're
saying that for Thumb2, while it'd be 4 for Thumb 1?

If you're talking about something else, not DBGWFAR (what I think
Luis pointed out before), then can you give a pointer to where these
other watch point events are documented?

> 
> The linux kernel adjusts the reported PC by subtracting 8 such that
> the ptrace interface will indicate the address of the faulting
> instruction.

Hmm.  So when the data abort triggers at fault+8, the instruction
that triggered the abort hasn't actually completed, right?  No memory
has changed yet.

So if nothing does the adjustment, like Gareth found out happens with
the Black Magic Probe, then we'll resume execution from the
wrong address/instruction (with the effects of the skipped instructions
missing, including the memory write...).  Did I understand that
right?  (Gareth, is that what you see?)

> Peter Maydell's proposed qemu patch referenced in the thread above
> appears to me to align the gdbstub behaviour in qemu with the linux
> kernel ptrace() interface behaviour.
> 
> w.r.t DBGWFAR, it's use is described as deprecated in  ARM ARMv7-A&R
> Issue C.c  c11.11.45. It is not used by linux kernel.

Thanks,
Pedro Alves

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Joel Brobecker]

Hello all,

> Hmm.  So when the data abort triggers at fault+8, the instruction
> that triggered the abort hasn't actually completed, right?  No memory
> has changed yet.
> 
> So if nothing does the adjustment, like Gareth found out happens with
> the Black Magic Probe, then we'll resume execution from the
> wrong address/instruction (with the effects of the skipped instructions
> missing, including the memory write...).  Did I understand that
> right?  (Gareth, is that what you see?)

I have been trying to understand the various contributions, and
I admit I am still not quite sure...

Does it look like the patch I proposed is correct? It seems to be
supported by Terry Guo's experiments as well...

Thanks!
-- 
Joel

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Luis Machado]

On 09/29/2014 02:51 PM, Joel Brobecker wrote:
> Hello all,
>
>> Hmm.  So when the data abort triggers at fault+8, the instruction
>> that triggered the abort hasn't actually completed, right?  No memory
>> has changed yet.
>>
>> So if nothing does the adjustment, like Gareth found out happens with
>> the Black Magic Probe, then we'll resume execution from the
>> wrong address/instruction (with the effects of the skipped instructions
>> missing, including the memory write...).  Did I understand that
>> right?  (Gareth, is that what you see?)
>
> I have been trying to understand the various contributions, and
> I admit I am still not quite sure...
>
> Does it look like the patch I proposed is correct? It seems to be
> supported by Terry Guo's experiments as well...
>
> Thanks!
>

 From previous mails, it does not seem to be correct for Linux, where 
the ptrace interface adjusts the data fault address to point to the 
address of the instruction that caused the trigger. So it looks like the 
current behavior of GDB is correct for Linux, though it may not be 
correct for QEMU or bare metal.

Luis

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Pedro Alves]

On 09/29/2014 06:51 PM, Joel Brobecker wrote:
> Hello all,
> 
>> Hmm.  So when the data abort triggers at fault+8, the instruction
>> that triggered the abort hasn't actually completed, right?  No memory
>> has changed yet.
>>
>> So if nothing does the adjustment, like Gareth found out happens with
>> the Black Magic Probe, then we'll resume execution from the
>> wrong address/instruction (with the effects of the skipped instructions
>> missing, including the memory write...).  Did I understand that
>> right?  (Gareth, is that what you see?)
> 
> I have been trying to understand the various contributions, and
> I admit I am still not quite sure...
> 
> Does it look like the patch I proposed is correct? It seems to be
> supported by Terry Guo's experiments as well...

Nope, Terry's experiments supported the current code.

The experiments (which were on Linux) showed that the watchpoint was
reported to GDB first with the PC pointing at the instruction that
accessed memory, and then GDB single-stepped once, and the PC ends up
pointing at one instruction after the instruction that changed memory.

Thanks,
Pedro Alves

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint.

[Will Deacon]

On Mon, Sep 29, 2014 at 10:04:11PM +0100, Pedro Alves wrote:
> On 09/29/2014 06:51 PM, Joel Brobecker wrote:
> > Hello all,
> > 
> >> Hmm.  So when the data abort triggers at fault+8, the instruction
> >> that triggered the abort hasn't actually completed, right?  No memory
> >> has changed yet.
> >>
> >> So if nothing does the adjustment, like Gareth found out happens with
> >> the Black Magic Probe, then we'll resume execution from the
> >> wrong address/instruction (with the effects of the skipped instructions
> >> missing, including the memory write...).  Did I understand that
> >> right?  (Gareth, is that what you see?)
> > 
> > I have been trying to understand the various contributions, and
> > I admit I am still not quite sure...
> > 
> > Does it look like the patch I proposed is correct? It seems to be
> > supported by Terry Guo's experiments as well...
> 
> Nope, Terry's experiments supported the current code.
> 
> The experiments (which were on Linux) showed that the watchpoint was
> reported to GDB first with the PC pointing at the instruction that
> accessed memory, and then GDB single-stepped once, and the PC ends up
> pointing at one instruction after the instruction that changed memory.

FWIW, that also matches the intention of the kernel-side code. The same
logic applies to arm64, despite the availability of hardware single-step
there (the PTRACE_SINGLESTEP request can be used to access that feature).

Furthermore, this also matches the ARMv7/8 debug architectures; a
watchpoint data abort will be taken before the faulting instruction has
executed.

Will

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Peter Maydell]

On 30 September 2014 15:26, Joel Brobecker <brobecker@adacore.com> wrote:
> That's very interesting. When I talked to one of our QEMU developers
> in house about this, he wasn't even sure if it was possible for
> QEMU to stop at the instruction triggering the watchpoint. Now,
> we know it is, but it could be simpler for QEMU to implement the
> same policy on all targets.

QEMU's stub has to implement "act like the hardware does" for
each individual target CPU, because that's what existing gdb
implementations in the field expect. "Act the same in the
stub regardless of target CPU" is what we used to do and
the result was not stopping in the right place.

The actual in-QEMU implementation is pretty trivial, since
we already need to support both behaviours in order to
correctly emulate the architectural debug support we
expose to guest OSes, and it's then just a matter of
setting or not setting a flag when we set up the wp.

-- PMM

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Joel Brobecker]

> I assume that WFAR/DSCR are privileged registers though.
> On Linux, for example, they're not exported to userspace.

Indeed, I was expecting as much...

> > Informing the user about how
> > far would certainly be a useful info for the user. The only
> > part I'm unclear about is whether it's OK to check WFAR when
> > in synchronous mode, and whether it'll have a WFAR=0 in case
> > of a synchronous breakpoint...
> 
> I think it'd be better leave those details to the
> remote stub / OS though.  E.g., this way, qemu's gdbserver
> stub may support watchpoint variants that the hardware
> qemu is emulating doesn't support.

Sure! That's what I meant, and was thinking of our gdbserver
implementation. But now that I think this through, of course
GDBserver is mostly implemented... on top of an OS! Duh...

> For instance, as a natural extension of this, we could
> make it possible for qemu to have non-continuable watchpoints
> (trap before the instruction that changes memory executes) on all
> targets, even x86.  Or have it trap after the instruction
> that changes memory, but tell GDB the address of the instruction
> that triggered the watchpoint (there's no magic number to subtract
> on x86, due to variable-length instructions).

That's very interesting. When I talked to one of our QEMU developers
in house about this, he wasn't even sure if it was possible for
QEMU to stop at the instruction triggering the watchpoint. Now,
we know it is, but it could be simpler for QEMU to implement the
same policy on all targets.

-- 
Joel

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Pedro Alves]

On 09/30/2014 02:50 PM, Joel Brobecker wrote:
>> BTW, given v7-m behaves like this as well, it sounds
>> like this may not be the last we hear about asynchronous
>> watchpoints (thinking bare-metal here).
>>
>> But, I've given this further thought while cooking lunch.  :-)
>>
>> Given that with asynchronous watchpoints, any number
>> of instructions could have been executed, which isn't
>> exactly the same as always triggering the exception just
>> after the instruction completes, and, since the instruction
>> that triggered the watchpoint can be discovered (in WFAR), I
>> think we should indeed assume synchronous watchpoints by
>> default, and then handle asynchronous watchpoints by
>> augmenting the watchpoint event (packet) reported to GDB
>> by indicating the asyncness and the instruction
>> that triggered the exception (if known).  On such targets,
>> GDB could be a bit more helpful and if execution stops far
>> from where the watchpoint triggered, it could tell that to
>> the user.  On Linux, if we wanted to expose this to the
>> ptracer, we'd stuff it somewhere in the SIGTRAP's siginfo.
>>
>> How does that sound?
>>
>> In a nutshell, less guesswork for GDB, by making the
>> target be more precise in its event reporting.
> 
> I was thinking about something along the same lines; a little
> less sophisticated perhaps: check WFAR, and if far enough,
> then cancel the single-step. 

I assume that WFAR/DSCR are privileged registers though.
On Linux, for example, they're not exported to userspace.

> Informing the user about how
> far would certainly be a useful info for the user. The only
> part I'm unclear about is whether it's OK to check WFAR when
> in synchronous mode, and whether it'll have a WFAR=0 in case
> of a synchronous breakpoint...

I think it'd be better leave those details to the
remote stub / OS though.  E.g., this way, qemu's gdbserver
stub may support watchpoint variants that the hardware
qemu is emulating doesn't support.

For instance, as a natural extension of this, we could
make it possible for qemu to have non-continuable watchpoints
(trap before the instruction that changes memory executes) on all
targets, even x86.  Or have it trap after the instruction
that changes memory, but tell GDB the address of the instruction
that triggered the watchpoint (there's no magic number to subtract
on x86, due to variable-length instructions).

Thanks,
Pedro Alves

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Joel Brobecker]

> BTW, given v7-m behaves like this as well, it sounds
> like this may not be the last we hear about asynchronous
> watchpoints (thinking bare-metal here).
> 
> But, I've given this further thought while cooking lunch.  :-)
> 
> Given that with asynchronous watchpoints, any number
> of instructions could have been executed, which isn't
> exactly the same as always triggering the exception just
> after the instruction completes, and, since the instruction
> that triggered the watchpoint can be discovered (in WFAR), I
> think we should indeed assume synchronous watchpoints by
> default, and then handle asynchronous watchpoints by
> augmenting the watchpoint event (packet) reported to GDB
> by indicating the asyncness and the instruction
> that triggered the exception (if known).  On such targets,
> GDB could be a bit more helpful and if execution stops far
> from where the watchpoint triggered, it could tell that to
> the user.  On Linux, if we wanted to expose this to the
> ptracer, we'd stuff it somewhere in the SIGTRAP's siginfo.
> 
> How does that sound?
> 
> In a nutshell, less guesswork for GDB, by making the
> target be more precise in its event reporting.

I was thinking about something along the same lines; a little
less sophisticated perhaps: check WFAR, and if far enough,
then cancel the single-step. Informing the user about how
far would certainly be a useful info for the user. The only
part I'm unclear about is whether it's OK to check WFAR when
in synchronous mode, and whether it'll have a WFAR=0 in case
of a synchronous breakpoint...

-- 
Joel

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Pedro Alves]

On 09/30/2014 11:34 AM, Pedro Alves wrote:

>> (Similarly,
>> we provide watchpoint support in our stub even if
>> the CPU we're emulating has no watchpoint support
>> of its own at all. Think of us as like a JTAG probe.)
> 
> Well, it seems to me that GDB, on v6-and-lower is
> doing the wrong thing for real halt-mode/jtag probes.
> If we fix that in GDB, then your qemu patch breaks
> things on v6.

BTW, given v7-m behaves like this as well, it sounds
like this may not be the last we hear about asynchronous
watchpoints (thinking bare-metal here).

But, I've given this further thought while cooking lunch.  :-)

Given that with asynchronous watchpoints, any number
of instructions could have been executed, which isn't
exactly the same as always triggering the exception just
after the instruction completes, and, since the instruction
that triggered the watchpoint can be discovered (in WFAR), I
think we should indeed assume synchronous watchpoints by
default, and then handle asynchronous watchpoints by
augmenting the watchpoint event (packet) reported to GDB
by indicating the asyncness and the instruction
that triggered the exception (if known).  On such targets,
GDB could be a bit more helpful and if execution stops far
from where the watchpoint triggered, it could tell that to
the user.  On Linux, if we wanted to expose this to the
ptracer, we'd stuff it somewhere in the SIGTRAP's siginfo.

How does that sound?

In a nutshell, less guesswork for GDB, by making the
target be more precise in its event reporting.

Thanks,
Pedro Alves

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Pedro Alves]

On 09/30/2014 11:17 AM, Peter Maydell wrote:
> On 30 September 2014 11:07, Pedro Alves <palves@redhat.com> wrote:
>>  WFAR - Watchpoint Fault Address Register
>>
>>  The WFAR is updated to indicate the address of the instruction that
>>  accessed the watchpointed address:
>>
>>  - the address of the instruction + 8 in ARM state
>>  - the address of the instruction + 4 in Thumb® state
>>
>> What wasn't clear to me was whether this meant that the
>> instruction at the address of the instruction, and
>> at the the address of the instruction +4/+2 (ARM/Thumb) had
>> executed completely or not.  It's my understanding now that,
>> yes, both the instruction that caused the watchpoint and the
>> instruction after that one have already been executed.
> 
> No, you are misinterpreting this. The WFAR records
> the address of the offending instruction + an offset
> which you have to correct for. The offset does *not*
> have any relation to how many further instructions
> the CPU has executed after the offending instruction,
> which could be none, one, two or ten. Asynchronous
> watchpoints are *asynchronous*, which means there
> is no timing guarantee about how soon the CPU will
> notice that one has fired and stop executing insns.
> The only guarantee you get in v7 is that you'll get
> the watchpoint before any exception that might be
> caused by a following instruction.

OK.  It's clear now.

Thanks you!

Pedro Alves

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Pedro Alves]

On 09/30/2014 11:00 AM, Peter Maydell wrote:
> On 30 September 2014 10:08, Pedro Alves <palves@redhat.com> wrote:
>> On 09/29/2014 11:53 PM, Peter Maydell wrote:
>>> There's an assertion in this LKML post from 2010:
>>> https://lkml.org/lkml/2010/4/14/127
>>> that v7 cores do actually all generate synchronous
>>> watchpoint exceptions (even though architecturally
>>> they're permitted not to). Was your test h/w a v6?
>>
>> Joel's test was against qemu (without your patch).
>>
>> Terry's tests were against armv7l and armv8.  Both synchronous.
>>
>> The report that confuses me is Gareth's:
>>
>>   https://sourceware.org/ml/gdb/2014-09/msg00013.html
>>
>> As it sounds like he has v7-m hardware that has asynchronous
>> behavior.  Gareth, can you confirm this, please?
> 
> In general it's unwise to assume that statements
> about the ARM A and R profiles carry across to M
> profile... 

Ah.  Noted.

> v7M profile watchpoints are rather
> different from v7AR watchpoints in terms of how you
> set them, how they're reported, etc, and they're
> always asynchronous (other insns may execute after
> the one which triggers the wp before the debug event
> fires).
> 
>> Still, in any case, from that LKML post:
>>
>>  "v6 cores are the opposite; they only generate asynchronous
>>   watchpoint exceptions".
>>
>> So, eh!?  Does your qemu patch take this into account?  Seems
>> like it should.
> 
> My QEMU patch is for the built in gdbstub, which is
> completely different code to the emulation of the
> CPU's own architected debug hardware. (We implement
> the latter only for v7 and above, not v6.)
> 
> It doesn't seem very sensible to me to deliberately
> provide unhelpful asynchronous watchpoint support
> on v6-and-lower guest CPUs just because that's what
> the hardware does, especially since it would mean we
> wouldn't interoperate with current gdb. 

But current GDB is wrong, and it wasn't always wrong.

I see now that GDB switched to assuming synchronous
watchpoints not that long ago:

  https://sourceware.org/ml/gdb-patches/2010-07/msg00110.html

That's git e3039479.

That was only when we added support for Linux watchpoints, after
7.2.  It seems like non-Linux / bare-metal was forgotten in
that patch, and so that broke qemu watchpoints.

IOW, in GDB 7.2 and before, GDB assumed asynchronous watchpoints
on ARM, and that's what qemu implemented.

So that was really a regression that went by unnoticed.

Probably nobody complained so far because usually one
doesn't notice GDB stopped execution one instruction too
far.  See Joel's hypothesis, which I agree with:

  https://sourceware.org/ml/gdb/2014-09/msg00015.html

> (Similarly,
> we provide watchpoint support in our stub even if
> the CPU we're emulating has no watchpoint support
> of its own at all. Think of us as like a JTAG probe.)

Well, it seems to me that GDB, on v6-and-lower is
doing the wrong thing for real halt-mode/jtag probes.
If we fix that in GDB, then your qemu patch breaks
things on v6.

> 
>> Now I'm confused on the mention of the Linux kernel
>> subtracting 8 from the PC to help GDB.  I can't find that
>> anywhere in the kernel's sources.
> 
> This is a reference to the standard ARM exception
> entry behaviour where the value saved to the link
> register may be +2, +4 or +8 from the "preferred
> return address" for the exception. The kernel handles
> this via a 'vector_stub' macro that adjusts the
> value read from LR so the rest of the kernel can
> deal simply in preferred return addresses. Since
> sync. watchpoints are a kind of data abort they
> go through here, with a correction value of 8:
> http://lxr.free-electrons.com/source/arch/arm/kernel/entry-armv.S#L1043

Ah.  Thanks.

Thanks,
Pedro Alves

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Peter Maydell]

On 30 September 2014 11:07, Pedro Alves <palves@redhat.com> wrote:
>  WFAR - Watchpoint Fault Address Register
>
>  The WFAR is updated to indicate the address of the instruction that
>  accessed the watchpointed address:
>
>  - the address of the instruction + 8 in ARM state
>  - the address of the instruction + 4 in Thumb® state
>
> What wasn't clear to me was whether this meant that the
> instruction at the address of the instruction, and
> at the the address of the instruction +4/+2 (ARM/Thumb) had
> executed completely or not.  It's my understanding now that,
> yes, both the instruction that caused the watchpoint and the
> instruction after that one have already been executed.

No, you are misinterpreting this. The WFAR records
the address of the offending instruction + an offset
which you have to correct for. The offset does *not*
have any relation to how many further instructions
the CPU has executed after the offending instruction,
which could be none, one, two or ten. Asynchronous
watchpoints are *asynchronous*, which means there
is no timing guarantee about how soon the CPU will
notice that one has fired and stop executing insns.
The only guarantee you get in v7 is that you'll get
the watchpoint before any exception that might be
caused by a following instruction.

-- PMM

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Pedro Alves]

On 09/30/2014 10:18 AM, Will Deacon wrote:
> On Tue, Sep 30, 2014 at 10:08:09AM +0100, Pedro Alves wrote:
>> The report that confuses me is Gareth's:
>>
>>   https://sourceware.org/ml/gdb/2014-09/msg00013.html
>>
>> As it sounds like he has v7-m hardware that has asynchronous
>> behavior.  Gareth, can you confirm this, please?
> 
> Running Linux or bare-metal? The hw_breakpoint code in the kernel certainly
> wasn't written with v7-m in mind and I'd be surprised if it even probed
> successfully at boot.

Judging from the black magic probes' description at:
http://www.blacksphere.co.nz/main/blackmagic, definitely bare-metal.

> 
>> Still, in any case, from that LKML post:
>>
>>  "v6 cores are the opposite; they only generate asynchronous
>>   watchpoint exceptions".
>>
>> So, eh!?  Does your qemu patch take this into account?  Seems
>> like it should.
> 
> Hmm, I didn't realise v6 was on the table here.

GDB supports that, and older.  Doesn't qemu do v6?

> In that case, you need to signal an async exception and set the WFAR
> register to indicate the watchpointed instruction. Not that Linux uses this...

Yeah, even though Linux doesn't use this, this then must be what bare metal
probes and other OSs will necessarily be using to support watchpoints on < v7.

OK, then on v6 (and earlier), the WFAR will be (as Luis pointed out
elsewhere), from:

(ARM1156T2F-S, IIUC, that's a v6)
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0290g/Bhcdjdcg.html

 WFAR - Watchpoint Fault Address Register

 The WFAR is updated to indicate the address of the instruction that
 accessed the watchpointed address:

 - the address of the instruction + 8 in ARM state
 - the address of the instruction + 4 in Thumb® state

What wasn't clear to me was whether this meant that the
instruction at the address of the instruction, and
at the the address of the instruction +4/+2 (ARM/Thumb) had
executed completely or not.  It's my understanding now that,
yes, both the instruction that caused the watchpoint and the
instruction after that one have already been executed.

I may be missing something, but that page leaves me wondering
what happens if the instruction after the instruction that caused
the watchpoint is a branch.  Will the WFAR point still point
at "address + 8" ?  Or will it point at the branch destination?

In any case, it sounds like _nothing_ should be unwinding the PC
back -8 nor -4 to help the debugger, as that would make the
program re-execute instructions that had already executed.

For further confirmation, I looked at the v6-m manual I have
handy (DDI 0419C).  There, I see:

 C1.5.2:

  The following are asynchronous debug events:
  - Watchpoint debug events, including PC match watchpoints

And then on C1.6.4

 The DebugReturnAddress value

 DebugReturnAddress is the address of the first instruction to be executed on exit from Debug state. This
 address indicates the point in the execution stream where the debug event was invoked. For a hardware or
 a software breakpoint, this is the address of the breakpointed instruction. For all other debug events,
 including PC match watchpoints, DebugReturnAddress is the address of the first instruction that both:

 - in a simple sequential execution of the program, executes after the instruction that caused the debug
 event

 - has not been executed.

And in C1.7.1

 A watchpoint event is asynchronous to the instruction that caused it. The DebugReturnAddress value for a
 watchpoint event must be that of an instruction to be executed after the instruction responsible for
 generating the watchpoint.

This seems to confirm my current understanding that indeed that +8/+4 offset
in WFAR should _never_ be adjusted, as the instructions at +0 and +4/+2 have
already been executed.


So, my current understanding is that when debugging a < v7 target, GDB
should always treat watchpoints as continuable (asynchronous), as that's
all the hardware supports/supported.

And when debugging a >= v7 target, GDB should treat watchpoints as
synchronous as that's all real hardware supports (even though
the architecture permits both sync and async, in theory).

And if we do this in GDB, then it's be better that qemu likewise
treats watchpoints as asynchronous on < v7, so that GDB doesn't need to
know whether it's talking to real bare-metal and against qemu.

Does that make sense?

> The comment/code above is about finding the address of the memory access
> that triggered the watchpoint, as opposed to the address of the instruction.
> (e.g. if I load from address FOO, then I only get told about FOO in v7.1).

Understood.

Thanks,
Pedro Alves

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Peter Maydell]

On 30 September 2014 10:08, Pedro Alves <palves@redhat.com> wrote:
> On 09/29/2014 11:53 PM, Peter Maydell wrote:
>> There's an assertion in this LKML post from 2010:
>> https://lkml.org/lkml/2010/4/14/127
>> that v7 cores do actually all generate synchronous
>> watchpoint exceptions (even though architecturally
>> they're permitted not to). Was your test h/w a v6?
>
> Joel's test was against qemu (without your patch).
>
> Terry's tests were against armv7l and armv8.  Both synchronous.
>
> The report that confuses me is Gareth's:
>
>   https://sourceware.org/ml/gdb/2014-09/msg00013.html
>
> As it sounds like he has v7-m hardware that has asynchronous
> behavior.  Gareth, can you confirm this, please?

In general it's unwise to assume that statements
about the ARM A and R profiles carry across to M
profile... v7M profile watchpoints are rather
different from v7AR watchpoints in terms of how you
set them, how they're reported, etc, and they're
always asynchronous (other insns may execute after
the one which triggers the wp before the debug event
fires).

> Still, in any case, from that LKML post:
>
>  "v6 cores are the opposite; they only generate asynchronous
>   watchpoint exceptions".
>
> So, eh!?  Does your qemu patch take this into account?  Seems
> like it should.

My QEMU patch is for the built in gdbstub, which is
completely different code to the emulation of the
CPU's own architected debug hardware. (We implement
the latter only for v7 and above, not v6.)

It doesn't seem very sensible to me to deliberately
provide unhelpful asynchronous watchpoint support
on v6-and-lower guest CPUs just because that's what
the hardware does, especially since it would mean we
wouldn't interoperate with current gdb. (Similarly,
we provide watchpoint support in our stub even if
the CPU we're emulating has no watchpoint support
of its own at all. Think of us as like a JTAG probe.)

> Now I'm confused on the mention of the Linux kernel
> subtracting 8 from the PC to help GDB.  I can't find that
> anywhere in the kernel's sources.

This is a reference to the standard ARM exception
entry behaviour where the value saved to the link
register may be +2, +4 or +8 from the "preferred
return address" for the exception. The kernel handles
this via a 'vector_stub' macro that adjusts the
value read from LR so the rest of the kernel can
deal simply in preferred return addresses. Since
sync. watchpoints are a kind of data abort they
go through here, with a correction value of 8:
http://lxr.free-electrons.com/source/arch/arm/kernel/entry-armv.S#L1043

thanks
-- PMM

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Will Deacon]

On Tue, Sep 30, 2014 at 10:14:20AM +0100, Pedro Alves wrote:
> On 09/30/2014 09:57 AM, Will Deacon wrote:
> > On Mon, Sep 29, 2014 at 07:23:05PM +0100, Peter Maydell wrote:
> >> Joel Brobecker wrote:
> >>> I have been trying to understand the various contributions, and
> >>> I admit I am still not quite sure...
> >>>
> >>> Does it look like the patch I proposed is correct? It seems to be
> >>> supported by Terry Guo's experiments as well...
> >>
> >> Note that the ARMv7 architecture allows watchpoints to
> >> be implemented as *asynchronous*, in which case what
> >> you will see is that you take a watchpoint exception
> >> but it may not fire until after the instruction that
> >> triggers the watchpoint and possibly several following
> >> instructions have all finished execution. This may be
> >> what you are seeing in your hardware tests.
> > 
> > No you won't; the kernel will swallow the async watchpoint and complain in
> > dmesg. 
> 
> It doesn't seem to swallow it; only warn.  In hw_breakpoint_pending:
> 
> 
> 	/* Perform perf callbacks. */
> 	switch (ARM_DSCR_MOE(dscr)) {
> 	case ARM_ENTRY_BREAKPOINT:
> 		breakpoint_handler(addr, regs);
> 		break;
> 	case ARM_ENTRY_ASYNC_WATCHPOINT:
> 		WARN(1, "Asynchronous watchpoint exception taken. Debugging results may be unreliable\n");
> 	case ARM_ENTRY_SYNC_WATCHPOINT:
> 		watchpoint_handler(addr, fsr, regs);
> 		break;
> 
> Note the fallthrough.
> 
> In any case, GDB has to care about halt mode, jtag, system, etc.
> debugging, not just Linux.
> 
> > I'm not aware of any CPU implementations that actually generate these.
> 
> Thanks.  Until we hear a clearer report otherwise, that's what I'll
> assume going forward.

Now that I've realised you were also talking about ARMv6, I should clarify
that I was only referring to v7-A (and v8, since that's architected) CPUs.

For v6, all watchpoint exceptions are reported via the asynchronous method.
You can use the WFAR register to determine the instruction responsible for
the fault.

Will

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Will Deacon]

On Tue, Sep 30, 2014 at 10:08:09AM +0100, Pedro Alves wrote:
> The report that confuses me is Gareth's:
> 
>   https://sourceware.org/ml/gdb/2014-09/msg00013.html
> 
> As it sounds like he has v7-m hardware that has asynchronous
> behavior.  Gareth, can you confirm this, please?

Running Linux or bare-metal? The hw_breakpoint code in the kernel certainly
wasn't written with v7-m in mind and I'd be surprised if it even probed
successfully at boot.

> Still, in any case, from that LKML post:
> 
>  "v6 cores are the opposite; they only generate asynchronous
>   watchpoint exceptions".
> 
> So, eh!?  Does your qemu patch take this into account?  Seems
> like it should.

Hmm, I didn't realise v6 was on the table here. In that case, you need to
signal an async exception and set the WFAR register to indicate the
watchpointed instruction. Not that Linux uses this...

> In Linux's sources I see this:
> 
> /* Determine number of usable WRPs available. */
> static int get_num_wrps(void)
> {
> 	/*
> 	 * On debug architectures prior to 7.1, when a watchpoint fires, the
> 	 * only way to work out which watchpoint it was is by disassembling
> 	 * the faulting instruction and working out the address of the memory
> 	 * access.
> 	 *
> 	 * Furthermore, we can only do this if the watchpoint was precise
> 	 * since imprecise watchpoints prevent us from calculating register
> 	 * based addresses.
> 	 *
> 	 * Providing we have more than 1 breakpoint register, we only report
> 	 * a single watchpoint register for the time being. This way, we always
> 	 * know which watchpoint fired. In the future we can either add a
> 	 * disassembler and address generation emulator, or we can insert a
> 	 * check to see if the DFAR is set on watchpoint exception entry
> 	 * [the ARM ARM states that the DFAR is UNKNOWN, but experience shows
> 	 * that it is set on some implementations].
> 	 */
> 	if (get_debug_arch() < ARM_DEBUG_ARCH_V7_1)
> 		return 1;
> 
> So, even on Linux, on v6, etc. (< v7.1), it seems to me that we're
> indeed very likely to get _asynchronous_ watchpoints reported to GDB,
> and so this in GDB:

The comment/code above is about finding the address of the memory access
that triggered the watchpoint, as opposed to the address of the instruction.
(e.g. if I load from address FOO, then I only get told about FOO in v7.1).

Will

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Pedro Alves]

On 09/30/2014 09:57 AM, Will Deacon wrote:
> On Mon, Sep 29, 2014 at 07:23:05PM +0100, Peter Maydell wrote:
>> Joel Brobecker wrote:
>>> I have been trying to understand the various contributions, and
>>> I admit I am still not quite sure...
>>>
>>> Does it look like the patch I proposed is correct? It seems to be
>>> supported by Terry Guo's experiments as well...
>>
>> Note that the ARMv7 architecture allows watchpoints to
>> be implemented as *asynchronous*, in which case what
>> you will see is that you take a watchpoint exception
>> but it may not fire until after the instruction that
>> triggers the watchpoint and possibly several following
>> instructions have all finished execution. This may be
>> what you are seeing in your hardware tests.
> 
> No you won't; the kernel will swallow the async watchpoint and complain in
> dmesg. 

It doesn't seem to swallow it; only warn.  In hw_breakpoint_pending:


	/* Perform perf callbacks. */
	switch (ARM_DSCR_MOE(dscr)) {
	case ARM_ENTRY_BREAKPOINT:
		breakpoint_handler(addr, regs);
		break;
	case ARM_ENTRY_ASYNC_WATCHPOINT:
		WARN(1, "Asynchronous watchpoint exception taken. Debugging results may be unreliable\n");
	case ARM_ENTRY_SYNC_WATCHPOINT:
		watchpoint_handler(addr, fsr, regs);
		break;

Note the fallthrough.

In any case, GDB has to care about halt mode, jtag, system, etc.
debugging, not just Linux.

> I'm not aware of any CPU implementations that actually generate these.

Thanks.  Until we hear a clearer report otherwise, that's what I'll
assume going forward.

Thanks,
Pedro Alves

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Pedro Alves]

On 09/29/2014 11:53 PM, Peter Maydell wrote:
> On 29 September 2014 23:15, Pedro Alves <palves@redhat.com> wrote:
>> On 09/29/2014 07:23 PM, Peter Maydell wrote:


>> "Incorrect" may be too strong then, but understood.
> 
> I wrote the QEMU patch; I'm happy to call our old
> behaviour incorrect :-)

:-)

>> I think the most flexible would be if the watchpoint
>> event reported to GDB indicated which type it got, so
>> that'd support the case an arch ever supports mixing both
>> kinds of watchpoints.
> 
> Ha, I hadn't noticed that the architecture permits an
> implementation to provide both kinds (or indeed to
> have one watchpoint that might fire in either way), but
> you're right that it's theoretically allowed.

Yeah.  But not just ARM -- but more flexible for all archs
and emulators.

>> Or we just forget all this, assuming that ARM chips that
>> have async watchpoints will disappear into obsolescence
>> forever soon enough.  :-)
> 
> There's an assertion in this LKML post from 2010:
> https://lkml.org/lkml/2010/4/14/127
> that v7 cores do actually all generate synchronous
> watchpoint exceptions (even though architecturally
> they're permitted not to). Was your test h/w a v6?

Joel's test was against qemu (without your patch).

Terry's tests were against armv7l and armv8.  Both synchronous.

The report that confuses me is Gareth's:

  https://sourceware.org/ml/gdb/2014-09/msg00013.html

As it sounds like he has v7-m hardware that has asynchronous
behavior.  Gareth, can you confirm this, please?

Still, in any case, from that LKML post:

 "v6 cores are the opposite; they only generate asynchronous
  watchpoint exceptions".

So, eh!?  Does your qemu patch take this into account?  Seems
like it should.

In Linux's sources I see this:

/* Determine number of usable WRPs available. */
static int get_num_wrps(void)
{
	/*
	 * On debug architectures prior to 7.1, when a watchpoint fires, the
	 * only way to work out which watchpoint it was is by disassembling
	 * the faulting instruction and working out the address of the memory
	 * access.
	 *
	 * Furthermore, we can only do this if the watchpoint was precise
	 * since imprecise watchpoints prevent us from calculating register
	 * based addresses.
	 *
	 * Providing we have more than 1 breakpoint register, we only report
	 * a single watchpoint register for the time being. This way, we always
	 * know which watchpoint fired. In the future we can either add a
	 * disassembler and address generation emulator, or we can insert a
	 * check to see if the DFAR is set on watchpoint exception entry
	 * [the ARM ARM states that the DFAR is UNKNOWN, but experience shows
	 * that it is set on some implementations].
	 */
	if (get_debug_arch() < ARM_DEBUG_ARCH_V7_1)
		return 1;

So, even on Linux, on v6, etc. (< v7.1), it seems to me that we're
indeed very likely to get _asynchronous_ watchpoints reported to GDB,
and so this in GDB:

  /* Watchpoints are not steppable.  */
  set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);

should be skipped on < v7.1 ...

> If this is a v6-and-earlier thing I'd certainly be tempted
> to sweep the issue under the carpet...

Thanks,
Pedro Alves

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Will Deacon]

On Tue, Sep 30, 2014 at 09:57:46AM +0100, Will Deacon wrote:
> On Mon, Sep 29, 2014 at 07:23:05PM +0100, Peter Maydell wrote:
> > Joel Brobecker wrote:
> > > I have been trying to understand the various contributions, and
> > > I admit I am still not quite sure...
> > >
> > > Does it look like the patch I proposed is correct? It seems to be
> > > supported by Terry Guo's experiments as well...
> > 
> > Note that the ARMv7 architecture allows watchpoints to
> > be implemented as *asynchronous*, in which case what
> > you will see is that you take a watchpoint exception
> > but it may not fire until after the instruction that
> > triggers the watchpoint and possibly several following
> > instructions have all finished execution. This may be
> > what you are seeing in your hardware tests.
> 
> No you won't; the kernel will swallow the async watchpoint and complain in
> dmesg. I'm not aware of any CPU implementations that actually generate
> these.

D'oh, the lack of morning coffee means I missed the fall-through that Pedro
pointed out. I should go find the author of that code...

Will

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Will Deacon]

On Mon, Sep 29, 2014 at 07:23:05PM +0100, Peter Maydell wrote:
> Joel Brobecker wrote:
> > I have been trying to understand the various contributions, and
> > I admit I am still not quite sure...
> >
> > Does it look like the patch I proposed is correct? It seems to be
> > supported by Terry Guo's experiments as well...
> 
> Note that the ARMv7 architecture allows watchpoints to
> be implemented as *asynchronous*, in which case what
> you will see is that you take a watchpoint exception
> but it may not fire until after the instruction that
> triggers the watchpoint and possibly several following
> instructions have all finished execution. This may be
> what you are seeing in your hardware tests.

No you won't; the kernel will swallow the async watchpoint and complain in
dmesg. I'm not aware of any CPU implementations that actually generate
these.

Will

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Peter Maydell]

On 29 September 2014 23:15, Pedro Alves <palves@redhat.com> wrote:
> On 09/29/2014 07:23 PM, Peter Maydell wrote:
>> Note that the ARMv7 architecture allows watchpoints to
>> be implemented as *asynchronous*

> I only have DDI 0406C.b handy, which says:
>
>  ARMv7 permits watchpoints to be either synchronous or asynchronous. An implementation can implement
>  synchronous watchpoints, asynchronous watchpoints, or both. It is IMPLEMENTATION DEFINED under what
>  circumstances a watchpoint is synchronous or asynchronous.
>
> Ouch.  So this IMPLEMENTATION DEFINED note means this isn't really
> in control of the software/debugger, i.e., nothing a stub/probe
> could tweak, but instead baked into the specific ARM chip?

Correct. IMPDEF means that it is the choice of the
CPU implementation which behaviour to take.

>> QEMU's built in gdbstub was incorrectly not implementing
>> synchronous watchpoints (ie it was halting after the
>> execution of the offending insn, not before). This is fixed
>> by the QEMU patch referenced earlier, and with that patch
>> QEMU and GDB interoperate correctly (on ARM and also on
>> other architectures which have the "stop before insn"
>> watchpoint semantics).
>
> "Incorrect" may be too strong then, but understood.

I wrote the QEMU patch; I'm happy to call our old
behaviour incorrect :-)

> So even on Linux, iiuc, it's possible to see a watchpoint
> trigger after the write has already happened; it'll depend on
> hardware implementation.

Yes.

> I think the most flexible would be if the watchpoint
> event reported to GDB indicated which type it got, so
> that'd support the case an arch ever supports mixing both
> kinds of watchpoints.

Ha, I hadn't noticed that the architecture permits an
implementation to provide both kinds (or indeed to
have one watchpoint that might fire in either way), but
you're right that it's theoretically allowed.

> The next option would be something in the xml target description.
> It's be a global per-target, so no mixing types of watchpoints.
> (That is either sent to gdb by the stub, or loaded manually
> with "set tdesc filename".)
>
> Failing all that, we may want a "set arm ..." knob to
> override the default...
>
> Or we just forget all this, assuming that ARM chips that
> have async watchpoints will disappear into obsolescence
> forever soon enough.  :-)

There's an assertion in this LKML post from 2010:
https://lkml.org/lkml/2010/4/14/127
that v7 cores do actually all generate synchronous
watchpoint exceptions (even though architecturally
they're permitted not to). Was your test h/w a v6?

If this is a v6-and-earlier thing I'd certainly be tempted
to sweep the issue under the carpet...

thanks
-- PMM

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Pedro Alves]

On 09/29/2014 07:23 PM, Peter Maydell wrote:
> Joel Brobecker wrote:
>> I have been trying to understand the various contributions, and
>> I admit I am still not quite sure...
>>
>> Does it look like the patch I proposed is correct? It seems to be
>> supported by Terry Guo's experiments as well...
> 
> Note that the ARMv7 architecture allows watchpoints to
> be implemented as *asynchronous*, in which case what
> you will see is that you take a watchpoint exception
> but it may not fire until after the instruction that
> triggers the watchpoint and possibly several following
> instructions have all finished execution. This may be
> what you are seeing in your hardware tests.
> 
> For *synchronous* watchpoints, the behaviour is that the
> CPU stops *before* execution of the instruction which
> triggers the fault, and the memory access does not take
> place. This is pretty clearly described in the ARM ARM
> (see DDI0406C.c section C3.4.4 "Synchronous and asynchronous
> Watchpoint debug events" and the referenced "Effects of
> data-aborted instructions" section).

I only have DDI 0406C.b handy, which says:

 ARMv7 permits watchpoints to be either synchronous or asynchronous. An implementation can implement
 synchronous watchpoints, asynchronous watchpoints, or both. It is IMPLEMENTATION DEFINED under what
 circumstances a watchpoint is synchronous or asynchronous.

Ouch.  So this IMPLEMENTATION DEFINED note means this isn't really
in control of the software/debugger, i.e., nothing a stub/probe
could tweak, but instead baked into the specific ARM chip?
Or is there some register the probe could tweak to change
the this behavior?

Now I'm confused on the mention of the Linux kernel
subtracting 8 from the PC to help GDB.  I can't find that
anywhere in the kernel's sources.

> For ARMv8 (so including all AArch64 CPUs) watchpoints must
> be synchronous.

Ah, so this issue will eventually go away on its own.  :-)

> 
> QEMU's built in gdbstub was incorrectly not implementing
> synchronous watchpoints (ie it was halting after the
> execution of the offending insn, not before). This is fixed
> by the QEMU patch referenced earlier, and with that patch
> QEMU and GDB interoperate correctly (on ARM and also on
> other architectures which have the "stop before insn"
> watchpoint semantics).

"Incorrect" may be too strong then, but understood.

> 
> GDB should continue to set have_nonsteppable_watchpoint
> for ARM architectures, indicating:
>  * watchpoints fire before the insn executes
>  * you need to disable the watchpoint to successfully
>    singlestep the insn
> as this is correct for synchronous watchpoints.
> 
> If you have h/w with asynchronous watchpoints then there
> really isn't much you can do about stopping in the
> right place. Ideally I guess gdb would not do a step
> in that case, but I don't think it has access to
> enough info about the target CPU to know this (the
> kernel does get this info in the DBGDSCR.MOE register
> field, which is different for synchronous and
> asynchronous watchpoint events).

Ah.  In the kernel I have handy, I see:

/*
 * Called from either the Data Abort Handler [watchpoint] or the
 * Prefetch Abort Handler [breakpoint] with interrupts disabled.
 */
static int hw_breakpoint_pending(unsigned long addr, unsigned int fsr,
				 struct pt_regs *regs)
{
	int ret = 0;
	u32 dscr;

	preempt_disable();

	if (interrupts_enabled(regs))
		local_irq_enable();

	/* We only handle watchpoints and hardware breakpoints. */
	ARM_DBG_READ(c0, c1, 0, dscr);

	/* Perform perf callbacks. */
	switch (ARM_DSCR_MOE(dscr)) {
	case ARM_ENTRY_BREAKPOINT:
		breakpoint_handler(addr, regs);
		break;
	case ARM_ENTRY_ASYNC_WATCHPOINT:
		WARN(1, "Asynchronous watchpoint exception taken. Debugging results may be unreliable\n");
	case ARM_ENTRY_SYNC_WATCHPOINT:
		watchpoint_handler(addr, fsr, regs);
		break;
	default:
		ret = 1; /* Unhandled fault. */
	}

	preempt_enable();

	return ret;
}


(note the ARM_ENTRY_ASYNC_WATCHPOINT case falls-through.)

So even on Linux, iiuc, it's possible to see a watchpoint
trigger after the write has already happened; it'll depend on
hardware implementation.

I think the most flexible would be if the watchpoint
event reported to GDB indicated which type it got, so
that'd support the case an arch ever supports mixing both
kinds of watchpoints.

The next option would be something in the xml target description.
It's be a global per-target, so no mixing types of watchpoints.
(That is either sent to gdb by the stub, or loaded manually
with "set tdesc filename".)

Failing all that, we may want a "set arm ..." knob to
override the default...

Or we just forget all this, assuming that ARM chips that
have async watchpoints will disappear into obsolescence
forever soon enough.  :-)

Thanks,
Pedro Alves

Re: [RFA/commit] arm-tdep.c: Do not single-step after hitting a watchpoint

[Peter Maydell]

Joel Brobecker wrote:
> I have been trying to understand the various contributions, and
> I admit I am still not quite sure...
>
> Does it look like the patch I proposed is correct? It seems to be
> supported by Terry Guo's experiments as well...

Note that the ARMv7 architecture allows watchpoints to
be implemented as *asynchronous*, in which case what
you will see is that you take a watchpoint exception
but it may not fire until after the instruction that
triggers the watchpoint and possibly several following
instructions have all finished execution. This may be
what you are seeing in your hardware tests.

For *synchronous* watchpoints, the behaviour is that the
CPU stops *before* execution of the instruction which
triggers the fault, and the memory access does not take
place. This is pretty clearly described in the ARM ARM
(see DDI0406C.c section C3.4.4 "Synchronous and asynchronous
Watchpoint debug events" and the referenced "Effects of
data-aborted instructions" section).

For ARMv8 (so including all AArch64 CPUs) watchpoints must
be synchronous.

QEMU's built in gdbstub was incorrectly not implementing
synchronous watchpoints (ie it was halting after the
execution of the offending insn, not before). This is fixed
by the QEMU patch referenced earlier, and with that patch
QEMU and GDB interoperate correctly (on ARM and also on
other architectures which have the "stop before insn"
watchpoint semantics).

GDB should continue to set have_nonsteppable_watchpoint
for ARM architectures, indicating:
 * watchpoints fire before the insn executes
 * you need to disable the watchpoint to successfully
   singlestep the insn
as this is correct for synchronous watchpoints.

If you have h/w with asynchronous watchpoints then there
really isn't much you can do about stopping in the
right place. Ideally I guess gdb would not do a step
in that case, but I don't think it has access to
enough info about the target CPU to know this (the
kernel does get this info in the DBGDSCR.MOE register
field, which is different for synchronous and
asynchronous watchpoint events).

thanks
-- PMM