From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18337 invoked by alias); 3 Aug 2012 00:44:27 -0000 Received: (qmail 18328 invoked by uid 22791); 3 Aug 2012 00:44:25 -0000 X-SWARE-Spam-Status: No, hits=-7.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,MISSING_HEADERS,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 03 Aug 2012 00:44:07 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q730i5mt020461 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 2 Aug 2012 20:44:05 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q730i24W014474; Thu, 2 Aug 2012 20:44:03 -0400 Message-ID: <501B1ED2.8070208@redhat.com> Date: Fri, 03 Aug 2012 00:44:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120717 Thunderbird/14.0 MIME-Version: 1.0 CC: Marc Khouzam , "'Tom Tromey'" , "'gdb-patches@sourceware.org'" Subject: disable breakpoints with invalid condition (Re: [Patch] Cannot set pending bp if condition set explicitly) References: <87zk6nkghi.fsf@fleche.redhat.com> <501194E0.5040109@redhat.com> <5016A5DD.8040502@redhat.com> In-Reply-To: <5016A5DD.8040502@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 X-SW-Source: 2012-08/txt/msg00091.txt.bz2 On 07/30/2012 04:18 PM, Pedro Alves wrote: >>> >> This currently works: >>> >> >>> >> (gdb) b main if 1 == 1 thread 1 >>> >> Breakpoint 2 at 0x4572bb: file ../../src/gdb/gdb.c, line 29. >>> >> >>> >> But this does not: >>> >> >>> >> (gdb) condition 2 1 == 1 thread 1 >>> >> Junk at end of expression >> > >> > On a side-note, I see that although there is an error printed and >> > set_breakpoint_condition() is aborted, the bad condition is not >> > cleaned-up, as shown here: >> > >> > (gdb) info b >> > Num Type Disp Enb Address What >> > 1 breakpoint keep y 0x0804851d in main at ../../../src/gdb/testsuite/gdb.base/pending.c:26 >> > stop only if k==1 thread 1 >> > >> > Shouldn't there be some kind of cleanup in set_breakpoint_condition()? > Indeed. Hmm, that, or disable the location, with a warning. We parse the condition in the > context of each location. A condition might well make sense and parse okay > for a location, but not for another (e.g., different locals/parameters). Disabling > is also what we do if we fail to parse a location later on with "break foo if bar". > Here's a patch implementing this idea. Examples (currently, the condition text is left stale, and the breakpoint becomes uncondition): (gdb) b main Breakpoint 2 at 0x4572bb: file ../../src/gdb/gdb.c, line 29. (gdb) condition 2 foo bar warning: Breakpoint 2.1 disabled: No symbol "foo" in current context. (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep y stop only if foo bar 2.1 n 0x00000000004572bb in main at ../../src/gdb/gdb.c:29 (gdb) condition 2 Breakpoint 2 now unconditional. (gdb) enable 2.1 (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep y 0x00000000004572bb in main at ../../src/gdb/gdb.c:29 (gdb) watch args Hardware watchpoint 3: args (gdb) condition 3 Breakpoint 3 now unconditional. (gdb) condition 3 foo bar warning: Watchpoint 3 disabled: No symbol "foo" in current context. (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep y 0x00000000004572bb in main at ../../src/gdb/gdb.c:29 3 hw watchpoint keep n args stop only if foo bar (gdb) condition 3 1 == 1 thread 1 warning: Watchpoint 3 disabled - junk at end of expression: thread 1 (gdb) There are several little details we get wrong that I haven't addressed. For instance, in this case, the re-enabling of a watchpoint is prevented: (gdb) condition 3 asdf warning: Watchpoint 3 disabled: No symbol "asdf" in current context. (gdb) info breakpoints 3 Num Type Disp Enb Address What 3 hw watchpoint keep n args stop only if asdf breakpoint already hit 1 time (gdb) enable 3 Cannot enable watchpoint 3: No symbol "asdf" in current context. While this doesn't: (gdb) condition 3 1 == 1 thread 1 warning: Watchpoint 3 disabled - junk at end of expression: thread 1 (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep y 0x00000000004572bb in main at ../../src/gdb/gdb.c:29 3 hw watchpoint keep n args stop only if 1 == 1 thread 1 breakpoint already hit 1 time (gdb) enable 3 (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep y 0x00000000004572bb in main at ../../src/gdb/gdb.c:29 3 hw watchpoint keep y args stop only if 1 == 1 thread 1 breakpoint already hit 1 time (gdb) For regular breakpoints, you can always "enable" them, but when you let the program run, you'll find they are unconditional, and if something causes a re-set, they get disabled again: warning: failed to reevaluate condition for breakpoint 4: blah blah. I'd appreciate hearing opinions on whether this change is a good direction or not. macscp.exp had a case of an invalid condition being set on a breakpoint, and then the test assuming the breakpoint would be hit. 2012-08-02 Pedro Alves gdb/ * breakpoint.c (set_breakpoint_condition): If condition fails to parse, disable the watchpoint or breakpoint location, and warn. gdb/doc/ * gdb.texinfo (condition command): Mention that locations are disabled when the condition has a problem. gdb/testsuite/ * gdb.base/condbreak.exp: New tests for invalid conditions with "condition" command. * gdb.base/macscp.exp: After setting an invalid condition to a breakpoint, make the breakpoint unconditional and re-enable it. --- gdb/testsuite/gdb.base/condbreak.exp | 9 +++++++++ gdb/testsuite/gdb.base/macscp.exp | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 03719d4..b8484a7 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -956,26 +956,59 @@ set_breakpoint_condition (struct breakpoint *b, char *exp, if (is_watchpoint (b)) { struct watchpoint *w = (struct watchpoint *) b; + volatile struct gdb_exception e; innermost_block = NULL; arg = exp; - w->cond_exp = parse_exp_1 (&arg, 0, 0, 0); - if (*arg) - error (_("Junk at end of expression")); - w->cond_exp_valid_block = innermost_block; + TRY_CATCH (e, RETURN_MASK_ERROR) + { + w->cond_exp = parse_exp_1 (&arg, 0, 0, 0); + } + if (e.reason < 0) + { + b->enable_state = bp_disabled; + warning (_("Watchpoint %d disabled: %s"), + b->number, e.message); + } + else if (*arg) + { + b->enable_state = bp_disabled; + warning (_("Watchpoint %d disabled - " + "junk at end of expression: %s"), + b->number, arg); + } + else + w->cond_exp_valid_block = innermost_block; } else { struct bp_location *loc; + int loc_n; - for (loc = b->loc; loc; loc = loc->next) + for (loc = b->loc, loc_n = 1; loc; loc = loc->next, loc_n++) { + volatile struct gdb_exception e; + arg = exp; - loc->cond = - parse_exp_1 (&arg, loc->address, - block_for_pc (loc->address), 0); - if (*arg) - error (_("Junk at end of expression")); + + TRY_CATCH (e, RETURN_MASK_ERROR) + { + loc->cond = parse_exp_1 (&arg, loc->address, + block_for_pc (loc->address), 0); + } + if (e.reason < 0) + { + loc->enabled = 0; + warning (_("Breakpoint %d.%d disabled: %s"), + b->number, loc_n, e.message); + } + else if (*arg) + { + loc->enabled = 0; + warning (_("Breakpoint %d.%d disabled - " + "junk at end of expression: %s"), + b->number, loc_n, arg); + } } } } diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index facca8f..8518d2f 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -4487,11 +4487,12 @@ breakpoint @var{bnum} stops your program only if the value of @code{condition}, @value{GDBN} checks @var{expression} immediately for syntactic correctness, and to determine whether symbols in it have referents in the context of your breakpoint. If @var{expression} uses -symbols not referenced in the context of the breakpoint, @value{GDBN} -prints an error message: +symbols not referenced in the context any of the breakpoint's +locations, @value{GDBN} prints a warning message and disables the +corresponding location: @smallexample -No symbol "foo" in current context. +warning: Breakpoint 1.1 disabled: No symbol "foo" in current context. @end smallexample @noindent diff --git a/gdb/testsuite/gdb.base/condbreak.exp b/gdb/testsuite/gdb.base/condbreak.exp index e8e0d84..9d91fab 100644 --- a/gdb/testsuite/gdb.base/condbreak.exp +++ b/gdb/testsuite/gdb.base/condbreak.exp @@ -266,3 +266,12 @@ gdb_test "complete cond 1" "cond 1" gdb_test "set variable \$var = 1" gdb_test "complete cond \$v" "cond \\\$var" gdb_test "complete cond 1 values\[0\].a" "cond 1 values.0..a_field" + +gdb_test "cond 1 foo bar" ".*" "set invalid condition" + +gdb_test "info break 1" \ + "Num Type\[ \]+Disp Enb Address\[ \]+What.* +1\[\t \]+breakpoint keep y.* .* +\[\t \]+stop only if foo bar.* +1.1\[\t \]+n\[\t \]+.*in main at .*$srcfile:$bp_location6.*" \ + "location with invalid condition is disabled" diff --git a/gdb/testsuite/gdb.base/macscp.exp b/gdb/testsuite/gdb.base/macscp.exp index 1f995d5..ad18245 100644 --- a/gdb/testsuite/gdb.base/macscp.exp +++ b/gdb/testsuite/gdb.base/macscp.exp @@ -444,6 +444,13 @@ gdb_test "cond \$bpnum foo == MACRO_TO_EXPAND" \ "No symbol \"MACRO_TO_EXPAND\" in current context\." \ "macro MACRO_TO_EXPAND not in scope at breakpoint" +gdb_test "cond \$bpnum" \ + "Breakpoint .* now unconditional\." \ + "make breakpoint unconditional" + +gdb_test_no_output "enable \$bpnum.1" \ + "re-enable breakpoint" + # Note that we choose the condition so that this breakpoint never # stops. set l2 [gdb_get_line_number "set second breakpoint here"]