This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] RISC-V: enable have_nonsteppable_watchpoint by default
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Craig Blackmore <craig dot blackmore at embecosm dot com>
- Cc: Andrew Burgess <andrew dot burgess at embecosm dot com>, gdb-patches at sourceware dot org
- Date: Wed, 3 Oct 2018 15:37:03 -0700
- Subject: Re: [PATCH] RISC-V: enable have_nonsteppable_watchpoint by default
- References: <c83e39fb-b1e8-cc67-85b5-c7cc66197078@embecosm.com> <20180917103409.GJ5952@embecosm.com> <77978648-c391-0011-6c03-c7fd38429914@embecosm.com>
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