[PATCH] RISC-V: enable have_nonsteppable_watchpoint by default

[PATCH] RISC-V: enable have_nonsteppable_watchpoint by default

[Craig Blackmore]

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

---

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;
+
+/* 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 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\
+targets that follow this behaviour, otherwise set to 0."),
+			   set_have_nonsteppable_watchpoint,
+			   NULL,
+			   &setriscvcmdlist,
+			   &showriscvcmdlist);
 }

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

[Andrew Burgess]

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

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

[Pedro Alves]

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

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

[Andrew Burgess]

* 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

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

[Craig Blackmore]

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

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

[Joel Brobecker]

Hi Craig, hi Andrew,

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

I assume this patch is waiting for review from Andrew?

I took a look, since I am interested in it as well. Here are my
comments.

You forgot to document the introduction of riscv_have_nonsteppable_watchpoint
in your ChangeLog above; eg:

        * riscv-tdep.c (riscv_have_nonsteppable_watchpoint): New static
        global.

You also forgot to document that you're adding new subcommands.

The rest looks good to me.

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

-- 
Joel

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

[Craig Blackmore]

On 03/10/18 23:37, Joel Brobecker wrote:
> I assume this patch is waiting for review from Andrew?
>
> I took a look, since I am interested in it as well. Here are my
> comments.
>
> You forgot to document the introduction of riscv_have_nonsteppable_watchpoint
> in your ChangeLog above; eg:
>
>         * riscv-tdep.c (riscv_have_nonsteppable_watchpoint): New static
>         global.
>
> You also forgot to document that you're adding new subcommands.
>
> The rest looks good to me.
Hi Joel,

Thanks for the review. I have updated the ChangeLog in the patch below.

Craig

---

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 (riscv_have_nonsteppable_watchpoint): New static
	global.
	(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.
	(_initialize_riscv_tdep): Add 'set/show riscv
	have_nonsteppable_watchpoint' subcommands.

---

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

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

[Andrew Burgess]

* Joel Brobecker <brobecker@adacore.com> [2018-10-03 15:37:03 -0700]:

> Hi Craig, hi Andrew,
> 
> > ---
> > 
> > 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.
> 
> I assume this patch is waiting for review from Andrew?

No, I was hoping for some feedback from Pedro, he commented in this
post:
   https://sourceware.org/ml/gdb-patches/2018-09/msg00570.html

that he wasn't happy with the approach Craig took. He would like to
see the switching done automatically from the target description.  In
this post:
   https://sourceware.org/ml/gdb-patches/2018-09/msg00572.html

I agree with Pedro, but take the position that as riscv doesn't
currently have any target description support (I hope to work on some
of this soon) then I'd like this patch to go in as it is and improve
on it later.

However, as Pedro is a global maintainer, I don't feel I can OK the
patch with his negative feedback outstanding...

Thanks,
Andrew





> 
> I took a look, since I am interested in it as well. Here are my
> comments.
> 
> You forgot to document the introduction of riscv_have_nonsteppable_watchpoint
> in your ChangeLog above; eg:
> 
>         * riscv-tdep.c (riscv_have_nonsteppable_watchpoint): New static
>         global.
> 
> You also forgot to document that you're adding new subcommands.
> 
> The rest looks good to me.
> 
> > ---
> > 
> > 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);
> >  }
> > 
> 
> -- 
> Joel

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

[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

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

[Pedro Alves]

On 10/08/2018 10:58 AM, Andrew Burgess wrote:
> * Joel Brobecker <brobecker@adacore.com> [2018-10-03 15:37:03 -0700]:
> 
>> Hi Craig, hi Andrew,
>>
>>> ---
>>>
>>> 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.
>>
>> I assume this patch is waiting for review from Andrew?
> 
> No, I was hoping for some feedback from Pedro, he commented in this
> post:
>    https://sourceware.org/ml/gdb-patches/2018-09/msg00570.html

Sorry, replied now.

> that he wasn't happy with the approach Craig took. He would like to
> see the switching done automatically from the target description.  In
> this post:
>    https://sourceware.org/ml/gdb-patches/2018-09/msg00572.html
> 
> I agree with Pedro, but take the position that as riscv doesn't
> currently have any target description support (I hope to work on some
> of this soon) then I'd like this patch to go in as it is and improve
> on it later.
> 
> However, as Pedro is a global maintainer, I don't feel I can OK the
> patch with his negative feedback outstanding...

Sorry, I don't mean to hold people back, but indeed I haven't managed
to be very responsive in the last few weeks...

There are a number of issues with the patch as is.  Off hand:

 #1 - Missing NEWS.

 #2 - Missing manual/docs change.

 #3 - set_have_nonsteppable_watchpoint works on whatever's
      the current inferior's gdbarch, so it means that if
      you're running a --enable-targets=all gdb against
      something non-RISC-V, the command will affect that
      architecture.

 #4 - OTOH, the command is documented as a RISC-V command.  But it's
      in the global "set" namespace right?  Not something like
      "set riscv have-nonsteppable-watchpoint"?

 #5 - I think "have-nonsteppable-watchpoint" is a bad user-facing
      name.  It is exposing outdated gdb-internal lingo ("nonsteppable").
      The property in question is whether the watchpoint triggers
      before or after the instruction is executed.  How that 
      translates to "nonsteppable" for users is going to be quite
      cryptic.

The above, keeping in mind that it'd be much better for GDB to figure
this stuff out itself, and coupled with the fact that I'm not sure
whether there are in fact implementations of riscv that trigger
watchpoints after the write, makes me wonder, do we really need this?

Thanks,
Pedro Alves

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

[Joel Brobecker]

> [...] coupled with the fact that I'm not sure
> whether there are in fact implementations of riscv that trigger
> watchpoints after the write, makes me wonder, do we really need this?

Actually - that's a very good point. Do we know of any architecture
where the watchpoint triggers after the write?

-- 
Joel

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

[Paul Koning]

> On Oct 8, 2018, at 10:25 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> 
>> [...] coupled with the fact that I'm not sure
>> whether there are in fact implementations of riscv that trigger
>> watchpoints after the write, makes me wonder, do we really need this?
> 
> Actually - that's a very good point. Do we know of any architecture
> where the watchpoint triggers after the write?

I think MIPS is one.  The documentation is not entirely clear but that's what I remember from using it.

	paul

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

[Pedro Alves]

On 10/08/2018 03:37 PM, Paul Koning wrote:
> 
> 
>> On Oct 8, 2018, at 10:25 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>>
>>> [...] coupled with the fact that I'm not sure
>>> whether there are in fact implementations of riscv that trigger
>>> watchpoints after the write, makes me wonder, do we really need this?
>>
>> Actually - that's a very good point. Do we know of any architecture
>> where the watchpoint triggers after the write?
> 
> I think MIPS is one.  The documentation is not entirely clear but that's what I remember from using it.
x86 is another.  But my question is -- do we know of any RISC-V
implementation that triggers after the write, given that the spec
says it should trigger before the write.

Thanks,
Pedro Alves

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

[Andreas Schwab]

On Okt 08 2018, Paul Koning <paulkoning@comcast.net> wrote:

> I think MIPS is one.  The documentation is not entirely clear but that's what I remember from using it.

According to mips-tdep.c, mips is nonsteppable.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Joel Brobecker]

> > I think MIPS is one.  The documentation is not entirely clear but
> > that's what I remember from using it.
> x86 is another.  But my question is -- do we know of any RISC-V
> implementation that triggers after the write, given that the spec
> says it should trigger before the write.

That was what I meant as well; I agree with Pedro that we don't
really need to do anything fancy if:
  - the spec's recommendation is to trigger before the write
  - and we don't know of any system that decided to go against
    the recommendation.
The day we discover a system that does in fact go against the
recommendation, we can simply deal with it then and decide what
the best course of action is.

-- 
Joel

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

[Craig Blackmore]

On 08/10/18 15:51, Joel Brobecker wrote:
>>> I think MIPS is one.  The documentation is not entirely clear but
>>> that's what I remember from using it.
>> x86 is another.  But my question is -- do we know of any RISC-V
>> implementation that triggers after the write, given that the spec
>> says it should trigger before the write.
I don't know of any RISC-V implementations that trigger after the write.
The debug spec has 'suggested breakpoint timings' but the triggers are
allowed to fire at whatever point is most convenient for the implementation.

I suggest that Joel's earlier patch
(https://sourceware.org/ml/gdb-patches/2018-09/msg00821.html) be
upstreamed so that things work for the majority of systems. We can
handle implementations with other timings later, if they appear.

Thanks,
Craig

> That was what I meant as well; I agree with Pedro that we don't
> really need to do anything fancy if:
>   - the spec's recommendation is to trigger before the write
>   - and we don't know of any system that decided to go against
>     the recommendation.
> The day we discover a system that does in fact go against the
> recommendation, we can simply deal with it then and decide what
> the best course of action is.
>

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

[Paul Koning]

> On Oct 9, 2018, at 1:20 PM, Craig Blackmore <craig.blackmore@embecosm.com> wrote:
> 
> 
> 
> On 08/10/18 15:51, Joel Brobecker wrote:
>>>> I think MIPS is one.  The documentation is not entirely clear but
>>>> that's what I remember from using it.
>>> x86 is another.  But my question is -- do we know of any RISC-V
>>> implementation that triggers after the write, given that the spec
>>> says it should trigger before the write.
> I don't know of any RISC-V implementations that trigger after the write.
> The debug spec has 'suggested breakpoint timings' but the triggers are
> allowed to fire at whatever point is most convenient for the implementation.

I missed that the question was specific to RISC-V.

If the spec says that timing is up to the implementation, that seems to mean GDB can't rely on the break occurring before the write -- the fact that current implementations do so isn't sufficient if later implementation are allowed to differ.

I assume GDB cares which it is, which suggests that the implementation has to tell GDB which flavor of write watchpoint it has.

	paul

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

[Pedro Alves]

-- 
Thanks,
Pedro Alves
On 10/09/2018 06:29 PM, Paul Koning wrote:
> 
> 
>> On Oct 9, 2018, at 1:20 PM, Craig Blackmore <craig.blackmore@embecosm.com> wrote:
>>
>>
>>
>> On 08/10/18 15:51, Joel Brobecker wrote:
>>>>> I think MIPS is one.  The documentation is not entirely clear but
>>>>> that's what I remember from using it.
>>>> x86 is another.  But my question is -- do we know of any RISC-V
>>>> implementation that triggers after the write, given that the spec
>>>> says it should trigger before the write.
>> I don't know of any RISC-V implementations that trigger after the write.
>> The debug spec has 'suggested breakpoint timings' but the triggers are
>> allowed to fire at whatever point is most convenient for the implementation.
> 
> I missed that the question was specific to RISC-V.
> 
> If the spec says that timing is up to the implementation, that seems to mean GDB can't rely on the break occurring before the write -- the fact that current implementations do so isn't sufficient if later implementation are allowed to differ.
> 
> I assume GDB cares which it is, which suggests that the implementation has to tell GDB which flavor of write watchpoint it has.

Yes, which is what I had suggested earlier in the thread.  But the
thing is, no one knows about any implementation that doesn't
trap before the write.  Does the "point is most convenient"
include a few instructions/cycles after the write (more than
one insn?).  Because, there are archs like that (some ARM
variants, IIRC).  If so, before/after will not be sufficient.
Thus, I still think we should go with the simple approach until
we learn about some real implementation that needs something else.

Thanks,
Pedro Alves

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

[Andrew Burgess]

* Craig Blackmore <craig.blackmore@embecosm.com> [2018-10-09 18:20:04 +0100]:

> 
> 
> On 08/10/18 15:51, Joel Brobecker wrote:
> >>> I think MIPS is one.  The documentation is not entirely clear but
> >>> that's what I remember from using it.
> >> x86 is another.  But my question is -- do we know of any RISC-V
> >> implementation that triggers after the write, given that the spec
> >> says it should trigger before the write.
> I don't know of any RISC-V implementations that trigger after the write.
> The debug spec has 'suggested breakpoint timings' but the triggers are
> allowed to fire at whatever point is most convenient for the implementation.
> 
> I suggest that Joel's earlier patch
> (https://sourceware.org/ml/gdb-patches/2018-09/msg00821.html) be
> upstreamed so that things work for the majority of systems. We can
> handle implementations with other timings later, if they appear.

I've gone ahead and pushed Joel's patch into master, commit
5a77b1b49f4.

Thanks,
Andrew



> 
> Thanks,
> Craig
> 
> > That was what I meant as well; I agree with Pedro that we don't
> > really need to do anything fancy if:
> >   - the spec's recommendation is to trigger before the write
> >   - and we don't know of any system that decided to go against
> >     the recommendation.
> > The day we discover a system that does in fact go against the
> > recommendation, we can simply deal with it then and decide what
> > the best course of action is.
> >
> 
>