Re: [PATCH] Fix for PR15117

Keith Seitz
2013-07-30T18:56:44Z

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