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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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);
>  }
> 
>