Re: [PATCH] Fix for PR15117

[Keith Seitz <keiths@redhat.com>]

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