From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3441 invoked by alias); 18 Oct 2013 09:14:23 -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 3420 invoked by uid 89); 18 Oct 2013 09:14:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 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; Fri, 18 Oct 2013 09:14:20 +0000 Received: from [10.9.208.53] by mms1.broadcom.com with ESMTP (Broadcom SMTP Relay (Email Firewall v6.5)); Fri, 18 Oct 2013 02:14:11 -0700 X-Server-Uuid: 06151B78-6688-425E-9DE2-57CB27892261 Received: from IRVEXCHSMTP3.corp.ad.broadcom.com (10.9.207.53) by IRVEXCHCAS06.corp.ad.broadcom.com (10.9.208.53) with Microsoft SMTP Server (TLS) id 14.1.438.0; Fri, 18 Oct 2013 02:14:13 -0700 Received: from mail-irva-13.broadcom.com (10.10.10.20) by IRVEXCHSMTP3.corp.ad.broadcom.com (10.9.207.53) with Microsoft SMTP Server id 14.1.438.0; Fri, 18 Oct 2013 02:14:13 -0700 Received: from [10.177.73.67] (unknown [10.177.73.67]) by mail-irva-13.broadcom.com (Postfix) with ESMTP id 6F972246A4; Fri, 18 Oct 2013 02:14:12 -0700 (PDT) Message-ID: <5260FBE3.8030200@broadcom.com> Date: Fri, 18 Oct 2013 09:14: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> <525EB424.6010407@broadcom.com> <525EBEA7.5060205@redhat.com> In-Reply-To: <525EBEA7.5060205@redhat.com> Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2013-10/txt/msg00550.txt.bz2 On 16/10/2013 5:28 PM, Pedro Alves wrote: > On 10/16/2013 04:43 PM, Andrew Burgess wrote: >> On 16/10/2013 1:32 PM, Pedro Alves wrote: >>> ... 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. > >> gdb/ChangeLog >> >> * breakpoint.c (update_watchpoint): Create software watchpoints if >> the inferior has not yet started and hardware watchpoints are >> turned off. > > "Create" isn't really accurate. This function is called for resets too. > So, something like > "file foo; start; watch; kill; set can-use-hw-watchpoint 0; file foo" > will trigger that code path too, which I think will end up resulting > in access/read watchpoints being disabled, with something like: > > "file foo; start; awatch; kill; set can-use-hw-watchpoint 0; file foo" You're sort-of correct, without my patch the above commands will not trigger any error, then when the inferior is started I see 4 errors like this: "Error in re-setting breakpoint 2: Expression cannot be implemented with read/access watchpoint." Then the inferior starts and the the watchpoint is hit, we've placed a hardware watchpoint. The reason for this is that the code in breakpoint_re_set catches the error from update_watchpoint, but does nothing with it. This feels a little strange and I have a follow-up patch in this area. After my patch then we do indeed see the error earlier, when the second "file foo" is issued. For the same reason as above, this error doesn't have and impact on the watchpoint, and when we start the inferior we see another 4 errors followed by a hardware watchpoint being placed. > Anyway, I suggest: > > * breakpoint.c (update_watchpoint): If hardware watchpoints are > forced off, downgrade them to software watchpoints if possible, > and error out if not possible. Fixed. > The "Set software watchpoint before starting the inferior" string will > appear in gdb.sum for the three tests. Please make those unique per test. Fixed. > > Otherwise OK. I've included the latest version here, is this OK to apply? Thanks, Andrew gdb/ChangeLog * breakpoint.c (update_watchpoint): If hardware watchpoints are forced off, downgrade them to software watchpoints if possible, and error out if not possible. (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..c630b87 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -1795,11 +1795,18 @@ 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 (_("Software read/access watchpoints not supported.")); + } } else if (within_current_scope && b->exp) { @@ -11081,13 +11088,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 +11124,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..63c47ce 100644 --- a/gdb/testsuite/gdb.base/watchpoints.exp +++ b/gdb/testsuite/gdb.base/watchpoints.exp @@ -29,6 +29,29 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} { return -1 } +with_test_prefix "before inferior start" { + # 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" \ + "create watchpoint" + + # 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." \ + "create access watchpoint" + gdb_test "rwatch ival1" \ + "Target does not support software watchpoints of this type." \ + "create read watchpoint" +} + + # 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" ""