RISC-V: enable have_nonsteppable_watchpoint by default

  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.
    
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

---
  

* Craig Blackmore <craig.blackmore@embecosm.com> [2018-09-16 01:13:08 +0100]:

> 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.

Thanks for this.  Just a few minor formatting issues which I've
pointed out below.

Thanks,
Andrew

>     
> 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

Proper sentences in ChangeLogs please, capital letters and
full-stops.

> 
> ---
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 254914c9c7..8f301d8b01 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -226,6 +226,20 @@ show_use_compressed_breakpoints (struct ui_file *file, int from_tty,
>  		      "to %s%s.\n"), value, additional_info);
>  }
>  
> +static int riscv_have_nonsteppable_watchpoint = 1;

You need to add a comment for this variable.

> +
> +/* The set callback for 'set riscv have-nonsteppable-watchpoint'. */

Two whitespace after the full-stop and before the comment close please.

> +
> +static void
> +set_have_nonsteppable_watchpoint (const char *args, int from_tty,
> +			       struct cmd_list_element *c)
> +{
> +  struct gdbarch *gdbarch = target_gdbarch ();
> +
> +  set_gdbarch_have_nonsteppable_watchpoint (gdbarch,
> +					    riscv_have_nonsteppable_watchpoint);
> +}
> +
>  /* The set and show lists for 'set riscv' and 'show riscv' prefixes.  */
>  
>  static struct cmd_list_element *setriscvcmdlist = NULL;
> @@ -2736,6 +2750,8 @@ riscv_gdbarch_init (struct gdbarch_info info,
>    set_gdbarch_return_value (gdbarch, riscv_return_value);
>    set_gdbarch_breakpoint_kind_from_pc (gdbarch, riscv_breakpoint_kind_from_pc);
>    set_gdbarch_sw_breakpoint_from_kind (gdbarch, riscv_sw_breakpoint_from_kind);
> +  set_gdbarch_have_nonsteppable_watchpoint (gdbarch,
> +					    riscv_have_nonsteppable_watchpoint);
>  
>    /* Register architecture.  */
>    set_gdbarch_num_regs (gdbarch, RISCV_LAST_REGNUM + 1);
> @@ -2980,4 +2996,20 @@ can be used."),
>  				show_use_compressed_breakpoints,
>  				&setriscvcmdlist,
>  				&showriscvcmdlist);
> +
> +  add_setshow_boolean_cmd ("have-nonsteppable-watchpoint", no_class,
> +			   &riscv_have_nonsteppable_watchpoint,
> +			   _("\
> +Set whether debugger must step over hardware watchpoints"),
> +			   _("\
> +Show whether debugger must step over hardware watchpoints"),
> +			   _("\
> +The RISC-V debug spec recommends that hardware write watchpoints fire before\n\
> +the write is committed, in which case, GDB must step over the watchpoint\n\
> +before checking the old and new values. Set this option to 1 (default) for\n\

Two whitespace after full-stop again please.

> +targets that follow this behaviour, otherwise set to 0."),

As this is a boolean command can you replace references to 1 and 0
both here and in the commit message with on and off.

> +			   set_have_nonsteppable_watchpoint,
> +			   NULL,

You need to implement the show method too to allow for
internationalisation.

> +			   &setriscvcmdlist,
> +			   &showriscvcmdlist);
>  }
> 
>
  

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.
>     
> 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.

Thanks,
Pedro Alves
  

* 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.
> >     
> > 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.

... 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.

What do you think?

Thanks,
Andrew


> 
> Thanks,
> Pedro Alves
  

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