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

[Pedro Alves <palves at redhat dot com>]

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