Re: [PATCH] Fix for PR15117

[ali_anwar <ali_anwar at codesourcery dot com>]

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

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*"] {