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

[Pedro Alves <palves@redhat.com>]

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