From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17786 invoked by alias); 2 Aug 2013 12:14:50 -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 17765 invoked by uid 89); 2 Aug 2013 12:14:49 -0000 X-Spam-SWARE-Status: No, score=-3.0 required=5.0 tests=AWL,BAYES_20,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RDNS_NONE autolearn=no version=3.3.1 Received: from Unknown (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 02 Aug 2013 12:14:48 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1V5EFk-00003e-2Y from Ali_Anwar@mentor.com ; Fri, 02 Aug 2013 05:14:40 -0700 Received: from SVR-ORW-FEM-04.mgc.mentorg.com ([147.34.97.41]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 2 Aug 2013 05:14:40 -0700 Received: from [137.202.157.39] (147.34.91.1) by SVR-ORW-FEM-04.mgc.mentorg.com (147.34.97.41) with Microsoft SMTP Server (TLS) id 14.2.247.3; Fri, 2 Aug 2013 05:14:39 -0700 Message-ID: <51FBA2A6.8000307@codesourcery.com> Date: Fri, 02 Aug 2013 12:14:00 -0000 From: ali_anwar User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130623 Thunderbird/17.0.7 MIME-Version: 1.0 To: Keith Seitz CC: Subject: Re: [PATCH] Fix for PR15117 References: <51F7EFF1.6030609@codesourcery.com> <51F80C61.9080308@redhat.com> In-Reply-To: <51F80C61.9080308@redhat.com> Content-Type: multipart/mixed; boundary="------------000309080902010605050600" X-Virus-Found: No X-SW-Source: 2013-08/txt/msg00071.txt.bz2 --------------000309080902010605050600 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Content-length: 5251 On 07/30/2013 11:56 PM, Keith Seitz wrote: > On 07/30/2013 09:55 AM, ali_anwar wrote: >> Attached patch fixes PR15117. I did regression testing be executing >> gdb.base/* tests. > > Thanks for the patch. This has been one of the linespec enhancements > that I've meant to get around to, but it keeps getting forgotten/pushed > back. > Thanks for reviewing the patch in detail and suggesting improvements. > Please be aware that there are actually two regressions in ls-errs.exp > with this patch: > > +FAIL: gdb.linespec/ls-errs.exp: break $zippo > +FAIL: gdb.linespec/ls-errs.exp: break ls-errs.c:$zippo > Fixed. > For future reference, don't post patches to ChangeLogs, just post the > new entry. They invariably have to be pasted in by hand anyway. > ChangeLog 2013-08-02 Ali Anwar PR breakpoints/15117 * linespec.c (linespec_parse_basic): Check for convenience variable or history value while parsing. testsutie/ChangeLog 2013-08-02 Ali Anwar * gdb.base/break.exp: Test break via convenience variable with file name. >> Index: gdb/linespec.c >> =================================================================== >> RCS file: /cvs/src/src/gdb/linespec.c,v >> retrieving revision 1.185 >> diff -u -r1.185 linespec.c >> --- gdb/linespec.c 30 May 2013 16:57:38 -0000 1.185 >> +++ gdb/linespec.c 30 Jul 2013 16:33:43 -0000 >> @@ -1660,6 +1660,17 @@ >> symbols = NULL; >> discard_cleanups (cleanup); >> } >> + else if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN >> (token).ptr == '$') > > This line is too long. > Fixed >> + { >> + char *var; >> + >> + /* User specified a convenience variable or history value. */ >> + var = copy_token_string (token); >> + cleanup = make_cleanup (xfree, var); >> + PARSER_RESULT (parser)->line_offset >> + = linespec_parse_variable (PARSER_STATE (parser), var); >> + do_cleanups (cleanup); >> + } > > At this point, NAME is still valid, so you can use that directly. > Fixed > However, I don't think this handling is in the correct place. This > block of code is attempting to process the first string token in a > linespec, which is usually a file name, function name, or label name. > File name was handled already, so we are determining whether this > (string) token is a function or a label. > > Parsing the linespec "foo.c:$junk" (where "$junk" is undefined), we end > up in the block you've added. linespec_parse_variable will then attempt > to parse "$junk" and actually do nothing. It will return an invalid > offset ({0, LINE_OFFSET_UNKNOWN}). > > The next token will be EOI, and linespec_parse_basic will return. Yet it > hasn't actually found a valid location. The parser result will only > contain a symtab (for foo.c), which is an insufficient specification of > a location. > I have added a check for it. The undefined variable case is now properly handled. As the error is thrown as soon as gdb is not able to locate function name or label name so that is why handling is added in this place. > A little later down in linespec_parse_basic, you can see the comment: > > /* User specified a label or a lineno. */ > > This is probably where a convenience variable should be checked. > I gave it a try. > You can use gdb_test_no_output for this. > I would also not hard code the line number. We have a convenience > function to assist with this: > > set line [gdb_get_line_number "some comment in the source file"] > gdb_test_no_output "set \$l = $line" > >> + >> +gdb_test "break $srcfile:\$l" \ >> + "Breakpoint.*at.* file .*$srcfile, line 92." \ >> + "breakpoint convenience variable" > > We have a convenience function for setting breakpoints, too, > gdb_breakpoint. You should use that here. Substitute $line for the > hard-coded line number, "92". > Fixed > ls-errs.exp already tests the case where the variable is undefined, so > this test should be sufficient. > > Come to think of it, I believe this (revised) patch will probably also > make the linespec "$line" work (e.g., "break $foo"). If it does, could > you please add tests for that? [Three will be needed: $foo undefined, > $foo integer (passing test), $foo defined but not integer/valid.] > 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. line 633, gdb.base/break.exp # Verify that a breakpoint can be set via a convenience variable. # gdb_test_no_output "set \$foo=81.5" \ "set convenience variable \$foo to 81.5" gdb_test "break \$foo" \ "Breakpoint (\[0-9\]*) at .*, line $bp_location11.*" \ "set breakpoint via convenience variable" line 642, gdb.base/break.exp # Verify that GDB responds gracefully to an attempt to set a # breakpoint via a convenience variable whose type is not integer. # gdb_test_no_output "set \$foo=81.5" \ "set convenience variable \$foo to 81.5" gdb_test "break \$foo" \ "Breakpoint (\[0-9\]*) at .*, line $bp_location11.*" \ "set breakpoint via convenience variable" -Ali --------------000309080902010605050600 Content-Type: text/x-patch; name="PR15117_v2.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="PR15117_v2.patch" Content-length: 2224 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*/ + 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" +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*"] { --------------000309080902010605050600--