Re: [PATCH] RISC-V: enable have_nonsteppable_watchpoint by default

[Pedro Alves <palves@redhat.com>]

On 09/17/2018 02:34 PM, Andrew Burgess wrote:
> * Pedro Alves <palves@redhat.com> [2018-09-17 13:54:38 +0100]:
> 
>> On 09/16/2018 01:13 AM, Craig Blackmore wrote:
>>> The RISC-V debug spec 0.13 recommends that write triggers fire before
>>> the write is committed. If the target follows this behaviour, then
>>> have_nonsteppable_watchpoint needs to be set to 1 so that GDB will step
>>> over the watchpoint before checking if the value has changed.
>>>     
>>> This patch adds a setshow for have_nonsteppable_watchpoint which defaults
>>> to 1 to match the recommended behaviour. If a target does not follow
>>> this timing, then 'set riscv have_nonsteppable_watchpoint 0' will need
>>> to be issued on the command line.

Do you know of any implementation that _doesn't_ follow the spec?
Wondering whether we're adding a knob/complexity for nothing.

>>>     
>>> gdb/ChangeLog:
>>>     
>>> 	* riscv-tdep.c (set_have_nonsteppable_watchpoint): add
>>> 	callback for 'set riscv have_nonsteppable_watchpoint'
>>> 	(riscv_gdbarch_init): initialise gdbarch setting for
>>> 	have_nonesteppable_watchpoint
>>
>> This is something the target/stub knows, right?  I'd be much
>> better to make this automatic, so that users wouldn't have to
>> know to tweak anything.
> 
> Sure, you're thinking something like (to pick one at random) how the
> 'org.gnu.gdb.arm.neon' feature on ARM in the target description tells
> GDB how to operate, right?  I totally agree.

I wasn't thinking of a target feature, but either a qSupported feature
or maybe better, an extension to the watchpoint stop reply ("stopped before/after").

This came up recently here, btw:
  https://sourceware.org/ml/gdb/2018-08/msg00047.html

> 
> ... but.... we'd still probably want to keep the flag around (though
> as an auto/on/off maybe) so the user could, if they wanted, override a
> badly behaving target...
> 
> ....and.... there's no current remote description support for RiscV at
> all, so having implement that as a prerequisite seems a little steep
> (to me).
> 
> My preference would be to allow this in basically as is, then make it
> automatic once we have target description support in place.
> 
> Alternatively we could remove the control switch for now, and just go
> with:
> 
>   set_gdbarch_have_nonsteppable_watchpoint (gdbarch, 1);
> 
> for everyone.  But if there's anyone out there not following the
> recommendation that makes things a little harder for them in the short
> term.

But is there any evidence of any implementation deviating from
the spec's suggestion?  From

 https://sourceware.org/ml/gdb/2018-08/msg00047.html

I had assumed that we'd just fix gdb to follow the spec and be done
with it.

> 
> What do you think?
> 
Thanks,
Pedro Alves