From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11569 invoked by alias); 30 Jul 2013 18:56:44 -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 11544 invoked by uid 89); 30 Jul 2013 18:56:44 -0000 X-Spam-SWARE-Status: No, score=-5.5 required=5.0 tests=AWL,BAYES_40,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; Tue, 30 Jul 2013 18:56:43 +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 r6UIuZDN032434 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 30 Jul 2013 14:56:35 -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 r6UIuXfT026240 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 30 Jul 2013 14:56:34 -0400 Message-ID: <51F80C61.9080308@redhat.com> Date: Tue, 30 Jul 2013 18:56: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> In-Reply-To: <51F7EFF1.6030609@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2013-07/txt/msg00787.txt.bz2 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. 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 > Index: gdb/ChangeLog > =================================================================== > RCS file: /cvs/src/src/gdb/ChangeLog,v > retrieving revision 1.15798 > diff -u -r1.15798 ChangeLog > --- gdb/ChangeLog 15 Jul 2013 11:14:32 -0000 1.15798 > +++ gdb/ChangeLog 30 Jul 2013 16:35:10 -0000 > @@ -1,3 +1,9 @@ > +2013-07-30 Ali Anwar > + > + PR breakpoints/15117 > + * linespec.c (linespec_parse_basic): Check for convenience > + variable or history value while parsing. > + For future reference, don't post patches to ChangeLogs, just post the new entry. They invariably have to be pasted in by hand anyway. Most people (or at least I) post something like: ChangeLog 2013-07-30 Ali Anwar PR breakpoints/15117 * linespec.c (linespec_parse_basic): Check for convenience variable or history value while parsing. testsutie/ChangeLog 2013-07-30 Ali Anwar * gdb.base/break.exp: Test break via convenience variable. Also, please note, the formatting for ChangeLog entries is: DATE space space Name space space > 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. > + { > + 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. 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. 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. > else > { > /* The name is also not a label. Abort parsing. Do not throw > Index: gdb/testsuite/ChangeLog > =================================================================== > RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v > retrieving revision 1.3727 > diff -u -r1.3727 ChangeLog > --- gdb/testsuite/ChangeLog 10 Jul 2013 00:10:36 -0000 1.3727 > +++ gdb/testsuite/ChangeLog 30 Jul 2013 16:29:01 -0000 > @@ -1,3 +1,7 @@ > +2013-07-30 Ali Anwar > + > + * gdb.base/break.exp: Test break via convenience variable. > + Discussed above. > Index: gdb/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 > --- gdb/testsuite/gdb.base/break.exp 7 Jun 2013 17:31:07 -0000 1.58 > +++ gdb/testsuite/gdb.base/break.exp 30 Jul 2013 16:29:01 -0000 > @@ -957,6 +957,18 @@ > } > } > > +# > +# test break via convenience variable > +# > +send_gdb "set \$l = 92\n" > +gdb_expect { > + -re ".*$gdb_prompt $" { pass "Set convenience variable" } > + timeout { fail "Set convenience variable (timeout)" } > +} 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". 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.] If it does not end up fixing that case, then don't bother trying to make it work (unless you really want to). > > # Reset the default arguments for VxWorks > if [istarget "*-*-vxworks*"] { > Keith