[PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Hui Zhu]

Hi,

This issue is because function linespec_lexer_lex_one handle the
string that begin with decimal numbers decimal numbers directly and
use linespec_lexer_lex_number handle it directly.
So when there are a file name that begin with decimal numbers, will a
lot of error around it.

I post a patch make linespec_lexer_lex_number if number followed by
non-space string, use linespec_lexer_lex_string handle linespec as a
string.

Joel, this issue affect 7.5.  Does this patch can add to 7.5 branch?

Thanks,
Hui

2012-07-22  Hui Zhu  <hui_zhu@mentor.com>

	* linespec.c (linespec_lexer_lex_number): Call
	linespec_lexer_lex_string if the number is followed by
	non-space string.

---
 linespec.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- a/linespec.c
+++ b/linespec.c
@@ -368,6 +368,8 @@ static const char *const linespec_quote_
 /* Lex a number from the input in PARSER.  This only supports
    decimal numbers.  */

+static linespec_token linespec_lexer_lex_string (linespec_parser *parser);
+
 static linespec_token
 linespec_lexer_lex_number (linespec_parser *parser)
 {
@@ -390,6 +392,12 @@ linespec_lexer_lex_number (linespec_pars
       ++(PARSER_STREAM (parser));
     }

+  if (*PARSER_STREAM (parser) != '\0' && !isspace(*PARSER_STREAM (parser)))
+    {
+      PARSER_STREAM (parser) = LS_TOKEN_STOKEN (token).ptr;
+      return linespec_lexer_lex_string (parser);
+    }
+
   return token;
 }

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Jan Kratochvil]

On Sun, 22 Jul 2012 08:30:02 +0200, Hui Zhu wrote:
> This issue is because function linespec_lexer_lex_one handle the
> string that begin with decimal numbers decimal numbers directly and
> use linespec_lexer_lex_number handle it directly.

There should be a testcase for it.  (Not a review yet.)


Thanks,
Jan

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Hui Zhu]

On Sun, Jul 22, 2012 at 3:10 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Sun, 22 Jul 2012 08:30:02 +0200, Hui Zhu wrote:
>> This issue is because function linespec_lexer_lex_one handle the
>> string that begin with decimal numbers decimal numbers directly and
>> use linespec_lexer_lex_number handle it directly.
>
> There should be a testcase for it.  (Not a review yet.)
>
>
> Thanks,
> Jan

Agree.  I will add it later.

Thanks,
Hui

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Hui Zhu]

On Sun, Jul 22, 2012 at 4:55 PM, Hui Zhu <teawater@gmail.com> wrote:
> On Sun, Jul 22, 2012 at 3:10 PM, Jan Kratochvil
> <jan.kratochvil@redhat.com> wrote:
>> On Sun, 22 Jul 2012 08:30:02 +0200, Hui Zhu wrote:
>>> This issue is because function linespec_lexer_lex_one handle the
>>> string that begin with decimal numbers decimal numbers directly and
>>> use linespec_lexer_lex_number handle it directly.
>>
>> There should be a testcase for it.  (Not a review yet.)
>>
>>
>> Thanks,
>> Jan
>
> Agree.  I will add it later.
>
> Thanks,
> Hui

Hi,

I got a trouble with test.

This issue must build test code without dir, for example: "1.c".
But "gdb_compile" or "prepare_for_testing" will build the test code
with dir, for example:"src/gdb/testsuite/gdb.base/1.c".

Could you tell me how to handle it?

Thanks,
Hui

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Jan Kratochvil]

On Sun, 22 Jul 2012 12:10:45 +0200, Hui Zhu wrote:
> This issue must build test code without dir, for example: "1.c".
> But "gdb_compile" or "prepare_for_testing" will build the test code
> with dir, for example:"src/gdb/testsuite/gdb.base/1.c".
> 
> Could you tell me how to handle it?

Oops, OK, I did not expect difficulties.

The easier should be just to use TCL commands 'cd' (and restore it to 'pwd'),
and then do not pass $srcdir/$subdir to gdb_compile (you cannot use
build_executable and you also cannot use prepare_for_testing).

The more safe way against compiler output changes would be to generate DWARF.
This case should be the part
	compdir_absolute_ldir_missing__file_basename
from not yet committed patch/testcase:
	http://sourceware.org/ml/gdb-patches/2012-04/msg00106.html
so one could simplify that testcase; the testcase is a bit complex, though.


Thanks,
Jan

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Tom Tromey]

>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:

>> This issue must build test code without dir, for example: "1.c".

Why is that?
It seems to me that the problem is in the linespec lexer, and that
"break 1.c:5" or whatever ought to work ok.

Tom

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Keith Seitz]

On 07/23/2012 07:26 AM, Tom Tromey wrote:
>>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:
>
>>> This issue must build test code without dir, for example: "1.c".
>
> Why is that?
> It seems to me that the problem is in the linespec lexer, and that
> "break 1.c:5" or whatever ought to work ok.

It should also be pretty trivial to add a test for this case, too:

--- a/gdb/testsuite/gdb.linespec/ls-errs.exp
+++ b/gdb/testsuite/gdb.linespec/ls-errs.exp
@@ -124,7 +124,8 @@ foreach x [list "this_file_doesn't_exist.c" \
                "'this 'file' has quotes.c'" \
                "\"this 'file' has quotes.c\"" \
                "\"spaces: and :colons.c\"" \
-              "'more: :spaces: :and  colons::.c'"] {
+              "'more: :spaces: :and  colons::.c'" \
+              "1.c"] {
      # Remove any quoting from FILENAME for the error message.
      add the_tests "$x:3" invalid_file [string trim $x \"']
  }


Keith

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Keith Seitz]

On 07/21/2012 11:30 PM, Hui Zhu wrote:
> @@ -390,6 +392,12 @@ linespec_lexer_lex_number (linespec_pars
>         ++(PARSER_STREAM (parser));
>       }
>
> +  if (*PARSER_STREAM (parser) != '\0' && !isspace(*PARSER_STREAM (parser)))
> +    {
> +      PARSER_STREAM (parser) = LS_TOKEN_STOKEN (token).ptr;
> +      return linespec_lexer_lex_string (parser);
> +    }
> +
>     return token;
>   }
>

This obfuscates the meaning of linespec_lexer_lex_number. A better place 
to deal with this is in linespec_lexer_lex_one directly where the 
decision to call linespec_lexer_lex_number is made.

That can either be done by inspecting the input stream directly in 
*_lex_one or by having *_lex_number error (add a new error token type) 
and then having linespec_lexer_lex_one fallback to string lexing.

Keith

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Hui Zhu]

On Sun, Jul 22, 2012 at 10:11 PM, Keith Seitz <keiths@redhat.com> wrote:
> On 07/21/2012 11:30 PM, Hui Zhu wrote:
>>
>> @@ -390,6 +392,12 @@ linespec_lexer_lex_number (linespec_pars
>>         ++(PARSER_STREAM (parser));
>>       }
>>
>> +  if (*PARSER_STREAM (parser) != '\0' && !isspace(*PARSER_STREAM
>> (parser)))
>> +    {
>> +      PARSER_STREAM (parser) = LS_TOKEN_STOKEN (token).ptr;
>> +      return linespec_lexer_lex_string (parser);
>> +    }
>> +
>>     return token;
>>   }
>>
>
> This obfuscates the meaning of linespec_lexer_lex_number. A better place to
> deal with this is in linespec_lexer_lex_one directly where the decision to
> call linespec_lexer_lex_number is made.
>
> That can either be done by inspecting the input stream directly in *_lex_one
> or by having *_lex_number error (add a new error token type) and then having
> linespec_lexer_lex_one fallback to string lexing.
>
> Keith

The string begin with a number is not a error, why handle it as a
error?  I think that will really make this part obfuscates.
What I thought is change the function name to
linespec_lexer_lex_number_string to make it clear.

Thanks,
Hui

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Tom Tromey]

>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:

>> The string begin with a number is not a error, why handle it as a
>> error?  I think that will really make this part obfuscates.
>> What I thought is change the function name to
>> linespec_lexer_lex_number_string to make it clear.

How about changing linespec_lexer_lex_number to return a boolean
indicating whether it succeeded?  This is basically the same idea, but
without introducing a new token.

Tom

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Keith Seitz]

On 07/23/2012 07:28 AM, Tom Tromey wrote:
> How about changing linespec_lexer_lex_number to return a boolean
> indicating whether it succeeded?  This is basically the same idea, but
> without introducing a new token.

That's sounds even better than the sleep-deprived solution I came up with.

Keith

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Hui Zhu]

On Mon, Jul 23, 2012 at 10:28 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:
>
>>> The string begin with a number is not a error, why handle it as a
>>> error?  I think that will really make this part obfuscates.
>>> What I thought is change the function name to
>>> linespec_lexer_lex_number_string to make it clear.
>
> How about changing linespec_lexer_lex_number to return a boolean
> indicating whether it succeeded?  This is basically the same idea, but
> without introducing a new token.
>
> Tom

Agree with you.  Post a new patch according to your comments.

Thanks,
Hui

2012-07-24  Hui Zhu  <hui_zhu@mentor.com>

	* linespec.c (linespec_lexer_lex_number): Update comments,
	change the return and add check to make sure the input is
	the decimal numbers.
	(linespec_lexer_lex_one): If linespec_lexer_lex_number return
	false, call linespec_lexer_lex_string.

---
 linespec.c |   28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

--- a/linespec.c
+++ b/linespec.c
@@ -366,31 +366,38 @@ static const char *const linespec_quote_
 /* Lexer functions.  */

 /* Lex a number from the input in PARSER.  This only supports
-   decimal numbers.  */
+   decimal numbers.\
+   Return true if input is decimal numbers.  Return false if not.  */

-static linespec_token
-linespec_lexer_lex_number (linespec_parser *parser)
+static int
+linespec_lexer_lex_number (linespec_parser *parser, linespec_token *tokenp)
 {
   linespec_token token;

-  token.type = LSTOKEN_NUMBER;
-  LS_TOKEN_STOKEN (token).length = 0;
-  LS_TOKEN_STOKEN (token).ptr = PARSER_STREAM (parser);
+  tokenp->type = LSTOKEN_NUMBER;
+  LS_TOKEN_STOKEN (*tokenp).length = 0;
+  LS_TOKEN_STOKEN (*tokenp).ptr = PARSER_STREAM (parser);

   /* Keep any sign at the start of the stream.  */
   if (*PARSER_STREAM (parser) == '+' || *PARSER_STREAM (parser) == '-')
     {
-      ++LS_TOKEN_STOKEN (token).length;
+      ++LS_TOKEN_STOKEN (*tokenp).length;
       ++(PARSER_STREAM (parser));
     }

   while (isdigit (*PARSER_STREAM (parser)))
     {
-      ++LS_TOKEN_STOKEN (token).length;
+      ++LS_TOKEN_STOKEN (*tokenp).length;
       ++(PARSER_STREAM (parser));
     }

-  return token;
+  if (*PARSER_STREAM (parser) != '\0' && !isspace(*PARSER_STREAM (parser)))
+    {
+      PARSER_STREAM (parser) = LS_TOKEN_STOKEN (*tokenp).ptr;
+      return 0;
+    }
+
+  return 1;
 }

 /* Does P represent one of the keywords?  If so, return
@@ -724,7 +731,8 @@ linespec_lexer_lex_one (linespec_parser
 	case '+': case '-':
 	case '0': case '1': case '2': case '3': case '4':
         case '5': case '6': case '7': case '8': case '9':
-          parser->lexer.current = linespec_lexer_lex_number (parser);
+           if (!linespec_lexer_lex_number (parser, &(parser->lexer.current)))
+	     parser->lexer.current = linespec_lexer_lex_string (parser);
           break;

 	case ':':

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Tom Tromey]

>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:

>> 2012-07-24  Hui Zhu  <hui_zhu@mentor.com>
>> 	* linespec.c (linespec_lexer_lex_number): Update comments,
>> 	change the return and add check to make sure the input is
>> 	the decimal numbers.
>> 	(linespec_lexer_lex_one): If linespec_lexer_lex_number return
>> 	false, call linespec_lexer_lex_string.

>> +static int
>> +linespec_lexer_lex_number (linespec_parser *parser, linespec_token *tokenp)
>>  {
>>    linespec_token token;

After this change, I think 'token' is unused.

The patch is ok if you update it to remove this.

thanks,
Tom

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Hui Zhu]

On Wed, Jul 25, 2012 at 3:35 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> ">" == Hui Zhu <teawater@gmail.com> writes:
>
>>> 2012-07-24  Hui Zhu  <hui_zhu@mentor.com>
>>>      * linespec.c (linespec_lexer_lex_number): Update comments,
>>>      change the return and add check to make sure the input is
>>>      the decimal numbers.
>>>      (linespec_lexer_lex_one): If linespec_lexer_lex_number return
>>>      false, call linespec_lexer_lex_string.
>
>>> +static int
>>> +linespec_lexer_lex_number (linespec_parser *parser, linespec_token *tokenp)
>>>  {
>>>    linespec_token token;
>
> After this change, I think 'token' is unused.

Oops.

>
> The patch is ok if you update it to remove this.
>
> thanks,
> Tom

I fixed it and committed this patch to trunk and 7.5.

Thanks,
Hui

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 2124 bytes --]

On 07/25/2012 05:56 AM, Hui Zhu wrote:

>>>> 2012-07-24  Hui Zhu  <hui_zhu@mentor.com>
>>>>       * linespec.c (linespec_lexer_lex_number): Update comments,
>>>>       change the return and add check to make sure the input is
>>>>       the decimal numbers.
>>>>       (linespec_lexer_lex_one): If linespec_lexer_lex_number return
>>>>       false, call linespec_lexer_lex_string.

This patch caused several regressions:

FAIL: gdb.base/dprintf.exp: dprintf 26,"arg=%d, g=%d\n", arg, g (got 
interactive prompt)
FAIL: gdb.base/dprintf.exp: 1st dprintf, gdb
FAIL: gdb.base/dprintf.exp: 2nd dprintf, gdb
FAIL: gdb.base/dprintf.exp: 1st dprintf, call
FAIL: gdb.base/dprintf.exp: 2nd dprintf, call
FAIL: gdb.base/dprintf.exp: 1st dprintf, fprintf
FAIL: gdb.base/dprintf.exp: 2nd dprintf, fprintf
FAIL: gdb.base/list.exp: list range; filename:line1,filename:line2
FAIL: gdb.base/list.exp: list range; line1,line2
FAIL: gdb.base/list.exp: list range; upper bound past EOF
FAIL: gdb.base/list.exp: list range; both bounds past EOF
FAIL: gdb.base/list.exp: list range, must be same files
FAIL: gdb.linespec/ls-errs.exp: break 3:
FAIL: gdb.linespec/ls-errs.exp: break +10:
FAIL: gdb.linespec/ls-errs.exp: break -10:
FAIL: gdb.linespec/ls-errs.exp: break 3:
FAIL: gdb.linespec/ls-errs.exp: break +10:
FAIL: gdb.linespec/ls-errs.exp: break -10:
FAIL: gdb.linespec/ls-errs.exp: break 3	:
FAIL: gdb.linespec/ls-errs.exp: break +10	:
FAIL: gdb.linespec/ls-errs.exp: break -10	:
FAIL: gdb.linespec/ls-errs.exp: break 3	:
FAIL: gdb.linespec/ls-errs.exp: break +10	:
FAIL: gdb.linespec/ls-errs.exp: break -10	:
ERROR: (DejaGnu) proc "all tests in this module will fail.\"" does not 
exist. (backtrace.exp)

This happens because numeric tokens may be terminated by a comma or 
colon (in addition to whitespace).

The attached patch fixes these regressions (and fixes a typo in the 
comment for linespec_lexer_lex_one).

Ok for HEAD and 7.5?

Keith

ChangeLog
2012-07-25  Keith Seitz  <keiths@redhat.com>

         * linespec.c (linespec_lexer_lex_number): The input
         is also a valid number if the next character is a comma
	or colon.

[-- Attachment #2: lex_number-comma.patch --]
[-- Type: text/x-patch, Size: 1013 bytes --]

diff --git a/gdb/linespec.c b/gdb/linespec.c
index c72bb4b..9218db4 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -366,7 +366,8 @@ static const char *const linespec_quote_characters = "\"\'";
 /* Lexer functions.  */
 
 /* Lex a number from the input in PARSER.  This only supports
-   decimal numbers.\
+   decimal numbers.
+
    Return true if input is decimal numbers.  Return false if not.  */
 
 static int
@@ -389,7 +390,11 @@ linespec_lexer_lex_number (linespec_parser *parser, linespec_token *tokenp)
       ++(PARSER_STREAM (parser));
     }
 
-  if (*PARSER_STREAM (parser) != '\0' && !isspace(*PARSER_STREAM (parser)))
+  /* If the next character in the input buffer is not a space, comma,
+     or colon, this input does not represent a number.  */
+  if (*PARSER_STREAM (parser) != '\0'
+      && !isspace(*PARSER_STREAM (parser)) && *PARSER_STREAM (parser) != ','
+      && *PARSER_STREAM (parser) != ':')
     {
       PARSER_STREAM (parser) = LS_TOKEN_STOKEN (*tokenp).ptr;
       return 0;

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> Ok for HEAD and 7.5?

One nit...

Keith> +      && !isspace(*PARSER_STREAM (parser)) && *PARSER_STREAM (parser) != ','

Space after "isspace".

Ok with that fix, thanks.

Tom

Re: [PATCH Bug breakpoints/14381] Fix linespec to parse file name that begin with decimal numbers

[Keith Seitz]

On 07/26/2012 06:52 AM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
>
> Keith> Ok for HEAD and 7.5?
>
> One nit...
>
> Keith> +      && !isspace(*PARSER_STREAM (parser)) && *PARSER_STREAM (parser) != ','
>
> Space after "isspace"

Ha. I didn't even notice that had been committed.

> Ok with that fix, thanks.
>

Committed to HEAD and 7.5. Thank you for the review.

Keith