* [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).