From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30360 invoked by alias); 8 Nov 2013 11:43:11 -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 30347 invoked by uid 89); 8 Nov 2013 11:43:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=2.5 required=5.0 tests=AWL,BAYES_50,RDNS_NONE,SPAM_SUBJECT,SPF_HELO_PASS,URIBL_BLOCKED autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 08 Nov 2013 11:43:09 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rA8Bgxn4032033 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 8 Nov 2013 06:43:00 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rA8BgwOW025342; Fri, 8 Nov 2013 06:42:58 -0500 Message-ID: <527CCE41.7020203@redhat.com> Date: Fri, 08 Nov 2013 12:03:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: "Jose E. Marchesi" CC: gdb-patches@sourceware.org Subject: Re: [PATCH] hardware watchpoints turned off, inferior not yet started References: <87fvr8xwo8.fsf@oracle.com> In-Reply-To: <87fvr8xwo8.fsf@oracle.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-11/txt/msg00229.txt.bz2 On 11/07/2013 11:45 AM, Jose E. Marchesi wrote: > > [Following https://sourceware.org/ml/gdb-patches/2013-10/msg00563.html] > > Hi. > > The patch posted above introduced a regression in sparc64-*-linux-gnu, > and possibly in other targets not supporting hardware watchpoints at > all. > > Today I was surprised to find this in a sparc64-*-linux-gnu target: > > $ gdb foo > ... intro text ... > (gdb) watch global_var > Hardware watchpoint 1: global_var > > The cause for this regression is this code in update_watchpoint > (breakpoint.c): > > + if (!target_has_execution) > { > /* Without execution, memory can't change. No use to try and > set watchpoint locations. The watchpoint will be reset when > the target gains execution, through breakpoint_re_set. */ > + if (!can_use_hw_watchpoints) > + { > + if (b->base.ops->works_in_software_mode (&b->base)) > + b->base.type = bp_watchpoint; > + else > + error (_("Software read/access watchpoints not supported.")); > + } I'm not following how this could have caused this regression. Before the patch, AFAICS, there was _nothing_ here that degraded the watchpoint. I actually don't think there's a regression here at all. Only if you compare back a few GDB releases could you perhaps call this a regression (to when before watchpoint support was made to go all through the target vector). > > The watchpoint must be downgraded to a software watchpoint if the target > does not support hw watchpoints, even if can_use_hw_watchpoints is 1. If the program isn't running yet, then target_can_use_hardware_watchpoint will always return false for all native targets. So this must also be e.g., changing behavior for x86? At this point, before starting the inferior, we don't really know whether the user will do "run" (spawning a local process), or "target remote" (or some other target), so we can't really know whether hardware watchpoints will work or not. If you're connected to an extended-remote gdbserver, then remote_check_watch_resources does end up being called by target_can_use_hardware_watchpoint, so one could claim that it makes some sense to call it here, like in your patch (though I'm of the opinion that all this resource accounting stuff is fundamentally broken). But then your new test will fail for extended-remote gdbserver, because a hardware watchpoint will indeed be created. Maybe we should just declare that "watch" if the program is not running always creates a software watchpoint, that might or not get "upgraded" once execution starts, making all targets behave the same... > The following patch fixes this and also adds an additional test to > testsuite/watchpoints.exp to catch this problem. > > 2013-11-07 Jose E. Marchesi > > * breakpoint.c (update_watchpoint): Downgrade hw watchpoints to sw > watchpoints in targets not supporting them, when the inferior is > not running. > > 2013-11-07 Jose E. Marchesi > > * gdb.base/watchpoints.exp: Add a test to ensure that a watchpoint > is not created as a hw watchpoint in targets not supporting them, > even if can-use-hw-watchpoints is 1 when the inferior is not > running. > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index ffe73fd..597e6f9 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -1800,7 +1800,10 @@ update_watchpoint (struct watchpoint *b, int reparse) > /* Without execution, memory can't change. No use to try and > set watchpoint locations. The watchpoint will be reset when > the target gains execution, through breakpoint_re_set. */ > - if (!can_use_hw_watchpoints) > + int i = hw_breakpoint_used_count (); > + int target_supports_hw_watchpoints = > + target_can_use_hardware_watchpoint (bp_hardware_watchpoint, i + 1, 0); '=' goes on the next line. The breakpoint type here might be an access or read watchpoint too. It's really not correct to always pass bp_hardware_watchpoint down to target_can_use_hardware_watchpoint. "supports" in the variable name is a bit misleading, because "i+1" might be too many resources, even though the target might support hardware watchpoints. > + if (!target_supports_hw_watchpoints || !can_use_hw_watchpoints) > { > if (b->base.ops->works_in_software_mode (&b->base)) > b->base.type = bp_watchpoint; > --- a/gdb/testsuite/gdb.base/watchpoints.exp > +++ b/gdb/testsuite/gdb.base/watchpoints.exp > @@ -30,6 +30,15 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} { > } > > with_test_prefix "before inferior start" { > + if [target_info exists gdb,no_hardware_watchpoints] { > + # Ensure that a watchpoint is not created as a hw watchpoint > + # even if can-use-hw-watchpoints is 1 when the inferior is not > + # running. > + gdb_test_no_output "set can-use-hw-watchpoints 1" > + gdb_test "watch ival1" "Watchpoint \[0-9\]+: ival1" \ > + "create sw watchpoint" > + } > + > # Ensure that if we turn off hardware watchpoints and set a watch point > # before starting the inferior the watchpoint created will not be a > # hardware watchpoint. > > -- Pedro Alves