From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 36283 invoked by alias); 3 Oct 2018 22:37:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 36273 invoked by uid 89); 3 Oct 2018 22:37:08 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=watched, timing, riscvtdepc, riscv-tdep.c X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 03 Oct 2018 22:37:07 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 9EE085607F; Wed, 3 Oct 2018 18:37:05 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id iGfPlgtOn9i7; Wed, 3 Oct 2018 18:37:05 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 6A5705607B; Wed, 3 Oct 2018 18:37:05 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 85B0E82C67; Wed, 3 Oct 2018 15:37:03 -0700 (PDT) Date: Wed, 03 Oct 2018 22:37:00 -0000 From: Joel Brobecker To: Craig Blackmore Cc: Andrew Burgess , gdb-patches@sourceware.org Subject: Re: [PATCH] RISC-V: enable have_nonsteppable_watchpoint by default Message-ID: <20181003223703.GA22933@adacore.com> References: <20180917103409.GJ5952@embecosm.com> <77978648-c391-0011-6c03-c7fd38429914@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <77978648-c391-0011-6c03-c7fd38429914@embecosm.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-SW-Source: 2018-10/txt/msg00099.txt.bz2 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