* [PATCH] Fix for PR15117
@ 2013-07-30 16:55 ali_anwar
2013-07-30 18:56 ` Keith Seitz
0 siblings, 1 reply; 10+ messages in thread
From: ali_anwar @ 2013-07-30 16:55 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 128 bytes --]
Hi,
Attached patch fixes PR15117. I did regression testing be executing
gdb.base/* tests.
Kindly review it.
Regards,
-Ali
[-- Attachment #2: PR15117.patch --]
[-- Type: text/x-patch, Size: 2705 bytes --]
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 <ali_anwar@codesourcery.com>
+
+ PR breakpoints/15117
+ * linespec.c (linespec_parse_basic): Check for convenience
+ variable or history value while parsing.
+
2013-07-15 Ali Anwar <ali_anwar@codesourcery.com>
PR threads/13217
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 == '$')
+ {
+ 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);
+ }
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 <alianwar@codesourcery.com>
+
+ * gdb.base/break.exp: Test break via convenience variable.
+
2013-07-09 Joel Brobecker <brobecker@adacore.com>
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)" }
+}
+
+gdb_test "break $srcfile:\$l" \
+ "Breakpoint.*at.* file .*$srcfile, line 92." \
+ "breakpoint convenience variable"
# Reset the default arguments for VxWorks
if [istarget "*-*-vxworks*"] {
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] Fix for PR15117 2013-07-30 16:55 [PATCH] Fix for PR15117 ali_anwar @ 2013-07-30 18:56 ` Keith Seitz 2013-08-02 12:14 ` ali_anwar 0 siblings, 1 reply; 10+ messages in thread From: Keith Seitz @ 2013-07-30 18:56 UTC (permalink / raw) To: ali_anwar; +Cc: gdb-patches 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<ali_anwar@codesourcery.com> > + > + 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 <ali_anwar@codesourcery.com> PR breakpoints/15117 * linespec.c (linespec_parse_basic): Check for convenience variable or history value while parsing. testsutie/ChangeLog 2013-07-30 Ali Anwar <alianwar@codesourcery.com> * gdb.base/break.exp: Test break via convenience variable. Also, please note, the formatting for ChangeLog entries is: DATE space space Name space space <email> > 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<alianwar@codesourcery.com> > + > + * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix for PR15117 2013-07-30 18:56 ` Keith Seitz @ 2013-08-02 12:14 ` ali_anwar 2013-08-02 17:49 ` Keith Seitz 0 siblings, 1 reply; 10+ messages in thread From: ali_anwar @ 2013-08-02 12:14 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 5251 bytes --] 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 <ali_anwar@codesourcery.com> PR breakpoints/15117 * linespec.c (linespec_parse_basic): Check for convenience variable or history value while parsing. testsutie/ChangeLog 2013-08-02 Ali Anwar <alianwar@codesourcery.com> * 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 [-- Attachment #2: PR15117_v2.patch --] [-- Type: text/x-patch, Size: 2224 bytes --] 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*"] { ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix for PR15117 2013-08-02 12:14 ` ali_anwar @ 2013-08-02 17:49 ` Keith Seitz 2013-08-06 19:51 ` ali_anwar 2013-08-07 5:42 ` ali_anwar 0 siblings, 2 replies; 10+ messages in thread From: Keith Seitz @ 2013-08-02 17:49 UTC (permalink / raw) To: ali_anwar; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix for PR15117 2013-08-02 17:49 ` Keith Seitz @ 2013-08-06 19:51 ` ali_anwar 2013-08-07 5:42 ` ali_anwar 1 sibling, 0 replies; 10+ messages in thread From: ali_anwar @ 2013-08-06 19:51 UTC (permalink / raw) To: gdb-patches; +Cc: Keith Seitz On 08/02/2013 10:49 PM, Keith Seitz wrote: > > With those trivial things fixed, I think this patch is ready for a > global maintainer to review and approve. > Ping. -Ali ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix for PR15117 2013-08-02 17:49 ` Keith Seitz 2013-08-06 19:51 ` ali_anwar @ 2013-08-07 5:42 ` ali_anwar 2013-08-07 20:01 ` Tom Tromey 1 sibling, 1 reply; 10+ messages in thread From: ali_anwar @ 2013-08-07 5:42 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 1005 bytes --] On 08/02/2013 10:49 PM, Keith Seitz wrote: > > Two tiny nits: > >> + { >> + /* 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] Fixed. >> +# 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? > Fixed. > With those trivial things fixed, I think this patch is ready for a > global maintainer to review and approve. > Thank you for reviewing the patch again. Regards, -Ali [-- Attachment #2: PR15117_v3.patch --] [-- Type: text/x-patch, Size: 2227 bytes --] 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 7 Aug 2013 05:32:04 -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) + { + /* The user-specified variable was not valid. 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 7 Aug 2013 05:40:12 -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*"] { ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix for PR15117 2013-08-07 5:42 ` ali_anwar @ 2013-08-07 20:01 ` Tom Tromey 2013-08-08 14:56 ` Pedro Alves 2013-08-12 12:12 ` ali_anwar 0 siblings, 2 replies; 10+ messages in thread From: Tom Tromey @ 2013-08-07 20:01 UTC (permalink / raw) To: ali_anwar; +Cc: Keith Seitz, gdb-patches >>>>> "Ali" == ali anwar <ali_anwar@codesourcery.com> writes: Ali> Thank you for reviewing the patch again. It's customary to always send the ChangeLog entry when re-sending a patch. I personally do this by putting the ChangeLog entry into the git commit and using git send-email, but there are many ways to manage it. The patch is ok with the ChangeLog from up-thread. FWIW I am not especially fond of convenience variables in linespecs. They seem odd to me. Like, won't the breakpoint move at re-set if the variable changes? Also I note that they aren't documented... But your patch is just fixing an error in the existing support, which I think makes it ok to go in. thanks, Tom ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix for PR15117 2013-08-07 20:01 ` Tom Tromey @ 2013-08-08 14:56 ` Pedro Alves 2013-08-08 20:05 ` Tom Tromey 2013-08-12 12:12 ` ali_anwar 1 sibling, 1 reply; 10+ messages in thread From: Pedro Alves @ 2013-08-08 14:56 UTC (permalink / raw) To: Tom Tromey; +Cc: ali_anwar, Keith Seitz, gdb-patches On 08/07/2013 09:01 PM, Tom Tromey wrote: > FWIW I am not especially fond of convenience variables in linespecs. > They seem odd to me. Like, won't the breakpoint move at re-set if the > variable changes? Ugh, that'd be super odd. I certainly hope not. -- Pedro Alves ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix for PR15117 2013-08-08 14:56 ` Pedro Alves @ 2013-08-08 20:05 ` Tom Tromey 0 siblings, 0 replies; 10+ messages in thread From: Tom Tromey @ 2013-08-08 20:05 UTC (permalink / raw) To: Pedro Alves; +Cc: ali_anwar, Keith Seitz, gdb-patches >> FWIW I am not especially fond of convenience variables in linespecs. >> They seem odd to me. Like, won't the breakpoint move at re-set if the >> variable changes? Pedro> Ugh, that'd be super odd. I certainly hope not. On irc Keith pointed out that the current value of the convenience variable is put into the linespec's canonical form. So, re-setting doesn't move the breakpoint. Tom ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix for PR15117 2013-08-07 20:01 ` Tom Tromey 2013-08-08 14:56 ` Pedro Alves @ 2013-08-12 12:12 ` ali_anwar 1 sibling, 0 replies; 10+ messages in thread From: ali_anwar @ 2013-08-12 12:12 UTC (permalink / raw) To: Tom Tromey; +Cc: Keith Seitz, gdb-patches, Pedro Alves On 08/08/2013 01:01 AM, Tom Tromey wrote: >>>>>> "Ali" == ali anwar <ali_anwar@codesourcery.com> writes: > > Ali> Thank you for reviewing the patch again. > > It's customary to always send the ChangeLog entry when re-sending a > patch. > > I personally do this by putting the ChangeLog entry into the git commit > and using git send-email, but there are many ways to manage it. > > The patch is ok with the ChangeLog from up-thread. > Committed, but while committing I passed an incorrect file as a commit message, is there a way or a need to change the commit message? I guess only the person with admin rights could do that. Following are the contents of commit message I intended to pass while committing. PR breakpoints/15117 * linespec.c (linespec_parse_basic): Check for convenience variable or history value while parsing. * testsuite/gdb.base/break.exp: Test break via convenience variable with file name. I apologize for the inconvenience. Regards, -Ali ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-08-12 12:12 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-30 16:55 [PATCH] Fix for PR15117 ali_anwar 2013-07-30 18:56 ` Keith Seitz 2013-08-02 12:14 ` ali_anwar 2013-08-02 17:49 ` Keith Seitz 2013-08-06 19:51 ` ali_anwar 2013-08-07 5:42 ` ali_anwar 2013-08-07 20:01 ` Tom Tromey 2013-08-08 14:56 ` Pedro Alves 2013-08-08 20:05 ` Tom Tromey 2013-08-12 12:12 ` ali_anwar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).