[RFA/RFC] riscv: expect hardware watchpoints to trigger before the memory is written

Message ID 20180924214228.14578-1-brobecker@adacore.com
State New, archived
Headers

Commit Message

Joel Brobecker Sept. 24, 2018, 9:42 p.m. UTC
  Hello,

below is a patch that fixes hardware watchpoint support, at least
when using QEMU. The reason why I say "at least" is because I'm not
entirely clear whether RISCV defined if GDB is expected to be notified
before or after the memory is being written. At the moment, the notification
comes before, but I am wondering if it should be after.

That reminded me of https://sourceware.org/ml/gdb/2018-08/msg00047.html,
where it was said the debug spec suggests the event be triggered before.
However, I couldn't find confirmation of that, or its contrary, in the
document I could find at https://github.com/riscv/riscv-debug-spec.  It
looks to me like the behavior might not be specified. Does anyone know?
I'm copying Tim Newsome (author of the thread quoted above) in case he
knows more.

If not, I suggest we put this patch in. At worst, GDB does one extra
instruction single-step. Without it, the GDB / QEMU duo get stuck in
an infinite loop.

----------------------------------------------------------------------

When using QEMU as a RISCV simulator, hardware watchpoint events are
reported to GDB before the target memory gets written. GDB currently
expects the event to be reported after it is written. As a result of
this mismatch, upon receiving the event, GDB sees that the target
memory region has not changed, and therefore decides to ignore the
event. It therefore resumes the program's execution with a continue,
which is the start of an infinite loop between QEMU repeatedly
reporting the same watchpoint event over and over, and GDB repeatedly
ignoring it.

This patch fixes the issue by telling GDB to expect the watchpoint
event to be reported ahead of the memory region being modified.
Upon receiving the event, GDB then single-steps the program before
checking the watched memory value.

gdb/ChangeLog:

        * riscv-tdep.c (riscv_gdbarch_init): Set the gdbarch's
        have_nonsteppable_watchpoint attribute to 1.

Tested on riscv64-elf using AdaCore's testsuite.

---
 gdb/riscv-tdep.c | 1 +
 1 file changed, 1 insertion(+)
  

Comments

Joel Brobecker Sept. 24, 2018, 9:46 p.m. UTC | #1
> gdb/ChangeLog:
> 
>         * riscv-tdep.c (riscv_gdbarch_init): Set the gdbarch's
>         have_nonsteppable_watchpoint attribute to 1.
> 
> Tested on riscv64-elf using AdaCore's testsuite.

Gah! I missed the fact that someone was already on it
(https://www.sourceware.org/ml/gdb-patches/2018-09/msg00801.html).
That patch implements a more general solution, so I am withdrawing
my patch.
  

Patch

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 8bd78a24b48..ab3763349bf 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -2737,6 +2737,7 @@  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, 1);
 
   /* Register architecture.  */
   set_gdbarch_num_regs (gdbarch, RISCV_LAST_REGNUM + 1);