From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28896 invoked by alias); 16 Oct 2013 15:43:45 -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 28884 invoked by uid 89); 16 Oct 2013 15:43:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mms1.broadcom.com Received: from mms1.broadcom.com (HELO mms1.broadcom.com) (216.31.210.17) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 16 Oct 2013 15:43:43 +0000 Received: from [10.9.208.55] by mms1.broadcom.com with ESMTP (Broadcom SMTP Relay (Email Firewall v6.5)); Wed, 16 Oct 2013 08:43:32 -0700 X-Server-Uuid: 06151B78-6688-425E-9DE2-57CB27892261 Received: from IRVEXCHSMTP1.corp.ad.broadcom.com (10.9.207.51) by IRVEXCHCAS07.corp.ad.broadcom.com (10.9.208.55) with Microsoft SMTP Server (TLS) id 14.1.438.0; Wed, 16 Oct 2013 08:43:34 -0700 Received: from mail-irva-13.broadcom.com (10.10.10.20) by IRVEXCHSMTP1.corp.ad.broadcom.com (10.9.207.51) with Microsoft SMTP Server id 14.1.438.0; Wed, 16 Oct 2013 08:43:34 -0700 Received: from [10.177.73.67] (unknown [10.177.73.67]) by mail-irva-13.broadcom.com (Postfix) with ESMTP id A344E246A3; Wed, 16 Oct 2013 08:43:33 -0700 (PDT) Message-ID: <525EB424.6010407@broadcom.com> Date: Wed, 16 Oct 2013 15:43:00 -0000 From: "Andrew Burgess" User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Thunderbird/24.0.1 MIME-Version: 1.0 To: "gdb-patches@sourceware.org" cc: "Pedro Alves" Subject: Re: [PATCH] hardware watchpoints turned off, inferior not yet started References: <525E5EB6.4070305@broadcom.com> <525E8761.8060405@redhat.com> In-Reply-To: <525E8761.8060405@redhat.com> Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2013-10/txt/msg00489.txt.bz2 On 16/10/2013 1:32 PM, Pedro Alves wrote: > On 10/16/2013 10:39 AM, Andrew Burgess wrote: >> The following seems confusing to me: >> >> $ gdb -q watch.x >> Reading symbols from /some/path/to/watch.x...done. >> (gdb) set can-use-hw-watchpoints 0 >> (gdb) watch -l my_var >> Hardware watchpoint 1: -location my_var >> (gdb) >> >> Notice that despite turning hardware watchpoints off >> the watchpoint created is reported to be a hardware >> watchpoint. Once I actually start the inferior the >> watchpoint is downgraded to a software watchpoint, >> but still, this feels like it might cause confusion, >> and is pretty easy to fix. > > It seems wrong to me to create it as bp_hardware_watchpoint > in the first place. That's done in watch_command_1, with: > > if (accessflag == hw_read) > bp_type = bp_read_watchpoint; > else if (accessflag == hw_access) > bp_type = bp_access_watchpoint; > else > bp_type = bp_hardware_watchpoint; > > If "can-use-hw-watchpoints" is off, then I think it'd > be also better to prohibit creating read and access > watchpoints around here. Though I agree, I looked at doing this and given that we'd ideally use the 'works_in_software_mode' method from the breakpoint ops strucutre (rather than replicate the code inline here) it felt a little messy. Instead I added a comment here explaining the reasoning, then fixed up the code below... > >> @@ -1800,6 +1800,8 @@ 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) >> + b->base.type = bp_watchpoint; >> } > > ... this change I think makes it so that access/read > watchpoints get converted to software watchpoints, which is wrong. OK I see that now, I got confused by code later within update_watchpoint that seems to unconditionally fallback to bp_watchpoint, but I see now how the read/access watchpoints actually result in an error. Thanks for pointing this out. I extended this code to cover this case and added tests. I know this isn't exactly what you asked for but would this be acceptable? If you're still unhappy I'll rewrite to create the watchpoints in software more. Thanks, Andrew gdb/ChangeLog * breakpoint.c (update_watchpoint): Create software watchpoints if the inferior has not yet started and hardware watchpoints are turned off. (watch_command_1): Move watchpoint type selection closer to watchpoint creation, and extend the comments. gdb/testsuite/ChangeLog * gdb.base/watchpoints.exp: Add test for setting software watchpoints of different types before starting the inferior. diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 5ce50de..e8a2ea8 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1795,11 +1795,19 @@ update_watchpoint (struct watchpoint *b, int reparse) don't try to insert watchpoint. We don't automatically delete such watchpoint, though, since failure to parse expression is different from out-of-scope watchpoint. */ - if ( !target_has_execution) + 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 (_("Target does not support software " + "watchpoints of this type.")); + } } else if (within_current_scope && b->exp) { @@ -11081,13 +11089,6 @@ watch_command_1 (const char *arg, int accessflag, int from_tty, if (*tok) error (_("Junk at end of command.")); - if (accessflag == hw_read) - bp_type = bp_read_watchpoint; - else if (accessflag == hw_access) - bp_type = bp_access_watchpoint; - else - bp_type = bp_hardware_watchpoint; - frame = block_innermost_frame (exp_valid_block); /* If the expression is "local", then set up a "watchpoint scope" @@ -11124,7 +11125,17 @@ watch_command_1 (const char *arg, int accessflag, int from_tty, } } - /* Now set up the breakpoint. */ + /* Now set up the breakpoint. We create all watchpoints as hardware + watchpoints here even if hardware watchpoints are turned off, a call + to update_watchpoint later in this function will cause the type to + drop back to bp_watchpoint (software watchpoint) if required. */ + + if (accessflag == hw_read) + bp_type = bp_read_watchpoint; + else if (accessflag == hw_access) + bp_type = bp_access_watchpoint; + else + bp_type = bp_hardware_watchpoint; w = XCNEW (struct watchpoint); b = &w->base; diff --git a/gdb/testsuite/gdb.base/watchpoints.exp b/gdb/testsuite/gdb.base/watchpoints.exp index 7c10d81..ced4fb2 100644 --- a/gdb/testsuite/gdb.base/watchpoints.exp +++ b/gdb/testsuite/gdb.base/watchpoints.exp @@ -29,6 +29,27 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} { return -1 } + # 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. + gdb_test_no_output "set can-use-hw-watchpoints 0" "" + gdb_test "watch ival1" "Watchpoint \[0-9\]+: ival1" \ + "Set software watchpoint before starting the inferior" + + # The next tests are written to match the current state of gdb: access + # and read watchpoints require hardware watchpoint support, with this + # turned off these can't be created. + gdb_test "awatch ival1" \ + "Target does not support software watchpoints of this type." \ + "Set software watchpoint before starting the inferior" + gdb_test "rwatch ival1" \ + "Target does not support software watchpoints of this type." \ + "Set software watchpoint before starting the inferior" + + # This will turn hardware watchpoints back on and delete the watchpoint + # we just created. + clean_restart ${binfile} + # Disable hardware watchpoints if necessary. if [target_info exists gdb,no_hardware_watchpoints] { gdb_test_no_output "set can-use-hw-watchpoints 0" ""