From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12928 invoked by alias); 5 Apr 2013 17:50:47 -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 12917 invoked by uid 89); 5 Apr 2013 17:50:46 -0000 X-Spam-SWARE-Status: No, score=-8.9 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 05 Apr 2013 17:50:43 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r35Hoe7V014107 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 5 Apr 2013 13:50:40 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r35HocPL019365; Fri, 5 Apr 2013 13:50:38 -0400 Message-ID: <515F0EED.4040105@redhat.com> Date: Fri, 05 Apr 2013 19:18:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Keith Seitz CC: Hui Zhu , Tom Tromey , Hui Zhu , gdb-patches ml Subject: Re: [PATCH] Fix create pending breakpoint handle extra_string issue if not parse_condition_and_thread References: <514E8D6C.2010606@mentor.com> <514EEB43.6040101@redhat.com> <87620ftn9f.fsf@fleche.redhat.com> <515092F2.2000307@redhat.com> In-Reply-To: <515092F2.2000307@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-04/txt/msg00135.txt.bz2 Hello, On 03/25/2013 06:09 PM, Keith Seitz wrote: >> > > Actually I /was/ talking about create_breakpoint. As you stated, the only way to demonstrate the problem is via MI, so that's what I used to demonstrate how I think the situation should be handled. > > Here's a patch which does exactly what I consider the "right" way to react to having both cond_string and a condition inside arg: I think you're on the right track here. > Index: breakpoint.c > =================================================================== > RCS file: /cvs/src/src/gdb/breakpoint.c,v > retrieving revision 1.747 > diff -u -p -r1.747 breakpoint.c > --- breakpoint.c 20 Mar 2013 22:17:18 -0000 1.747 > +++ breakpoint.c 25 Mar 2013 17:59:36 -0000 > @@ -9659,6 +9659,11 @@ create_breakpoint (struct gdbarch *gdbar > extra_string = xstrdup (extra_string); > make_cleanup (xfree, extra_string); > } > + else if (*arg != '\000') > + { > + extra_string = xstrdup (arg); > + make_cleanup (xfree, extra_string); > + } This however, leaves the erroring out to ops->create_breakpoints_sal, and ultimately to init_breakpoint_sal: /* Dynamic printf requires and uses additional arguments on the command line, otherwise it's an error. */ if (type == bp_dprintf) { if (b->extra_string) update_dprintf_command_list (b); else error (_("Format string required")); } else if (b->extra_string) error (_("Garbage '%s' at end of command"), b->extra_string); That'd mean that e.g., for a future MI dprintf command, we'd accept the remainder of ARG as extra string. A MI command / python API for dprintf should want to pass the format string separately from the location. Given that create_breakpoint has an explicit "extra_string" parameter, I think the below is more appropriate. That is, if !PARSE_CONDITION_AND_THREAD, then ARG is just the location, nothing else. The fact that the describing comment of create_breakpoint doesn't mention this just looks like an oversight of when extra_string was added. "parse_condition_and_thread" has been a misnomer ever since extra_string was added. Better rename it avoid more confusion. I made it "parse_arg", as that'll remain stable even when more explicit parameters are added, and, break_command_1: create_breakpoint (get_current_arch (), arg, NULL, 0, NULL, 1 /* parse arg */, we even have comments calling it that already too. > } > > ops->create_breakpoints_sal (gdbarch, &canonical, lsal, WDYT? ------- gdb/ 2013-04-05 Pedro Alves Keith Seitz * breakpoint.c (create_breakpoint): Rename "parse_condition_and_thread" parameter to "parse_arg". Update describing comment. If !PARSE_ARG, then error out if ARG is not the empty string after extracting the location. * breakpoint.h (create_breakpoint): Rename "parse_condition_and_thread" parameter to "parse_arg". gdb/testsuite/ 2013-04-05 Pedro Alves * gdb.mi/mi-break.exp (test_error): Add tests with garbage after the location. --- gdb/breakpoint.c | 25 ++++++++++++++----------- gdb/breakpoint.h | 2 +- gdb/testsuite/gdb.mi/mi-break.exp | 12 ++++++++++++ 3 files changed, 27 insertions(+), 12 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 5ba1f2f..89f1a53 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -9510,20 +9510,20 @@ decode_static_tracepoint_spec (char **arg_p) /* Set a breakpoint. This function is shared between CLI and MI functions for setting a breakpoint. This function has two major - modes of operations, selected by the PARSE_CONDITION_AND_THREAD - parameter. If non-zero, the function will parse arg, extracting - breakpoint location, address and thread. Otherwise, ARG is just - the location of breakpoint, with condition and thread specified by - the COND_STRING and THREAD parameters. If INTERNAL is non-zero, - the breakpoint number will be allocated from the internal - breakpoint count. Returns true if any breakpoint was created; - false otherwise. */ + modes of operations, selected by the PARSE_ARG parameter. If + non-zero, the function will parse ARG, extracting location, + address, thread and extra string. Otherwise, ARG is just the + breakpoint's location, with condition, thread, and extra string + specified by the COND_STRING, THREAD and EXTRA_STRING parameters. + If INTERNAL is non-zero, the breakpoint number will be allocated + from the internal breakpoint count. Returns true if any breakpoint + was created; false otherwise. */ int create_breakpoint (struct gdbarch *gdbarch, char *arg, char *cond_string, int thread, char *extra_string, - int parse_condition_and_thread, + int parse_arg, int tempflag, enum bptype type_wanted, int ignore_count, enum auto_boolean pending_break_support, @@ -9641,7 +9641,7 @@ create_breakpoint (struct gdbarch *gdbarch, lsal = VEC_index (linespec_sals, canonical.sals, 0); - if (parse_condition_and_thread) + if (parse_arg) { char *rest; /* Here we only parse 'arg' to separate condition @@ -9660,6 +9660,9 @@ create_breakpoint (struct gdbarch *gdbarch, } else { + if (*arg != '\0') + error (_("Garbage '%s' at end of location"), arg); + /* Create a private copy of condition string. */ if (cond_string) { @@ -9699,7 +9702,7 @@ create_breakpoint (struct gdbarch *gdbarch, init_raw_breakpoint_without_location (b, gdbarch, type_wanted, ops); b->addr_string = copy_arg; - if (parse_condition_and_thread) + if (parse_arg) b->cond_string = NULL; else { diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index 68f3ed9..d740625 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -1269,7 +1269,7 @@ enum breakpoint_create_flags extern int create_breakpoint (struct gdbarch *gdbarch, char *arg, char *cond_string, int thread, char *extra_string, - int parse_condition_and_thread, + int parse_arg, int tempflag, enum bptype wanted_type, int ignore_count, enum auto_boolean pending_break_support, diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp index 9cf0126..d5d58c7 100644 --- a/gdb/testsuite/gdb.mi/mi-break.exp +++ b/gdb/testsuite/gdb.mi/mi-break.exp @@ -196,6 +196,18 @@ proc test_error {} { mi_gdb_test "-var-update *" \ "\\^done,changelist=\\\[\\\]" \ "update varobj for function call" + + # Try setting breakpoints with garbage after the location. + + # "if" only works in the CLI. It's not supposed to be accepted by + # MI. The way to specify a condition is with -c. + mi_gdb_test "-break-insert \"callme if i < 4\"" \ + ".*\\^error,msg=\"Garbage 'if i < 4' at end of location\"" \ + "breakpoint with garbage after location" + + mi_gdb_test "-break-insert -c i==4 \"callme if i < 4\"" \ + ".*\\^error,msg=\"Garbage 'if i < 4' at end of location\"" \ + "conditional breakpoint with garbage after location" } proc test_disabled_creation {} {