Re: [PATCH] Fix for PR15117

Keith Seitz
2013-08-02T17:49:38Z

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