From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 111065 invoked by alias); 8 Oct 2018 09:58:46 -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 111052 invoked by uid 89); 8 Oct 2018 09:58:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.6 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=H*RU:sk:host86-, Hx-spam-relays-external:sk:host86-, H*r:sk:host86-, Controls X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 08 Oct 2018 09:58:43 +0000 Received: by mail-wr1-f65.google.com with SMTP id g15-v6so17467425wru.9 for ; Mon, 08 Oct 2018 02:58:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=7p8D121SY18P83vQNMdKX7hcTtPkkFdmDnT79r3xwio=; b=WLASnHudm02qmaMwlV4VIQXlgRxvX4xG49giCrSp0Nwo9VmquUhpnt8/a2WGM7ZzOg MhJLjL9leBCVlcgmu93QI0zX5kPL2/I9IziLDqQMEha5FY0RBuccGIwOGzbNfpTn+gKf N1UE7NQiPq5QRqDAHXvcqW2MH8DIscRHDBh8T5hmz9kxjbKm574Tlvo750RNsVJe0lBf EwwtcXWTscLWfHkQEjnBXc6BBhQhC6IvO3369tnFgw2HjFJnaicvvnL+5x7esc0nvV4g C1uSPCAPBM5vbvIwERQ2ROAQKx2RvYfZaBuITSssxsBczSVl6tvPPHetPW7az3/yOeoB QnSQ== Return-Path: Received: from localhost (host86-190-190-207.range86-190.btcentralplus.com. [86.190.190.207]) by smtp.gmail.com with ESMTPSA id a105-v6sm39795481wrc.23.2018.10.08.02.58.40 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 08 Oct 2018 02:58:40 -0700 (PDT) Date: Mon, 08 Oct 2018 09:58:00 -0000 From: Andrew Burgess To: Joel Brobecker Cc: Craig Blackmore , gdb-patches@sourceware.org, Pedro Alves Subject: Re: [PATCH] RISC-V: enable have_nonsteppable_watchpoint by default Message-ID: <20181008095839.GC5952@embecosm.com> References: <20180917103409.GJ5952@embecosm.com> <77978648-c391-0011-6c03-c7fd38429914@embecosm.com> <20181003223703.GA22933@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181003223703.GA22933@adacore.com> X-Fortune: I've Been Moved! X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg00170.txt.bz2 * Joel Brobecker [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