From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23218 invoked by alias); 2 Aug 2013 17:49:38 -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 23209 invoked by uid 89); 2 Aug 2013 17:49:37 -0000 X-Spam-SWARE-Status: No, score=-6.4 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RDNS_NONE,SPF_HELO_PASS,SPF_PASS autolearn=no version=3.3.1 Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 02 Aug 2013 17:49:37 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r72HnSDR011099 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 2 Aug 2013 13:49:29 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r72HnRlG000621 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 2 Aug 2013 13:49:28 -0400 Message-ID: <51FBF127.7000108@redhat.com> Date: Fri, 02 Aug 2013 17:49:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: ali_anwar CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Fix for PR15117 References: <51F7EFF1.6030609@codesourcery.com> <51F80C61.9080308@redhat.com> <51FBA2A6.8000307@codesourcery.com> In-Reply-To: <51FBA2A6.8000307@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-08/txt/msg00091.txt.bz2 This looks great! Thank you for the expanded comments. That cleared a misconception I had. I was incorrect asserting that your convenience variable change was not located properly. On 08/02/2013 05:14 AM, ali_anwar wrote: > There are already test cases in the gdb.base/break.exp for this scenario > (break $foo) and they did get pass. The undefined scenario is already > covered in gdb.linespec/ls-errs.exp and all test cases in gdb.linespec/* > got passed. [snip] Great! Thank you for pointing that out. Two tiny nits: > Index: linespec.c > =================================================================== > RCS file: /cvs/src/src/gdb/linespec.c,v > retrieving revision 1.185 > diff -u -r1.185 linespec.c > --- linespec.c 30 May 2013 16:57:38 -0000 1.185 > +++ linespec.c 2 Aug 2013 11:43:57 -0000 > @@ -1649,7 +1649,7 @@ > else > { > /* NAME was not a function or a method. So it must be a label > - name. */ > + name or user specified variable like "break foo.c:$zippo". */ > labels = find_label_symbols (PARSER_STATE (parser), NULL, > &symbols, name); > if (labels != NULL) > @@ -1660,6 +1660,22 @@ > symbols = NULL; > discard_cleanups (cleanup); > } > + else if (token.type == LSTOKEN_STRING > + && *LS_TOKEN_STOKEN (token).ptr == '$') > + { > + /* User specified a convenience variable or history value. */ > + PARSER_RESULT (parser)->line_offset > + = linespec_parse_variable (PARSER_STATE (parser), name); > + > + if (PARSER_RESULT (parser)->line_offset.sign == LINE_OFFSET_UNKNOWN) > + { > + /* Not able to parse user specified variable. Do not > + throw an error here. parse_linespec will do it for us*/ This comment is not formatted properly: two spaces after '.'. Use complete sentences (where possible/feasible). This should probably read: /* The user-specified variable was not valid. Do not throw an error here. parse_linespec will do it for us. */ [i.e, just copy the bits from the following block] > + PARSER_RESULT (parser)->function_name = name; > + discard_cleanups (cleanup); > + return; > + } > + } > else > { > /* The name is also not a label. Abort parsing. Do not throw > Index: testsuite/gdb.base/break.exp > =================================================================== > RCS file: /cvs/src/src/gdb/testsuite/gdb.base/break.exp,v > retrieving revision 1.58 > diff -u -r1.58 break.exp > --- testsuite/gdb.base/break.exp 7 Jun 2013 17:31:07 -0000 1.58 > +++ testsuite/gdb.base/break.exp 2 Aug 2013 11:54:40 -0000 > @@ -957,6 +957,18 @@ > } > } > > +# > +# Test break via convenience variable with file name > +# > +set line [gdb_get_line_number "set breakpoint 1 here"] > +gdb_test_no_output "set \$l = $line" I'm showing extra whitespace at the end of the above line. Could you double-check that before committing? > +gdb_breakpoint ${srcfile}:\$l > + > +gdb_test_no_output "set \$foo=81.5" \ > + "set convenience variable \$foo to 81.5" > +gdb_test "break $srcfile:\$foo" \ > + "Convenience variables used in line specs must have integer values.*" \ > + "set breakpoint via non-integer convenience variable disallowed" > > # Reset the default arguments for VxWorks > if [istarget "*-*-vxworks*"] { > With those trivial things fixed, I think this patch is ready for a global maintainer to review and approve. Thanks again! Keith