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

[Craig Blackmore <craig.blackmore@embecosm.com>]

On 17/09/18 11:34, Andrew Burgess wrote:
> * 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);
>>  }
>>
>>
Thanks for these comments, I have updated the patch accordingly.

---

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 'on' 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 'on' to match the recommended behaviour. If a target does not follow
this timing, then 'set riscv have_nonsteppable_watchpoint off' 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'.
	(show_have_nonsteppable_watchpoint): Add callback for
	'show riscv have_nonsteppable_watchpoint'.
	(riscv_gdbarch_init): Initialise gdbarch setting for
	have_nonesteppable_watchpoint.

---

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 254914c..857c5d1 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -226,6 +226,36 @@ show_use_compressed_breakpoints (struct ui_file *file, int from_tty,
 		      "to %s%s.\n"), value, additional_info);
 }
 
+/* Controls whether the debugger should step over hardware watchpoints before
+   checking if the watched variable has changed.  If true, then the debugger
+   will step over the watchpoint.  */
+
+static int riscv_have_nonsteppable_watchpoint = 1;
+
+/* The set callback for 'set riscv have-nonsteppable-watchpoint'.  */
+
+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 show callback for 'show riscv have-nonsteppable-watchpoint'.  */
+
+static void
+show_have_nonsteppable_watchpoint (struct ui_file *file, int from_tty,
+				   struct cmd_list_element *c,
+				   const char *value)
+{
+  fprintf_filtered (file,
+		    _("Debugger must step over hardware watchpoints is set to "
+		      "%s.\n"), value);
+}
+
 /* The set and show lists for 'set riscv' and 'show riscv' prefixes.  */
 
 static struct cmd_list_element *setriscvcmdlist = NULL;
@@ -2736,6 +2766,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 +3012,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 'on' (default)\n\
+for targets that follow this behaviour, otherwise set to 'off'."),
+			   set_have_nonsteppable_watchpoint,
+			   show_have_nonsteppable_watchpoint,
+			   &setriscvcmdlist,
+			   &showriscvcmdlist);
 }