From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 40641 invoked by alias); 8 Oct 2018 11:56:03 -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 40629 invoked by uid 89); 8 Oct 2018 11:56:02 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=responsive, managed, OTOH, inferiors X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 08 Oct 2018 11:56:01 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D1F93307D847; Mon, 8 Oct 2018 11:55:59 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id C1B872A2E4; Mon, 8 Oct 2018 11:55:58 +0000 (UTC) Subject: Re: [PATCH] RISC-V: enable have_nonsteppable_watchpoint by default To: Andrew Burgess , Joel Brobecker References: <20180917103409.GJ5952@embecosm.com> <77978648-c391-0011-6c03-c7fd38429914@embecosm.com> <20181003223703.GA22933@adacore.com> <20181008095839.GC5952@embecosm.com> Cc: Craig Blackmore , gdb-patches@sourceware.org From: Pedro Alves Message-ID: <4c4c1369-0f5c-549a-ed82-51563c5e6dd6@redhat.com> Date: Mon, 08 Oct 2018 11:56:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181008095839.GC5952@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-10/txt/msg00173.txt.bz2 On 10/08/2018 10:58 AM, Andrew Burgess wrote: > * 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 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